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.

Sign up for GitHub

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

Jump to bottom

Unseparated literal suffix #7726

Merged
merged 3 commits into from
Nov 2, 2021
Merged

Unseparated literal suffix #7726

merged 3 commits into from
Nov 2, 2021

Conversation

dswij
Copy link
Member

@dswij dswij commented Sep 27, 2021

Closes #7658

Since literal_suffix style is opinionated, we should disable by default and only enforce if it's stated as so.

changelog: [unseparated_literal_suffix] is renamed to literal_suffix, adds a new configuration literal-suffix-style to enforce a certain style writing literal_suffix. Possible values for literal-suffix-style: "separated", "unseparated"

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 27, 2021
@dswij
Copy link
Member Author

dswij commented Sep 27, 2021

Not sure what's happening there at https://github.com/rust-lang/rust-clippy/runs/3717748423#step:5:12

@xFrednet
Copy link
Member

The log states:

.github/PULL_REQUEST_TEMPLATE.md: no issues found
CHANGELOG.md
  2407:70-2407:100  warning  Found reference to undefined definition  no-undefined-references  remark-lint

⚠ 1 warning
CODE_OF_CONDUCT.md: no issues found

The correlating text inside CHANGELOG.md looks like this:

2405 ## 0.0.81 — 2016-08-14
2406 * Rustup to *rustc 1.12.0-nightly (1deb02ea6 2016-08-12)*
2407 * New lints: [`eval_order_dependence`], [`mixed_case_hex_literals`], [`unseparated_literal_suffix`]
2408 * False positive fix in [`too_many_arguments`]

https://github.com/rust-lang/rust-clippy/blame/master/CHANGELOG.md#L2407

However, I'm not sure what causes the warning in this case.

@dswij
Copy link
Member Author

dswij commented Sep 27, 2021 •
edited
Loading

Ah, there is a CHANGELOG.md changes on 45a89b9. unseparated_literal_suffix is renamed to literal_suffix in the auto-generated links, so we have an empty reference. But not changing it also causes an error:

Run cargo dev update_lints --check
   Compiling clippy_dev v0.0.1 (/home/runner/work/rust-clippy/rust-clippy/clippy_dev)
    Finished dev [unoptimized + debuginfo] target(s) in 3.67s
     Running `target/debug/clippy_dev update_lints --check`
Not all lints defined properly. Please run `cargo dev update_lints` to make sure all lints are defined properly.
Error: Process completed with exit code 1.

https://github.com/rust-lang/rust-clippy/runs/3718070585#step:6:9

Not sure what's the best way to get around it. We can probably remove all old links to unseparated_literal_suffix in CHANGELOG.

@camsteffen
Copy link
Contributor

Actually I think I would prefer having two lints over adding a config option for these "one or the other" scenarios. And this would avoid a lint rename.

@dswij
Copy link
Member Author

dswij commented Sep 27, 2021

Actually I think I would prefer having two lints over adding a config option for these "one or the other" scenarios. And this would avoid a lint rename.

This also crossed my mind. The only downside of this approach is that someone might get confused as to why two different and opposing lints co-exists. Maybe a documentation for the two lints can describe this, but I'm open to suggestions.

@camsteffen
Copy link
Contributor

Yes the docs should absolutely mention the other lint.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 28, 2021
@bors
Copy link
Contributor

bors commented Sep 30, 2021

☔ The latest upstream changes (presumably #7673) made this pull request unmergeable. Please resolve the merge conflicts.

@dswij dswij force-pushed the unseparated-literal-suffix branch from c7da7d6 to 21137cd Compare October 4, 2021 04:07
@dswij dswij force-pushed the unseparated-literal-suffix branch 2 times, most recently from 846a943 to b25c59f Compare October 4, 2021 04:27
@flip1995 flip1995 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 from the author. (Use `@rustbot ready` to update this status) labels Oct 4, 2021
flip1995
flip1995 reviewed Oct 28, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Sorry for not reviewing this for so long! Only a few comments.

clippy_lints/src/misc_early/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/misc_early/mod.rs Outdated Show resolved Hide resolved
tests/ui/literals.stderr Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 28, 2021
@dswij dswij requested a review from flip1995 October 29, 2021 05:48
flip1995
flip1995 approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks! Please squash your commits and this is ready to merge.

dswij added 2 commits November 1, 2021 22:26
@dswij
Add separated_literal_suffix as an alternative for
1085df5 
`unseparated_literal_suffix`

This commit adds a configuration `literal-suffix-style` to enforce a
specific style for unseparated_literal_suffix. The configuration accepts
two values:
- "separated"
    enforce all literals to be written separately (e.g. `123_i32`)
- "unseparated"
    enforce all literals to be written as unseparated (e.g. `123i32`)

Not specifying a value means that there is no preference on style and
any style should not be warned.
@dswij
Update CHANGELOG to include separated_literal_suffix
bb1cf72
@dswij dswij force-pushed the unseparated-literal-suffix branch from 23f5736 to bb1cf72 Compare November 1, 2021 14:26
@dswij
Copy link
Member Author

dswij commented Nov 1, 2021

@flip1995 squashed 🙂

@flip1995
Copy link
Member

flip1995 commented Nov 2, 2021

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Nov 2, 2021

📌 Commit bb1cf72 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Nov 2, 2021

⌛ Testing commit bb1cf72 with merge 38d8025...

@bors
Copy link
Contributor

bors commented Nov 2, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 38d8025 to master...

@bors bors merged commit 38d8025 into rust-lang:master Nov 2, 2021
@leonardo-m
Copy link

Generally it's better to write 10_u8, it allows to tell apart better the value from the type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unseparated_literal_suffix is very opinion oriented 0u8 versus 0_u8
7 participants
@dswij @rust-highfive @xFrednet @camsteffen @bors @flip1995 @leonardo-m