Bump rustc version to nightly-2023-06-01 (1.72.0-nightly) #7109
Conversation
This file contains 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
vitvakatu
commented
Jun 22, 2023
| @@ -4,7 +4,7 @@ | |||
| #![feature(associated_type_bounds)] | |||
| #![feature(drain_filter)] | |||
| #![feature(iter_order_by)] | |||
| #![feature(option_result_contains)] | |||
There was a problem hiding this comment.
Change number one – no more option_result_contains. Our build system is a primary victim, though, IMO changes are not bad. I also implemented our version of this feature in the prelude, so the IDE code didn't notice anything.
|
|
||
| // ============= | ||
| // === Tests === | ||
| // ============= |
There was a problem hiding this comment.
Don't worry, tests just got copy-pasted to another module to prevent name conflict.
| @@ -1175,10 +1175,11 @@ pub struct LibraryComponentGroup { | |||
| /// | |||
| /// For more information, see | |||
| /// https://github.com/enso-org/design/blob/main/epics/basic-libraries/write-action-control/design.md. | |||
| #[derive(Hash, Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | |||
| #[derive(Hash, Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] | |||
There was a problem hiding this comment.
Clippy now requires Default derives, and it is a good thing for most places (except macros-generated code, urgh...)
| @@ -129,17 +129,17 @@ struct NonChildTreeCrumb; | |||
| /// | |||
| /// It provides way to easily convert vector of specific crumbs (e.g. | |||
| /// `[InfixCrumb::LeftoOperand, ..]` without calling into on each element. | |||
| pub trait IntoCrumbs: IntoIterator<Item: Into<Crumb>> + Sized { | |||
| pub trait IntoCrumbs<I: Into<Crumb>>: IntoIterator<Item = I> + Sized { | |||
There was a problem hiding this comment.
I don't understand why this change is needed. If anyone can fix the old code – you're welcome.
There was a problem hiding this comment.
I think the better fix would be:
- Not making
IntoCrumbsa generic trait - Remove the inheritance
: IntoIterator<Item: Into<Crumb> - implement
into_crumbswhere we implement the trait (currently in line 139).
We can also think about sealing this trait (as we don't expect anyone providing their implementations ofinto_crumbs).
| @@ -273,7 +273,7 @@ macro_rules! define_widget_modules( | |||
| } | |||
|
|
|||
| $( | |||
| impl const From<<$module::Widget as SpanWidget>::Config> for DynConfig { | |||
There was a problem hiding this comment.
No more const traits in the standard library, as well (removed in rust-lang/rust#110393)
| @@ -21,6 +21,8 @@ use crate::display::render::passes::SymbolsRenderPass; | |||
| use crate::display::scene::DomPath; | |||
| use crate::display::scene::Scene; | |||
| use crate::display::scene::UpdateStatus; | |||
| #[allow(clippy::useless_attribute)] | |||
| #[allow(hidden_glob_reexports)] | |||
| use crate::display::shape::primitive::glsl; | |||
There was a problem hiding this comment.
Pretty much another glob reexport conflict. We have multiple glsl modules.
| @@ -1,5 +1,8 @@ | |||
| //! `BufferUsage` specifies the intended usage pattern of the data store for optimization purposes. | |||
|
|
|||
| // === Non-Standard Linter Configuration === | |||
| #![allow(clippy::derivable_impls)] | |||
There was a problem hiding this comment.
I got lost in macros internals and couldn't place this attribute in the macro-generated code.
Comment on lines
+319
to
+321
| // Clippy thinks that the clone is redundant, but it is not true, | ||
| // `resolved_validator` is used outside this `if` statement. | ||
| #[allow(clippy::redundant_clone)] |
There was a problem hiding this comment.
Clippy is an absolute lunatic in this version.
lib/rust/prelude/src/zeroable.rs
Outdated
Comment on lines
190
to
192
| #[cfg(not(version("1.69")))] | ||
| #[cfg(version("1.72"))] | ||
| cell: core::cell::RefCell<T>, | ||
| #[cfg(version("1.69"))] | ||
| #[cfg(not(version("1.72")))] |
There was a problem hiding this comment.
The cfg conditions were swapped, now it should work correctly. RefCell didn't change, btw.
There was a problem hiding this comment.
The
cfgconditions were swapped, now it should work correctly. RefCell didn't change, btw.
No, they were good. version means _this version or higher. You should just replace 1.69 with 1.73.
| @@ -399,7 +399,7 @@ macro_rules! shared2 { | |||
|
|
|||
| impl {$( | |||
| $(#$fn_meta:tt)* | |||
| $fn_vis:vis fn $fn_name:ident $(<$($fn_param:ident : $fn_param_ty:ty),*>)? | |||
| $fn_vis:vis fn $fn_name:ident $(<$($fn_param:ident : $fn_param_ty:path),*>)? | |||
There was a problem hiding this comment.
Funny change, I don't know how it worked before with types like callback::NoArgs (which is obviously path, not ty)
vitvakatu
requested review from
4e6,
JaroslavTulach,
radeusgd,
MichaelMauderer,
wdanilo,
farmaazon,
kazcw and
mwu-tow
as code owners
farmaazon
requested changes
Jun 22, 2023
| @@ -129,17 +129,17 @@ struct NonChildTreeCrumb; | |||
| /// | |||
| /// It provides way to easily convert vector of specific crumbs (e.g. | |||
| /// `[InfixCrumb::LeftoOperand, ..]` without calling into on each element. | |||
| pub trait IntoCrumbs: IntoIterator<Item: Into<Crumb>> + Sized { | |||
| pub trait IntoCrumbs<I: Into<Crumb>>: IntoIterator<Item = I> + Sized { | |||
There was a problem hiding this comment.
I think the better fix would be:
- Not making
IntoCrumbsa generic trait - Remove the inheritance
: IntoIterator<Item: Into<Crumb> - implement
into_crumbswhere we implement the trait (currently in line 139).
We can also think about sealing this trait (as we don't expect anyone providing their implementations ofinto_crumbs).
| @@ -1,3 +1,6 @@ | |||
| // === Non-Standard Linter Configuration === | |||
| #![allow(unused_qualifications)] | |||
There was a problem hiding this comment.
To remove this attribute, we must update
clapto the most recent version. I failed to do that, it's too dangerous to mix that with other changes. See clap-rs/clap#4951
Add a comment here with the link you've posted.
Comment on lines
3
to
7
| // Clippy thinks that `Item` is unused in [`Slot`] definition, but it is not correct. | ||
| // It also does not seem to be possible to disable this lint for the particular struct only. | ||
|
|
||
| // === Non-Standard Linter Configuration === | ||
| #![allow(clippy::extra_unused_type_parameters)] |
There was a problem hiding this comment.
Let's file them an issue (with link to our code - it's open source after all), and link it here.
Comment on lines
+11
to
+14
|
|
||
|
|
||
|
|
||
| mod unit; |
There was a problem hiding this comment.
Why has it become private?
There was a problem hiding this comment.
There is a reexport conflict between ensogl_core::prelude and ensogl_core::display::shape.
INFO ide_ci::program::command: cargo⚠️ warning: ambiguous glob re-exports
INFO ide_ci::program::command: cargo⚠️ --> lib/rust/ensogl/component/grid-view/src/lib.rs:59:13
INFO ide_ci::program::command: cargo⚠️ |
INFO ide_ci::program::command: cargo⚠️ 59 | pub use ensogl_core::display::shape::*;
INFO ide_ci::program::command: cargo⚠️ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the name `unit` in the type namespace is first re-exported here
INFO ide_ci::program::command: cargo⚠️ 60 | pub use ensogl_core::prelude::*;
INFO ide_ci::program::command: cargo⚠️ | ----------------------- but the name `unit` in the type namespace is also re-exported here
INFO ide_ci::program::command: cargo⚠️ |
INFO ide_ci::program::command: cargo⚠️ = note: `#[warn(ambiguous_glob_reexports)]` on by default
But making this unit private doesn't change anything – all its contents are reexported in this file a few lines below. I find this way of organizing reexports weird, though.
lib/rust/prelude/src/zeroable.rs
Outdated
Comment on lines
190
to
192
| #[cfg(not(version("1.69")))] | ||
| #[cfg(version("1.72"))] | ||
| cell: core::cell::RefCell<T>, | ||
| #[cfg(version("1.69"))] | ||
| #[cfg(not(version("1.72")))] |
There was a problem hiding this comment.
The
cfgconditions were swapped, now it should work correctly. RefCell didn't change, btw.
No, they were good. version means _this version or higher. You should just replace 1.69 with 1.73.
|
I try to |
kazcw
approved these changes
Jun 22, 2023
vitvakatu
added
the
CI: Clean build required
|
Apparently linux build fails for some reason, marking as draft until resolved. |
|
Probably rustwasm/wasm-bindgen#3457 |
|
I don't think we should merge anything that has warnings during compilation, so the warnings should be addressed in this PR. What changes are needed to fix them? |
|
It is not fixed on my machine. I found another bug we perhaps trigger: rust-lang/rust#111888 |
|
We won't have time to investigate it properly. @vitvakatu please merge this branch ASAP after restoring old rustc version - the changes are worth keeping regardless if version is bumped or not. |
wdanilo
requested changes
Jul 4, 2023
There was a problem hiding this comment.
- Many changes are nice, several make the code much worse to read (I left comments in these places). I need to understand what forced us to change code this way. Maybe disabling some lints is the way to go instead ...
- @farmaazon I'm afraid this might not be mergeable with currently used Rust version as it seems like new Rust version stabilized/added some APIs that we are using here.
- Several places in the code are not cleaned - I left comments about it.
Comment on lines
-153
to
158
| vector.drain_filter(|(range, id)| { | ||
| preferred.get(range).map(|preferred_id| id != preferred_id).unwrap_or(false) | ||
| }); | ||
| vector | ||
| .extract_if(|(range, id)| { | ||
| preferred.get(range).map(|preferred_id| id != preferred_id).unwrap_or(false) | ||
| }) | ||
| .for_each(drop); | ||
| } |
There was a problem hiding this comment.
This is such a strange change. Did clip told to write it this way? for_each(drop) is tacky as hell. The previous version was much more understandable.
There was a problem hiding this comment.
extract_if returns iterator, not using it will cause incorrect behavior. Alternative solution is to use retain and flip the condition in the closure, but I was not filling comfortable doing it for such complex expression.
Comment on lines
+626
to
+627
|
|
||
|
|
There was a problem hiding this comment.
This code is copy-pasted from another place. I will fix that though
| self.expressions_of_node.drain_filter(|node_id, _| !nodes.contains(node_id)); | ||
| self.expressions_of_node.extract_if(|node_id, _| !nodes.contains(node_id)); |
There was a problem hiding this comment.
this is such a strange name. Im surprised. drain_filter was much nicer. Is it removed? :O
There was a problem hiding this comment.
Is it really? I think drain was clear to us, because we know what drain means in this context. But it's not obvious.
I actually think extract is a better word for remove some elements from the vector and return them.
There was a problem hiding this comment.
It is removed in the latest versions of the compiler. I will have to revert these changes though, because we need to return back to the previous compiler version.
Comment on lines
+48
to
+57
| // ) -> BoxFuture<'static, Result<Self::Artifact>> { let WithDestination { inner, destination } | ||
| // = job; let this = self.clone(); async move { let paths = | ||
| // crate::paths::Paths::new_versions(&inner.repo_root, inner.versions)?; let context = | ||
| // crate::engine::context::RunContext { operation: crate::engine::Operation::Build, goodies: | ||
| // GoodieDatabase::new()?, config: BuildConfigurationFlags { clean_repo: false, | ||
| // build_engine_package: true, ..crate::engine::NIGHTLY } .into(), inner: context, paths, }; | ||
| // let artifacts = context.build().await?; let engine_distribution = | ||
| // artifacts.packages.engine.context("Missing Engine Distribution!")?; | ||
| // ide_ci::fs::mirror_directory(&engine_distribution.dir, &destination).await?; | ||
| // this.adapt_artifact(destination).await } .boxed() |
There was a problem hiding this comment.
this is unformatted. The previous version was much nicer.
There was a problem hiding this comment.
Also the question is: why do we keep entirely-commented file in our repo? @mwu-tow ?
There was a problem hiding this comment.
Obviously I wasn't touching commented code, it must be rustfmt doing that. I also don't understand why we have commented code in the repo, I was always told on reviews to remove it.
Comment on lines
+174
to
179
| // ) -> BoxFuture<'static, Result<Self::Watcher>> { async move { let BuildInput { build_info, | ||
| // repo_root, wasm } = build_input; let ide = IdeDesktop::new(&repo_root.app.ide_desktop); let | ||
| // watch_process = ide.watch_content(wasm, &build_info.await?).await?; let artifact = | ||
| // Self::Artifact::from_existing(output_path).await?; Ok(Self::Watcher { watch_process, | ||
| // artifact }) } .boxed() | ||
| // } |
| //! 2. IntelliJ does not support environment variable expansion in the run configuration command, | ||
| //! see: https://github.com/intellij-rust/intellij-rust/issues/9621. | ||
| //! 3. IntelliJ does not support custom cargo commands, | ||
| //! see: https://youtrack.jetbrains.com/issue/CPP-30882 | ||
| //! 3. IntelliJ does not support custom cargo commands, see: https://youtrack.jetbrains.com/issue/CPP-30882 |
| @@ -30,6 +29,13 @@ pub struct State<T> { | |||
| id: usize, | |||
| } | |||
|
|
|||
| // See https://github.com/mcarton/rust-derivative/issues/112. | |||
There was a problem hiding this comment.
This is sooo bad :( Instead, we might want to disable this lint for now. Adding explicit code ike that makes the code harder to maintain and harder to fix when the lint gets fixed.
There was a problem hiding this comment.
I will revert this change, as it is a bug in derivative.
| @@ -14,6 +14,8 @@ pub mod physics; | |||
| mod frp; | |||
| mod loops; | |||
|
|
|||
| #[allow(clippy::useless_attribute)] | |||
| #[allow(ambiguous_glob_reexports)] | |||
Comment on lines
-152
to
+159
| const DEFAULT_SAMPLER: Sampler = Default::default(); | ||
| const DEFAULT_SAMPLER: Sampler = Sampler::default(); |
There was a problem hiding this comment.
Why this change? Repeating the Sampler name does not make any sense. What lint caused it?
There was a problem hiding this comment.
Default is not a const function, so can't be used here.
Comment on lines
-425
to
+428
| let ready_jobs = self.jobs.compile_poll.drain_filter(|job| { | ||
| job.khr.is_shader_ready(&self.context, &job.program).unwrap_or(true) | ||
| }); | ||
| let ready_jobs = self | ||
| .jobs | ||
| .compile_poll | ||
| .extract_if(|job| job.khr.is_shader_ready(&self.context, &job.program).unwrap_or(true)); |
There was a problem hiding this comment.
another example of extract_if being much worse than drain_filter regarding code readability
There was a problem hiding this comment.
I don't understand your comment. It's literally change in name + rustfmt, and it reads perfectly fine ("ready jobs = extract by condition from compile_poll" instead of "ready jobs = drain filter compile poll by condition")
|
To all reviewers: PR is WIP, and there are changes expected. Reviewing it now is not advisable. My idea is to revert change in compiler version until rust-lang/rust#111888 is fixed. The issue is that I need to carefully revert changes that don't allow using previous compiler, while keeping most of other changes intact to facilitate future updates. To not block my current work on IDE, I will do that slowly. |
I'm only afraid merge conflicts will kill your progress when doing slowly. |
|
I second @farmaazon worry. We will have big trouble with conflicts. What about applying as much changes as we can to the current codebase leaving the current compiler and then bumping it when new Rust comes out? |
|
Not in our current priorities. |
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.
Pull Request Description
Fixes #6895
We have this warning that needs to be addressed by a separate task.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.