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

Add test to check unicode identifier version #134043

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Dec 8, 2024

This adds a test to verify which version of Unicode is used for identifiers. This is part of the language, documented at https://doc.rust-lang.org/nightly/reference/identifiers.html#r-ident.unicode. The version here often changes implicitly due to dependency updates pulling in new versions, and thus we often don't notice it has changed leaving the documentation out of date. The intent here is to have a canary to give us a notification when it changes so that we can update the documentation.

@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself 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 Dec 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2024

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

@mattheww
Copy link
Contributor

mattheww commented Dec 8, 2024

unicode-normalization has its own UNICODE_VERSION constant. Would it be worth testing that that is at least as new as unicode-xids?

As far as I can tell there's nothing that makes sure those two dependencies are kept in sync, and I think having unicode-normalization use a unicode version older than unicode-xid would be a plain bug, not just an indication of outdated documentation.

@@ -0,0 +1 @@
Unicode version is: (16, 0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I think you could put this in a static assert in the crate itself (const _: () = assert!(rustc_lexer::UNICODE_VERSION == (16, 0, 0)) or so? That would probably avoid a CI cycle when this gets bumped to find the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not clear, what CI cycle would that avoid? My assumption is that when Cargo.lock is updated, a PR is opened and all the tests run there.

I'm fine with putting it in the crate, though it would need to be a dedicated module just for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about that more, I would slightly prefer to keep it as a test here to keep the link with the reference test/identifier. Otherwise there is nothing that would be verifying the actual version in use that we could link to.

Copy link
Member

@jieyouxu jieyouxu Dec 9, 2024

Choose a reason for hiding this comment

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

I'm fine with putting it in the crate, though it would need to be a dedicated module just for that.

It can just be a compiler/rustc_lexer/src/tests.rs test if you want it to be a proper crate test I think? Like this file. Static assert would be even faster feedback as that will fail at build time locally.

Copy link
Member

@jieyouxu jieyouxu Dec 9, 2024

Choose a reason for hiding this comment

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

If anything, we should have such a test in compiler/rustc_lexer/src/tests.rs to make sure we don't unknowingly change the Unicode version we are using.

Copy link
Member

Choose a reason for hiding this comment

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

what CI cycle would that avoid

For me at least I wouldn't expect to re-run tests locally after bumping Unicode or similar, while I probably would "happen to" re-compile with something like x.py check, which a static assert should catch and tell me to update this, vs. here where I need to push a PR and wait for the CI to fail and then push a trivial update.

But not a big deal anyway.

@cjgillot
Copy link
Contributor

cjgillot commented Dec 8, 2024

r? compiler

@rustbot rustbot assigned jieyouxu and unassigned cjgillot Dec 8, 2024
triagebot.toml Outdated Show resolved Hide resolved
tests/ui-fulldeps/lexer/unicode-version.rs Show resolved Hide resolved
@jieyouxu
Copy link
Member

jieyouxu commented Dec 9, 2024

@rustbot author (left some interim feedback)

@rustbot rustbot 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 Dec 9, 2024
@ehuss
Copy link
Contributor Author

ehuss commented Dec 9, 2024

@rustbot ready

@rustbot rustbot 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 Dec 9, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks!

@jieyouxu
Copy link
Member

jieyouxu commented Dec 9, 2024

You can r=me after PR CI is green.

@ehuss
Copy link
Contributor Author

ehuss commented Dec 9, 2024

@bors r=jieyouxu rollup

@bors
Copy link
Contributor

bors commented Dec 9, 2024

📌 Commit a97404e has been approved by jieyouxu

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 Dec 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`)
 - rust-lang#134012 (Grammar fixes)
 - rust-lang#134032 (docs: better examples for `std::ops::ControlFlow`)
 - rust-lang#134040 (bootstrap: print{ln}! -> eprint{ln}! (take 2))
 - rust-lang#134043 (Add test to check unicode identifier version)
 - rust-lang#134053 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 10))
 - rust-lang#134055 (interpret: clean up deduplicating allocation functions)
 - rust-lang#134073 (dataflow_const_prop: do not eval a ptr address in SwitchInt)
 - rust-lang#134084 (Fix typo in RFC mention 3598 -> 3593)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0cb12f9 into rust-lang:master Dec 10, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2024
Rollup merge of rust-lang#134043 - ehuss:unicode-version, r=jieyouxu

Add test to check unicode identifier version

This adds a test to verify which version of Unicode is used for identifiers. This is part of the language, documented at https://doc.rust-lang.org/nightly/reference/identifiers.html#r-ident.unicode. The version here often changes implicitly due to dependency updates pulling in new versions, and thus we often don't notice it has changed leaving the documentation out of date. The intent here is to have a canary to give us a notification when it changes so that we can update the documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself 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.

7 participants