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

Rename some Rust 2021 lints to better names #86717

Merged
merged 11 commits into from
Jul 7, 2021
Merged

Conversation

rylev
Copy link
Member

@rylev rylev commented Jun 29, 2021

Based on conversation in #85894.

Rename a bunch of Rust 2021 related lints:

Lints that are officially renamed because they are already in beta or stable:

  • disjoint_capture_migration => rust_2021_incompatible_closure_captures
  • or_patterns_back_compat => rust_2021_incompatible_or_patterns
  • non_fmt_panic => non_fmt_panics

Lints that are renamed but don't require any back -compat work since they aren't yet in stable:

  • future_prelude_collision => rust_2021_prelude_collisions
  • reserved_prefix => rust_2021_token_prefixes

Lints that have been discussed but that I did not rename:

  • non_fmt_panic and bare_trait_object: is making this plural worth the headache we might cause users?
  • array_into_iter: I'm unsure of a good name and whether bothering users with a name change is worth it.

r? @nikomatsakis

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 29, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

library/core/src/lib.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 29, 2021

  • bare_trait_object: is making this plural worth the headache we might cause users?

Apparently, yes:

$ rustc -Dbare_trait_object /dev/null
warning: lint `bare_trait_object` has been renamed to `bare_trait_objects`

(Seems like this already happened a while ago, and I just missed the s in my overview of the edition lints. ^^)

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Nice!

I'm only wondering if reserved_prefixes is the right name. If we add f"" or z"" or k#yeet later in Rust 2021, then these aren't really 'reserved' prefixes. Just prefixes that have a meaning in Rust 2021 and not before.

Maybe rust_2021_incompatible_prefix or just rust_2021_prefix? Or something. Not sure.

library/core/src/lib.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

reserved_prefixes -> reserved_token_prefixes perhaps?
"Prefix" alone is a very generic word, it's not clear what it refers to without having the specific context of #85359.

@camsteffen
Copy link
Contributor

camsteffen commented Jun 29, 2021

reserved_prefixes -> future_prefixes

To me reserved_prefixes sounds like "you are using a reserved prefix" rather than "this will be parsed as a prefix in the future".

Edit: or future_prefix_syntax

@rylev
Copy link
Member Author

rylev commented Jun 29, 2021

Suggestions (from above and from me):

  • rust_2021_incompatible_prefix
  • rust_2021_prefix
  • reserved_token_prefixes
  • future_prefixes
  • future_prefix_syntax
  • future_possible_prefix

I'm sympathetic to @petrochenkov's concern about "prefix" being pretty generic, but I do worry that "token_prefix" is too jargon filled.

Here's a Zulip thread to bikeshed.

@petrochenkov
Copy link
Contributor

but I do worry that "token_prefix" is too jargon filled

"Token" is a pretty central terminology for the proc macro API which is stable and public.

@nikomatsakis
Copy link
Contributor

I don't think it's worth stressing too much about "prefixes" vs "token prefixes" -- isn't this an "allow by default" lint? Few people will ever name it explicitly. I do like "rust_2021_incompatible" instead of "reserved". I think that's a good convention for "allow by default" lints.

@nikomatsakis
Copy link
Contributor

@rylev I guess your thinking with "rust_2021_token_prefixes" is that this only warns on things that became a prefix in Rust 2021 (e.g., f"foo") but not things like r"xx" which already were a recognized prefix?

That makes sense to me, I can run live with it.

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved
@rylev
Copy link
Member Author

rylev commented Jul 1, 2021

I was previously in favor of rust_2021_token_prefixes. I believe it to be both descriptive of what type of prefixes and clear that rust_2021 implied that the prefixes used in Rust 2021 but not before.

@m-ou-se had a counter argument though:

i think a key part that's easily missed is that all of these new edition lints are for Rust 2015/2018 and warn about things that would work differently if the code would be interpreted Rust 2021. so if you deny() that, you deny things that are incompatible with the new edition, even though it's currently not yet interpreted that way. so the prefix warning warns about identifiers that will no longer be parsed/tokenized as identifiers when parsed in Rust 2021. these lints do not run in Rust 2021 code. deny(rust_2021_prefixes) makes it sound like that'd deny f"" in Rust 2021. but there it's just a syntax error or a format string. instead, it denies f"" (two tokens) in Rust 2015/2018, because that syntax is changing and incompatible with Rust 2021.

I'm unsure that I agree with the argument "deny(rust_2021_prefixes) makes it sound like that'd deny f"" in Rust 2021", but as @danielhenrymantilla mentioned, there's no real penalty for being verbose, but there could be for being too terse.

