-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add E0790 as more specific variant of E0283 #98028
Conversation
Some changes occurred in diagnostic error codes |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #96591) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #98222) made this pull request unmergeable. Please resolve the merge conflicts. |
Some changes occurred in diagnostic error codes |
The error explanation looks good to me. Let's wait for @estebank for the approval on the compiler change. |
☔ The latest upstream changes (presumably #98910) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #99054) made this pull request unmergeable. Please resolve the merge conflicts. |
I've slightly changed the structure of the code now in hope to make the diff simpler, but nothing fundamentally changed in case you already started looking into this @estebank |
@bors r+ |
Add E0789 as more specific variant of E0283 Fixes rust-lang#81701 I think this should be good to go, there are only two things where I am somewhat unsure: - Is there a better way to get the fully-qualified path for the suggestion? I tried `self.tcx.def_path_str`, but that didn't seem to always give a correct path for the context. - Should all this be extracted into it's own method or is it fine where it is? r? `@estebank`
failed in rollup ci |
@bors r- |
Rebased and changed the error code to E0790. |
@bors r=estebank |
…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#98028 (Add E0790 as more specific variant of E0283) - rust-lang#99384 (use body's param-env when checking if type needs drop) - rust-lang#99401 (Avoid `Symbol` to `&str` conversions) - rust-lang#99419 (Stabilize `core::task::ready!`) - rust-lang#99435 (Revert "Stabilize $$ in Rust 1.63.0") - rust-lang#99438 (Improve suggestions for `NonZeroT` <- `T` coercion error) - rust-lang#99441 (Update mdbook) - rust-lang#99453 (:arrow_up: rust-analyzer) - rust-lang#99457 (use `par_for_each_in` in `par_body_owners` and `collect_crate_mono_items`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
if let (Some(body_id), Some(ty::subst::GenericArgKind::Type(_))) = | ||
(body_id, subst.map(|subst| subst.unpack())) | ||
{ | ||
struct FindExprBySpan<'hir> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could introduce a new ObligationCauseCode
that stores the hir id of the ExprKind::Path
that this obligation comes from, so we wouldn't need to use this span-based search method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea.
| | ||
= note: cannot satisfy `_: Generator` | ||
LL | let cont: u32 = <::Impl as Generator>::create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ::Impl
path is kinda funky, is there a reason we used to_string_no_crate_verbose
instead of just tcx.def_path_str
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall exactly anymore, but if I remember correctly I tried that and got paths as a result that didn't make sense in the context where they were suggested. But I could have made a mistake then. If you'd like I could try it again when I find the time and make a follow-up PR with that or at least find out why it didn't work in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, either put up a follow up or I can also look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I was involved in the development of this PR)
I think it was along the lines of it returning Impl
instead of the context-sensitive value inner::Impl
in the case of nested modules.
@compiler-errors Yes, this does fix #98938 as well, I just double checked. The resulting error message in that case is
|
Fixes #81701
I think this should be good to go, there are only two things where I am somewhat unsure:
self.tcx.def_path_str
, but that didn't seem to always give a correct path for the context.r? @estebank