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

Attributes used with derive have incorrect span information #47941

Closed
sgrif opened this issue Feb 1, 2018 · 8 comments · Fixed by #52536
Closed

Attributes used with derive have incorrect span information #47941

sgrif opened this issue Feb 1, 2018 · 8 comments · Fixed by #52536
Labels
A-attributes Area: #[attributes(..)] A-macros-1.2 Area: Declarative macros 1.2 A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@sgrif
Copy link
Contributor

sgrif commented Feb 1, 2018

Given the following struct:

#[derive(AsChangeset)]
#[table_name = "users"]
struct UserForm {
    id: i32,
    name: String,
}

the input to the custom derive will have incorrect span information for every token in the table_name attribute except for the pound. Every other token will have the span Span { lo: BytePos(0), hi: BytePos(0), ctxt: #0 }.

@sgrif sgrif changed the title Attributes have incorrect span information Attributes used with derive have incorrect span information Feb 1, 2018
@gsollazzo gsollazzo added A-attributes Area: #[attributes(..)] T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Feb 1, 2018
@dtolnay dtolnay added the A-macros-2.0 Area: Declarative macros 2.0 (#39412) label Feb 2, 2018
@dtolnay
Copy link
Member

dtolnay commented Feb 2, 2018

I was able to reproduce this with the following macro on rustc 1.25.0-nightly (56733bc 2018-02-01). The four tokens in the attribute, [...], table_name, =, "users", all have span Span { lo: BytePos(0), hi: BytePos(0), ctxt: #0 }.

src/main.rs

#![feature(proc_macro)]

extern crate repro;
use repro::AsChangeset;

#[derive(AsChangeset)]
#[table_name = "users"]
struct UserForm {
    id: i32,
    name: String,
}

fn main() {}

src/lib.rs

#![feature(proc_macro)]

extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(AsChangeset, attributes(table_name))]
pub fn derive(input: TokenStream) -> TokenStream {
    for tt in input {
        println!("{:#?}", tt);
    }
    TokenStream::empty()
}

@sgrif
Copy link
Contributor Author

sgrif commented Feb 2, 2018

The span of the pound token is wrong as well. An error on that span shows the right source code, but has the wrong file/line numbers displayed.

error: `treat_none_as_null` must be in the form `treat_none_as_null = "true"`
 --> <macro expansion>:1:1
  |
1 | #[changeset_options(treat_none_as_null("true"))]
  | ^

sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 2, 2018
This was a pretty straightforward rewrite. Overall I feel much better
about this code structure than what we had before. We're having to do a
bit of funkyness to avoid
rust-lang/rust#47941, but other than that
everything here was pretty straightforward. I had the UI tests guide me
the entire time.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 2, 2018
The actual derive itself is pretty straightforward (this isn't exactly a
complex trait). Most of the tests were brought over from the macro
tests. I dropped a few that were obviously redundant (that were testing
our struct parser not the derive). I've added a few for things that
weren't working before (and were actually broken) like tuple structs.

This does contain what is technically a breaking change in behavior. The
old implementation would look for the PK fields by field name rather
than column name. I consider this a bug. Everything else links by column
name, and if we actually went by field name, tuple structs couldn't
work.

I didn't add explicit UI tests for the table not being in scope, or for
bad syntax, as those are redundant with AsChangeset. We did have to do a
little bit of dancing to work around
rust-lang/rust#47941 when the PK names cause
an error.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 3, 2018
We still have to work around
rust-lang/rust#47941 until it gets fixed
upstream, but we can at least do it in a way that's less pervasive, and
will be easier to remove in the future.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 3, 2018
This was a pretty straightforward rewrite. Overall I feel much better
about this code structure than what we had before. We're having to do a
bit of funkyness to avoid
rust-lang/rust#47941, but other than that
everything here was pretty straightforward. I had the UI tests guide me
the entire time.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 3, 2018
We still have to work around
rust-lang/rust#47941 until it gets fixed
upstream, but we can at least do it in a way that's less pervasive, and
will be easier to remove in the future.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 3, 2018
This was a pretty straightforward rewrite. Overall I feel much better
about this code structure than what we had before. We're having to do a
bit of funkyness to avoid
rust-lang/rust#47941, but other than that
everything here was pretty straightforward. I had the UI tests guide me
the entire time.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 3, 2018
We still have to work around
rust-lang/rust#47941 until it gets fixed
upstream, but we can at least do it in a way that's less pervasive, and
will be easier to remove in the future.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 4, 2018
We still have to work around
rust-lang/rust#47941 until it gets fixed
upstream, but we can at least do it in a way that's less pervasive, and
will be easier to remove in the future.
sgrif added a commit to diesel-rs/diesel that referenced this issue Feb 4, 2018
We still have to work around
rust-lang/rust#47941 until it gets fixed
upstream, but we can at least do it in a way that's less pervasive, and
will be easier to remove in the future.
@sgrif
Copy link
Contributor Author

sgrif commented May 2, 2018

This has regressed significantly on recent nightlies. All tokens except for the first token have the useless dummy span, and the first token still has the incorrect file and line number.

sgrif added a commit to diesel-rs/diesel that referenced this issue May 2, 2018
parking_lot decided to bump their minimum required Rust version in a
patch release, which means that we have to update to a newer nightly.
While this pretty much always means updating our clippy version,
the version of `proc_macro2` we were using stopped working. So we had to
bump syn as well...

It appears that rust-lang/rust#47941 has
gotten worse, so a lot of our error messages have regressed on newer
nightlies.

I also bumped rustfmt because why not.
sgrif added a commit to diesel-rs/diesel that referenced this issue May 2, 2018
parking_lot decided to bump their minimum required Rust version in a
patch release, which means that we have to update to a newer nightly.
While this pretty much always means updating our clippy version,
the version of `proc_macro2` we were using stopped working. So we had to
bump syn as well...

It appears that rust-lang/rust#47941 has
gotten worse, so a lot of our error messages have regressed on newer
nightlies.

I also bumped rustfmt because why not.
sgrif added a commit to diesel-rs/diesel that referenced this issue May 3, 2018
parking_lot decided to bump their minimum required Rust version in a
patch release, which means that we have to update to a newer nightly.
While this pretty much always means updating our clippy version,
the version of proc_macro2 we were using stopped working. So we had to
bump syn as well...

It appears that rust-lang/rust#47941 has
gotten worse, so a lot of our error messages have regressed on newer
nightlies.
@alexcrichton alexcrichton added the A-macros-1.2 Area: Declarative macros 1.2 label May 22, 2018
@alexcrichton
Copy link
Member

This is caused by #43081, specifically this block of code

@peterhuene
Copy link

I suspect #52162 is a dupe of this issue? I'll close.

@sgrif
Copy link
Contributor Author

sgrif commented Jul 18, 2018

This has continued to regress. In the most recent nightly, not even the pound token (first token) of the attribute has a meaningful span.

@peterhuene
Copy link

peterhuene commented Jul 18, 2018

Indeed, that was the behavior I was observing in my repro (https://github.com/peterhuene/inner-attr-span-repro).

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 19, 2018
This commit updates the tokenization of items which are subsequently passed to
`proc_macro` to ensure that span information is preserved on attributes as much
as possible. Previously this area of the code suffered from rust-lang#43081 where we
haven't actually implemented converting an attribute to to a token tree yet, but
a local fix was possible here.

Closes rust-lang#47941
pietroalbini added a commit to pietroalbini/rust that referenced this issue Jul 20, 2018
…akis

proc_macro: Preserve spans of attributes on functions

This commit updates the tokenization of items which are subsequently passed to
`proc_macro` to ensure that span information is preserved on attributes as much
as possible. Previously this area of the code suffered from rust-lang#43081 where we
haven't actually implemented converting an attribute to to a token tree yet, but
a local fix was possible here.

Closes rust-lang#47941
pietroalbini added a commit to pietroalbini/rust that referenced this issue Jul 20, 2018
…akis

proc_macro: Preserve spans of attributes on functions

This commit updates the tokenization of items which are subsequently passed to
`proc_macro` to ensure that span information is preserved on attributes as much
as possible. Previously this area of the code suffered from rust-lang#43081 where we
haven't actually implemented converting an attribute to to a token tree yet, but
a local fix was possible here.

Closes rust-lang#47941
bors added a commit that referenced this issue Jul 21, 2018
proc_macro: Preserve spans of attributes on functions

This commit updates the tokenization of items which are subsequently passed to
`proc_macro` to ensure that span information is preserved on attributes as much
as possible. Previously this area of the code suffered from #43081 where we
haven't actually implemented converting an attribute to to a token tree yet, but
a local fix was possible here.

Closes #47941
@peterhuene
Copy link

I've verified the fix in the latest nightly. Thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] A-macros-1.2 Area: Declarative macros 1.2 A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-bug Category: This is a bug. 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.

5 participants