-
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
Provide existing ref suggestions for more E0308 errors #51822
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
@@ -342,11 +329,14 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
// a macro; if so, it's hard to extract the text and make a good | |||
// suggestion, so don't bother.) | |||
if self.infcx.can_sub(self.param_env, checked, &expected).is_ok() && | |||
expr.span.ctxt().outer().expn_info().is_none() { | |||
sp.ctxt().outer().expn_info().is_none() { |
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.
since we switched to call_site_if_macro
here, do we know that is_none()
will always be true?
r=me modulo the nit above; if you think we can simplify, seems good, otherwise it's fine as is |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
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.
seems good, but travis is unhappy now? r=me modulo travis and below nit
src/librustc_typeck/check/demand.rs
Outdated
@@ -328,8 +328,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
// (But, also check check the `expn_info()` to see if this is | |||
// a macro; if so, it's hard to extract the text and make a good | |||
// suggestion, so don't bother.) | |||
if self.infcx.can_sub(self.param_env, checked, &expected).is_ok() && | |||
sp.ctxt().outer().expn_info().is_none() { |
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.
(maybe we should make it assert?)
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.
No, I think it'd be better to revert that last commit. The checks are in different spans and we should only suggest if both spans are outside of macros. Originally I thought that it would make sense to suggest if the macro was local (meaning that the user could do something about it), but in that case the span already points at the problematic place, and I will assume that anyone using macros would have enough knowledge of the language to deal with an E0308 between u32
and &{integer}
. What do you think?
[00:48:07] 50 --> $DIR/deref-suggestion.rs:12:20
[00:48:07] 51 |
[00:48:07] 52 LL | ($x:expr) => { &$x } //~ ERROR mismatched types
[00:48:07] - | ^^^ expected u32, found &{integer}
[00:48:07] + | ^^^
[00:48:07] + | |
[00:48:07] + | expected u32, found &{integer}
[00:48:07] + | help: consider removing the borrow: `0`
[00:48:07] 54 ...
[00:48:07] 55 LL | foo3(borrow!(0));
[00:48:07] 56 | ---------- in this macro invocation
d12eaa0
to
d2161c7
Compare
This comment has been minimized.
This comment has been minimized.
@bors r=nikomatsakis Assuming that I'm being reasonable with #51822 (comment). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors r=nikomatsakis |
📌 Commit 54a04b3 has been approved by |
@bors rollup |
Provide existing ref suggestions for more E0308 errors
Rollup of 6 pull requests Successful merges: - #51636 (Refactor error reporting of constants) - #51765 (Use assert_eq! in copy_from_slice) - #51822 (Provide existing ref suggestions for more E0308 errors) - #51839 (Detect overflows of non u32 shifts) - #51868 (Remove process::id from 'Stabilized APIs' in 1.27.0 release notes) - #51875 (Explicitely disable WASM code generation for Emscripten) Failed merges: r? @ghost
No description provided.