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

Lower the priority of unstable methods when picking a candidate. #48552

Merged
merged 6 commits into from
Mar 24, 2018

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Feb 26, 2018

Previously, when searching for the impl of a method, we do not consider the stability of the impl. This leads to lots of insta-inference-regressions due to method ambiguity when a popular name is chosen. This has happened multiple times in Rust's history e.g.

This PR changes the probing order so that unstable items are considered last. If a stable item is found, the unstable items will not be considered (but a future-incompatible warning will still be emitted), thus allowing stable code continue to function without using qualified names.

Once the unstable feature is stabilized, the ambiguity error will still be emitted, but the user can also use newly stable std methods, while the current situation is that downstream user is forced to update the code without any immediate benefit.

(I hope that we could bring back Ord::clamp if this PR is merged.)

@kennytm kennytm added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 26, 2018
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2018
@Mark-Simulacrum
Copy link
Member

I think something along these lines is a good idea, and I especially like how it moves breakage considerations mostly to stabilization PRs where we can better evaluate the relative worth of a given feature compared to the breakage it could cause.

@kennytm kennytm mentioned this pull request Feb 26, 2018
@bluss
Copy link
Member

bluss commented Feb 26, 2018

That's a fantastic idea. If you do this, you can have all the best parts of itertools 😄 and I don't mind.

@kennytm kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2018
@petrochenkov
Copy link
Contributor

Previous iteration rust-lang/rfcs#1321
cc @rust-lang/lang

@petrochenkov
Copy link
Contributor

This is a big decision.
r? @nikomatsakis

@kennytm
Copy link
Member Author

kennytm commented Feb 26, 2018

(Note: the CI error is due to tcx.visibility() returning Public for the trait core::iter_private::TrustedRandomAccess.)

(Edit: Bypassed by treating unmarked APIs as stable.)

@clarfonthey
Copy link
Contributor

Another thing to consider is that people could use the semver trick to hot-swap inherent methods and types with ones from the standard library once they've stabilised.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2018
@kennytm
Copy link
Member Author

kennytm commented Feb 27, 2018

Thanks for the pointer @petrochenkov :)

I’ve checked RFC 1321, this PR is slightly different from the RFC that unstable items are still considered (instead of completely hidden) but only with lowest priority. This corresponds to the “weak” alternative in the RFC:

Unstable items could instead be considered "weak", where external items can override them when there are conflicts, but would otherwise behave as they currently do.

The reason of closing the RFC were that:

  1. Implemention subtleties, where stability checking 2 years ago was a mess and consequence of messing up the type checker would be huge. In this PR I reused the existing TyCtxt::check_stability function which should reduce problems due to implementation differences, though I do agree that its interaction with item privacy is still less than ideal. The change in the type-checker side seems straightforward enough that won’t cause big problems.

  2. We can learn about conflicting names early. As mentioned in the RFC, we could add a future-compatibility lint if we want it, but I don’t think a hard error is the correct response. Basically what Unstable visibility RFC rfcs#1321 (comment) said.

@aturon
Copy link
Member

aturon commented Feb 27, 2018

Thanks for this PR!

I don't have time to leave an in-depth comment right now, but yes the major problem with an approach like this is that, in some sense, it defeats the purpose of nightly for testing; we want to know about these conflicts.

That said, I agree that doing this via a warning instead gives the best of all worlds. @nikomatsakis said something similar on the original thread, and also pointed out how difficult it may be to get this right. I'll let him comment on the current state of affairs.

@kennytm kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2018
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2018
@bors
Copy link
Contributor

bors commented Mar 1, 2018

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

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2018
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 23, 2018
@bors
Copy link
Contributor

bors commented Mar 23, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Mar 23, 2018

📌 Commit 27f92ef has been approved by nikomatsakis

@SimonSapin
Copy link
Contributor

@kennytm That explains why it so in the current implementation, but as a compiler user it still seems wrong to me to warn about potential conflicts with traits that would actually not conflict because they are not in scope.

@bors
Copy link
Contributor

bors commented Mar 23, 2018

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

@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 Mar 23, 2018
@kennytm
Copy link
Member Author

kennytm commented Mar 23, 2018

@SimonSapin the scope problem only affects suggestions. If the code is real, the out-of-scope traits won't be considered and thus won't have any spurious warnings. The last commit also disabled the warnings if the probe is used for suggestions (the cause of all spurious warnings so far).

@bors r=nikomatsakis

(Renamed "epoch" to "edition".)

@bors
Copy link
Contributor

bors commented Mar 23, 2018

📌 Commit 17cc3d7 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 Mar 23, 2018
@bors
Copy link
Contributor

bors commented Mar 24, 2018

⌛ Testing commit 17cc3d7 with merge a0b0f5f...

bors added a commit that referenced this pull request Mar 24, 2018
Lower the priority of unstable methods when picking a candidate.

Previously, when searching for the impl of a method, we do not consider the stability of the impl. This leads to lots of insta-inference-regressions due to method ambiguity when a popular name is chosen. This has happened multiple times in Rust's history e.g.

* `f64::from_bits` #40470
* `Ord::{min, max}` #42496
* `Ord::clamp` #44095 (eventually got reverted due to these breakages)
* `Iterator::flatten` #48115 (recently added)

This PR changes the probing order so that unstable items are considered last. If a stable item is found, the unstable items will not be considered (but a future-incompatible warning will still be emitted), thus allowing stable code continue to function without using qualified names.

Once the unstable feature is stabilized, the ambiguity error will still be emitted, but the user can also use newly stable std methods, while the current situation is that downstream user is forced to update the code without any immediate benefit.

(I hope that we could bring back `Ord::clamp` if this PR is merged.)
@bors
Copy link
Contributor

bors commented Mar 24, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.