Skip to content

Do not suggest adding semicolon/changing delimiters for macros in item position that originates in macros #97377

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

Merged
merged 1 commit into from
Jun 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 25 additions & 21 deletions compiler/rustc_parse/src/parser/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1775,30 +1775,34 @@ impl<'a> Parser<'a> {
span,
"macros that expand to items must be delimited with braces or followed by a semicolon",
);
if self.unclosed_delims.is_empty() {
let DelimSpan { open, close } = match args {
MacArgs::Empty | MacArgs::Eq(..) => unreachable!(),
MacArgs::Delimited(dspan, ..) => *dspan,
};
err.multipart_suggestion(
"change the delimiters to curly braces",
vec![(open, "{".to_string()), (close, '}'.to_string())],
Applicability::MaybeIncorrect,
);
} 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() {
Copy link
Contributor

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.

Suggested change
if !span.from_expansion() {
if !span.can_be_used_for_suggestions() {

Copy link
Contributor Author

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.

/// 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())))
}

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank ^^^ Done.

if self.unclosed_delims.is_empty() {
let DelimSpan { open, close } = match args {
MacArgs::Empty | MacArgs::Eq(..) => unreachable!(),
MacArgs::Delimited(dspan, ..) => *dspan,
};
err.multipart_suggestion(
"change the delimiters to curly braces",
vec![(open, "{".to_string()), (close, '}'.to_string())],
Applicability::MaybeIncorrect,
);
} else {
err.span_suggestion(
span,
"change the delimiters to curly braces",
" { /* items */ }",
Applicability::HasPlaceholders,
);
}
err.span_suggestion(
span,
"change the delimiters to curly braces",
" { /* items */ }",
Applicability::HasPlaceholders,
span.shrink_to_hi(),
"add a semicolon",
';',
Applicability::MaybeIncorrect,
);
}
err.span_suggestion(
span.shrink_to_hi(),
"add a semicolon",
';',
Applicability::MaybeIncorrect,
);
err.emit();
}

Expand Down
26 changes: 26 additions & 0 deletions src/test/ui/proc-macro/auxiliary/issue-91800-macro.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// force-host
// no-prefer-dynamic

#![crate_type = "proc-macro"]

extern crate proc_macro;

use proc_macro::TokenStream;

fn compile_error() -> TokenStream {
r#"compile_error!("")"#.parse().unwrap()
}

#[proc_macro_derive(MyTrait)]
pub fn derive(input: TokenStream) -> TokenStream {
compile_error()
}
#[proc_macro_attribute]
pub fn attribute_macro(_attr: TokenStream, mut input: TokenStream) -> TokenStream {
input.extend(compile_error());
input
}
#[proc_macro]
pub fn fn_macro(_item: TokenStream) -> TokenStream {
compile_error()
}
16 changes: 16 additions & 0 deletions src/test/ui/proc-macro/issue-91800.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// aux-build: issue-91800-macro.rs

#[macro_use]
extern crate issue_91800_macro;

#[derive(MyTrait)]
//~^ ERROR macros that expand to items must be delimited with braces or followed by a semicolon
//~| ERROR proc-macro derive produced unparseable tokens
#[attribute_macro]
//~^ ERROR macros that expand to items must be delimited with braces or followed by a semicolon
struct MyStruct;

fn_macro! {}
//~^ ERROR macros that expand to items must be delimited with braces or followed by a semicolon

fn main() {}
56 changes: 56 additions & 0 deletions src/test/ui/proc-macro/issue-91800.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
error: macros that expand to items must be delimited with braces or followed by a semicolon
--> $DIR/issue-91800.rs:6:10
|
LL | #[derive(MyTrait)]
| ^^^^^^^
|
= note: this error originates in the derive macro `MyTrait` (in Nightly builds, run with -Z macro-backtrace for more info)

error: proc-macro derive produced unparseable tokens
--> $DIR/issue-91800.rs:6:10
|
LL | #[derive(MyTrait)]
| ^^^^^^^

error:
--> $DIR/issue-91800.rs:6:10
|
LL | #[derive(MyTrait)]
| ^^^^^^^
|
= note: this error originates in the derive macro `MyTrait` (in Nightly builds, run with -Z macro-backtrace for more info)

error: macros that expand to items must be delimited with braces or followed by a semicolon
--> $DIR/issue-91800.rs:9:1
|
LL | #[attribute_macro]
| ^^^^^^^^^^^^^^^^^^
|
= note: this error originates in the attribute macro `attribute_macro` (in Nightly builds, run with -Z macro-backtrace for more info)

error:
--> $DIR/issue-91800.rs:9:1
|
LL | #[attribute_macro]
| ^^^^^^^^^^^^^^^^^^
|
= note: this error originates in the attribute macro `attribute_macro` (in Nightly builds, run with -Z macro-backtrace for more info)

error: macros that expand to items must be delimited with braces or followed by a semicolon
--> $DIR/issue-91800.rs:13:1
|
LL | fn_macro! {}
| ^^^^^^^^^^^^
|
= note: this error originates in the macro `fn_macro` (in Nightly builds, run with -Z macro-backtrace for more info)

error:
--> $DIR/issue-91800.rs:13:1
|
LL | fn_macro! {}
| ^^^^^^^^^^^^
|
= note: this error originates in the macro `fn_macro` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 7 previous errors