Adjust default object bounds [breaking change] #1156
Conversation
|
I won't repeat myself, but 👎. Rust 1.0 should be considered the end of the era of breaking changes and going around fixing things to work in the latest compiler, period. |
|
@comex would you prefer a 2.0.0 this early? |
|
I'd prefer that either (a) the change not be made at all, or (b) a version number attribute be used. In regard to the latter, I recognize the argument from the comment linked in the PR summary about Rust 1.16 vs. 1.17, but when changes to standard libraries are considered in addition to the language (especially adding new public methods to types and traits, which is always potentially breaking), I believe the advantage of being able to relatively easily extend the compiler to allow true backwards compatibility (e.g. by hiding those methods, which I've elaborated on elsewhere) outweighs the drawback of version number confusion. |
|
I would prefer a version number be used as well, in general. As far as this change goes: I think it's early enough that it is okay (under the theory that backwards incompatibility only matters if it really breaks something), but I'm also not someone using Rust in production (which the crates.io lint isn't going to catch). |
|
I doubt that something like this backward-incompatible change would even be considered if it wasn't suggested by someone from core team. I would prefer not making such non-safety related backward-incompatible changes until 2.0. Something like Python's |
|
(Should this and the other RFCs mentioned in the subteam report be tagged final-comment-period?) |
|
In the last week or so, I've seen two instances of people being tripped up by this. There's this StackOverflow question, and this exchange on #rust. I'm in 100% agreement that the current rules are unintuitive and baffling, the error message confusing, and the solution completely non-obvious, nor particularly discoverable. It turns out that the current rules are rubbish. :P Given that, and the lengths that have been gone to in order to locate code this breaks... I think this is an acceptable break under these specific circumstances. |
|
I agree the current rule is unintuitive, but a partial alternative solution to your complaint would be improving the error messages for such cases... |
|
@comex On the other hand, I saw a question today from a user where the solution was explicitly spelled out in the error message. Improving the message would help, but it doesn't hold a candle to changing the rules so the problem itself happens less often. If we take it as read that opt-in to breaking changes isn't going to happen (which appears to be the way the wind is blowing at the moment), then I'd rather see this particular change happen now than not. It sucks, as is almost certainly going to give Rust a bit of a black eye for a while... but it's that or being stuck with it. I certainly don't know which will have been the correct choice in retrospect, which is why I don't envy the core team on this. :P |
|
👍 |
|
@DanielKeep @nikomatsakis I fully agree that this change should be made as soon as possible. In my somewhat related discussion of breaking API changes, I not only proposed to allow breaking changes for fixing security holes, but also for footguns, if the resulting breakage from the change is less than the breakage resulting from the mis-feature. Following DanielKeep's observations, that seems to be the case here. Edit: I've written a more thorough treatment of the ideas outlined above at internals |
|
+1
Is it possible to provide either
or
during at least one full release cycle, i.e., from 1.1. to 1.2? I am fine with minor breaking changes that can be automatically fixed or at least offer a good diagnostic between point releases. Major breaking changes that cannot be automatically identified (or anything worse) should wait till 2.0. This is bending the rules of semantic versioning a bit, but I would consider this a good compromise. |
👍 I agree with https://github.com/nikomatsakis/rfcs/blob/language-semver/text/0000-language-semver.md and with llogiq, the impact is minimal and the change will not disrupt the user experience, it will improve it. |
|
I agree with @tbu- that something like python's "future imports" is a better solution, like:
Along with a warning if this attribute is missing: "The current rule for default object bounds is deprecated, please opt-in the new rule by adding ..." |
|
While I agree that we should go ahead and make the change, I think we should also go the extra mile to make sure that any code that might be affected can keep working, be automatically fixed, or something of the sort. (The version-based opt-in, for example.) Importantly, I think we should do so even if there's no indication from crates.io that it's strictly necessary, as a gesture that we take the backwards compatibility issue seriously. |
|
Would it be possible to try and infer some superposition of |
|
Hear ye, hear ye. This RFC was moved into Final Comment Period as of Wed, June 10th. (Sorry, forgot to add this notice!) |
|
@aturon just suggested to me that we should add an opt out marker to restore the older defaults. This marker could hang around indefinitely. This means that even if your code breaks, all you have to do is add this marker to the top of your crate to get back the old defaults for now (and you can remove it at your leisure). This would be very simple to maintain in the compiler. Seems like a good idea. My main concern about the opt-out marker, though, is that I would want to use an attribute name that we can generalize in the future, and I'm not sure what that should be. I propose
Misuse of this attribute would not be an error, but rather a lint, to allow for future expansion. |
|
Regarding diagnostics, I'd like to investigate how to improve them to be more targeted (and perhaps suggest the |
A |
|
+1 let's not shoot ourselves in the foot by allowing the problem to persist. It's already known that the impact is minimal. |
Why is that? Let's say you have inference A and B. Let A result in the old, B in the new behaviour. Do the lifetime analysis with B. If an error occurs, back up, retry with A. If an error still occurs, show it, otherwise issue a warning that the code relies on outdated behaviour that may go away in the next version and go on. This may be a naive and slow implementation, but I wouldn't call it impossible. |
I spoke too strongly. I don't think it's possible to fix this in a local, simple fashion. I agree that if we exhaustively searched throughout the entire crate and tweaked defaults, we could do it, but keep in mind that if the type appears in a fn boundary, then we have to go check the callers to decide which usage they expected (and of course callers can disagree). So I should say it's not feasible to accept crates and deduce what default they were expecting. In any case, I also consider that undesirable -- I want simple rules, not some curious hodgepodge. That is, if we tried to accept the older defaults, we'd in fact be creating a scheme with very complex inference behavior that would be very challenging to support. I'd much rather just stick with the current defaults if that were the alternative. :) |
|
I added some text about the |
|
I'm worried this could set a dangerous precedent, especially for something that seems to be more a quality of life change than anything else. |
|
|
||
| This is a breaking change, and hence it behooves us to evaluate the | ||
| impact and describe a procedure for making the change as painless as | ||
| possible. One nice propery of this change is that it only affects |
|
I only have one comment about the #[legacy(default_object_bounds)] attribute. What will be the name of the next tweak of default object bounds? Can this name be changed to something more descriptive? In my experience naming something legacy is not too robust, it only works if that stuff has only two versions. I don't know enough about this issue and I admit that I did not read all these comments:) Ignore this if my argument is not applicable:) |
0b60af9
to
e17464c
Compare
…ct RFC for more visibility
I feel you and I wish that we will never need a Rust 2.0. I wish that the solution would allow for a future Rust 1.15 crate to seamlessly depend on a Rust 1.0 crate.
What if one could also pass them to rustc as a parameter? |
This would get very messy, fast, once you have multiple dependencies that may need different legacy flag combinations. It would make deployment of any rust-based solution much more complicated than necessary, because _dammit, the information about what language version the code uses belongs in the code_! (Sorry, did I become loud? breathe deeply...now I'm better. Ok,) I've been arguing for target-version based API / language evolution for some time now. So much that I fear it's getting stale. People still stick to the idea of opt-in or opt-out, as if it was an easy or even workable solution (pretty pretty please read my RFC PR #1147 for an explanation why it's not). I've already answered your comment, but I feel I need to stress another point:
Ideally, this Edit: With the above, I mean |
Oh, you are right. I was thinking that I could tell cargo externally that a package requires Rust version 1.0. If I try to compile it with Rust 1.13 cargo could look up the flags it needs to pass to rustc to compile this package. It could potentially be a very long list of flags, and certainly messy for cargo developers, but as a users I wouldn't need to worry about it much.
I gotta sleep over this but right now I would agree that it belongs at the crate level at least in the Cargo.toml. Having it in the source files would make me right now a bit uncomfortable, but I'm open to give this some more thought over the next couple of days.
I think that a fine grained solution is a good starting point for a discussion tho. We can have legacy attributes for each breaking change, legacy attributes for whole rust versions |
|
@gnzlbg I actually included opt-in by flag (be it legacy flag or otherwise) as an alternative/extension in my RFC PR, and I intend to move them into the main proposal once I have thought the consequences through. I agree that allowing for more fine-grained control is a worthy goal, in addition to setting up a good default by the target version. This means we get sane defaults, break no code and still can have all unstable or legacy features we may want by inserting the corresponding flags. |
|
As a newcomer to Rust and having been badly bitten by this issue, I'd say yes, merge ASAP. Now, on the subject on how to make breaking changes on point releases, llogiq's idea of a phased change looks to me to be the best trade off. Possibly make the change available early using the existing compiler features mechanism. As for opt-out and target API version, keep in mind that in a thriving ecosystem, unmaintained code that just works never stays unmaintained for long: if some piece of code is really useful, there's always someone to fork it, dust it off and add the latest bells and whistles to it. That said, it could come in handy when a major release of Rust comes out with loads of breaking changes. But please, not for point releases, we'd end up in a dependency hell: what if you have code that uses libc targeted at the current Rust version while using at the same time a crate that uses a libc version targeted at previous, incompatible, Rust version? I know it's doable, but no thanks. When Rust 12.0 comes out, the best place for a target API flag would however be in the Cargo.toml file:
And make it so that Rust 12.0 can be installed along with previous versions (like gcc does for example) so that cargo can invoke the appropriate version. Oh, and provide some vetting tool to help transition code from one version to the other. |
This is expressly not about unmaintained code. It's about enterprise users who may have invested money in vetting one version of a given solution, which you now ask to spend more money to switch to another. It's about users of (not yet existing, but conceivable) proprietary crates, which they are contractually obligated not to change. Finally it's about all users, because we're going to lose an awful lot of them once the next few point-releases break their build and they start to fear new Rust versions.
libc hasn't changed too much in a long time, even in the face of severe security issues. Also we're already abstracting over it, so I don't think this will be a problem. Even if it comes to that, we can figure out how to deal with it then.
I thought about this, too, but this has two downsides:
I sure hope you meant this as a rule for Rust 12.0.0 – because if enacted now it would break all builds with the next incompatible release. |
|
Well, working for an open source minded shop, I didn't think about the untouchable proprietary crate. My concern here is that making provisions to target
I only picked libc as an example, my point is still valid for any other crate.
I fully agree. Then don't allow breaking changes at all. As much as I'd like to see this PR merged, I'd rather see it postponed to Rust 2.0 and properly documented as a known issue than open Pandora's box to dependency hell (nothing wrong with known issues as long as they are properly documented).
Why not, as long as it's only for major releases ;) But that's not incompatible with having it as a config value in Cargo.toml or a crate root attribute.
Well, I jumped to version 12 for a reason ;) |
|
@db47h I don't see your point. Why should we stop evolving the language (and API) when we can do so safely – that is, as long as the old behavior is accessible via target version/legacy/future combination and we can ensure crates of all target versions continue to work together? Where is this dependency hell you keep alluding to? Yes, it would force people to upgrade their rust versions to use code that is written for the new versions. That is a good thing and it's no different than in, say, Java, or C++. We want people to use the newest Rust, because if they don't, they'll miss out on all the fixes for all problems we may have found in the meantime.
Currently, that will be 1.0, because we cannot ask people to retrofit a target version to all their code. And yes, this is much like the default of Also future incompatible versions will likely have incompatible APIs, which (as I've written in my RFC PR) could be use to determine a lower bound for the target version. In any event, we may have some time until the next major release hits, so once most code declares its target version, a warning + best-effort-compilation with the current version could be the most useful way to go (this could of course be overridden by a compiler argument). |
|
@llogiq : Took me a while to find the RFC PR you mention :-P And yes, makes more sense now that I've read it. I'm not saying the language should stop evolving, not at all. Sorry if that's how I came through. There's nothing wrong with your RFC-PR, on the contrary. The dependency hell I'm referring to occurs for example when your code and some other crate it depends on both use a different target version of yet another crate (see my poorly chosen libc example). I don't know about Java, but in my experience this is all too common with C/C++ (game engines come to mind) . You refer to it as "breakage" in your RFC-PR, I used a different term, coming from a different perspective, that's all. Of course, it won't happen if your PR goes through and if everyone adopts newer Rust versions quickly. |
|
Please note that target version as currently defined refers to rust (including its std) only. Whether we want to allow others to use the concept is an open question (however, I've made the syntax extensible to allow for this case). Also as of now, crates already can refer to different versions of other crates via Cargo. |
I'd generally disagree with this - haven't you ever found some website offering (relatively obscure) software in a tarball that's 5 years old, with no development, upstream or fork, in sight? I've encountered many. Or even on GitHub, where there may be, say, 125 "forks", of which, according to GitHub's network view, the vast majority are unchanged from their fork date, a dozen have one or more commits added, which may or may not fix the problem you're having, and none show signs of active ongoing development. Those numbers come from a real example - a C library where the maintainer introduced a trivial but critical bug back in 2013 and has not paid attention to the resulting issue report, or practically any issues/PRs, since. At least GitHub makes it easier to fork a repository, but all the links on the Internet still point to the old version, and committing to the necessary advertising plus attention to the issue/PR queue to make your fork 'canonical' is... not exactly easy.
To slightly elaborate on what @llogiq just said, this question wouldn't make sense, because depending on a different version of a crate is orthogonal to a Rust version attribute; the attribute, for its part, would be per crate, and crates targeting different versions would fully interoperate. |
I meant "useful unmaintained code", and discounted the relatively obscure software ;) I just wouldn't worry too much about making sure that old bitrotted code still compiles 10 years from now. |
|
Most software is relatively obscure - the so-called long tail. |
|
@db47h besides, that kind of thinking brought us the y2k problem. It's shortsighted and bad engineering. Just a heads-up: I have rewritten my RFC proposal (which is now called "Target Version") to include the points made here. As it stands, it would also apply to this proposal if accepted (thus removing any remaining breakage risk from this change). |
|
It's official... the language subteam has decided to accept this RFC and go forward with this specific change. Given its significance, this PR was also discussed specifically at a recent core team meeting. Note: although it's been decided to make this particular change, this is explicitly not considered a binding precedent. The more general policy questions are still under active discussion and we plan to eventually open a separate RFC on that topic. For the moment, I am currently working on a post for internals summarizing my thoughts on breaking changes, LTS releases, major versions, and so forth, but this is not yet complete. (Given the upcoming Mozilla work week, this may not get posted right away.) And of course there is also RFC #1147 by @llogiq, which though it was born from a discussion on API deprecation is clearly relevant. --nmatsakis |
|
@nikomatsakis: Thank you for working with us to make this change a reality. I believe this is a good change for Rust and for us all. Incidentially – and mostly inspired by the discussion here and elsewhere – I have rewritten and streamlined my proposal considerably. As it is now, it incorporates the things that were discussed here, so double thanks to all who argued with me. 😄 |
|
Merged. Tracking issue |
…komatsakis This is an implementation of RFC rust-lang/rfcs#1156. It includes the code to implement the new rules, but that code is currently disabled. It also includes code to issue warnings when the change will cause breakage. These warnings try hard to be targeted but are also somewhat approximate. They could, with some effort, be made *more* targeted by adjusting the code in ty_relate that propagates the "will change" flag to consider the specific operation. Might be worth doing. r? @pnkfelix (I think you understand region inference best)
Adjust the object default bound algorithm for cases like
&'x Box<Trait>and&'x Arc<Trait>. The existing algorithm would default to&'x Box<Trait+'x>. The proposed change is to default to&'x Box<Trait+'static>. A new attribute,#[legacy(default_object_bounds)], can be used to restore the current behavior to ease the transition.Note: This is a BREAKING CHANGE. The change has been implemented and its impact has been evaluated. It was found to cause no root regressions on
crates.io.This change was initially discussed as a pre-RFC. In particular there is much discussion there about the strategy towards breaking changes. Based on the reasoning in my last comment, I'm advancing this to "full RFC" status. Note in particular that if we're going to make this change, we have to do it quickly. :)
UPDATE: The RFC has been adjusted to include an opt-out that restores the older behavior. The summary here was also updated appropriately.
Rendered view.