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 suggestion to slice if scrutinee is a Result or Option #91343

Merged
merged 3 commits into from
Feb 1, 2022
Merged

Fix suggestion to slice if scrutinee is a Result or Option #91343

merged 3 commits into from
Feb 1, 2022

Conversation

FabianWolff
Copy link
Contributor

Fixes #91328.

@rust-highfive
Copy link
Collaborator

r? @jackh726

(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 Nov 29, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 1, 2021
jackh726
jackh726 approved these changes Dec 2, 2021
View reviewed changes
compiler/rustc_typeck/src/check/pat.rs
None
}
ty::Adt(adt_def, _)
if self.tcx.is_diagnostic_item(sym::Vec, adt_def.did) =>
Copy link
Member

Choose a reason for hiding this comment

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

So...I think I'm learning something today about how Rust's SliceIndex works (at least, unless I've missed something). So...I guess when we do a_vec[..], what's actually happening is there is an implicit deref: (a_vec.deref())[..]) - because SliceIndex is only implemented for slice, then the implicit deref is basically "forced". (At least, I think this is the case...please someone correct me if I'm wrong.)

I bring this up because rather than checking for Vec exactly, we could check if the type impls Deref<Target=X>?

Of course, this could be done as followup. This looks good otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could in general be smarter above too for Result<T> to only suggest if T: Deref<Target=X>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be nicer, but I don't know how to check for such a trait bound here. Let's leave it to a follow-up (or do you know an easy way to implement this?).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's leave this as a followup. Can you add a FIXME comment though?

src/test/ui/typeck/issue-91328.fixed Show resolved Hide resolved
@jackh726 jackh726 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 Dec 6, 2021
@FabianWolff
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2022
@jackh726
Copy link
Member

@bors delegate+ rollup

r=me with FIXME

@bors
Copy link
Contributor

bors commented Jan 30, 2022

✌️ @FabianWolff can now approve this pull request

@jackh726 jackh726 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 Jan 30, 2022
@FabianWolff
Copy link
Contributor Author

Done, thanks for reviewing this!

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Jan 31, 2022

📌 Commit 95344c0 has been approved by jackh726

@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 Jan 31, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 31, 2022
@matthiaskrgr
Rollup merge of rust-lang#91343 - FabianWolff:issue-91328-as-deref, r…
a937dd5 
…=jackh726

Fix suggestion to slice if scrutinee is a `Result` or `Option`

Fixes rust-lang#91328.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 31, 2022
Closed
@ehuss ehuss mentioned this pull request Feb 1, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2022
@bors
Auto merge of rust-lang#93534 - ehuss:rollup-9ecozo9, r=ehuss
93e8201 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#91343 (Fix suggestion to slice if scrutinee is a `Result` or `Option`)
 - rust-lang#93019 (If an integer is entered with an upper-case base prefix (0Xbeef, 0O755, 0B1010), suggest to make it lowercase)
 - rust-lang#93090 (`impl Display for io::ErrorKind`)
 - rust-lang#93456 (Remove an unnecessary transmute from opaque::Encoder)
 - rust-lang#93492 (Hide failed command unless in verbose mode)
 - rust-lang#93504 (kmc-solid: Increase the default stack size)
 - rust-lang#93513 (Allow any pretty printed line to have at least 60 chars)
 - rust-lang#93532 (Update books)
 - rust-lang#93533 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5159c01 into rust-lang:master Feb 1, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 1, 2022
@ChayimFriedman2
Copy link
Contributor

This still does not work with references:

pub fn foo(v: &Option<Vec<i32>>) {
    match v {
        Some([]) => {}
        _ => {}
    }
}
error[E0529]
 --> src/lib.rs:3:14
  |
2 |     match v {
  |           - help: consider slicing here: `v[..]`
3 |         Some([]) => {}
  |              ^^ pattern cannot match with input type `Vec<i32>`

Playground.

@ChayimFriedman2 ChayimFriedman2 mentioned this pull request Feb 14, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 16, 2022
@matthiaskrgr
Rollup merge of rust-lang#93981 - ChayimFriedman2:slice-pat-reference…
c52e064 
…-option-result, r=davidtwco

Fix suggestion to slice if scurtinee is a reference to `Result` or `Option`

Fixes rust-lang#91343 (comment) and rust-lang#91343 (comment).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 16, 2022
@matthiaskrgr
Rollup merge of rust-lang#93981 - ChayimFriedman2:slice-pat-reference…
e4335b6 
…-option-result, r=davidtwco

Fix suggestion to slice if scurtinee is a reference to `Result` or `Option`

Fixes rust-lang#91343 (comment) and rust-lang#91343 (comment).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 17, 2022
@matthiaskrgr
Rollup merge of rust-lang#93981 - ChayimFriedman2:slice-pat-reference…
2c0df80 
…-option-result, r=davidtwco

Fix suggestion to slice if scurtinee is a reference to `Result` or `Option`

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

@jackh726 jackh726 jackh726 approved these changes

Assignees

@jackh726 jackh726

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.60.0
Development

Successfully merging this pull request may close these issues.

Incorrect slicing suggestion for pattern matching against a Vec inside a Result or Option
7 participants
@FabianWolff @rust-highfive @jackh726 @bors @ChayimFriedman2 @apiraino @rustbot