Upgrade Rust version #1604
Merged
Upgrade Rust version #1604
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolve new clippy lints on the way
Comment on lines
9
to
10
| license.workspace = true | ||
| lints.workspace = true | ||
| publish.workspace = true |
Member
There was a problem hiding this comment.
Does this remove root lints from the individual packages? (reverting them to defaults)
https://doc.rust-lang.org/cargo/reference/workspaces.html#the-lints-table
Need to check whether this
lints.workspace = true
is the same as suggested in the docs
[lints]
workspace = true
(deepseek says yes)
In the much praised toml format you can express things in different ways, that's why I don't like it and always do mistakes.
But we definitely want to inherit lints from the root Cargo.toml, no matter what the syntax for it is.
Another suspicion is that the upgrade goes quite a few versions forward, but you only have fixed a few lines of lints. This does not seem realistic. With the aggressive config we have, the lint warnings typically come in massive amounts after such upgrades. Lack of warnings kinda hints that the lints simply no longer work, outside of the default set (which I found lacking).
Comment on lines
245
to
247
| let input_ref = input_ref | ||
| .as_ref() | ||
| .cloned() | ||
| .clone() | ||
| .or_else(|| { |
Member
There was a problem hiding this comment.
Hmm... Is this equivalent? What was the warning?
Member
Author
There was a problem hiding this comment.
warning: cloning an `Option<_>` using `.as_ref().cloned()`
--> packages/nextclade-cli/src/dataset/dataset_download.rs:246:6
|
246 | .as_ref()
| ______^
247 | | .cloned()
| |___________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_cloned
= note: `-W clippy::option-as-ref-cloned` implied by `-W clippy::pedantic`
= help: to override `-W clippy::pedantic` add `#[allow(clippy::option_as_ref_cloned)]`
help: this can be written more concisely by cloning the `Option<_>` directly
|
246 | .clone()
| ~~~~~
|
|
||
| /// Runs analysis on one sequence and returns its result. This runs in many webworkers concurrently. | ||
| pub fn analyze(&mut self, input: &str) -> Result<String, JsError> { | ||
| pub fn analyze(& self, input: &str) -> Result<String, JsError> { |
Member
There was a problem hiding this comment.
Extra space. Let's also run full reformat after we are done in this PR.
Comment on lines
4
to
6
| use num_traits::{clamp, clamp_max, clamp_min, AsPrimitive}; | ||
| use schemars::gen::SchemaGenerator; | ||
| use schemars::r#gen::SchemaGenerator; | ||
| use schemars::schema::Schema; |
Member
There was a problem hiding this comment.
Oh no! They reserved the gen keyword :)
Comment on lines
118
to
+119
|
|
||
| record.seq_name = self.line[1..].trim().to_owned(); | ||
| self.line[1..].trim().clone_into(&mut record.seq_name); |
Member
There was a problem hiding this comment.
Probably bogus lint to disable (unless it isn't :))
Member
Author
There was a problem hiding this comment.
I think the "avoiding allocations" will appeal to the perf in you ;)
https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones
Custom Clone::clone_from() or ToOwned::clone_into implementations allow the objects to share resources and therefore avoid allocations.
warning: assigning the result of `ToOwned::to_owned()` may be inefficient
--> packages/nextclade/src/io/fasta.rs:119:5
|
119 | record.seq_name = self.line[1..].trim().to_owned();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `self.line[1..].trim().clone_into(&mut record.seq_name)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones
= note: `-W clippy::assigning-clones` implied by `-W clippy::pedantic`
= help: to override `-W clippy::pedantic` add `#[allow(clippy::assigning_clones)]`
Comment on lines
312
to
314
|
|
||
| let builtin_attrs = vec![o!("clade")]; | ||
| let builtin_attrs = [o!("clade")]; | ||
| let attrs = chain!( |
Member
There was a problem hiding this comment.
Not a big deal, but this adds an extra heap allocation that seems to be useless
Member
Author
There was a problem hiding this comment.
warning: useless use of `vec!`
--> packages/nextclade/src/io/nextclade_csv.rs:313:25
|
313 | let builtin_attrs = vec![o!("clade")];
| ^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[o!("clade")]`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_vec
= note: `-W clippy::useless-vec` implied by `-W clippy::all`
= help: to override `-W clippy::all` add `#[allow(clippy::useless_vec)]`
Member
|
I am a bit busy on a large feature Richard waits for. Not a fantastic time for an upgrade :) Is this something that we need right now? Rust Analyzer, meaning VSCode plugin stopped working or what? I thought rust analyzer comes with the toolchain and is always compatible. Any particular reason it's now rust 1.80.0? (not the minimum required, and not the latest, but something in between) I love updates too, but what keeps me from updating Nextclade in particular is the wasm and especially dev mode of Nextclade Web. The wasm-bindgen based setup is very fragile. Last time I touched something I've got white screen in the browser and this blocked dev process entirely. So we need to ensure web prod and dev works (not just builds). But you can only know if you actually try and change something in the web app (rust and ts). As I mentioned, it is expected that there's a massive amount of work on fixing or disabling new lints after rust upgrade. As I don't see it in the diff, and don't see any warnings, I assume lints no longer work. But I could be wrong. Regarding disk space failure - no idea. Perhaps the caches are too big and might need to reset them? But don't have time to check this currently. |
Member
|
Oh no, I think that [package]
lints.workspace = trueis not the same as [lints]
workspace = trueThe lints might have never worked in the first place. I think they only started to respect this config recently. So yes, toml is a lovely format :) |
Member
Author
|
Oh I just wanted to play with Rust, no pressure here! Ah now I remember you mentioning the challenge with wasm and dev. The time lines are not the same! But cargo said this entry wasn't used at all and emitted a warning in 1.80, so I trusted it. Was it or I wrong? 😄 Lints still work afaict, I resolved and/or allowed the ones that came up. |
Member
Author
|
Here are the full lints if I just upgrade the toolchain and not change anything else :) Details |
Without loose setting in next config, I got an error ``` cross-env NODE_ENV=development BABEL_ENV=development babel-node --config-file "./babel-node.config.js" --extensions ".ts" tools/monkeyPatch.ts $ cross-env NODE_ENV=development BABEL_ENV=development NODE_OPTIONS=--max_old_space_size=8192 babel-node --config-file "./babel-node.config.js" --config-file "./babel-node.config.js" --extensions ".js,.ts" -- node_modules/.bin/next dev --hostname 0.0.0.0 --port 3000 ready - started server on 0.0.0.0:3000, url: http://localhost:3000 ✔ @nextstrain/nextclade-web Compiled successfully in 4.72s Browserslist: caniuse-lite is outdated. Please run: npx update-browserslist-db@latest Why you should do it regularly: https://github.com/browserslist/update-db#readme Browserslist: caniuse-lite is outdated. Please run: npx update-browserslist-db@latest Why you should do it regularly: https://github.com/browserslist/update-db#readme DONE Compiled successfully in 4726ms 3:02:32 PM error - ./src/gen/nextclade-wasm_bg.wasm Module not found: ESM packages (./nextclade-wasm_bg.js) need to be imported. Use 'import' to reference the package instead. https://nextjs.org/docs/messages/import-esm-externals ```
resolves #1605 - Replace deprecated `FileReader.readAsBinaryString(file)` with recommended `readAsArrayBuffer(file)` - Adapt readFile function to use array buffer - Make encoding detection use at most 64kB (perf, borrowed from VS Code) It's unclear why Chromium 136 suddenly caused the crash, but moving away from the deprecated method seems to fix it, so why not.
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rust Analyzer currently supports >=1.78 and we are on 1.76 so upgrading is useful (apart from the general improvements in new versions)
I'm surprised that things still worked on Ubuntu 12.04 till upgrade to rustc 1.80, given Rust 1.64 started requiring glibc >= 2.17
Unit tests fail due to: