Bump IMPLIED_BOUNDS_ENTAILMENT to Deny + ReportNow by compiler-errors · Pull Request #106465 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits2 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

compiler-errors

#105575 (comment)

and then later in the same cycle increase the lint to deny and change it to FutureCompatReportNow in this nightly cycle.

r? @lcnr when they're back from holiday 😄

@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

Jan 4, 2023

@jackh726

Agh, I think I missed the exact wording in the other PR.

I think we should change the lint to ReportNow at the end of this cycle, but I'm wary of bumping to deny.

Regardless, if (/when) we do bump to deny, we need to run crater to assess the impact.

@lcnr

still have 10 days until the next beta branch. I believe that this PR is the correct approach here as even if we still have the same impact as in #105483 (comment) we should quickly move this towards a hard error.

I do feel like it should be possible to get more accurate spans for the lint though 😅 it would be awesome if we're able to print a signature with the correct types '^^ not sure how easy that is though :/

@jackh726

@bors

⌛ Trying commit 7369c52cf1bc76f73f3f9643144030271cf57056 with merge 3ac4ab8283c0bed583d0a33ab92c6a77c486aa98...

@bors

☀️ Try build successful - checks-actions
Build commit: 3ac4ab8283c0bed583d0a33ab92c6a77c486aa98 (3ac4ab8283c0bed583d0a33ab92c6a77c486aa98)

@jackh726

@craterbot

👌 Experiment pr-106465 created and queued.
🤖 Automatically detected try build 3ac4ab8283c0bed583d0a33ab92c6a77c486aa98
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

🚧 Experiment pr-106465 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@lcnr

I've talked about this PR with @jackh726 yesterday and our thoughts were that to land this we have to improve the diagnostic to be more actionable for users.

I think the easiest change would be to look at the errors in CheckImpliedWfMode and get back the method argument which was responsible for the error. Then point at that argument in the error (you could emit this lint separately for each argument involved) and also print the "expected argument", i.e. the argument you would get by substituting the argument of the trait method with the impl substs.

We should also file PRs to the affected crates where possible. Going to do that myself rn.

@lqd

It'd then also be great to release a fixed version of rustc-serialize, somehow notify users (with a rustsec advisory maybe) that a simple cargo update would fix the future-incompat report they'll be encountering, etc.

Fixing the method signature applies cleanly, and apart from logistics about un-archiving the repo and a crates.io release, this all sounds very doable and I'd like to help if t-types agrees we should do this.

@lcnr

@lcnr

It'd then also be great to release a fixed version of rustc-serialize, somehow notify users (with a rustsec advisory maybe) that a simple cargo update would fix the future-incompat report they'll be encountering, etc.

Fixing the method signature applies cleanly, and apart from logistics about un-archiving the repo and a crates.io release, this all sounds very doable and I'd like to help if t-types agrees we should do this.

yes, but also. Having a future compat report now lint for something we've stopped maintaining for 6 years now might be a good way to get people to finally switch to a newer crate 😁

we should definitely do that before making this lint a hard error though ^^

@craterbot

@compiler-errors

@compiler-errors

I pushed some logic to make the spans better, + a best effort suggestion for what to replace a type with.

@compiler-errors

lcnr

Comment on lines +440 to +443

let mapping = std::iter::zip(
tcx.fn_sig(trait_m.def_id).bound_vars(),
tcx.fn_sig(impl_m.def_id).bound_vars(),
)

Choose a reason for hiding this comment

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

that's wrong in subtle ways because impls aren't required to order their bound vars in the same way. That seems fine considering that this lint will only exist for a few releases and works correctly for all the issues we've found via crater.

@lcnr

@bors

📌 Commit eaa7cc8 has been approved by lcnr

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-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jan 13, 2023

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 13, 2023

@matthiaskrgr

…S_ENTAILMENT, r=lcnr

Bump IMPLIED_BOUNDS_ENTAILMENT to Deny + ReportNow

rust-lang#105575 (comment)

and then later in the same cycle increase the lint to deny and change it to FutureCompatReportNow in this nightly cycle.

r? @lcnr when they're back from holiday 😄

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 13, 2023

@matthiaskrgr

…S_ENTAILMENT, r=lcnr

Bump IMPLIED_BOUNDS_ENTAILMENT to Deny + ReportNow

rust-lang#105575 (comment)

and then later in the same cycle increase the lint to deny and change it to FutureCompatReportNow in this nightly cycle.

r? @lcnr when they're back from holiday 😄

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 13, 2023

@bors

…iaskrgr

Rollup of 10 pull requests

Successful merges:

Failed merges:

r? @ghost @rustbot modify labels: rollup

@lcnr lcnr mentioned this pull request

Jan 19, 2023

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Mar 20, 2023

@he32

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Apr 8, 2023

@he32

Pkgsrc changes:

Upstream changes:

Version 1.68.2 (2023-03-28)

Version 1.68.1 (2023-03-23)

Version 1.68.0 (2023-03-09)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Misc

Compatibility Notes

nternal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.67.0 (2023-01-26)

Language

Compiler

Added and removed targets:

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

@lcnr lcnr mentioned this pull request

Nov 15, 2023

@lcnr lcnr mentioned this pull request

Dec 1, 2023

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.