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

Implement raw lifetimes and labels ('r#ident) #126452

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

compiler-errors
Copy link
Member

This PR implements a syntax for raw lifetimes and labels as requested in #126335.

This PR also moves keyword lifetime validation (i.e. checking that we don't have something like 'async) from ast validation to the parser, similar to how we handle raw identifiers, since we don't distinguish raw identifiers after parsing.

This PR additionally extends the keyword_idents_2024 lint to also check identifiers.

cc @traviscross
r? parser

@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 Jun 14, 2024
@compiler-errors
Copy link
Member Author

compiler-errors commented Jun 14, 2024

The only theoretical breakage is this:

macro_rules! lt {
    ($lt:lifetime # $id:ident) => {}
}

lt!('r#a);

edit: whoops, @fmease also notes that it breaks:

#[cfg(any())] fn f<'async>(_: &'async ()) {}

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

Also, this doesn't reserve all prefixed lifetimes -- it just implements 'r#. Not sure if T-lang wants all prefixes to be reserved as well.

@compiler-errors compiler-errors added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jun 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2024

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@compiler-errors
Copy link
Member Author

r-a and rustfmt changes are required due to changes to lexer and parser, respectively.

Copy link
Contributor

Choose a reason for hiding this comment

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

rustfmt changes look straightforward to me. Do you think it's worth adding a basic test in src/tools/rustfmt/tests/target for these new raw lifetimes and labels?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good point. I have no idea if raw lifetimes even format correctly 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

🤞🏼fingers crossed they just work out of the box. Given the changes to the parser does the span include the whole 'r#label or 'r#lifetime?

Copy link
Member Author

Choose a reason for hiding this comment

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

should include the whole span, so if we extract the lifetime name from the span (and not from the ident.name Symbol) then we should be fine 👍 I'll implement those tests tho

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  |

........
Mismatch at src/macros.rs:1097:
         | TokenKind::OpenDelim(_)
         | TokenKind::CloseDelim(_) => SpaceState::Never,
 
-        TokenKind::Literal(..) | TokenKind::Ident(..) | TokenKind::Lifetime(..) => SpaceState::Ident,
+        TokenKind::Literal(..) | TokenKind::Ident(..) | TokenKind::Lifetime(..) => {
+        }
 
         _ => SpaceState::Always,
     }
     }
{ "type": "test", "name": "test::self_tests", "event": "failed", "stdout": "Ran 5 self tests.\nthread 'test::self_tests' panicked at src/tools/rustfmt/src/test/mod.rs:400:5:\nassertion `left == right` failed: 1 self tests failed\n  left: 1\n right: 0\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }

test result: FAILED. 173 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 984.12ms

error: test failed, to rerun pass `--lib`

@@ -1201,7 +1201,14 @@ impl<'a> Parser<'a> {

/// Parses a single lifetime `'a` or panics.
pub(super) fn expect_lifetime(&mut self) -> Lifetime {
if let Some(ident) = self.token.lifetime() {
if let Some((ident, is_raw)) = self.token.lifetime() {
Copy link
Member

Choose a reason for hiding this comment

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

This PR also moves keyword lifetime validation (i.e. checking that we don't have something like 'async) from ast validation to the parser

For the T-lang meeting: Theoretical breakage of stable code:

#[cfg(any())] fn f<'async>(_: &'async ()) {}

@compiler-errors
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jun 14, 2024

⌛ Trying commit 0129478 with merge d9cbfeb...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2024
Implement raw lifetimes and labels (`'r#ident`)

This PR implements a syntax for raw lifetimes and labels as requested in rust-lang#126335.

This PR also moves keyword lifetime validation (i.e. checking that we don't have something like `'async`) from ast validation to the parser, similar to how we handle raw identifiers, since we don't distinguish raw identifiers after parsing.

This PR additionally extends the `keyword_idents_2024` lint to also check identifiers.

cc `@traviscross`
r? parser
@bors
Copy link
Contributor

bors commented Jun 14, 2024

☀️ Try build successful - checks-actions
Build commit: d9cbfeb (d9cbfeb054d7bd02cf942e6d32c1c0a91f042ce9)

@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-126452 created and queued.
🤖 Automatically detected try build d9cbfeb
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-126452 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-126452 is completed!
📊 11 regressed and 2 fixed (472186 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 16, 2024
@craterbot craterbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2024
@compiler-errors
Copy link
Member Author

There's a crate that specifically abuses 'ref which is a keyword lifetime. Might have to find a workaround for this.

@danielhenrymantilla
Copy link
Contributor

FWIW, as documented over https://docs.rs/with_locals/0.3.2/with_locals/index.html#remarks, the 'ref lifetime choice is just a default one. Users can always provide their own lifetime marker instead. This means that it is possible for users to adjust their code to the change of this PR if needs be.

  • (if we could afford the wait, I could even patch the crate to start warning about the direct 'ref usage (DIY FCW, sort of))

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the lang triage call today.

We confirmed that we mean for all prefixed lifetimes to be reserved, not just 'r# ones. We view both the missing reservation and the missing support for 'r# as bug fixes consistent with the intent of RFC 3101. The result of this reservation is that these prefixed lifetimes will be parsed as single tokens rather than as many.

Secondly, we confirmed that we're OK with moving the validation of keywords in lifetimes to pre-expansion from post-expansion. We similarly consider this a bug fix. While the breakage of the convenience feature of the with_locals crate that relies on this is unfortunate, and we wish we had not overlooked this earlier for that reason, we're fortunate that the breakage is contained to only one crate, and we're going to accept this breakage as the extra complexity we'd need to carry in the compiler to work around this isn't deemed worth it.

Thanks in particular to @danielhenrymantilla for being responsive on this and understanding of the situation.

@rustbot rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants