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

Do not emit coerce_suggestions for expr from destructuring assignment desugaring #112476

Merged
merged 1 commit into from
Jun 25, 2023

Conversation

chenyukang
Copy link
Member

Fixes #109991

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2023

r? @b-naber

(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 Jun 9, 2023
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I don't think this works for other kinds of destructuring patterns, like arrays:

fn main() {
    let x: i32;
    [x] = [&1];
}

Also, does this work to fix the nonsense suggestion for this example?

fn main() {
    let x: &i32;
    [x] = [1];
}

I think the problem here is that for destructuring assignments, the spans are wrong and don't really correspond correctly...

I don't know if fixing this one specific suggestion addresses the larger problem that other suggestions are still broken -- maybe we need a more thorough solution?

@compiler-errors
Copy link
Member

Maybe the best compromise here is to detect if the expression is from a destructuring assignment desugaring and just not offer any of the suggestions in emit_coerce_suggestions or emit_type_mismatch_suggestions...

That isn't perfect, but it should also help not suggesting nonsense like:

fn main() {
    let x: String;
    (x,) = (1,);
}
error[[E0308]](https://doc.rust-lang.org/stable/error_codes/E0308.html): mismatched types
 --> src/main.rs:3:6
  |
2 |     let x: String;
  |            ------ expected due to this type
3 |     (x,) = (1,);
  |      ^- help: try using a conversion method: `.to_string()`
  |      |
  |      expected `String`, found integer

@compiler-errors compiler-errors 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 Jun 10, 2023
@chenyukang
Copy link
Member Author

I don't think this works for other kinds of destructuring patterns, like arrays:

fn main() {
    let x: i32;
    [x] = [&1];
}

Also, does this work to fix the nonsense suggestion for this example?

fn main() {
    let x: &i32;
    [x] = [1];
}

I think the problem here is that for destructuring assignments, the spans are wrong and don't really correspond correctly...

I don't know if fixing this one specific suggestion addresses the larger problem that other suggestions are still broken -- maybe we need a more thorough solution?

Yes, we need a better general solution if we want to suggest like this:

help: consider dereferencing the borrow
   |
17 |     (a, b) = (123, *&mut 123);
   |                    +

we don't have expr for &mut 123 passed into suggest_deref_or_ref.

@compiler-errors
Copy link
Member

I don't think it's, in general, possible to suggest what you said above. You can do destructuring assignments where the expressions do not align, like:

let x = (1, 2);
(a, b) = x;

so there's no way to in general pair a with 1 and b with 2 in the type mismatch. it's better to just suppress the suggestions altogether.

@chenyukang chenyukang changed the title Do not suggests dereferencing LHS of tuple-assign Do not emit coerce_suggestions for expr from destructuring assignment desugaring Jun 24, 2023
@chenyukang
Copy link
Member Author

Updated code to suppress the suggestions altogether.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

r=me after blessing tests and squashing the commits

@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@chenyukang
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jun 25, 2023

📌 Commit 33f73c2 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2023
@bors
Copy link
Contributor

bors commented Jun 25, 2023

⌛ Testing commit 33f73c2 with merge 3c5d71a...

@bors
Copy link
Contributor

bors commented Jun 25, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 3c5d71a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 25, 2023
@bors bors merged commit 3c5d71a into rust-lang:master Jun 25, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 25, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3c5d71a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-0.8%, -0.5%] 6
Improvements ✅
(secondary)
-1.0% [-1.1%, -1.0%] 4
All ❌✅ (primary) -0.6% [-0.8%, -0.5%] 6

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.7% [1.0%, 4.5%] 11
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.7% [1.0%, 4.5%] 11

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 662.562s -> 663.05s (0.07%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

Compiler suggests dereferencing LHS of tuple-assign
7 participants