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

Fix two variable binding issues in lint let_underscore #119704

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Fix two variable binding issues in lint let_underscore #119704

merged 1 commit into from
Jan 9, 2024

Conversation

chenyukang
Copy link
Member

Fixes #119696
Fixes #119697

@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2024

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 7, 2024
Noratrieb
Noratrieb approved these changes Jan 7, 2024
View reviewed changes
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

If you think relying on default_binding_mode like this is fine, r=me. If you want to change it, then do that and I'll take another look.

compiler/rustc_lint/src/lints.rs Outdated
@@ -960,10 +961,11 @@ impl AddToDiagnostic for NonBindingLetSub {
rustc_errors::SubdiagnosticMessage,
) -> rustc_errors::SubdiagnosticMessage,
{
let prefix = if self.default_binding_mode { "" } else { "let " };
Copy link
Member

Choose a reason for hiding this comment

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

the comment on default_binding_mode says

At present, this is false only for destructuring assignment.

I don't like how that doesn't strongly imply that this truly is for destructuring assignment. This makes the code a bit harder to understand and potentially breaks in the future. I'm not sure what the nicest way to address this would be. If you agree.

This is also wrong and broken for non-trivial patterns like (_, _) = (Droppy, Droppy), but oh well, those don't even lint today, so Not Your Problem. I'm gonna try fixing these missing lints myself, so I think it's fine to merge this as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

update the code to use assign_desugar, maybe a better choice?

Copy link
Member Author

Choose a reason for hiding this comment

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

This pr seems forget to update comments for AssignDesugar
chenyukang@8cf3564

@Noratrieb Noratrieb mentioned this pull request Jan 7, 2024
Merged
Noratrieb
Noratrieb reviewed Jan 8, 2024
View reviewed changes
tests/ui/lint/issue-119696-err-on-fn.rs Outdated
@@ -0,0 +1,21 @@
// edition: 2021
Copy link
Member

@Noratrieb Noratrieb Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

there's a let_underscore_drop subdirectory, let's put it there

Noratrieb
Noratrieb approved these changes Jan 8, 2024
View reviewed changes
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

that's nicer :)
r=me after moving the test

@chenyukang
Copy link
Member Author

@bors r=Nilstrieb

@bors
Copy link
Contributor

bors commented Jan 8, 2024

📌 Commit 75df38e has been approved by Nilstrieb

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 8, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2024
@matthiaskrgr
Rollup merge of rust-lang#119704 - chenyukang:yukang-fix-let_undersco…
96fbd21 
…re, r=Nilstrieb

Fix two variable binding issues in lint let_underscore

Fixes rust-lang#119696
Fixes rust-lang#119697
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 8, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2024
@bors
Auto merge of rust-lang#119737 - matthiaskrgr:rollup-xre9fp9, r=matth…
ab94f98 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#118903 (Improved support of collapse_debuginfo attribute for macros.)
 - rust-lang#119033 (coverage: `llvm-cov` expects column numbers to be bytes, not code points)
 - rust-lang#119654 (bump bootstrap dependencies)
 - rust-lang#119660 (remove an unnecessary stderr-per-bitwidth)
 - rust-lang#119663 (tests: Normalize `\r\n` to `\n` in some run-make tests)
 - rust-lang#119681 (coverage: Anonymize line numbers in branch views)
 - rust-lang#119704 (Fix two variable binding issues in lint let_underscore)
 - rust-lang#119725 (Add helper for when we want to know if an item has a host param)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2024
@matthiaskrgr
Rollup merge of rust-lang#119704 - chenyukang:yukang-fix-let_undersco…
abafcab 
…re, r=Nilstrieb

Fix two variable binding issues in lint let_underscore

Fixes rust-lang#119696
Fixes rust-lang#119697
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 8, 2024
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2024
@matthiaskrgr
Rollup merge of rust-lang#119704 - chenyukang:yukang-fix-let_undersco…
ef89314 
…re, r=Nilstrieb

Fix two variable binding issues in lint let_underscore

Fixes rust-lang#119696
Fixes rust-lang#119697
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 8, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2024
@bors
Auto merge of rust-lang#119747 - matthiaskrgr:rollup-cm6htwo, r=matth…
b1babda 
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#116343 (Stop mentioning internal lang items in no_std binary errors)
 - rust-lang#118903 (Improved support of collapse_debuginfo attribute for macros.)
 - rust-lang#119033 (coverage: `llvm-cov` expects column numbers to be bytes, not code points)
 - rust-lang#119598 (Fix a typo in core::ops::Deref's doc)
 - rust-lang#119660 (remove an unnecessary stderr-per-bitwidth)
 - rust-lang#119663 (tests: Normalize `\r\n` to `\n` in some run-make tests)
 - rust-lang#119681 (coverage: Anonymize line numbers in branch views)
 - rust-lang#119704 (Fix two variable binding issues in lint let_underscore)
 - rust-lang#119725 (Add helper for when we want to know if an item has a host param)
 - rust-lang#119738 (Add `riscv32imafc-esp-espidf` tier 3 target for the ESP32-P4.)
 - rust-lang#119740 (Remove crossbeam-channel)

Failed merges:

 - rust-lang#119723 (Remove `-Zdont-buffer-diagnostics`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 8, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
@bors
Auto merge of rust-lang#119754 - matthiaskrgr:rollup-7cht4m5, r=matth…
d6affcf 
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#118903 (Improved support of collapse_debuginfo attribute for macros.)
 - rust-lang#119033 (coverage: `llvm-cov` expects column numbers to be bytes, not code points)
 - rust-lang#119598 (Fix a typo in core::ops::Deref's doc)
 - rust-lang#119660 (remove an unnecessary stderr-per-bitwidth)
 - rust-lang#119663 (tests: Normalize `\r\n` to `\n` in some run-make tests)
 - rust-lang#119681 (coverage: Anonymize line numbers in branch views)
 - rust-lang#119704 (Fix two variable binding issues in lint let_underscore)
 - rust-lang#119725 (Add helper for when we want to know if an item has a host param)
 - rust-lang#119738 (Add `riscv32imafc-esp-espidf` tier 3 target for the ESP32-P4.)
 - rust-lang#119740 (Remove crossbeam-channel)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5efa69d into rust-lang:master Jan 9, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
@rust-timer
Unrolled build for rust-lang#119704
9f8b8bc 
Rollup merge of rust-lang#119704 - chenyukang:yukang-fix-let_underscore, r=Nilstrieb

Fix two variable binding issues in lint let_underscore

Fixes rust-lang#119696
Fixes rust-lang#119697
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
@matthiaskrgr
Rollup merge of rust-lang#119710 - Nilstrieb:let-_-=-oops, r=TaKO8Ki
72dddea 
Improve `let_underscore_lock`

- lint if the lock was in a nested pattern
- lint if the lock is inside a `Result<Lock, _>`

addresses rust-lang#119704 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
@rust-timer
Unrolled build for rust-lang#119710
b983667 
Rollup merge of rust-lang#119710 - Nilstrieb:let-_-=-oops, r=TaKO8Ki

Improve `let_underscore_lock`

- lint if the lock was in a nested pattern
- lint if the lock is inside a `Result<Lock, _>`

addresses rust-lang#119704 (comment)
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jan 25, 2024
@matthiaskrgr
Rollup merge of #119710 - Nilstrieb:let-_-=-oops, r=TaKO8Ki
a417366 
Improve `let_underscore_lock`

- lint if the lock was in a nested pattern
- lint if the lock is inside a `Result<Lock, _>`

addresses rust-lang/rust#119704 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb approved these changes

Assignees

@wesleywiser wesleywiser

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.77.0
5 participants
@chenyukang @rustbot @bors @Noratrieb @wesleywiser