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

Unstable visibility RFC #1321

Closed
wants to merge 2 commits into from
Closed

Unstable visibility RFC #1321

wants to merge 2 commits into from

Conversation

arcnmx
Copy link

@arcnmx arcnmx commented Oct 14, 2015

Proposal to prevent unstable items from causing name resolution conflicts with downstream code.

Rendered

@arcnmx arcnmx force-pushed the unstability branch 4 times, most recently from 7b570da to dc640d5 Compare October 14, 2015 19:39
@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Oct 15, 2015
@nikomatsakis nikomatsakis self-assigned this Oct 15, 2015
@nikomatsakis
Copy link
Contributor

It's worth pointing out that method scoping in Rust is somewhat distinct from other name resolution. In particular, the read_exact method is an unstable method in a stable trait, so just adjusting the lexical name resolution would still result in the trait being imported -- we have to also consider stability at method resolution time, when deciding whether a trait is "applicable".

I think this is perhaps a good idea -- but I guess we'd have to examine the complexity of implementing it. Ideally, we would report a "future compatibility" lint warning, rather than just silently allowing the code to compile. This would give people a good opportunity to complain loudly about the pending breaking change (or else prepare for it, by renaming methods).

@arcnmx
Copy link
Author

arcnmx commented Oct 17, 2015

Hm, I wonder if we could also use this RFC to make unstable trait impls "invisible" as well, as this currently seems to be a problem with landing unstable features.

In general I think it's a good step forward, as it's currently strange how unstable and experimental code can "leak" into the stable rust ecosystem... but the complexity of implementation and impact on the rustc codebase is definitely a concern - and that's the main point I'm not particularly familiar with.

@nikomatsakis
Copy link
Contributor

Hear ye, hear ye. This RFC is entering final comment period.

As of now, the general feeling was that we would most likely not accept this RFC. It can be annoying to get conflicts from unstable items, but on the other hand it only delays the day of reckoning -- those unstable items will become stable at some point in the future, and when that happens, it will be impossible to fix (since they are stable). In contrast, if we learn about conflict names early, there is still time to make adjustments. But no final decision has been made.

@arcnmx
Copy link
Author

arcnmx commented Mar 17, 2016

Yeah, not many opinions on this one...

those unstable items will become stable at some point in the future, and when that happens, it will be impossible to fix (since they are stable). In contrast, if we learn about conflict names early, there is still time to make adjustments. But no final decision has been made.

I'm not sure that's quite right... Learning about conflicts early is certainly valuable, but there's no way to make adjustments when it happens. It's a hard error, and you can't switch to using the official method because it's not stable yet. If the error happens once stable hits, you can simply remove the extension trait and migrate to using the new interface. Before then you're just plain stuck. A warning would be a bit more fair, maybe one that only triggers on nightly...

Personally, it seems valuable to me to clean up the current unstable story, as touched on in the RFC:

  1. trait impls are insta-stable, and really shouldn't be. PRs sometimes need to tiptoe around this when adding new functionality, with TODO markers or similar and remember to only add impls once something becomes stable.
  2. unstable methods and types appear in the stable docs, which is just plain confusing and strange...
  3. allow_internal_unstable is a wart that shouldn't really exist; if there's a use case of a library needing to use private internal APIs within macros and the like, it shouldn't really be an unstable feature specifically only available to libstd, and abusing unstable APIs.
  4. unstable methods still affect resolution, in what originally motivated this RFC.

It's just kind of unclean the way these unstable features and changes leak into the stable ecosystem. I think this particular instance/motivation is rare enough that it's not a huge deal, but it's indicative of a larger problem with stability in rust right now.

Also, was this meant to be tagged with the FCP label, and mentioned in TWIR, etc? I noticed it missing there alongside my other RFC, 1346

@aturon aturon added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed I-nominated labels Mar 17, 2016
@nikomatsakis
Copy link
Contributor

In the @rust-lang/lang meeting, we decided to extend the FCP period for another given that we forgot to tag with the final-comment-period label.

That said, I wanted to respond to this point:

A warning would be a bit more fair, maybe one that only triggers on nightly...Personally, it seems valuable to me to clean up the current unstable story

This is a very good point. I do agree that a warning might be the optimal outcome -- in general, we try to do "soft" transitions rather than hard errors -- and I also agree that there is plenty of room for improvement in our stability story. That said, one of the major obstacles here is that this code is really hard to get right. For example, having recently implemented #1445, I can personally attest that the code for deciding when a particular use of an unstable item is "stable" vs "unstable" is very subtle and sensitive.

I am personally quite concerned about trying to implement this RFC without a clear and well thought out plan for making that setup more robust. Basically we would be touching on two core -- and complicated -- aspects of the Rust compiler:

  • trait matching and method dispatch
  • stability checking / hygiene (stability is basically a poorly implemented hygiene check)

Both of these are under very active adjustment and refactoring as we speak. For example, we are working on major refactorings to trait matching to accommodate specialization, to handle associated types differently (lazy normalization, caching, etc), and also just general work as part of the MIR transition. Stability checking is kind of a mess now, and it would likely benefit from some of the more advanced forms of hygiene being contemplated as part of the plugin/macro effort (e.g., unsafety hygiene). Making e.g. impls unstable and issuing warnings is a non-trivial task. For example, for better or worse, the set of available impls can influence inference of unbound type variables, which means that errors that result can appear somewhat later.

It's worth nothing that the consequences of messing stuff up here would be large. Every time we fix a bug in the type system and trait checker, there is almost always some kind of fallout from crates that (unintentionally, typically) were relying on the bug. I would expect the same to be true here. (Plus, if we fail to enforce stability, it will lead to a mess as well.)

Therefore, my feeling is that while the ideas here are good, the time may not yet be ripe to think about implementing them (and if we were to do so, I would want to make sure we had a solid plan).

@retep998
Copy link
Member

This would have been useful for #1461. The methods had to be added as insta-stable, otherwise they would have caused trait methods from net2 to be overridden by the unstable inherent methods resulting in compiler errors.

@aturon
Copy link
Member

aturon commented Apr 1, 2016

The lang team has decided to close this RFC for the time being, for the reasons outlined by @nikomatsakis's comment. We could consider revisiting when the relevant compiler infrastructure is in a better place.

Thanks for the RFC, @arcnmx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants