10

Make underscore_literal_suffix a hard error. by nnethercote · Pull Request #1039...

 1 year ago
source link: https://github.com/rust-lang/rust/pull/103914
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Make underscore_literal_suffix a hard error. #103914

Conversation

Contributor

@nnethercote nnethercote commented Nov 3, 2022

It's been a warning for 5.5 years. Time to make it a hard error.

Closes #42326.

r? @pnkfelix

All reactions

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

Nov 3, 2022

Contributor

Author

nnethercote commented Nov 3, 2022

From #42326:

Next steps are:

  1. Run crater on experiementally moving it to hard error, to evaluate fallout.
  2. If fallout is non-trivial, then it needs to be moved to a proper lint based implementation
  3. Likewise, if fallout is non-trival, then ensure that it is included in the cargo report future-incompat output, when relevant.
  4. If fallout is trivial, then we can just move to hard error without going through the longer route of replacing with a lint and whatnot.

So let's do a try run, which is a prereq for a crater run.
@bors try

Contributor

bors commented Nov 3, 2022

hourglass Trying commit 33743a5 with merge d85a750...

Contributor

bors commented Nov 3, 2022

sunny Try build successful - checks-actions
Build commit: d85a750 (d85a7505e49363642b460e105973798063a683b4)

Contributor

Author

nnethercote commented Nov 3, 2022

@craterbot run mode=check-only

Collaborator

craterbot commented Nov 3, 2022

ok_hand Experiment pr-103914 created and queued.
robot Automatically detected try build d85a750
mag You can check out the queue and this experiment's details.

information_sourceCrater is a tool to run experiments across parts of the Rust ecosystem. Learn more

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

Nov 3, 2022

Collaborator

craterbot commented Nov 3, 2022

construction Experiment pr-103914 is now running

information_sourceCrater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Collaborator

craterbot commented Nov 4, 2022

tada Experiment pr-103914 is completed!
bar_chart 34 regressed and 9 fixed (247167 total)
newspaperOpen the full report.

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

Nilstrieb reacted with hooray emoji All reactions

craterbot

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-crater Status: Waiting on a crater run to be completed.

labels

Nov 4, 2022

Contributor

Author

nnethercote commented Nov 4, 2022

I don't know how to read crater reports. But from clicking around randomly I found at least one clear regression:

[INFO] [stderr]     Checking simple-replace-templete-engine v0.1.0 (/opt/rustwide/workdir)
[INFO] [stdout] error: underscore literal suffix is not allowed
[INFO] [stdout]   --> src/lib.rs:83:61
[INFO] [stdout]    |
[INFO] [stdout] 83 |         let templete_str = "hello _t_name_t_ _t_sec_name_t_"_.to_owned();
[INFO] [stdout]    |                                                    

But I can't find that crate on crates.io or anywhere else.

Contributor

Nilstrieb commented Nov 4, 2022

If you click on the name you'll get sent to the Github repository of the crate. Zero stars and the last commit from 3 years. The author is still active on Github, so I'll just send in a PR.

Contributor

Author

nnethercote commented Nov 5, 2022

From above:

  1. If fallout is trivial, then we can just move to hard error without going through the longer route of replacing with a lint and whatnot.

AFAICT we have a single failure in the crater run, for a crate that hasn't been touched in 3 years and has few if any users. @Nilstrieb has kindly filed a PR with the trivial fix anyway.

So I think we're good to land this.

r? @petrochenkov

Contributor

petrochenkov commented Nov 6, 2022

r=me after cleaning up the test.
@rustbot author

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

Nov 6, 2022

Contributor

Author

nnethercote commented Nov 6, 2022

@bors r=petrochenkov rollup

Contributor

bors commented Nov 6, 2022

pushpin Commit dba6fc3 has been approved by petrochenkov

It is now in the queue for this repository.

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-author Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Nov 6, 2022

bors

merged commit 19c780a into

rust-lang:master

Nov 7, 2022

10 checks passed

rustbot

added this to the 1.67.0 milestone

Nov 7, 2022

rust-timer

added a commit to rust-lang-ci/rust that referenced this pull request

Nov 7, 2022

nnethercote

deleted the close-42326 branch

Nov 7, 2022

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

Milestone

1.67.0

Development

Successfully merging this pull request may close these issues.

underscore_literal_suffix future-compatibility warnings

7 participants

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK