-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
#56411 do not suggest a fix for a import conflict in a macro #56937
Conversation
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
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.
One nitpick, otherwise fine by me.
|
||
struct T {} | ||
|
||
fn main() {} |
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.
These files usually go in an auxiliary
directory. I'm ok with this if @petrochenkov is too.
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.
yes, but // aux-build:
would allow to build it as an extern crate, not to use it as a module. And the auxiliary
folder doesn't contain a mod.rs
file, so you can't import a module from 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.
Could you rename the file into something less subtle than "same file, but with underscore", e.g. issue_56411_aux.rs
?
) = ( | ||
cm.span_to_snippet(binding.span), | ||
binding.kind.clone(), | ||
binding.span.is_dummy(), | ||
binding.span.ctxt().outer().expn_info().is_some(), |
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.
If you rely on source.span.hi().0 - binding.span.lo().0
being a valid index in binding.span
, then you should check at least the next conditions - !source.span.is_dummy() && !binding.span.is_dummy() && !from_macro(source.span) && !from_macro(binding.span)
.
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.
Actually, directly checking that source.span.hi().0 - binding.span.lo().0
doesn't underflow and results in a valid index into cm
would be the most reliable alternative.
This way we certainly don't get an ICE, but also keep the diagnostics for "good" macros.
ping from triage @mockersf waiting for your reply on the review comments |
Ping from triage @mockersf: Some changes have been requested on this PR. Do you think you'll be able to take care of those? |
Rebased and fixed in #57908 |
resolve: Fix span arithmetics in the import conflict error rust-lang#56937 rebased and fixed Fixes rust-lang#56411 Fixes rust-lang#57071 Fixes rust-lang#57787 r? @estebank
resolve: Fix span arithmetics in the import conflict error rust-lang#56937 rebased and fixed Fixes rust-lang#56411 Fixes rust-lang#57071 Fixes rust-lang#57787 r? @estebank
resolve: Fix span arithmetics in the import conflict error rust-lang#56937 rebased and fixed Fixes rust-lang#56411 Fixes rust-lang#57071 Fixes rust-lang#57787 r? @estebank
as a first fix, remove the suggestion when the conflict happens in a macro as it will be useless