With this in mind, @m-ou-se suggested rust_2021_prefix_incompatible_syntax. This is now the one I favor as well although I think rust_2021_prefixes_incompatible_syntax (i.e., prefixes instead of prefix) could be even more understandable.

@rylev
Copy link
Member Author

rylev commented Jul 1, 2021

@nikomatsakis the two remaining questions:

  • What should we rename rust_2021_token_prefixes to if anything? My current favorite is rust_2021_prefixes_incompatible_syntax
  • Shall we rename array_into_iter even if it's already quite widely used in the ecosystem?

@rylev rylev requested a review from nikomatsakis July 1, 2021 11:55
@nikomatsakis
Copy link
Contributor

What should we rename rust_2021_token_prefixes to if anything? My current favorite is rust_2021_prefixes_incompatible_syntax

That works for me.

Shall we rename array_into_iter even if it's already quite widely used in the ecosystem?

As a rename? This one is (iirc) warn by default, so the name is more likely to be something users are exposed to. Still, I think it's name is not wrong -- it just fails to describe the edition-dependent nature of it? I think we can leave it personally.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 5, 2021

Leaving array_into_iter seems fine to me. It used to warn about all cases of array.into_iter, but since the edition hack it only warns about array.into_iter in rust 2015 and 2018. Adding 2021 into the name wouldn't make as much sense as for some of the other lints, as it is warning about the hack in pre-2021 code, which doesn't exist in rust 2021. I guess it could be array_into_iter_by_ref or something, but probably not worth it.

@rylev rylev requested a review from nikomatsakis July 6, 2021 14:41
@bors
Copy link
Contributor

bors commented Jul 6, 2021

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2021
@nikomatsakis
Copy link
Contributor

@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 6, 2021

✌️ @rylev can now approve this pull request

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2021

📌 Commit d4e384b has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 7, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#80918 (Add Integer::log variants)
 - rust-lang#86717 (Rename some Rust 2021 lints to better names )
 - rust-lang#86819 (Clean up rustdoc IDs)
 - rust-lang#86880 (Test ManuallyDrop::clone_from.)
 - rust-lang#86906 (Replace deprecated compare_and_swap and fix typo in core::sync::atomic::{fence, compiler_fence} docs)
 - rust-lang#86907 (Migrate `cpu-usage-over-time.py` to Python 3)
 - rust-lang#86916 (rewrote documentation for thread::yield_now())
 - rust-lang#86919 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7e95290 into rust-lang:master Jul 7, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 7, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 15, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#80918 (Add Integer::log variants)
 - rust-lang#86717 (Rename some Rust 2021 lints to better names )
 - rust-lang#86819 (Clean up rustdoc IDs)
 - rust-lang#86880 (Test ManuallyDrop::clone_from.)
 - rust-lang#86906 (Replace deprecated compare_and_swap and fix typo in core::sync::atomic::{fence, compiler_fence} docs)
 - rust-lang#86907 (Migrate `cpu-usage-over-time.py` to Python 3)
 - rust-lang#86916 (rewrote documentation for thread::yield_now())
 - rust-lang#86919 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jul 29, 2021
Updated changelog for 1.55

This has again been a bit of work, but I'm happy to notice that my English is still improving, and I'm getting faster at these things. That's a very nice side effect of contributing and getting feedback on reviews 😊

Moving on, there were a few things that I was unsure about:
* The PR rust-lang/rust#86717 changes an old entry in the change log, is this worth mentioning? I've left it out for now.
* The stabilization of `cargo clippy --fix` is quite awesome and important IMO. It sadly gets a bit lost in the *Other* entry, as it's the last one. Do we maybe want to move it somewhere else or change the headline order for this release?
* I've listed the introduction of the new `suspicious` group under the *Moves and Deprecations* section. Is this alright, or should it be moved to the *Other* section as well?
* Last but definitely not least, some fun! I've used the 🎉 emoji in the `cargo clippy --fix` entry, is this okay?

Sorry for the bombardment of questions xD

---

The PR already includes the entries for the new metadata collection and website updates. These are not merged yet, but should probably be to make this correct. This might also require the commit hashes to be updated (Not sure on this, though). It would actually be super fitting to get this into this release as we also stabilize `--fix`. TODOs:
* [x] Merge metadata collection PRs:
  1. #7279
  2. #7298
  3. #7420 (Hope to not get any merge conflicts)

---

[Rendered 📰](https://github.com/xFrednet/rust-clippy/blob/changelog-1-55/CHANGELOG.md)

r? `@flip1995`

changelog: none
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants