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

lint: convert incoherent_fundamental_impls into hard error #49799

Merged

Conversation

@hdhoang
Copy link
Contributor

commented Apr 9, 2018

Summary for affected authors: If your crate depends on one of the following crates, please upgrade to a newer version:

  • gtk-rs: upgrade to at least 0.4
  • rusqlite: upgrade to at least 0.14
  • nalgebra: upgrade to at least 0.15, or the last patch version of 0.14
  • spade: upgrade or refresh the Cargo.lock file to use version 1.7
  • imageproc: upgrade to at least 0.16 (newer versions no longer use nalgebra)

implement #46205

r? @nikomatsakis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

This looks great, thanks!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

@bors try

cc @rust-lang/infra -- gonna want a check-only crater run of this

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

⌛️ Trying commit 70a2d80 with merge ecb60b3...

bors added a commit that referenced this pull request Apr 10, 2018

Auto merge of #49799 - hdhoang:46205_deny_incoherent_fundamental_impl…
…s, r=<try>

lint: convert incoherent_fundamental_impls into hard error

implement #46205

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

💥 Test timed out

@kennytm

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

@bors try- r- retry clean

The try build is successful.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Apr 14, 2018

Crater has been started in check-only mode, though we're testing a beta improved version of Crater so this could turn out to take up to 5-6 days with restarts.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49799/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

@hdhoang

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2018

I'm collecting the primary failures. Meanwhile lru-disk-cache 0.2.0 with the fix is on crates.io, and that fix links to this comment with several of the primaries mentioned: #46192 (comment)

Known issues

New

Spurious failure/success

  • rspotify: 300s timeout
  • verilog: toolchain 1 had read-only filesystem
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

@hdhoang awesome, great follow-up! I was away last week, so let me take a look.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

@hdhoang The 'known issues' were reported in the original issue or something like that?

As for nalgebra and pin-api, cc @sebcrozet and @withoutboats, seems like these crates are relying on a bug (INCOHERENT_FUNDAMENTAL_IMPLS) that we would like to remove support for. Have you any idea how to fix those errors? (I've not looked closely)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

OK, so, I would like to close this forwards compatibility lint, converting it into a hard error. However, I'm not psyched about breaking 131 crates. @hdhoang or @rust-lang/release, is there an easy way to understand which root crates are responsible for each failure? I'd like to see if there are a few places where we can push hard on getting a fix published in order to mitigate the impact.

@kennytm kennytm added the T-release label Apr 26, 2018

@pietroalbini

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

@nikomatsakis yes, there are 131 regressions. The "easiest" thing to do is to go and check all of them manually, but I see why you don't want to do that :P

Unfortunately, there is no way at the moment to automatically do that (as far as I know). I can try writing a small tool to build a dependency graph and check where the regressed crates are when I go home though.

@hdhoang

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

the known issues were discovered in #46192 by its crater run, with elaboration by @arielb1 here #46192 (comment)

all but bow already have fixes, either in later versions or an unmerged branch. A large swath is from glib or rusqlite, so we need the graph and then pinging the maintainers whose dependencies pull in those.

nalgebra is the root for a bunch of regressions though, and has no fix yet.

I looked at the first error from each regression txt to determine the root primary, but that wouldn't be enough enough to build an effective dep-graph to get the eco system upgraded though.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 6, 2019

It seems like the issues here relate to how we end up resolving issue #34596 and PR #59658, right?

In particular, many people (1, 2, 3) on this PR (#49799) have pointed out that a deny-by-default lint has landed, but such a lint will be silenced in dependent crates by --cap-lints, and thus there is insufficient incentive for crate clients to report bugs upstream to trhe maintainers of their dependent crates. That is exactly the problem described by issue #34596 (and with an attempted, though stalled, patch in PR #59658).

Should we postpone moving on this PR until issue #34596 is resolved? Or have I overlooked a detail here that motivates landing this change even with the overall future-incompat strategy still "up in the air"?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 6, 2019

nominating for discussion of previous comment at T-lang team meeting.

@pnkfelix pnkfelix added the I-nominated label May 6, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

We discussed this in the @rust-lang/lang meeting. Everyone who was there felt it was time to move forward, but that we ought to have an FCP. Therefore:

@rfcbot fcp merge

I propose to merge this PR, which will convert #46205 into a hard error. This is a longstanding soundness hole in our coherence guarantees, and it became a warning in Nov of 2017.

@hdhoang has heroically opened a lot of PRs (see attached comments on this PR). The number of known regressions on crates.io appears fairly minimal. I didn't count, but I would eyeball it at around 20-30 crates, none of which appeared to be major (but I might have missed some). I believe the errors can largely be corrected by upgrading dependencies (or merging pending PRs). There are also a fair number of regressions in random git repositories.

@rfcbot

This comment has been minimized.

Copy link

commented May 9, 2019

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

I'm leaving this nominated to remind people to check their boxes.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

(NB, I checked @aturon's box as they are absent)

@Centril

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@rfcbot reviewed

Please wake up bot... :)

@rfcbot

This comment has been minimized.

Copy link

commented May 16, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@Centril Centril removed the I-nominated label May 16, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

I'm going to approve since I think that FCP is very likely to complete without incident and we could always roll back if needed.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

📌 Commit 9982e7d has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

A 💯 ❤️ to @hdhoang for seeing this through!

@bors

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

⌛️ Testing commit 9982e7d with merge a614cee...

bors added a commit that referenced this pull request May 17, 2019

Auto merge of #49799 - hdhoang:46205_deny_incoherent_fundamental_impl…
…s, r=nikomatsakis

lint: convert incoherent_fundamental_impls into hard error

*Summary for affected authors:* If your crate depends on one of the following crates, please upgrade to a newer version:
- gtk-rs: upgrade to at least 0.4
- rusqlite: upgrade to at least 0.14
- nalgebra: upgrade to at least 0.15, or the last patch version of 0.14
- spade: upgrade or refresh the Cargo.lock file to use version 1.7
- imageproc: upgrade to at least 0.16 (newer versions no longer use nalgebra)

implement #46205

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: nikomatsakis
Pushing a614cee to master...

@bors bors added the merged-by-bors label May 18, 2019

@bors bors merged commit 9982e7d into rust-lang:master May 18, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@hdhoang hdhoang deleted the hdhoang:46205_deny_incoherent_fundamental_impls branch May 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.