Uh oh!
There was an error while loading. Please reload this page.
Fix dyn -> param suggestion in struct ICEs - #138238
Merged
Merged
Conversation
rustbot
commented
Mar 8, 2025
Collaborator
HIR ty lowering was modified cc @fmease |
| let mut sugg = vec![]; | ||
| let parent_id = tcx.hir_get_parent_item(self_ty.hir_id).def_id; | ||
| let parent_item = tcx.hir_node_by_def_id(parent_id).expect_item(); |
ContributorAuthor
There was a problem hiding this comment.
expect_item here is not valid, since there are other kinds of HIR parents that can own this dyn Trait, like extern items (or trait/impl items, closures, etc).
| let parent_hir_id = tcx.parent_hir_id(self_ty.hir_id); | ||
| let parent_item = tcx.hir_get_parent_item(self_ty.hir_id).def_id; | ||
| let generics = match tcx.hir_node_by_def_id(parent_item) { |
ContributorAuthor
There was a problem hiding this comment.
I added a check that we're suggesting this on a field directly. dyn may show up in other positions like in a where clause or in an expr on a default field value.
| if let hir::ItemKind::Enum(..) = parent_item.kind { | ||
| return false; | ||
| } | ||
| // Look at the direct HIR parent, since we care about the relationship between |
ContributorAuthor
There was a problem hiding this comment.
This was also expect_iteming unnecessarily. I took the logic and made it focus on the relationship between the ty and its parent node, so we don't suggest turning:
struct Foo { f: Box<Any>, /* more fields */ }
Into this:
struct Foo<T: Any> { f: Box<T>, /* more fields */ }
My general logic here is that if the ty's hir node parent is another ty, then it's probably just a misspelled dyn Trait. There is no perfect heuristic here, but I think it's less invasive to suggest the most naive thing here, which is just to insert dyn, unless we're very confident it will be wrong.
| @@ -13,7 +13,6 @@ struct Foo2 { | |||
| //~^ ERROR expected a type, found a trait | |||
ContributorAuthor
There was a problem hiding this comment.
I moved this test out of suggestions/ since that's a kitchen sink
| // If the parent item is a struct, check if self_ty is the last field. | ||
| if let hir::ItemKind::Struct(variant_data, _) = parent_item.kind { | ||
| if variant_data.fields().last().unwrap().ty.span != self_ty.span { |
ContributorAuthor
There was a problem hiding this comment.
We don't need to use span comparisons here, hir_id comparisons are more faithful.
Comment on lines
+168
to
+173
| let param = "TUV" | ||
| .chars() | ||
| .map(|c| c.to_string()) | ||
| .chain((0..).map(|i| format!("P{i}"))) | ||
| .find(|s| !generics.params.iter().any(|param| param.name.ident().as_str() == s)) | ||
| .expect("we definitely can find at least one param name to generate"); |
ContributorAuthor
There was a problem hiding this comment.
This logic didn't account for preexisting generic args, so this tries its best to suggest T, U, V, P1, P2, ...
nnethercote
commented
Mar 9, 2025
Contributor
@bors r+ Thank for fixing the problems. |
bors
commented
Mar 9, 2025
Collaborator
nnethercote
commented
Mar 9, 2025
Contributor
cc @xizheyin |
compiler-errors
commented
Mar 9, 2025
ContributorAuthor
error path only so @bors rollup |
matthiaskrgr added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 9, 2025
…struct, r=nnethercote Fix dyn -> param suggestion in struct ICEs Makes the logic from rust-lang#138042 a bit less ICEy and more clean. Also fixes an incorrect suggestion when the struct already has generics. I'll point out the major changes and observations in the code. Fixesrust-lang#138229Fixesrust-lang#138211 r? nnethercote since you reviewed the original pr, or re-roll if you don't want to review this
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 10, 2025
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#137279 (Make some invalid codegen attr errors structured/translatable) - rust-lang#137793 (Stablize anonymous pipe) - rust-lang#138238 (Fix dyn -> param suggestion in struct ICEs) - rust-lang#138270 (chore: Fix some comments) - rust-lang#138280 (fix ICE in pretty-printing `global_asm!`) - rust-lang#138286 (triagebot.toml: Don't label `test/rustdoc-json` as A-rustdoc-search (…) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 10, 2025
…struct, r=nnethercote Fix dyn -> param suggestion in struct ICEs Makes the logic from rust-lang#138042 a bit less ICEy and more clean. Also fixes an incorrect suggestion when the struct already has generics. I'll point out the major changes and observations in the code. Fixesrust-lang#138229Fixesrust-lang#138211 r? nnethercote since you reviewed the original pr, or re-roll if you don't want to review this
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 10, 2025
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#136395 (Update to rand 0.9.0) - rust-lang#137279 (Make some invalid codegen attr errors structured/translatable) - rust-lang#137585 (Update documentation to consistently use 'm' in atomic synchronization example) - rust-lang#137926 (Add a test for `-znostart-stop-gc` usage with LLD) - rust-lang#138074 (Support `File::seek` for Hermit) - rust-lang#138238 (Fix dyn -> param suggestion in struct ICEs) - rust-lang#138270 (chore: Fix some comments) - rust-lang#138280 (fix ICE in pretty-printing `global_asm!`) - rust-lang#138286 (triagebot.toml: Don't label `test/rustdoc-json` as A-rustdoc-search (…) r? `@ghost` `@rustbot` modify labels: rollup
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 10, 2025
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136395 (Update to rand 0.9.0) - rust-lang#137279 (Make some invalid codegen attr errors structured/translatable) - rust-lang#137585 (Update documentation to consistently use 'm' in atomic synchronization example) - rust-lang#137926 (Add a test for `-znostart-stop-gc` usage with LLD) - rust-lang#138074 (Support `File::seek` for Hermit) - rust-lang#138238 (Fix dyn -> param suggestion in struct ICEs) - rust-lang#138270 (chore: Fix some comments) - rust-lang#138286 (triagebot.toml: Don't label `test/rustdoc-json` as A-rustdoc-search (…) r? `@ghost` `@rustbot` modify labels: rollup
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 10, 2025
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136395 (Update to rand 0.9.0) - rust-lang#137279 (Make some invalid codegen attr errors structured/translatable) - rust-lang#137585 (Update documentation to consistently use 'm' in atomic synchronization example) - rust-lang#137926 (Add a test for `-znostart-stop-gc` usage with LLD) - rust-lang#138074 (Support `File::seek` for Hermit) - rust-lang#138238 (Fix dyn -> param suggestion in struct ICEs) - rust-lang#138270 (chore: Fix some comments) - rust-lang#138286 (triagebot.toml: Don't label `test/rustdoc-json` as A-rustdoc-search (…) r? `@ghost` `@rustbot` modify labels: rollup
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 10, 2025
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136395 (Update to rand 0.9.0) - rust-lang#137279 (Make some invalid codegen attr errors structured/translatable) - rust-lang#137585 (Update documentation to consistently use 'm' in atomic synchronization example) - rust-lang#137926 (Add a test for `-znostart-stop-gc` usage with LLD) - rust-lang#138074 (Support `File::seek` for Hermit) - rust-lang#138238 (Fix dyn -> param suggestion in struct ICEs) - rust-lang#138270 (chore: Fix some comments) - rust-lang#138286 (triagebot.toml: Don't label `test/rustdoc-json` as A-rustdoc-search (…) r? `@ghost` `@rustbot` modify labels: rollup
jieyouxu added a commit
to jieyouxu/rust
that referenced
this pull request
Mar 10, 2025
…struct, r=nnethercote Fix dyn -> param suggestion in struct ICEs Makes the logic from rust-lang#138042 a bit less ICEy and more clean. Also fixes an incorrect suggestion when the struct already has generics. I'll point out the major changes and observations in the code. Fixesrust-lang#138229Fixesrust-lang#138211 r? nnethercote since you reviewed the original pr, or re-roll if you don't want to review this
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 11, 2025
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136395 (Update to rand 0.9.0) - rust-lang#137279 (Make some invalid codegen attr errors structured/translatable) - rust-lang#137585 (Update documentation to consistently use 'm' in atomic synchronization example) - rust-lang#137926 (Add a test for `-znostart-stop-gc` usage with LLD) - rust-lang#138074 (Support `File::seek` for Hermit) - rust-lang#138238 (Fix dyn -> param suggestion in struct ICEs) - rust-lang#138270 (chore: Fix some comments) - rust-lang#138286 (triagebot.toml: Don't label `test/rustdoc-json` as A-rustdoc-search (…) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 11, 2025
Rollup merge of rust-lang#138238 - compiler-errors:dyn-suggestion-in-struct, r=nnethercote Fix dyn -> param suggestion in struct ICEs Makes the logic from rust-lang#138042 a bit less ICEy and more clean. Also fixes an incorrect suggestion when the struct already has generics. I'll point out the major changes and observations in the code. Fixesrust-lang#138229Fixesrust-lang#138211 r? nnethercote since you reviewed the original pr, or re-roll if you don't want to review this
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Makes the logic from #138042 a bit less ICEy and more clean. Also fixes an incorrect suggestion when the struct already has generics. I'll point out the major changes and observations in the code.
Fixes#138229
Fixes#138211
r? nnethercote since you reviewed the original pr, or re-roll if you don't want to review this