Skip to content

Macro diagnostics tweaks #55292

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.

Sign up for GitHub

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 3 commits into from
Oct 26, 2018
Merged

Macro diagnostics tweaks #55292

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8544db0
Add macro call span when lacking any other span in diagnostic
estebank Oct 23, 2018
ad144ac
Modify invalid macro in expression context diagnostic
estebank Oct 23, 2018
f8818cb
Fix incorrect semicolon suggestion
estebank Oct 24, 2018
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
24 changes: 20 additions & 4 deletions src/libsyntax/ext/expand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,10 +1036,26 @@ impl<'a> Parser<'a> {
// Avoid emitting backtrace info twice.
let def_site_span = self.span.with_ctxt(SyntaxContext::empty());
let mut err = self.diagnostic().struct_span_err(def_site_span, &msg);
let msg = format!("caused by the macro expansion here; the usage \
of `{}!` is likely invalid in {} context",
macro_path, kind_name);
err.span_note(span, &msg).emit();
err.span_label(span, "caused by the macro expansion here");
let msg = format!(
"the usage of `{}!` is likely invalid in {} context",
macro_path,
kind_name,
);
err.note(&msg);
let semi_span = self.sess.source_map().next_point(span);
match self.sess.source_map().span_to_snippet(semi_span) {
Ok(ref snippet) if &snippet[..] != ";" && kind_name == "expression" => {
err.span_suggestion_with_applicability(
semi_span,
"you might be missing a semicolon here",
";".to_owned(),
Applicability::MaybeIncorrect,
);
}
_ => {}
}
err.emit();
}
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/libsyntax/ext/tt/macro_rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@ pub struct ParserAnyMacro<'a> {
impl<'a> ParserAnyMacro<'a> {
pub fn make(mut self: Box<ParserAnyMacro<'a>>, kind: AstFragmentKind) -> AstFragment {
let ParserAnyMacro { site_span, macro_ident, ref mut parser } = *self;
let fragment = panictry!(parser.parse_ast_fragment(kind, true));
let fragment = panictry!(parser.parse_ast_fragment(kind, true).map_err(|mut e| {
if e.span.is_dummy() { // Get around lack of span in error (#30128)
e.set_span(site_span);
}
e
}));

// We allow semicolons at the end of expressions -- e.g. the semicolon in
// `macro_rules! m { () => { panic!(); } }` isn't parsed by `.parse_expr()`,
Expand Down
11 changes: 11 additions & 0 deletions src/libsyntax_pos/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,17 @@ impl MultiSpan {
&self.primary_spans
}

/// Returns `true` if this contains only a dummy primary span with any hygienic context.
pub fn is_dummy(&self) -> bool {
let mut is_dummy = true;
for span in &self.primary_spans {
if !span.is_dummy() {
is_dummy = false;
}
}
is_dummy
}

/// Replaces all occurrences of one Span with another. Used to move Spans in areas that don't
/// display well (like std macros). Returns true if replacements occurred.
pub fn replace(&mut self, before: Span, after: Span) -> bool {
Expand Down
9 changes: 4 additions & 5 deletions src/test/ui/issues/issue-30007.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ error: macro expansion ignores token `;` and any following
|
LL | () => ( String ; ); //~ ERROR macro expansion ignores token `;`
| ^
|
note: caused by the macro expansion here; the usage of `t!` is likely invalid in type context
--> $DIR/issue-30007.rs:16:16
|
...
LL | let i: Vec<t!()>;
| ^^^^
| ---- caused by the macro expansion here
|
= note: the usage of `t!` is likely invalid in type context

error: aborting due to previous error

29 changes: 14 additions & 15 deletions src/test/ui/macros/macro-context.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,35 @@ error: macro expansion ignores token `;` and any following
|
LL | () => ( i ; typeof ); //~ ERROR expected expression, found reserved keyword `typeof`
| ^
|
note: caused by the macro expansion here; the usage of `m!` is likely invalid in type context
--> $DIR/macro-context.rs:20:12
|
...
LL | let a: m!();
| ^^^^
| ---- caused by the macro expansion here
|
= note: the usage of `m!` is likely invalid in type context

error: macro expansion ignores token `typeof` and any following
--> $DIR/macro-context.rs:13:17
|
LL | () => ( i ; typeof ); //~ ERROR expected expression, found reserved keyword `typeof`
| ^^^^^^
|
note: caused by the macro expansion here; the usage of `m!` is likely invalid in expression context
--> $DIR/macro-context.rs:21:13
|
...
LL | let i = m!();
| ^^^^
| ----- help: you might be missing a semicolon here: `;`
Copy link
Member

@pnkfelix pnkfelix Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can we not peek ahead to the source text immediately following the span and see if there's a semicolon as the next character before making such an obviously inapplicable help message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're actually checking that, which makes me intrigued at why this isn't being picked up. My guess is that if spans were completely hygienic this suggestion would appear on line 13, after typeof. This seems to be the only edge-case caught by our tests (before I added the linked check, many other incorrect suggestions were being emitted). Given that, would you mind if we merge as is and add extra logic in a later PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

| |
| caused by the macro expansion here
|
= note: the usage of `m!` is likely invalid in expression context

error: macro expansion ignores token `;` and any following
--> $DIR/macro-context.rs:13:15
|
LL | () => ( i ; typeof ); //~ ERROR expected expression, found reserved keyword `typeof`
| ^
|
note: caused by the macro expansion here; the usage of `m!` is likely invalid in pattern context
--> $DIR/macro-context.rs:23:9
|
...
LL | m!() => {}
| ^^^^
| ---- caused by the macro expansion here
|
= note: the usage of `m!` is likely invalid in pattern context

error: expected expression, found reserved keyword `typeof`
--> $DIR/macro-context.rs:13:17
Expand Down
7 changes: 7 additions & 0 deletions src/test/ui/macros/macro-in-expression-context-2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
macro_rules! empty { () => () }

fn main() {
match 42 {
_ => { empty!() }
};
}
8 changes: 8 additions & 0 deletions src/test/ui/macros/macro-in-expression-context-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: expected expression, found `<eof>`
--> $DIR/macro-in-expression-context-2.rs:5:16
|
LL | _ => { empty!() }
| ^^^^^^^^

error: aborting due to previous error

15 changes: 15 additions & 0 deletions src/test/ui/macros/macro-in-expression-context.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// run-rustfix

macro_rules! foo {
() => {
assert_eq!("A", "A");
assert_eq!("B", "B");
}
//~^^ ERROR macro expansion ignores token `assert_eq` and any following
//~| NOTE the usage of `foo!` is likely invalid in expression context
}

fn main() {
foo!();
//~^ NOTE caused by the macro expansion here
}
15 changes: 15 additions & 0 deletions src/test/ui/macros/macro-in-expression-context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// run-rustfix

macro_rules! foo {
() => {
assert_eq!("A", "A");
assert_eq!("B", "B");
}
//~^^ ERROR macro expansion ignores token `assert_eq` and any following
//~| NOTE the usage of `foo!` is likely invalid in expression context
}

fn main() {
foo!()
//~^ NOTE caused by the macro expansion here
}
15 changes: 15 additions & 0 deletions src/test/ui/macros/macro-in-expression-context.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: macro expansion ignores token `assert_eq` and any following
--> $DIR/macro-in-expression-context.rs:6:9
|
LL | assert_eq!("B", "B");
| ^^^^^^^^^
...
LL | foo!()
| ------- help: you might be missing a semicolon here: `;`
| |
| caused by the macro expansion here
|
= note: the usage of `foo!` is likely invalid in expression context
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dtolnay can you take a look and verify wether this suggestion is reasonable (it won't be wrong more often than not)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one case where it'd be the wrong approach:

fn main() {
    macro_rules! a {
        ($e:expr) => { $e; 2}
    }
    a!(true);
}

error: macro expansion ignores token `2` and any following
 --> file.rs:3:28
  |
3 |         ($e:expr) => { $e; 2}
  |                            ^
4 |     }
5 |     a!(true)
  |     --------- help: you might be missing a semicolon here: `;`
  |     |
  |     caused by the macro expansion here
  |
  = note: the usage of `a!` is likely invalid in expression context

but the suggestion actually gets the code to compile...

Copy link
Member

Choose a reason for hiding this comment

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

I think this suggestion will be helpful. 👍


error: aborting due to previous error

18 changes: 8 additions & 10 deletions src/test/ui/parser/macro/macro-incomplete-parse.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ error: macro expansion ignores token `,` and any following
|
LL | , //~ ERROR macro expansion ignores token `,`
| ^
|
note: caused by the macro expansion here; the usage of `ignored_item!` is likely invalid in item context
--> $DIR/macro-incomplete-parse.rs:31:1
|
...
LL | ignored_item!();
| ^^^^^^^^^^^^^^^^
| ---------------- caused by the macro expansion here
|
= note: the usage of `ignored_item!` is likely invalid in item context

error: expected one of `.`, `;`, `?`, `}`, or an operator, found `,`
--> $DIR/macro-incomplete-parse.rs:22:14
Expand All @@ -24,12 +23,11 @@ error: macro expansion ignores token `,` and any following
|
LL | () => ( 1, 2 ) //~ ERROR macro expansion ignores token `,`
| ^
|
note: caused by the macro expansion here; the usage of `ignored_pat!` is likely invalid in pattern context
--> $DIR/macro-incomplete-parse.rs:36:9
|
...
LL | ignored_pat!() => (),
| ^^^^^^^^^^^^^^
| -------------- caused by the macro expansion here
|
= note: the usage of `ignored_pat!` is likely invalid in pattern context

error: aborting due to 3 previous errors