-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Do not suggest adding semicolon/changing delimiters for macros in item position that originates in macros #97377
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
Thanks for the PR, @ChayimFriedman2! That FIXME suggests that there's a diagnostics design decision to be made here. |
} else { | ||
// FIXME: This will make us not emit the help even for declarative | ||
// macros within the same crate (that we can fix), which is sad. | ||
if !span.from_expansion() { |
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.
What happens if you instead use the following? Is the FIXME still applicable then? I suspect it might not be.
if !span.from_expansion() { | |
if !span.can_be_used_for_suggestions() { |
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 doesn't work. can_be_used_for_suggestions()
only checks for derive macros where the span they emit is different from the span of themselves. This will both provide incorrect suggestions for those macros and still fail as with the FIXME.
rust/compiler/rustc_span/src/lib.rs
Lines 575 to 583 in 49c82f3
/// Gate suggestions that would not be appropriate in a context the user didn't write. | |
pub fn can_be_used_for_suggestions(self) -> bool { | |
!self.from_expansion() | |
// FIXME: If this span comes from a `derive` macro but it points at code the user wrote, | |
// the callsite span and the span will be pointing at different places. It also means that | |
// we can safely provide suggestions on this span. | |
|| (matches!(self.ctxt().outer_expn_data().kind, ExpnKind::Macro(MacroKind::Derive, _)) | |
&& self.parent_callsite().map(|p| (p.lo(), p.hi())) != Some((self.lo(), self.hi()))) | |
} |
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.
What about using in_derive_expansion
and only gate on those, then?
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've thought about that, but attribute macros should not suggest help either; and probably neither should declarative macros from separate crates.
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 then checks for !matches!(kind, ExprKind::Macro(MacroKind::Derive | MacroKind::Attr, _))
and using tcx.sess().source_map().lookup_char_pos(span.lo()).file
on both the suggestion span and the callsite span to see if they belong to the same file?
Also, an additional test for those cases so that we see when the suggestion should appear in the same file might be useful. That might be easy to do for a macro by example. You might need to expand the test to also have an "external crate" example to check for that case too.
Would you have time to get these things done in this PR?
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.
Soon. But should it be the same file or same crate?
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.
And what about function-like proc macros? How can I differentiate those?
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.
Ah, you're right. @petrochenkov what would the best way of checking that a span doesn't correspond to a proc-macro of any type nor a macro by example from a foreign crate, while parsing an item?
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.
@petrochenkov friendly ping, if you have advice re: ^
I'm ok with landing with if !span.from_expansion() {
, at least for now. Would you mind rebasing @ChayimFriedman2?
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.
@estebank ^^^ Done.
☔ The latest upstream changes (presumably #98066) made this pull request unmergeable. Please resolve the merge conflicts. |
498dc12
to
01ff48b
Compare
…m position that originates in macros
01ff48b
to
0ef4098
Compare
@bors r+ |
📌 Commit 0ef4098 has been approved by |
Rollup of 5 pull requests Successful merges: - rust-lang#97377 (Do not suggest adding semicolon/changing delimiters for macros in item position that originates in macros) - rust-lang#97675 (Make `std::mem::needs_drop` accept `?Sized`) - rust-lang#98118 (Test NLL fix of bad lifetime inference for reference captured in closure.) - rust-lang#98166 (Add rustdoc-json regression test for rust-lang#98009) - rust-lang#98169 (Keyword docs: Link to wikipedia article for dynamic dispatch) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #91800.