Add future incompatibility lint for array.into_iter()
#66017
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
5 tasks
This comment has been minimized.
This comment has been minimized.
|
(My way was a hack; a real lint like this is a much better plan.) I think a lint here is definitely valuable. Could even run it on slices as well, since in theory we might want to be able to use unsized locals to have |
scottmcm
reviewed
Nov 1, 2019
cramertj
reviewed
Nov 1, 2019
LukasKalbertodt
force-pushed
the
array-into-iter-lint
branch
from
7d495ef
to
9d16b5d
Compare
|
I just pushed a new version. I think it's fine now. My code now checks if the original expr type is an array and if autoref coercion happened. I think this should exactly what we want. At least the UI test now works as you expected. |
Aaron1011
reviewed
Nov 3, 2019
LukasKalbertodt
force-pushed
the
array-into-iter-lint
branch
from
9d16b5d
to
c3b20e2
Compare
This comment has been minimized.
This comment has been minimized.
LukasKalbertodt
force-pushed
the
array-into-iter-lint
branch
from
c3b20e2
to
a9ca85d
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Can someone tell me what I'm doing wrong here? |
|
It looks like the future-compat warnings include another warning (?) within the notes. Try changing the annotations like this: |
LukasKalbertodt
force-pushed
the
array-into-iter-lint
branch
from
a9ca85d
to
eedf2d2
Compare
|
Does this lint catch this example? trait IntoIteratorExt: IntoIterator + Sized {
fn into_iter_first(self) -> Option<Self::Item> {
self.into_iter().next()
}
}
impl<T: IntoIterator> IntoIteratorExt for T {}
fn foo(value: Option<&i32>) {
println!("{}", value.unwrap());
}
fn main() {
foo([1,2,3].into_iter_first());
}This could potentially be split over multiple crates. |
|
@TethysSvensson Nope, the lint does not catch that. I'd guess this is a lot more complicated to test for. As I won't be able to look into this in the next days, I'd say we merge this as is (if everything else is OK) to get it into 1.40 still. Unless it's too late for that anyway... From the regressed crates that I looked at, no crate failed due to something like you are suggesting. You are absolutely right that this would break, but as far as I know, we haven't seen this yet. Maybe we can – after merging this PR – create a PR that denies-by-default this lint and run crater on the regressed crates again. By checking how many still regress (this time due to the lint), we can see how many regressions the lint doesn't catch. |
2 tasks
LukasKalbertodt
force-pushed
the
array-into-iter-lint
branch
from
eedf2d2
to
51a4cd0
Compare
Centril
reviewed
Nov 6, 2019
| @@ -0,0 +1,87 @@ | |||
| use crate::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass}; | |||
| use rustc::{ | |||
There was a problem hiding this comment.
Nit: I believe we prefer single line imports.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r+ |
|
📌 Commit 51a4cd0fc62345ccdf4b698b3b283fcd193e9d59 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- |
bors
added
S-waiting-on-author
As we might want to add `IntoIterator` impls for arrays in the future, and since that introduces a breaking change, this lint warns and suggests using `iter()` instead (which is shorter and more explicit).
LukasKalbertodt
force-pushed
the
array-into-iter-lint
branch
from
51a4cd0
to
761ba89
Compare
|
Pushed again. Sorry for delay. Should be green now, I tested locally. |
|
@bors r=matthewjasper |
|
📌 Commit 761ba89 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Nov 7, 2019
…t, r=matthewjasper Add future incompatibility lint for `array.into_iter()` This is for rust-lang#65819. This lint warns when calling `into_iter` on an array directly. That's because today the method call resolves to `<&[T] as IntoIterator>::into_iter` but that would change when adding `IntoIterator` impls for arrays. This problem is discussed in detail in rust-lang#65819. We still haven't decided how to proceed exactly, but it seems like adding a lint is a good idea regardless? Also: this is the first time I implement a lint, so there are probably a lot of things I can improve. I used a different strategy than @scottmcm describes [here](rust-lang#65819 (comment)) since I already started implementing this before they commented. ### TODO - [x] Decide if we want this lint -> apparently [we want](rust-lang#65819 (comment)) - [x] Open a lint-tracking-issue and add the correct issue number in the code -> rust-lang#66145
bors
added a commit
that referenced
this pull request
Nov 7, 2019
Rollup of 5 pull requests Successful merges: - #59789 (Revert two unapproved changes to rustc_typeck.) - #65752 (Use structured suggestions for missing associated items) - #65884 (syntax: ABI-oblivious grammar) - #65974 (A scheme for more macro-matcher friendly pre-expansion gating) - #66017 (Add future incompatibility lint for `array.into_iter()`) Failed merges: - #66056 (rustc_metadata: Some reorganization of the module structure) r? @ghost
|
Well that deprecates the |
flip1995
added a commit
to Manishearth/rust-clippy
that referenced
this pull request
Nov 7, 2019
This lint was uplifted/reimplemented by rustc. Rustup to rust-lang/rust#66017
flip1995
added a commit
to Manishearth/rust-clippy
that referenced
this pull request
Nov 7, 2019
This lint was uplifted/reimplemented by rustc. Rustup to rust-lang/rust#66017
flip1995
added a commit
to Manishearth/rust-clippy
that referenced
this pull request
Nov 7, 2019
This lint was uplifted/reimplemented by rustc. Rustup to rust-lang/rust#66017
bors bot
added a commit
to rust-itertools/itertools
that referenced
this pull request
Sep 22, 2020
475: Trait impls for tuples of arity <= 12 r=jswrenn a=jswrenn I stopped at 12, because that's where libcore stops. fixes #428 fixes #398 476: Undeprecate and optimize `fold_while` r=jswrenn a=jswrenn Prompted by #469. 479: fix compiler warning on array.into_iter() r=jswrenn a=dmitris Fix the compile warnings listed below by changing `into_iter()` invocation to `iter()` in the `impl_zip_iter` macro as recommended in rust-lang/rust#66145. For additional background, see also rust-lang/rust#65819 and rust-lang/rust#66017 (the latter is the linter change producing the warning). ``` $ cargo build Compiling itertools v0.9.0 (/Users/dsavints/dev/hack/github.com/rust-itertools/itertools) warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added. --> src/ziptuple.rs:111:47 | 111 | let size = *[$( $B.len(), )*].into_iter().min().unwrap(); | ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter` ... 128 | impl_zip_iter!(A); | ------------------ in this macro invocation | = note: `#[warn(array_into_iter)]` on by default = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #66145 <rust-lang/rust#66145> = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added. --> src/ziptuple.rs:111:47 | 111 | let size = *[$( $B.len(), )*].into_iter().min().unwrap(); | ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter` ... 129 | impl_zip_iter!(A, B); | --------------------- in this macro invocation | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #66145 <rust-lang/rust#66145> = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) ``` Co-authored-by: Jack Wrenn <me@jswrenn.com> Co-authored-by: Dmitry Savintsev <dsavints@verizonmedia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is for #65819. This lint warns when calling
into_iteron an array directly. That's because today the method call resolves to<&[T] as IntoIterator>::into_iterbut that would change when addingIntoIteratorimpls for arrays. This problem is discussed in detail in #65819.We still haven't decided how to proceed exactly, but it seems like adding a lint is a good idea regardless?
Also: this is the first time I implement a lint, so there are probably a lot of things I can improve. I used a different strategy than @scottmcm describes here since I already started implementing this before they commented.
TODO
array_into_iterfuture compatibility lint #66145