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

Suggest using slice when encountering `let x = ""[..];` #46249

Merged
merged 4 commits into from Nov 28, 2017

Conversation

Projects
None yet
8 participants
@estebank
Contributor

estebank commented Nov 24, 2017

Fix #26319.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 24, 2017

Collaborator

r? @eddyb

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

Collaborator

rust-highfive commented Nov 24, 2017

r? @eddyb

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

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 24, 2017

Member

I don't think the index type matters, we should just not check it and just emit the suggestion.

Member

eddyb commented Nov 24, 2017

I don't think the index type matters, we should just not check it and just emit the suggestion.

Show outdated Hide outdated src/test/ui/suggestions/str-array-assignment.rs Outdated
@@ -821,6 +823,33 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
err.emit();
}
/// When encountering an assignment of an unsized trait, like `let x = ""[..];`, provide a

This comment has been minimized.

@oli-obk

oli-obk Nov 25, 2017

Contributor

Why just for assignments and not all the cases in the test?

@oli-obk

oli-obk Nov 25, 2017

Contributor

Why just for assignments and not all the cases in the test?

This comment has been minimized.

@estebank

estebank Nov 25, 2017

Contributor

The other cases in the test go through type check instead. I'll see if I can expand #46256 to also suggest in these cases. I included all the cases originally mentioned in order to make sure we don't regress in any of the cases (without noticing).

@estebank

estebank Nov 25, 2017

Contributor

The other cases in the test go through type check instead. I'll see if I can expand #46256 to also suggest in these cases. I included all the cases originally mentioned in order to make sure we don't regress in any of the cases (without noticing).

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Nov 25, 2017

Contributor

@eddyb I believe you were right so removed the index type check.

Contributor

estebank commented Nov 25, 2017

@eddyb I believe you were right so removed the index type check.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 25, 2017

Member

r? @nikomatsakis I still feel this is much more specific than it should be, but I don't have any good suggestions, other than perhaps that & isn't called "slice-ing" or "taking a slice".
a[..] does the slicing, & just borrows that slice. You can also have a[..].slice_method().

In general, both deref and indexing could use a suggestion to borrow the resulting lvalue.

Member

eddyb commented Nov 25, 2017

r? @nikomatsakis I still feel this is much more specific than it should be, but I don't have any good suggestions, other than perhaps that & isn't called "slice-ing" or "taking a slice".
a[..] does the slicing, & just borrows that slice. You can also have a[..].slice_method().

In general, both deref and indexing could use a suggestion to borrow the resulting lvalue.

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Nov 25, 2017

--> $DIR/str-array-assignment.rs:17:27
|
11 | fn main() { //~ NOTE expected `()` because of default return type
| - expected `()` because of default return type

This comment has been minimized.

@arielb1

arielb1 Nov 27, 2017

Contributor

this looks dubious. Could you look into this?

@arielb1

arielb1 Nov 27, 2017

Contributor

this looks dubious. Could you look into this?

This comment has been minimized.

@estebank

estebank Nov 27, 2017

Contributor

Yeah, I noticed that too. Still digging.

@estebank

estebank Nov 27, 2017

Contributor

Yeah, I noticed that too. Still digging.

This comment has been minimized.

@arielb1

arielb1 Nov 27, 2017

Contributor

it's also already the case on nightly. Feel free to ignore this and open an issue.

@arielb1

arielb1 Nov 27, 2017

Contributor

it's also already the case on nightly. Feel free to ignore this and open an issue.

This comment has been minimized.

@estebank
@estebank

estebank Nov 27, 2017

Contributor
@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 27, 2017

Contributor

This feels hacky to me, but I don't see any better way of implementing this (the rhs of locals is an lvalue, so checks based on it being an rvalue won't quite work).

btw, you should be checking that the problem type is a slice or string - if the return type is a struct or trait object, you don't want the error message to mention "slices".

Contributor

arielb1 commented Nov 27, 2017

This feels hacky to me, but I don't see any better way of implementing this (the rhs of locals is an lvalue, so checks based on it being an rvalue won't quite work).

btw, you should be checking that the problem type is a slice or string - if the return type is a struct or trait object, you don't want the error message to mention "slices".

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 27, 2017

Contributor

OTOH, having an Index implementation that returns trait objects is a bit weird - that would mean someone does some kind of indirection somewhere. But I think "consider borrowing here" would look better than "consider borrowing the slice" (also because str is not a "slice").

Contributor

arielb1 commented Nov 27, 2017

OTOH, having an Index implementation that returns trait objects is a bit weird - that would mean someone does some kind of indirection somewhere. But I think "consider borrowing here" would look better than "consider borrowing the slice" (also because str is not a "slice").

@arielb1 arielb1 closed this Nov 27, 2017

@arielb1 arielb1 reopened this Nov 27, 2017

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Nov 27, 2017

Contributor

@arielb1 reworded.

Contributor

estebank commented Nov 27, 2017

@arielb1 reworded.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 27, 2017

Contributor

@bors r+ rollup

Contributor

arielb1 commented Nov 27, 2017

@bors r+ rollup

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 27, 2017

Contributor

📌 Commit fa44927 has been approved by arielb1

Contributor

bors commented Nov 27, 2017

📌 Commit fa44927 has been approved by arielb1

kennytm added a commit to kennytm/rust that referenced this pull request Nov 27, 2017

Rollup merge of rust-lang#46249 - estebank:suggest-slice, r=arielb1
Suggest using slice when encountering `let x = ""[..];`

Fix rust-lang#26319.

bors added a commit that referenced this pull request Nov 27, 2017

Auto merge of #46312 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45506, #46174, #46231, #46240, #46249, #46258, #46262, #46275, #46282, #46285
- Failed merges:

bors added a commit that referenced this pull request Nov 27, 2017

Auto merge of #46312 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45506, #46174, #46231, #46240, #46249, #46258, #46262, #46275, #46282, #46285
- Failed merges:

@bors bors merged commit fa44927 into rust-lang:master Nov 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment