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

rewrite ... to ..= as an idiom lint for Rust 2018 edition #51043

Closed
nikomatsakis opened this issue May 24, 2018 · 10 comments
Closed

rewrite ... to ..= as an idiom lint for Rust 2018 edition #51043

nikomatsakis opened this issue May 24, 2018 · 10 comments
Assignees
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-rust-2018-preview Area: The 2018 edition preview C-enhancement Category: An issue proposing an enhancement or a PR with one. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 24, 2018

Current status

Implemented in #51149 but currently not marked as automatically applicable due to this issue which is explained below

@nikomatsakis nikomatsakis added the A-rust-2018-preview Area: The 2018 edition preview label May 24, 2018
@nikomatsakis
Copy link
Contributor Author

Do we have an existing lint for this?

@nikomatsakis nikomatsakis added the WG-epoch Working group: Epoch (2018) management label May 24, 2018
@zackmdavis
Copy link
Member

If we hope to someday get rid of ... in patterns (the current description of #28237 describes it as "(silently) deprecated"), wouldn't we want a future-incompatible lint rather than an idiom lint? (... in expressions is already illegal.)

@kennytm
Copy link
Member

kennytm commented May 26, 2018

One remaining issue of a..=b pattern is #48501. When stabilizing a..=b, to avoid the confusing interpretation of &a...b, in the following example:

fn main() {
    match &5 {
        &3...6 => {}
        _ => {}
    }
}

if we just s/.../..=/g the code will error with

error: the range pattern here has ambiguous interpretation
 --> src/main.rs:3:10
  |
3 |         &3..=6 => {}
  |          ^^^^^ help: add parentheses to clarify the precedence: `(3 ..=6)`

Note that pattern_parentheses (#48500) is still unstable 🙃. However, given that default binding modes (#42640) is already stable, an even more idiomatic fix would probably be

fn main() {
    match &5 {
        3..=6 => {}   // no &, no (), no whatever
        _ => {}
    }
}

@nikomatsakis
Copy link
Contributor Author

@zackmdavis

If we hope to someday get rid of ... in patterns (the current description of #28237 describes it as "(silently) deprecated"), wouldn't we want a future-incompatible lint rather than an idiom lint? (... in expressions is already illegal.)

I classified it as an idiom lint because the code will not cause a hard error in the new edition, even if it is frowned upon.

zackmdavis added a commit to zackmdavis/rust that referenced this issue Jun 24, 2018
Now that `..=` inclusive ranges are stabilized, people probably
shouldn't be using `...` even in patterns, even if it's still legal
there (see rust-lang#51043). To avoid drawing attention to `...` being a real
thing, let's reword this message to just say "unexpected token" rather
"cannot be used in expressions".
bors added a commit that referenced this issue Jun 25, 2018
three diagnostics upgrades

 * reword `...` expression syntax error to not imply that you should use it in patterns either (#51043) and make it a structured suggestion
 * shorten the top-line message for the trivial-casts lint by tucking the advisory sentence into a help note
 * structured suggestion for pattern-named-the-same-as-variant warning

r? @oli-obk
zackmdavis added a commit to zackmdavis/rust that referenced this issue Jun 26, 2018
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).
zackmdavis added a commit to zackmdavis/rust that referenced this issue Jun 26, 2018
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.
bors added a commit that referenced this issue Jun 26, 2018
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.)
@nrc nrc added the A-edition-2018-lints Area: Lints supporting the 2018 edition label Jul 2, 2018
@nrc nrc mentioned this issue Jul 4, 2018
15 tasks
@alexcrichton
Copy link
Member

@zackmdavis if you've got a moment, can you write up a listing of what's left after #51149 landed?

@zackmdavis
Copy link
Member

zackmdavis commented Jul 12, 2018

if you've got a moment, can you write up a listing of what's left after #51149 landed?

@alexcrichton: the comment above by @kennytm should still be addressed; the lint suggestion is marked as maybe-incorrect for this reason. If pattern-parentheses are stabilized (which itself reportedly needs more testing), using them for a correct suggestion when needed will be very easy.

@alexcrichton
Copy link
Member

Ok great, thanks!

@XAMPPRocky XAMPPRocky added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Nov 8, 2018

visited for T-compiler triage. It seems like it was blocked on stabilization of the pattern-parentheses feature. So what are the next steps, and who wants to own this?

@varkor varkor self-assigned this Nov 8, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Nov 8, 2018

more triage notes: P-high. @varkor has agreed to take this on and/or mentor it.

@pnkfelix pnkfelix added the P-high High priority label Nov 8, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 12, 2018
Rewrite `...` as `..=` as a `MachineApplicable` 2018 idiom lint

Fixes rust-lang#51043.
@nikomatsakis
Copy link
Contributor Author

Removing from the milestone, as idiom lints are not part of the Rust 2018 release.

@nikomatsakis nikomatsakis removed this from the Rust 2018 Release milestone Nov 13, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 15, 2018
Rewrite `...` as `..=` as a `MachineApplicable` 2018 idiom lint

Fixes rust-lang#51043.
eclipseo added a commit to eclipseo/difference.rs that referenced this issue Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-rust-2018-preview Area: The 2018 edition preview C-enhancement Category: An issue proposing an enhancement or a PR with one. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management
Projects
None yet
Development

No branches or pull requests

9 participants