lint to favor ..= over ... range patterns; migrate to ..= throughout codebase
#51149
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc_lint/builtin.rs
Outdated
|
|
||
|
|
||
| declare_lint! { | ||
| pub INCLUSIVE_RANGE_PATTERN_SYNTAX, |
There was a problem hiding this comment.
Let's try to follow the conventions again.
The basic rule is: the lint name should make sense when read as "allow lint-name" or "allow lint-name items".
"Deny inclusive range pattern syntax". Hmmm.
There was a problem hiding this comment.
the conventions
We should link this in both a code comment and the guide.
"Deny inclusive range pattern syntax".
The problem is that ... doesn't look like it should be a valid identifier, but "dot-dot-dot" is hideous. old_inclusive_range_patterns (counterargument: we don't say "old" anywhere else)? ellipsis_range_patterns (counterargument: the .. exclusive range syntax is ellipsis-like less one dot, which might be confusing)?
There was a problem hiding this comment.
Perhaps deprecated_inclusive_range_patterns? I think it's ok to call it deprecated. If we want to be more specific, than ellipsis_inclusive_range_patterns?
I don't think many people will write the name of this lint manually anyway; they will probably get it as part of the rust_2018_idioms group.
|
@zackmdavis did you do the migration via rustfix? |
src/librustc_lint/lib.rs
Outdated
| @@ -289,6 +290,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { | |||
| reference: "issue #50589 <https://github.com/rust-lang/rust/issues/50589>", | |||
| edition: None, | |||
| }, | |||
| FutureIncompatibleInfo { | |||
There was a problem hiding this comment.
I don't think this needs to be a future incompatible thing. I mean we have no plan to remove ... patterns as of yet, even if we got noisier about deprecating them. I'd rather we not make people think we are changing the language more than we are =)
nikomatsakis
added
S-waiting-on-author
kennytm
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@nikomatsakis (Travis is green now.)
No (not obvious how to use with
I thought it was odd to lint against some syntax without intending to remove it at some point, and to count it as a "Rust 2018" idiom if it's not related to any edition-2018-specific behavior. But I suppose I can see the case for wanting people to rustfix this at the same time they're doing their 2018 edition migration. The current revision of this PR uses the lint name |
I see your point, but I don't think that's necessarily odd. We deprecate methods, after all, but we never intend to remove those. In any case, there has been no firm decision to remove it, so it feels premature to me to tell people it's being removed. In any case, the rule is that lints only go in the migration category if the changes are necessary to adopt Rust 2018. The idioms category corresponds to changes that are recommended but not strictly necessary. |
src/librustc_lint/builtin.rs
Outdated
|
|
||
| declare_lint! { | ||
| pub ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, | ||
| Warn, |
There was a problem hiding this comment.
cc @rust-lang/lang — do we want to warn by default when people use ... notation (instead of the new and "shiny" ..= notation)? Or should we tie warnings to the Edition.
There was a problem hiding this comment.
It seems like the reasons to not use ..= are similar to the reasons to not use the edition, so an edition-tied warning (of some sort) seems reasonable.
There was a problem hiding this comment.
I guess there is a question of what our policy is about warning by default in the new edition. I think my preference would be:
- This is an "Rust 2018 idiom" lint.
- When you go to
edition = 2018, all Rust 2018 Idiom lints are warn by default. - Perhaps in the next edition, they will become hard errors, who knows.
| span, "use `..=` for an inclusive range", "..=".to_owned(), | ||
| // FIXME: outstanding problem with precedence in ref patterns: | ||
| // https://github.com/rust-lang/rust/issues/51043#issuecomment-392252285 | ||
| Applicability::MaybeIncorrect |
There was a problem hiding this comment.
I think the MaybeIncorrect setting probably prevents rustfix from acting (cc @killercup, confirm?) — but otherwise it'd be nice to have some automated tests that use the suggestion (e.g., using the new // rustfix mode).
There was a problem hiding this comment.
Yes -- MaybeIncorrect will prevent rustfix from applying this. It will not, however, prevent the rustfix compile tests as they currently exist from applying it! (Mostly because I haven't updated the test suite to also include a // run-rustfix --yolo mode.) So you can actually test edge cases if you want to. (Otherwise don't put them in the same file I guess.)
There was a problem hiding this comment.
OK, then @zackmdavis would you be up to add a // run-rustfix test for this? It's pretty straightforward: make a UI test and add that comment, and we will run rustfix and create a foo.fixed file. You can even use ./x.py test --bless to create the reference file automatically.
|
☔ The latest upstream changes (presumably #51448) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Unfortunately, a distressing personal matter has come up for me, and I don't think I'll have nearly as much rustc-dev time this week as I had anticipated. (I have not forgotten about #51321, #51104, or #51098, either.) I hope that the delay will not adversely affect my reputation as a contributor in good standing. You would think that rebasing this branch and adding a run-rustfix test would be a very simple task that would not require very much focused attention, but after rebasing, I'm seeing a lot of seemingly-unrelated UI test failures that I don't immediately know how to make sense of and can't bring myself to investigate further while I am distracted by my distressing personal matter. Thank you for your patience. |
pietroalbini
added
S-waiting-on-author
kennytm
removed
the
S-waiting-on-author
|
@nikomatsakis Sorry, I had forgotten the context of what this still needed. Latest push adds run-rustfix for the |
|
Ping from triage @nikomatsakis! This PR needs your review. |
|
@bors r+ |
|
📌 Commit c8a7dcc1942790532169c54f34f49ff784c27cc9 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit c8a7dcc1942790532169c54f34f49ff784c27cc9 with merge 3aeb307c74757847a83ceece5ed489a61610993b... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
These were stabilized in March 2018's rust-lang#47813, and are the Preferred Way to Do It going forward (q.v. rust-lang#51043).
Our implementation ends up changing the `PatKind::Range` variant in the AST to take a `Spanned<RangeEnd>` instead of just a `RangeEnd`, because the alternative would be to try to infer the span of the range operator from the spans of the start and end subexpressions, which is both hideous and nontrivial to get right (whereas getting the change to the AST right was a simple game of type tennis). This is concerning rust-lang#51043.
|
CI failure was due to implicit conflict where a |
|
@bors r+ |
|
📌 Commit 64365e4 has been approved by |
bors
added
S-waiting-on-bors
lint to favor `..=` over `...` range patterns; migrate to `..=` throughout codebase We probably need an RFC to actually deprecate the `...` syntax, but here's a candidate implementation for the lint considered in #51043. (My local build is super flaky, but hopefully I got all of the test revisions.)
|
☀️ Test successful - status-appveyor, status-travis |
We probably need an RFC to actually deprecate the
...syntax, but here's a candidate implementation for the lint considered in #51043. (My local build is super flaky, but hopefully I got all of the test revisions.)