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

On irrefutable let pattern lint point only at pattern #81366

Closed
wants to merge 4 commits into from
Closed

On irrefutable let pattern lint point only at pattern #81366

wants to merge 4 commits into from

Conversation

estebank
Copy link
Contributor

Add Span to let desugaring MatchSource variants to point only at
the let pattern when linting against irrefutable patterns in if let
and while let.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Jan 25, 2021
camelid
camelid reviewed Jan 25, 2021
View reviewed changes
src/test/ui/consts/const_limit/const_eval_limit_reached.stderr Outdated
Comment on lines 4 to 12
LL | / const X: usize = {
LL | | let mut x = 0;
LL | | while x != 1000 {
| |_____^
LL | ||
LL | || x += 1;
LL | || }
| ||_____^ exceeded interpreter step limit (see `#[const_eval_limit]`)
LL | |
LL | | x
LL | | };
| |__-
LL | / const X: usize = {
LL | | let mut x = 0;
LL | | while x != 1000 {
| | ^^^^^^^^^ exceeded interpreter step limit (see `#[const_eval_limit]`)
LL | |
... |
LL | | x
LL | | };
| |__-
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a regression :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looked fine to me. The span got more precise, so that was good. The interpreter spans never were very good around the resource limits

src/test/ui/pattern/usefulness/deny-irrefutable-let-patterns.stderr Outdated Show resolved Hide resolved
src/test/ui/while-let.stderr Outdated
Comment on lines 18 to 19
LL | while let $p = $e $b
| ^^^^^^^^^^^^^^^^^^^^
| ^^^
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spans involving macros are sometimes less than ideal :-/

@camelid
Copy link
Member

camelid commented Jan 25, 2021 •
edited
Loading

Can you also do this for if-let guards? (Unless they do this already, I forget 😄)

@camelid camelid added the A-diagnostics Area: Messages for errors, warnings, and lints label Jan 25, 2021
@estebank estebank force-pushed the if-let-pattern branch 2 times, most recently from cd71894 to b1fb8a7 Compare January 25, 2021 03:19
@estebank
Copy link
Contributor Author

estebank commented Jan 25, 2021 •
edited
Loading

Can you also do this for if-let guards?

I'll check, but I think those are handled elsewhere.

Edit: for the if let guard case we point at only the pattern, which isn't terrible, but I'll add the correct span for them to be consistent.

Edit: Done.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 18, 2021

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor Author

r? @oli-obk

@bors

This comment has been minimized.

Sign in to view
@estebank
On irrefutable let pattern lint point only at pattern
cc85d1f 
Add `Span` to let desugaring `MatchSource` variants to point only at
the `let` pattern when linting against irrefutable patterns in `if let`
and `while let`.
@estebank
Point at whole let guard on irrefutable let pattern
5b7c07d
@estebank
Add more detail to irrefutable let pattern lint
9fe2500
@estebank
Copy link
Contributor Author

Heads up, @rust-lang/clippy. Small changes incoming that would break clippy with fixes in the same PR.

@Manishearth
Copy link
Member

Seems fine, we have an easy sync process now

oli-obk
oli-obk reviewed Feb 26, 2021
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I feel like I'm missing something fundamental. Where do the spans that encompass entire if let or while let expressions come from? To me it seems that you replaced a lot of (or only?) arm.pat.spans with let_spans, which has some regressions, but also some improvements in situations that seem indistinguishable (to me) from situations that were good before. So...

  • Are there some pat.spans that refer to the entire match?
  • Do these point to the entire match only after compute_match_usefulness
  • Do these point to the entire match already during lowering?

I think we should first find out where these bad spans come from and if we can address them at the source instead of tracking additional spans.

compiler/rustc_mir_build/src/thir/pattern/check_match.rs
diag.help("consider instead using a `loop { ... }` with a `let` inside it");
diag
}
hir::MatchSource::IfLetGuardDesugar { .. }=> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hir::MatchSource::IfLetGuardDesugar { .. }=> {
hir::MatchSource::IfLetGuardDesugar { .. } => {

not sure why rustfmt didn't catch this

src/test/ui/uninhabited/uninhabited-patterns.stderr
Comment on lines 34 to +35
LL | while let Some(_y) = foo() {
| ^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a regression. That refutable let binding is not a pattern, the previous span was pointing to a pattern.

@bors
Copy link
Contributor

bors commented Mar 9, 2021

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

@camelid camelid 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 Mar 25, 2021
@Dylan-DPC-zz
Copy link

@estebank this is waiting for you to resolve conflicts + reply to the above review. Thanks

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 12, 2021
@camelid camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2021
@JohnCSimon
Copy link
Member

Ping again from triage:
@estebank this is waiting for you to resolve conflicts + reply to the above review. Thanks

@flip1995
Copy link
Member

This heavily collides with #80357 to a point where it has probably be redone once that is merged.

@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 22, 2021
@JohnCSimon JohnCSimon added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jul 6, 2021
@JohnCSimon
Copy link
Member

Blocked on #80357

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2021

This heavily collides with #80357 to a point where it has probably be redone once that is merged.

needs a rewrite, closing

@oli-obk oli-obk closed this Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
A-diagnostics Area: Messages for errors, warnings, and lints S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.