Skip to content
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

Spans for Paths can be incorrect #39450

Closed
nrc opened this issue Feb 1, 2017 · 2 comments · Fixed by #40597
Closed

Spans for Paths can be incorrect #39450

nrc opened this issue Feb 1, 2017 · 2 comments · Fixed by #40597
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nrc
Copy link
Member

nrc commented Feb 1, 2017

Consider the following example (from term-painter):

macro_rules! impl_format {
    ($symbol:expr, $fmt:ident) => {
        impl<T: fmt::$fmt> fmt::$fmt for Painted<T> {
            fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
                panic!()
            }
        }
    }
}

impl_format!("{}", Display);

On the third line are two paths of the form fmt::$fmt, after expansion, these are both fmt::Display. The way spans for paths are calculated we take the start of fmt and the end of Display (see parse_path in parser.rs), this means the span for the path is 9 lines long and pretty incoherent. Annoyingly, this span does not get an expansion id, and thus has no indication that it came from generated code. This is visible in error messages:

   |
9  |           impl<T: fmt::$fmt> fmt::$fmt for Painted<T> {
   |  _________________^ starting here...
10 | |             fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
11 | |                 panic!()
12 | |             }
13 | |         }
14 | |     }
15 | | }
16 | |
17 | | impl_format!("{}", Display);
   | | -------------------------^--
   | |_|________________________|
   |   |                        ...ending here
   |   in this macro invocation
@oli-obk
Copy link
Contributor

oli-obk commented Feb 2, 2017

We've had the same issue in clippy: https://github.com/Manishearth/rust-clippy/issues/1404#issuecomment-275861252

copying over the comment:

rustc expands issue_1404!(0) to macro_tokens 0 macro_tokens. When these tokens are parsed, the parser stores a tokens position, parses on, and creates a span from the stored position to where it is now. But when you parse 0 inside those macro_tokens, you get a span from inside the macro_rules to behind 0, which makes no sense.

@jseyfried
Copy link
Contributor

cc #23480

@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
bors added a commit that referenced this issue Mar 30, 2017
macros: improve `Span`'s expansion information

This PR improves `Span`'s expansion information. More specifically:
 - It refactors AST node span construction to preserve expansion information.
   - Today, we only use the underlying tokens' `BytePos`s, throwing away the `ExpnId`s.
   - This improves the accuracy of AST nodes' expansion information, fixing #30506.
 - It refactors `span.expn_id: ExpnId` to `span.ctxt: SyntaxContext` and removes `ExpnId`.
   - This gives all tokens as much hygiene information as `Ident`s.
   - This is groundwork for procedural macros 2.0 `TokenStream` API.
   - This is also groundwork for declarative macros 2.0, which will need this hygiene information for some non-`Ident` tokens.
 - It simplifies processing of spans' expansion information throughout the compiler.
 - It fixes #40649.
 - It fixes #39450 and fixes part of #23480.

r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants