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

Support async trait bounds in macros #121044

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

compiler-errors
Copy link
Member

r? fmease

This is similar to your work on const trait bounds. This theoretically regresses impl async $ident:ident in macros, but I doubt this is occurring in practice.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 13, 2024
@compiler-errors
Copy link
Member Author

impl async Fn() was previously being parsed outside of macro contexts because of this line:

|| (self.token.is_reserved_ident() && !self.token.is_keyword(kw::Where))))

But inside of macro contexts, where may_recover() is false, we weren't parsing async bounds properly.

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

r=me if my worries prove to be false

Comment on lines 781 to 782
|| self.is_kw_followed_by_ident(kw::Const)
|| self.is_kw_followed_by_ident(kw::Async)
Copy link
Member

Choose a reason for hiding this comment

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

Very nice!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, wait doesn't this stop impl const !Trait and impl const ?Trait from parsing (semantic error on master → syntactic error on this branch)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess it does. Do we particularly care about this, though?

Copy link
Member

Choose a reason for hiding this comment

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

Personally speaking I don't care. I just remember from when I was refactoring trait bound modifiers and looking through the git history that there was a PR years ago that explicitly set out to parse invalid modifier combinations to make it a semantic error.

I'm now on mobile and can't look for it, maybe it contains a motivation.

Copy link
Member

@fmease fmease Feb 17, 2024

Choose a reason for hiding this comment

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

#68140. This is not the PR I remember lol but it does mention the piece of information I'm after:

After a discussion in #wg-grammar on Discord, it was decided that the grammar should not encode the mutual exclusivity of trait bound modifiers. The grammar for trait bound modifiers remains [?const] [?]. To encode this, I add a dummy variant to ast::TraitBoundModifier that is used when the syntax ?const ? appears. This variant causes an error in AST validation and disappears during HIR lowering.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, I've changed this back to just a check_kw call. This does regress tests/ui/parser/bad-recover-kw-after-impl.rs, but I consider that to be a same manifestation of this impl async theoretical regression that we already have a test for.

Comment on lines 566 to 567
if (self.token.span.at_least_rust_2018() && self.token.is_keyword(kw::Async))
|| (self.token.span.is_rust_2015() && self.is_kw_followed_by_ident(kw::Async))
Copy link
Member

@fmease fmease Feb 14, 2024

Choose a reason for hiding this comment

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

Is it possible that the span/edition of an Interpolated ident differs from the span/edition of the corresponding uninterpolate'd ident? In other words does this PR break the following code?

// dep.rs  :::  rustc +stage1 dep.rs --crate-type=lib --edition=2021
#[macro_export]
macro_rules! gen { ($Trait:ident) => { impl $Trait for () {} } }

// usr.rs  :::  rustc +stage1 usr.rs --edition=2015 --extern=dep -L.
trait async {}
dep::gen! { async }
fn main() {}

If so, we should check if we're inside an expansion or uninterp it first?

Copy link
Member

@fmease fmease Feb 19, 2024

Choose a reason for hiding this comment

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

Just checked out this branch and it does error while it shouldn't:

stderr
error: `async` trait implementations are unsupported
 --> usr.rs:3:1
  |
3 | dep::generate!(async);
  | ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `dep::generate` (in Nightly builds, run with -Z macro-backtrace for more info)

error: missing trait in a trait impl
 --> usr.rs:3:1
  |
3 | dep::generate!(async);
  | ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `dep::generate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add a trait here
 --> /home/fmease/programming/rust2/dep.rs:2:56
  |
2 | macro_rules! generate { ($Trait:ident) => { impl $Trait Trait for () {} } }
  |                                                         +++++
help: for an inherent impl, drop this `for`
 --> /home/fmease/programming/rust2/dep.rs:2:56

2 - macro_rules! generate { ($Trait:ident) => { impl $Trait for () {} } }
2 + macro_rules! generate { ($Trait:ident) => { impl $Trait () {} } }
  |

warning: trait `async` should have an upper camel case name
 --> usr.rs:1:7
  |
1 | trait async {}
  |       ^^^^^ help: convert the identifier to upper camel case: `Async`
  |
  = note: `#[warn(non_camel_case_types)]` on by default

error: aborting due to 2 previous errors; 1 warning emitted

And uninterpolate'ing does indeed fix this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right, and turns out all the other async checks also consider the uninterpolated span. Fixed that.

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024
@rust-cloud-vms rust-cloud-vms bot force-pushed the mbe-async-trait-bounds branch from 77ab04d to 9c8b107 Compare February 20, 2024 16:09
@compiler-errors compiler-errors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 21, 2024
@fmease
Copy link
Member

fmease commented Feb 21, 2024

Looks good to me. Thanks for fixing the other check, too!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 21, 2024

📌 Commit 9c8b107 has been approved by fmease

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#121044 (Support async trait bounds in macros)
 - rust-lang#121175 (match lowering: test one or pattern at a time)
 - rust-lang#121340 (bootstrap: apply most of clippy's suggestions)
 - rust-lang#121347 (compiletest: support auxiliaries with auxiliaries)
 - rust-lang#121359 (miscellaneous type system improvements)
 - rust-lang#121366 (Remove `diagnostic_builder.rs`)
 - rust-lang#121379 (Remove an `unchecked_error_guaranteed` call.)
 - rust-lang#121396 (make it possible for outside crates to inspect a mir::ConstValue with the interpreter)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 082b97a into rust-lang:master Feb 21, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
Rollup merge of rust-lang#121044 - compiler-errors:mbe-async-trait-bounds, r=fmease

Support async trait bounds in macros

r? fmease

This is similar to your work on const trait bounds. This theoretically regresses `impl async $ident:ident` in macros, but I doubt this is occurring in practice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants