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

Update array_into_iter lint for 1.53 and edition changes. #85682

Merged
merged 7 commits into from
Jun 26, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented May 25, 2021

This updates the array_into_iter lint for Rust 1.53 and the edition changes.

See #84513

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented May 25, 2021

@Aaron1011 The src/test/ui/lint/issue-78660-cap-lints-future-compat.rs test breaks on this change. I'm not exactly sure what it tests, so I don't want to just --bless the output. (The .stderr file is gone, as there is no more output after this change.) Since you added that test, can you take a look?

@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member Author

m-ou-se commented May 25, 2021

This now produces the message

this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!

which isn't entirely accurate. It will not become a hard error per se, it will change meaning.

Should this use something other than @future_incompatible?

@rust-log-analyzer

This comment has been minimized.

@m-ou-se m-ou-se added the D-edition Diagnostics: An error or lint that should account for edition differences. label May 25, 2021
@Aaron1011
Copy link
Member

@m-ou-se The message about a hard error is emitted for all future compare lints - I can add a way to customize it on a per-lint basis.

Whether or not we still want the @future_incompatible is an interesting question. When I applied it to this lint, the plan was to change the array into_iter behavior on all editions, which would be a breaking change. As a result, we wanted it to be included in the cargo future-incompat reports, so that users would be alerted about dependencies that would eventually break.

Now that the behavior is edition gated, upstream dependencies will continue to work (since they'll be on the 2015 or 2018 edition). I think there's no longer any need to notify users about such crates in the report - while it would be nice to minimize the number of crate's relying on the impl method resolution hack, I don't think this justifies showing a notification to authors of unrelated crates.

I think we can now remove the @future_incompatible for this lint.

@estebank
Copy link
Contributor

I think it might be ok to land the lint with the slightly inaccurate note about the edition and fix it on a separate PR.


I didn't notice any 2021 edition tests to check that the lint doesn't trigger when it shouldn't. I don't expect them to be broken, but I feel more at ease when we have tests for any potential regression.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 2, 2021

Sounds like the future in future_incompatible doesn't necessarily refer to new editions, which are more like parallel timelines. ^^

Here, the comment suggests that future_incompatible lints with an edition set are 'not future incompatible':

// If this is a future incompatible that is not an edition fixing lint
// it'll become a hard error, so we have to emit *something*. Also,
// if this lint occurs in the expansion of a macro from an external crate,
// allow individual lints to opt-out from being reported.
let not_future_incompatible =
future_incompatible.map(|f| f.edition.is_some()).unwrap_or(true);

But here, it uses the 'standard message' in that case, which contains the "is being phased out" and "it will become a hard error" text:

} else if let Some(edition) = future_incompatible.edition {
format!("{} in the {} edition!", STANDARD_MESSAGE, edition)

That should be easy to change.

However, then the question remains: Do we consider these things 'future incompatibility lints' or not? Same question for the non_fmt_panic lint, which warns for panic!("{}") etc. That one is currently not marked as a future_incompatibility lint. Both of these lints have the property that they're also useful warnings even if you're not upgrading to the new edition, because they indicate some mistake or misunderstanding.

I'm not exactly sure what this future_incompatibility property of a lint implies, other than just adding that text to a message. Does it change behaviour anywhere?

@Aaron1011 The src/test/ui/lint/issue-78660-cap-lints-future-compat.rs test breaks on this change. I'm not exactly sure what it tests, so I don't want to just --bless the output. (The .stderr file is gone, as there is no more output after this change.) Since you added that test, can you take a look?

@Aaron1011 can you tell me what I should do with that test? That's currently blocking this PR from passing the CI tests.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 7, 2021

I'm wondering if it's worth backporting this. Otherwise, 1.53 will ship with IntoIterator for arrays, but with a lint that claims that implementation doesn't exist.

@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 10, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 10, 2021

@Aaron1011 The src/test/ui/lint/issue-78660-cap-lints-future-compat.rs test breaks on this change. I'm not exactly sure what it tests, so I don't want to just --bless the output. (The .stderr file is gone, as there is no more output after this change.) Since you added that test, can you take a look?

@Aaron1011 can you tell me what I should do with that test? That's currently blocking this PR from passing the CI tests.

@Aaron1011 I have removed that test to unblock this.

I didn't notice any 2021 edition tests to check that the lint doesn't trigger when it shouldn't.

That already exists here: src/test/ui/iterators/into-iter-on-arrays-2021.rs

I think we can now remove the @future_incompatible for this lint.

I have removed it for now. Once we have a way to change/disable the 'hard error' message, we should add it to make sure it becomes part of rust_2021_compatibility.

@m-ou-se m-ou-se added D-confusing Diagnostics: Confusing error or lint that should be reworked. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels Jun 10, 2021
@Mark-Simulacrum
Copy link
Member

This needs to get merged or at least approved in the next ~24 hours, ideally, if we're to backport it to beta, but I myself don't have enough time to investigate/review it. I suspect that means that 1.53 will not have the nice lint, unfortunately, but that doesn't seem too bad. We're always improving diagnostics anyway.

@pnkfelix
Copy link
Member

unilaterally declining for beta-backport to 1.53.

Its not critical for this to get in there; the inconsistency noted by @m-ou-se isn't that bad (i.e. its not fantastic for a diagnostic to claim an implementation doesn't exist when it in fact does, but that scenario is still way better than a diagnostic to claim an implementation does exist when it in fact does not.)

