Skip to content

Conversation

@sgrif
Copy link
Member

@sgrif sgrif commented Feb 1, 2018

When proc_macro2 has the nightly feature enabled, a lot of the rules
around hygiene change. Most of the changes needed appear to be due to
bugs in Rust (e.g. to access a tuple struct field, the span of the dot has to
be resolved specially, but not for a named struct field), but the span
changes for table names in particular is definitely required.

The change to hygiene rules randomly broke our const workaround (the
ability to use items defined in an expression like that was
potentially unintended to begin with). However, there's no reason it
needs to be a const, we can just have it be a module with no additional
problems.

I've added a few UI tests for the failure cases that are actually
affected by this change (there is more work to be done on this, of
course). Note that we can't actually run those tests yet, as they
require a nightly compiler, and we won't build on nightly until
tomorrow. However, I've left the tests in place so that the changes in
output can be seen.

The error for table_name isn't quite there, but due to a bug in Rust
it's the best we can do. Right now it looks like this:

1 | #[table_name = "users"]
  | ^ Use of undeclared type or module `users`

instead of like this:

1 | #[table_name = "users"]
  |                ^^^^^^^ Use of undeclared type or module `users`

When `proc_macro2` has the nightly feature enabled, a lot of the rules
around hygiene change. Most of the changes needed appear to be due to
bugs in Rust (e.g. to access a tuple struct field, the span of the dot has to
be resolved specially, but not for a named struct field), but the span
changes for table names in particular is definitely required.

The change to hygiene rules randomly broke our const workaround (the
ability to `use` items defined in an expression like that was
potentially unintended to begin with). However, there's no reason it
needs to be a const, we can just have it be a module with no additional
problems.

I've added a few UI tests for the failure cases that are actually
affected by this change (there is more work to be done on this, of
course). Note that we can't actually run those tests yet, as they
require a nightly compiler, and we won't build on nightly until
tomorrow. However, I've left the tests in place so that the changes in
output can be seen.

The error for `table_name` isn't quite there, but due to a bug in Rust
it's the best we can do. Right now it looks like this:

```
1 | #[table_name = "users"]
  | ^ Use of undeclared type or module `users`
```

instead of like this:

```
1 | #[table_name = "users"]
  |                ^^^^^^^ Use of undeclared type or module `users`
```
@sgrif sgrif requested a review from a team February 1, 2018 20:03
@sgrif
Copy link
Member Author

sgrif commented Feb 1, 2018

/cc @dtolnay

@sgrif
Copy link
Member Author

sgrif commented Feb 1, 2018

Note: Build is going to be red until tomorrow. Nothing I can do about that, but this should be fine to review

@dtolnay
Copy link

dtolnay commented Feb 1, 2018

What happens tomorrow?

@sgrif
Copy link
Member Author

sgrif commented Feb 1, 2018

rust-lang/rust#47738 will be in tomorrow's nightly.

Copy link

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Nicely done!

use proc_macro2::{Spacing, TokenNode, TokenTree};

// https://github.com/rust-lang/rust/issues/47312
tokens.append(TokenTree {
Copy link

Choose a reason for hiding this comment

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

quote_spanned! can make this more concise but it is up to you whether you prefer the explicit way or the concise way.

quote_spanned!(call_site=> .)

@sgrif sgrif merged commit 946347a into master Feb 2, 2018
@sgrif sgrif deleted the sg-as-changeset-spans branch February 2, 2018 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants