Explain origin of implicit Sized obligations and provide suggestions when possible
#85947
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
|
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
estebank
commented
Jun 2, 2021
Comment on lines
+61
to
+78
| ty::AssocKind::Type => { | ||
| let default = match tcx.hir().get_if_local(self.def_id) { | ||
| Some( | ||
| hir::Node::TraitItem(hir::TraitItem { | ||
| kind: hir::TraitItemKind::Type(_, Some(ty)), | ||
| .. | ||
| }) | ||
| | hir::Node::ImplItem(hir::ImplItem { | ||
| kind: hir::ImplItemKind::TyAlias(ty), | ||
| .. | ||
| }), | ||
| ) => { | ||
| format!(" = {:?}", ty) | ||
| } | ||
| _ => String::new(), | ||
| }; | ||
| format!("type {}{};", self.ident, default) | ||
| } |
There was a problem hiding this comment.
This doesn't have a test. It wasn't meant to be in this commit 🤦
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
type-trait-bound-span-2
branch
from
4a0568e to
974d94a
Compare
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
type-trait-bound-span-2
branch
from
974d94a to
d0b8678
Compare
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
type-trait-bound-span-2
branch
from
d0b8678 to
996225c
Compare
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
type-trait-bound-span-2
branch
from
996225c to
f0086ed
Compare
This comment has been minimized.
This comment has been minimized.
estebank
force-pushed
the
type-trait-bound-span-2
branch
from
f0086ed to
3b04c9f
Compare
This was referenced Jun 3, 2021
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #85954) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@bors try @rust-timer queue This should likely not be merged as is: the approach of signaling on the predicate isn't great. |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Jun 4, 2021
…r=estebank don't suggest unsized indirection in where-clauses Skip where-clauses when suggesting using indirection in combination with `?Sized` bounds on type parameters. Fixes rust-lang#85943. `@estebank` I think this doesn't conflict with your work in rust-lang#85947; please let me know if you'd like me to cherry pick it to a new branch based on yours instead.
estebank
force-pushed
the
type-trait-bound-span-2
branch
2 times, most recently
from
956b415 to
ee9375f
Compare
WIP: ordering of `Sized` obligations can affect the presence of the note. Some tweaks needed to make this the best it can be.
…ype to its own note
estebank
force-pushed
the
type-trait-bound-span-2
branch
from
ee9375f to
3956224
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
|
⌛ Trying commit 3956224 with merge 847b2268d073e67a8a5f3bebe53ca5846b2fcbe9... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Try build successful - checks-actions |
|
Queued 847b2268d073e67a8a5f3bebe53ca5846b2fcbe9 with parent 46ad16b, future comparison URL. |
|
Finished benchmarking try commit (847b2268d073e67a8a5f3bebe53ca5846b2fcbe9): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
I'm sorry if this is completely off base; I am not completely certain about my understanding of AST lowering and HIR.
To me, this looked like a HIR invariant being violated. I think the Also, I think the existing code that moves the On a hunch, I dumped the and got something including this (note the repeated (param name)(span; path resolution)(path resolution; trailing fields)(trailing fields)(leading fields; bounded_ty)(span; path resolution)(path resolution; trailing fields)(trailing fields) |
|
Also, I confirmed a build failure for the stage1
and success with
Same assertion failure: during and it looks like f338867 did change some relevant AST lowering code. |
jackh726
added
the
S-blocked
JohnCSimon
added
S-waiting-on-review
JohnTitor
removed
the
S-waiting-on-review
|
Closing as #85799 has all the commits here, I'll try to land everything together, and split it if asked by reviewer. |
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Oct 15, 2021
…stebank move implicit `Sized` predicate to end of list In `Bounds::predicates()`, move the implicit `Sized` predicate to the end of the generated list. This means that if there is an explicit `Sized` bound, it will be checked first, and any resulting diagnostics will have a more useful span. Fixes rust-lang#85998, at least partially. ~~Based on rust-lang#85979, but only the last 2 commits are new for this pull request.~~ (edit: rebased) A full fix would need to deal with where-clauses, and that seems difficult. Basically, predicates are being collected in multiple stages, and there are two places where implicit `Sized` predicates can be inserted: once for generic parameters, and once for where-clauses. I think this insertion is happening too early, and we should actually do it only at points where we collect all of the relevant trait bounds for a type parameter. I could use some help interpreting the changes to the stderr output. It looks like reordering the predicates changed some diagnostics that don't obviously have anything to do with `Sized` bounds. Possibly some error reporting code is making assumptions about ordering of predicates? The diagnostics for src/test/ui/derives/derives-span-Hash-*.rs seem to have improved, no longer pointing at the type parameter identifier, but src/test/ui/type-alias-impl-trait/generic_duplicate_param_use9.rs became less verbose for some reason. I also ran into an instance of rust-lang#84970 while working on this, but I kind of expected that could happen, because I'm reordering predicates. I can open a separate issue on that if it would be helpful. `@estebank` this seems likely to conflict (slightly?) with your work on rust-lang#85947; how would you like to resolve that?
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 15, 2021
…stebank move implicit `Sized` predicate to end of list In `Bounds::predicates()`, move the implicit `Sized` predicate to the end of the generated list. This means that if there is an explicit `Sized` bound, it will be checked first, and any resulting diagnostics will have a more useful span. Fixes rust-lang#85998, at least partially. ~~Based on rust-lang#85979, but only the last 2 commits are new for this pull request.~~ (edit: rebased) A full fix would need to deal with where-clauses, and that seems difficult. Basically, predicates are being collected in multiple stages, and there are two places where implicit `Sized` predicates can be inserted: once for generic parameters, and once for where-clauses. I think this insertion is happening too early, and we should actually do it only at points where we collect all of the relevant trait bounds for a type parameter. I could use some help interpreting the changes to the stderr output. It looks like reordering the predicates changed some diagnostics that don't obviously have anything to do with `Sized` bounds. Possibly some error reporting code is making assumptions about ordering of predicates? The diagnostics for src/test/ui/derives/derives-span-Hash-*.rs seem to have improved, no longer pointing at the type parameter identifier, but src/test/ui/type-alias-impl-trait/generic_duplicate_param_use9.rs became less verbose for some reason. I also ran into an instance of rust-lang#84970 while working on this, but I kind of expected that could happen, because I'm reordering predicates. I can open a separate issue on that if it would be helpful. ``@estebank`` this seems likely to conflict (slightly?) with your work on rust-lang#85947; how would you like to resolve that?
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.
Built on top of #85799. Only last two commits are relevant.