@pnkfelix pnkfelix removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 14, 2021
@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 22, 2021

📌 Commit a8614bc has been approved by nikomatsakis

@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 Jun 22, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 22, 2021
…tsakis

Update array_into_iter lint for 1.53 and edition changes.

This updates the array_into_iter lint for Rust 1.53 and the edition changes.

See rust-lang#84513

r? `@estebank`
@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 25, 2021

Apparently // run-rustfix uses Filter::Everything by default instead of Filter::MachineApplicableOnly. That doesn't work when the suggestions are mutually exclusive. Added // rustfix-only-machine-applicable.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 25, 2021

Alright, tests pass again after rebasing. Let's try again.

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 25, 2021

📌 Commit 3a92598a582607fc59676715408e80c52d2b8d12 has been approved by nikomatsakis

@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, 2021
@bors
Copy link
Contributor

bors commented Jun 25, 2021

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout array-into-iter-2 (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self array-into-iter-2 --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Removing src/test/ui/lint/issue-78660-cap-lints-future-compat.rs
Auto-merging src/test/ui/iterators/into-iter-on-arrays-lint.stderr
CONFLICT (content): Merge conflict in src/test/ui/iterators/into-iter-on-arrays-lint.stderr
Auto-merging src/test/ui/iterators/into-iter-on-arrays-lint.rs
CONFLICT (content): Merge conflict in src/test/ui/iterators/into-iter-on-arrays-lint.rs
Auto-merging src/test/ui/iterators/into-iter-on-arrays-lint.fixed
CONFLICT (content): Merge conflict in src/test/ui/iterators/into-iter-on-arrays-lint.fixed
Auto-merging src/test/ui/iterators/into-iter-on-arrays-2018.stderr
CONFLICT (content): Merge conflict in src/test/ui/iterators/into-iter-on-arrays-2018.stderr
Auto-merging src/test/ui/iterators/into-iter-on-arrays-2018.rs
CONFLICT (content): Merge conflict in src/test/ui/iterators/into-iter-on-arrays-2018.rs
Auto-merging compiler/rustc_lint/src/array_into_iter.rs
CONFLICT (content): Merge conflict in compiler/rustc_lint/src/array_into_iter.rs
Automatic merge failed; fix conflicts and then commit the result.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 25, 2021
@bors
Copy link
Contributor

bors commented Jun 25, 2021

☔ The latest upstream changes (presumably #86627) made this pull request unmergeable. Please resolve the merge conflicts.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 26, 2021

Rebased for #86330. This is now a future_incompatibility lint again (set to FutureIncompatibilityReason::EditionSemanticsChange(Edition::Edition2021) by #86330).

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 26, 2021

📌 Commit dbdf7c7 has been approved by nikomatsakis

@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 26, 2021
@bors
Copy link
Contributor

bors commented Jun 26, 2021

⌛ Testing commit dbdf7c7 with merge f2571a2...

@bors
Copy link
Contributor

bors commented Jun 26, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing f2571a2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 26, 2021
@bors bors merged commit f2571a2 into rust-lang:master Jun 26, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 26, 2021
@m-ou-se m-ou-se deleted the array-into-iter-2 branch June 26, 2021 20:27
@lcnr lcnr added the A-const-generics Area: const generics (parameters and arguments) label Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) D-confusing Diagnostics: Confusing error or lint that should be reworked. D-edition Diagnostics: An error or lint that should account for edition differences. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.