RFC: Policy on semver and API evolution #1105

Merged
merged 15 commits into from Jun 10, 2015

Projects

None yet
@aturon
Contributor
aturon commented May 4, 2015

This RFC proposes a comprehensive set of guidelines for which changes to
stable APIs are considered breaking from a semver perspective, and which are
not. These guidelines are intended for both the standard library and for the
crates.io ecosystem.

This does not mean that the standard library should be completely free to make
non-semver-breaking changes; there are sometimes still risks of ecosystem pain
that need to be taken into account. Rather, this RFC makes explicit an initial
set of changes that absolutely cannot be made without a semver bump.

Along the way, it also discusses some interactions with potential language
features that can help mitigate pain for non-breaking changes.

The RFC covers only API issues; other issues related to language features,
lints, type inference, command line arguments, Cargo, and so on are considered
out of scope.

Rendered

@aturon
Contributor
aturon commented May 4, 2015

@bstrie, I know you were interested in this.
@pnkfelix I could use your careful eyes on this one!
cc @alexcrichton @sfackler @brson @huonw @wycats @nikomatsakis

@aturon
Contributor
aturon commented May 4, 2015

cc @reem @carllerche -- I think both of you have also expressed some opinions on this topic as well. (This is an important piece of policy, so I want to ensure good visibility).

@sfackler sfackler and 1 other commented on an outdated diff May 4, 2015
text/0000-api-evolution.md
+## Overview
+
+### Principles of the policy
+
+The basic design of the policy is that **minor changes should require at most a
+few local *annotations* to the code you are developing, and in principle no
+changes to your dependencies.**
+
+In more detail:
+
+* Minor changes should require at most minor amounts of work upon upgrade. For
+ example, changes that may require occasional type annotations or use of UFCS
+ to disambiguate are not automatically "major" changes. (But in such cases, one
+ must evaluate how widespread these "minor" changes are).
+
+* In principle, it should be possible to produce a version of the code for any
@sfackler
sfackler May 4, 2015 Member

A slightly weaker way of phrasing this would be "given knowledge of any change, it should be possible to write a version of your code that works both before and after that change". It seems to me like that phrasing is significantly more important to guarantee than the version written, since it means libraries won't be "forced" to abandon old versions of Rust to support new versions.

@aturon
aturon May 11, 2015 Contributor

Yes, that's also an important constraint, and I'll try to rewrite to clarify.

That said, what's gradually becoming clear to me with this RFC is that we may be able to make a strictly stronger guarantee: that all minor but breaking changes can be worked around automatically with an elaboration process. (I think with a couple of very minor language features, we can get there.) Having a strong guarantee that you don't have to go make changes to your dependencies when you upgrade Rust -- while allowing the standard library to grow -- seems like a great place to be.

@sfackler sfackler commented on the diff May 5, 2015
text/0000-api-evolution.md
+#### Major change: adding a public field when no private field exists.
+
+This change retains the ability to use struct literals, but it breaks existing
+uses of such literals; it likewise breaks exhaustive matches against the struct.
+
+#### Minor change: adding or removing private fields when at least one already exists.
+
+No existing code could be relying on struct literals for the struct, nor on
+exhaustively matching its contents, and client code will likewise be oblivious
+to the addition of further private fields.
+
+For tuple structs, this is only a minor change if furthermore *all* fields are
+currently private. (Tuple structs with mixtures of public and private fields are
+bad practice in any case.)
+
+#### Minor change: going from a tuple struct with all private fields (with at least one field) to a normal struct, or vice versa.
@sfackler
sfackler May 5, 2015 Member

We could also update the compiler to allow both Foo(..) and Foo {..} if all fields are private. If the type's "totally opaque", it kind of makes sense that the kind of struct would be hidden as well.

@reem
reem May 5, 2015

I agree with @sfackler, I've viewed this as a wart and we could just fix it.

@nikomatsakis
nikomatsakis May 5, 2015 Contributor

Strictly speaking, Foo(..) looks up a match in the value namespace, I believe, though perhaps that is not necessary given our current semantics (basically, for anything where the Foo(..) pattern would be legal, it could also check the type namespace now).

@huonw huonw and 3 others commented on an outdated diff May 5, 2015
text/0000-api-evolution.md
+
+**Breaking changes are assumed to be major changes unless otherwise stated**.
+The RFC covers many, but not all breaking changes that are major; it covers
+*all* breaking changes that are considered minor.
+
+### Crates
+
+#### Major change: introducing `#[feature]` for the first time.
+
+Changing a crate from working on stable Rust to *requiring* a nightly is
+considered a breaking change. Crate authors should consider using Cargo
+"features" for their crate to make such use opt-in.
+
+#### Minor change: adding/removing crate dependencies.
+
+The author is not aware of any possible breakage in altering dependencies (which
@huonw
huonw May 5, 2015 Member

This may implicitly cause a crate to require nightly if a dep does.

@kballard
kballard May 5, 2015 Contributor

Perhaps this should just be covered under something like

Major change: any change that requires a nightly when the crate did not previously require a nightly

This includes introducing #[feature] for the first time, or adding/upgrading a dependency which requires a nightly.

@cmr
cmr May 10, 2015 Contributor

Additionally, adding/removing dependencies can modify the cargo feature set (not #[feature]) exposed to upstream, which can implicitly cause breaking changes if you accidentally used a #[cfg]d off function.

@aturon
aturon May 12, 2015 Contributor

@huonw @kballard Thanks, I will take @kballard's suggestion for the wording here.

@cmr That is an interesting scenario, but I don't think it should be considered a major change: if other crates were relying on the feature without specifying it, they're basically breaking the model (and can be easily fixed by opting into the feature themselves).

@huonw huonw commented on an outdated diff May 5, 2015
text/0000-api-evolution.md
+
+The author is not aware of any possible breakage in altering dependencies (which
+is essentially private anyway).
+
+### Modules
+
+#### Major change: renaming/moving/removing any public items.
+
+Although renaming an item might seem like a minor change, according to the
+general policy design this is not a permitted form of breakage: it's not
+possible to annotate code in advance to avoid the breakage, nor is it possible
+to prevent the breakage from affecting dependencies.
+
+Of course, much of the effect of renaming/moving/removing can be achieved by
+instead using deprecation and `pub use`, and the standard library should not be
+afraid to do so!In the long run, we should consider hiding at least some old
@huonw
huonw May 5, 2015 Member

(Missing space after !.)

@sfackler sfackler and 1 other commented on an outdated diff May 5, 2015
text/0000-api-evolution.md
+A struct or enum field can change from a concrete type to a generic type
+parameter, provided that the change results in an identical type for all
+existing use cases. For example, the following change is permitted:
+
+```rust
+// MINOR CHANGE
+
+// Before
+struct Foo(pub u8);
+
+// After
+struct Foo<T = u8>(pub T);
+```
+
+because existing uses of `Foo` are shorthand for `Foo<u8>` which yields the
+identical field type.
@sfackler
sfackler May 5, 2015 Member

This is not actually the case right now since #213 isn't fully implemented:

enum Foo<S = i32> {
    Bar,
    Baz(S)
}

fn f<S>(_: Foo<S>) {}

fn main() {
    f(Foo::Bar);
}
~ ❯ rustc test.rs
test.rs:9:5: 9:6 error: unable to infer enough type information about `_`; type annotations required [E0282]
test.rs:9     f(Foo::Bar);
              ^
error: aborting due to previous error
@petrochenkov
petrochenkov May 5, 2015 Contributor

Yep, until default type parameters drive type inference, generalization can cause surprisingly large breakage. For example heterogeneous comparisons for Option can't be implemented now because they would require type annotations on None in every comparison and that's a lot of annotations.
Default parameters is an important backward compatibility tool, I hope they'll be finished sooner.

@huonw huonw commented on the diff May 5, 2015
text/0000-api-evolution.md
+write, which can break code irreparably.
+
+#### Major change: adding a public field when no private field exists.
+
+This change retains the ability to use struct literals, but it breaks existing
+uses of such literals; it likewise breaks exhaustive matches against the struct.
+
+#### Minor change: adding or removing private fields when at least one already exists.
+
+No existing code could be relying on struct literals for the struct, nor on
+exhaustively matching its contents, and client code will likewise be oblivious
+to the addition of further private fields.
+
+For tuple structs, this is only a minor change if furthermore *all* fields are
+currently private. (Tuple structs with mixtures of public and private fields are
+bad practice in any case.)
@huonw
huonw May 5, 2015 Member

Should we have a lint?

@reem reem commented on the diff May 5, 2015
text/0000-api-evolution.md
+pub struct Foo(SomeType);
+
+// in downstream code
+let Foo(_) = foo;
+```
+
+Changing `Foo` to a normal struct can break code that matches on it -- but there
+is never any real reason to match on it in that circumstance, since you cannot
+extract any fields or learn anything of interest about the struct.
+
+### Enums
+
+See "[Signatures in type definitions](#signatures-in-type-definitions)" for some
+general remarks about changes to the actual types in an `enum` definition.
+
+#### Major change: adding new variants.
@reem
reem May 5, 2015

Does this mean we are in fact freezing io::ErrorKind?

@sfackler
sfackler May 5, 2015 Member

ErrorKind has an unstable variant that prevents stable code from exhaustively matching on it: https://github.com/rust-lang/rust/blob/master/src/libstd/io/error.rs#L118-L123

@sfackler sfackler and 3 others commented on an outdated diff May 5, 2015
text/0000-api-evolution.md
+features and the way that APIs using them can, and cannot, evolve in a minor
+release.
+
+**Breaking changes are assumed to be major changes unless otherwise stated**.
+The RFC covers many, but not all breaking changes that are major; it covers
+*all* breaking changes that are considered minor.
+
+### Crates
+
+#### Major change: introducing `#[feature]` for the first time.
+
+Changing a crate from working on stable Rust to *requiring* a nightly is
+considered a breaking change. Crate authors should consider using Cargo
+"features" for their crate to make such use opt-in.
+
+#### Minor change: adding/removing crate dependencies.
@sfackler
sfackler May 5, 2015 Member

A somewhat related concept would be changing the major version of a dependency. It seems like that could be either a minor or major change depending on if the crate is visible in your crate's public API or not.

@kballard
kballard May 5, 2015 Contributor

Does Cargo allow multiple versions of the same crate to exist in a single dependency tree, or does it restrict each crate to only resolving to a single version? My assumption has been the latter (although some language package managers do the former, such as npm). If it's the latter, then any major version upgrade for a dependency could be a breaking change if the dependency already exists anywhere else in the upstream crate's dependency tree.

By the same logic, the addition of any dependency (using anything other than * as the version requirement) could be a breaking change.

@alexcrichton
alexcrichton May 5, 2015 Member

Cargo allows multiple versions of a project, but each version must be semver incompatible with all other activated versions. For example 1.0.0 and 2.0.0 are allowed, but 1.0.0 and 1.1.0 is not.

@kballard
kballard May 5, 2015 Contributor

Interesting. So it sounds like it's still possible for the addition/upgrade of a dependency to be an incompatible change if the version requirement depends on anything other than the major version number, e.g. foo = ">1.1.0" would be a breaking change if a different project in the dependency tree had foo = "=1.0.5".

@aturon
aturon May 12, 2015 Contributor

Hm, this is a very interesting and somewhat thorny point.

My inclination would be to say that it's a minor change to increase the minor version number as long as the spec is of the form >X.Y.Z, and basically assume that a fully locked-down spec like =X.Y.Z is a corner case that opts out of the API stability guarantees.

Thoughts?

@huonw huonw commented on an outdated diff May 5, 2015
text/0000-api-evolution.md
+}
+
+fn use_both<T: Trait1 + Trait2>(t: &T) {
+ t.foo()
+}
+```
+
+If a `foo` method is added to `Trait1`, even with a default, it would cause a
+dispatch ambiguity in `use_both`, since the call to `foo` could be referring to
+either trait.
+
+(Note, however, that existing *implementations* of the trait are fine.)
+
+According to the basic principles of this RFC, such a change is minor: it is
+always possible to annotate the call `t.foo()` to be more explicit *in advance*
+using UFCS: `<Trait2>::foo(t)`. This kind of annotation could be done
@huonw
huonw May 5, 2015 Member

This would have to be Trait2::foo(t) or <T as Trait2>::foo(t), wouldn't it? (Otherwise it's trying to go via the trait object type?)

@huonw huonw commented on an outdated diff May 5, 2015
text/0000-api-evolution.md
+// Before
+struct Foo<T>(pub T, pub T);
+
+// After
+struct Foo<T, U = T>(pub T, pub U);
+```
+
+since, again, all existing uses of the type `Foo<T>` will yield the same field
+types as before.
+
+### Signatures in functions
+
+All of the changes mentioned below are considered major changes in the context
+of trait methods, since they can break implementors.
+
+#### Major change: adding new arguments.
@huonw
huonw May 5, 2015 Member

adding/removing

@huonw huonw commented on the diff May 5, 2015
text/0000-api-evolution.md
+// Before
+fn foo(t: &Trait);
+
+// After
+fn foo<T: Trait + ?Sized>(t: &T);
+```
+
+(The use of `?Sized` is essential; otherwise you couldn't recover the original
+signature).
+
+### Lints
+
+#### Minor change: introducing new lint warnings/errors
+
+Lints are considered advisory, and changes that cause downstream code to receive
+additional lint warnings/errors are still considered "minor" changes.
@huonw
huonw May 5, 2015 Member

Worth discussing #[deny(...)] causing compilation errors in more detail? (cc #1029 )

@aturon
aturon May 12, 2015 Contributor

I'm not sure whether this RFC is the right place to tackle that infrastructure, but I'll at least put in a reference.

@huonw huonw commented on an outdated diff May 5, 2015
text/0000-api-evolution.md
+regressions. This tool will be indispensable when weighing the costs of a minor
+change that might cause some breakage -- we can actually gauge what the breakage
+would look like in practice.
+
+While this would, of course, miss code not available publicly, the hope is that
+code on crates.io is a broadly representative sample, good enough to turn up
+problems.
+
+Any breaking, but minor change to the standard library must be evaluated through
+Crater before being committed.
+
+### Nightlies
+
+One line of defense against a "minor" change causing significant breakage is the
+nightly release channel: we can get feedback about breakage long before it makes
+even into a beta release.
@huonw
huonw May 5, 2015 Member

Similarly, the beta releases are designed for exactly this purpose.

@kballard kballard commented on the diff May 5, 2015
text/0000-api-evolution.md
+2. Even if that change were made, though, there is still the case where two glob
+ imports conflict without any explicit definition "covering" them. This is
+ permitted to produce an error under the principles of this RFC because the
+ glob imports could have been written as more explicit (expanded) `use`
+ statements. It is also plausible to do this expansion automatically for a
+ crate's dependencies, to prevent breakage in the first place.
+
+### Structs
+
+See "[Signatures in type definitions](#signatures-in-type-definitions)" for some
+general remarks about changes to the actual types in a `struct` definition.
+
+#### Major change: adding a private field when all current fields are public.
+
+This change has the effect of making external struct literals impossible to
+write, which can break code irreparably.
@kballard
kballard May 5, 2015 Contributor

FWIW, there is a way around this. The obvious answer is to implement Default for the struct, allowing literals of the form Foo { a: 1, b: 2, ..Default::default() }.

This solution won't work for struct literals found in const or statics (although a const containing a default value for the struct would). But there's plenty of other changes you can make to structs that would break struct literals found in const or static but not break struct literals found elsewhere (for example, adding a public field that has no public value that can be used in a const or static). Given that, requiring that any change not break the ability to use struct literals in const / static might be considered overly restrictive (especially as most types will never find themselves used in that fashion). And if that restriction is not enforced, then it seems plausible to treat a pre-existing implementation of Default as allowing for changes that would otherwise break struct literals.

@pnkfelix
pnkfelix May 11, 2015 Member

@kballard I take it you are assuming that RFC 736 would be reverted in order to enable this pattern?

That is, this pattern used to work, but it does not any longer, because the privacy breakage allowed inadvertent exposure of the internal representation and subsequent breakage of global invariants.

@kballard
kballard May 12, 2015 Contributor

@pnkfelix Oh. I actually had never even seen RFC 736. I missed that change completely.

Incidentally, is there some reason that this was disallowed completely, instead of only for types that aren't Copy? Using FRU for a Copy type is equivalent to copying it and mutating the public fields. I'll file an RFC on it. (Update: Filed as #1117)

@pnkfelix
pnkfelix May 12, 2015 Member

@kballard (i don't think there was any reason beyond adopting the "simplest" change that could possibly work, where "simplest" is measured according to some hand-wavy ordering w.r.t. the description of the language itself)

@Gankro Gankro commented on the diff May 5, 2015
text/0000-api-evolution.md
+
+Note that these strategies are not mutually exclusive. Rust's development
+processes involved a very steady, strong stream of breakage, and while we need
+to be very serious about stabilization, it is possible to take an iterative
+approach. The changes considered "major" by this RFC already move the bar *very
+significantly* from what was permitted pre-1.0. It may turn out that even the
+minor forms of breakage permitted here are, in the long run, too much to
+tolerate; at that point we could revise the policies here and explore some
+opt-in scheme, for example.
+
+# Unresolved questions
+
+## Behavioral issues
+
+- Is it permitted to change a contract from "abort" to "panic"? What about from
+ "panic" to "return an `Err`"?
@Gankro
Gankro May 5, 2015 Contributor

Exceedingly little code can even distinguish between an abort and a panic, so I wouldn't expect this to be a serious problem in practice. The only thing I can see being trouble is unsafe code, which can legitimately rely on the fact that something aborted to avoid setting up guards against unwinding. In such a case this could lead to UB! However if we had no_unwind assertions or something unsafe code could at least guard against such a change. Although adding no_unwind assertions would make this change even-more-breaking!

panic -> Err seems like a tricky semantic change that can cause weird error-handling bugs. For one, if API A declares that it panics on input X, and API B uses API A but also transitively reports the panic in one of its APIs for input Y, its documentation will at best be incorrect after an upgrade: it no longer panics on Y.

Otherwise you may just get bad error handling results from things like "what I thought was an effectively exhaustive match no longer is (new Error kinds are possible -- basically a soft version of adding a variant to an enum)" or "The system reports frobs are out of sync on error X since this is the only possibility in 1.0, but since 1.1 it can now also mean frams are misaligned (old Error kinds are now more overloaded)". This seems like a major breaking change from a semantic perspective.

@oli-obk oli-obk commented on the diff May 5, 2015
text/0000-api-evolution.md
+public item can break downstream code arbitrarily.
+
+It would be much better for API evolution (and for ergonomics and intuition) if
+explicitly-defined items trump glob imports. But this is left to a future RFC.
+
+**Globs with fine-grained control**
+
+Another useful tool for working with globs would be the ability to *exclude*
+certain items from a glob import, e.g. something like:
+
+```rust
+use some_module::{* without Foo};
+```
+
+This is especially useful for the case where multiple modules being glob
+imported happen to export items with the same name.
@oli-obk
oli-obk May 5, 2015 Contributor

This could rather be solved by importing neither, but noting the name. When the code uses an item with that name, an error "item not imported due to conflicting glob imports" could list all possibilities and suggest adding an explicit use

@aturon
aturon May 12, 2015 Contributor

Good point! I'll add that proposal as well.

@P1start P1start commented on the diff May 5, 2015
text/0000-api-evolution.md
+general remarks about changes to the actual types in an `enum` definition.
+
+#### Major change: adding new variants.
+
+Exhaustiveness checking means that a `match` that explicitly checks all the
+variants for an `enum` will break if a new variant is added. It is not currently
+possible to defend against this breakage in advance.
+
+A [postponed RFC](https://github.com/rust-lang/rfcs/pull/757) discusses a
+language feature that allows an enum to be marked as "extensible", which
+modifies the way that exhaustiveness checking is done and would make it possible
+to extend the enum without breakage.
+
+#### Major change: adding new fields to a variant.
+
+If the enum is public, so is the full contents of all of its variants. As per
@P1start
P1start May 5, 2015 Contributor

s/so is/so are/ – ‘the full contents’ is plural.

@P1start P1start commented on the diff May 5, 2015
text/0000-api-evolution.md
+```
+
+since, again, all existing uses of the type `Foo<T>` will yield the same field
+types as before.
+
+### Signatures in functions
+
+All of the changes mentioned below are considered major changes in the context
+of trait methods, since they can break implementors.
+
+#### Major change: adding new arguments.
+
+At the moment, Rust does not provide defaulted arguments, so any change in arity
+is a breaking change.
+
+#### Minor change: introducing a new type parameter.
@P1start
P1start May 5, 2015 Contributor

I believe that considering this a minor change this goes against the overview’s ‘principles of the policy’:

That means that any breakage in a minor release must be very "shallow": it must always be possible to locally fix the problem through some kind of disambiguation that could have been done in advance (by using more explicit forms) or other annotation (like disabling a lint). It means that minor changes can never leave you in a state that requires breaking changes to your own code.

As far as I know, there’s not always a way of ‘fix[ing] the problem through some kind of disambiguation that could have been done in advance’ for this kind of change. To explain with an example, if the breaking change example mentioned in the RFC (adding an extra type parameter to fn foo<T>) occurred in the standard library between, say, Rust 1.3 and 1.4, and my library (which depended on std) had code that called foo with an explicit type parameter (like foo::<u8>()), I’d have no way of fixing my code in a way that continues to support Rust 1.3 but also supports the newest version of Rust. In other words, I’d have to make a choice between supporting the older versions of Rust that I supported and supporting the newer versions (which have an extra type parameter on foo).

The section mentioned above does say ‘in principle’, but it also says that the RFC should suggest ‘additional tooling or language support’, neither of which seems to be outlined in the RFC to address this particular problem.

@oli-obk
oli-obk May 5, 2015 Contributor

In almost all situations I can think of, you can probably use type ascription either in the parameters or in the return value. Any situations where this isn't enough probably have global state and are abusing generic code.

@P1start
P1start May 6, 2015 Contributor

I disagree. To give a more concrete example, imagine that std provided a function for calculating the hash of a value:

pub fn hash<T>(value: &T) -> u64 where T: Hash {
    let mut h = SomeHasher::new();
    value.hash(&mut h);
    h.finish()
}

Let’s pretend that I have a library (which depends on std) that has code that looks like hash::<u8>(1). Now, in Rust 1.4, the function above has a new type parameter added to make it generic over the hasher used:

pub fn hash<T, H>(value: &T) -> u64 where H: Default + Hasher, T: Hash {
    let mut h: H = Default::default();
    value.hash(&mut h);
    h.finish()
}

According to the RFC, this would be a minor change. However, there is no way for me to update my code in a way that supports both 1.3 and 1.4—in 1.4, I have to specify the second type parameter, but in 1.3, I can’t.

I suppose this doesn’t apply to most functions (only those where not all type parameters can be inferred), but I think that the RFC should at least specify that this is only a minor change if there is a way of inferring the type of all type parameters (new and old).

@kballard
kballard May 7, 2015 Contributor

@P1start Your example there can infer its type parameter, and your example invocation hash::<u8>(1) won't work to begin with.

@huonw
huonw May 8, 2015 Member

I wonder if we could allow hinting only a prefix of the generic type parameters, this would make situations like this where the new type parameter is inferrable always just work. I have at least one API where this would be useful myself (functions are usually called like either foo::<X, _>(), with the _ essentially never needing to be specified).

@P1start
P1start May 8, 2015 Contributor

@kballard The second type parameter doesn’t seem to be inferred when using std’s existing Hash[er] traits. And the invocation should have been hash::<u8>(&1) (but that’s just a mistake and is beside the point).

@kballard
kballard May 8, 2015 Contributor

@P1start I was referring to that first code snippet. With the second code snippet, the addition of a non-inferrable type parameter would of course be a breaking change, and I would expect anyone making such a change to provide a default value for the type parameter (or if they really don't want to, then to consider it a major version bump).

@nikomatsakis
Contributor

@aturon I'll go over in detail, but one thought I had is that it might be useful to elaborate a kind of "best practices for forwards compatibility". That's probably not part of this RFC, but it might help illuminate nonetheless.

@nagisa nagisa commented on the diff May 5, 2015
text/0000-api-evolution.md
+# Summary
+
+This RFC proposes a comprehensive set of guidelines for which changes to
+*stable* APIs are considered breaking from a semver perspective, and which are
+not. These guidelines are intended for both the standard library and for the
+crates.io ecosystem.
+
+This does *not* mean that the standard library should be completely free to make
+non-semver-breaking changes; there are sometimes still risks of ecosystem pain
+that need to be taken into account. Rather, this RFC makes explicit an initial
+set of changes that absolutely *cannot* be made without a semver bump.
+
+Along the way, it also discusses some interactions with potential language
+features that can help mitigate pain for non-breaking changes.
+
+The RFC covers only API issues; other issues related to language features,
@nagisa
nagisa May 5, 2015 Contributor

Nit: redundant paragraph. Language, lints, type inference, CLI are not part of standard library (first paragraph), nor they are a part of crates.io ecosystem.

@pnkfelix
pnkfelix May 11, 2015 Member

In the past, for better or worse, sometimes changes to things like command line arguments to rustc were for various reasons classified as "library issues" (as opposed to language issues).

(Also, the list here covers things that might be part of semantic versioning for the Rust language itself -- I think the rules there still remain unwritten. So given that the title of this RFC is "policy on semver ...", it is natural for a reader to perhaps wonder if these issues are in fact addressed...)

While the first paragraph does indeed restrict the domain to "standard library" and "crates.io ecosystem", I think this paragraph can stay -- perhaps just modify it to make it clear that this paragraph is in fact a consequence a fact that all these things are not part of the standard library nor crates.io ecosystem.

@SimonSapin SimonSapin commented on the diff May 5, 2015
text/0000-api-evolution.md
+to be very serious about stabilization, it is possible to take an iterative
+approach. The changes considered "major" by this RFC already move the bar *very
+significantly* from what was permitted pre-1.0. It may turn out that even the
+minor forms of breakage permitted here are, in the long run, too much to
+tolerate; at that point we could revise the policies here and explore some
+opt-in scheme, for example.
+
+# Unresolved questions
+
+## Behavioral issues
+
+- Is it permitted to change a contract from "abort" to "panic"? What about from
+ "panic" to "return an `Err`"?
+
+- Should we try to lay out more specific guidance for behavioral changes at this
+ point?
@SimonSapin
SimonSapin May 5, 2015 Contributor

There are behavior changes like #23951 Changing the meaning of the count to splitn that are not breaking per the definition in this RFC (they do not cause downstream code to fail to compile), but still should be considered major as they break basic expectations that downstream code could reasonably make.

Unfortunately I don’t see a clear criteria to distinguish those from behavior changes that are more reasonable or expected like changing results of char::is_alphabetic when updating to a new Unicode version.

@aturon
aturon May 5, 2015 Contributor

@SimonSapin Did you see the earlier section on behavioral changes? The rough guideline would be that changes that break the explicit, documented behavioral contract of a function are considered breaking.

@BurntSushi
Member

I very much love this RFC. Nice work @aturon!

@Havvy Havvy commented on an outdated diff May 6, 2015
text/0000-api-evolution.md
+
+If the enum is public, so is the full contents of all of its variants. As per
+the rules for structs, this means it is not allowed to add any new fields (which
+will automatically be public).
+
+If you wish to allow for this kind of extensibility, consider introducing a new,
+explicit struct for the variant up front.
+
+### Traits
+
+#### Major change: adding a non-defaulted item.
+
+Adding any item without a default will immediately break all trait implementations.
+
+It's possible that in the future we will allow some kind of
+"[sealing](#sealed-traits)" to say that a trait can only be used as a bound, not
@Havvy
Havvy May 6, 2015 Contributor

This link is dead.

@theemathas

What happens if someone finds another way to cause unsafty using safe code and the standard library? We already had thread::scoped and Vec::drain.

@tbu- tbu- commented on the diff May 6, 2015
text/0000-api-evolution.md
+
+While the scenario of adding a defaulted method to a trait may seem somewhat
+obscure, the exact same hazards arise with *implementing existing traits* (see
+below), which is clearly vital to allow; we apply a similar policy to both.
+
+All that said, it is incumbent on library authors to ensure that such "minor"
+changes are in fact minor in practice: if a conflict like `t.foo()` is likely to
+arise at all often in downstream code, it would be advisable to explore a
+different choice of names. More guidelines for the standard library are given
+later on.
+
+#### Minor change: adding a defaulted type parameter.
+
+As with "[Signatures in type definitions](#signatures-in-type-definitions)",
+traits are permitted to add new type parameters as long as defaults are provided
+(which is backwards compatible).
@tbu-
tbu- May 6, 2015 Contributor

This is not true, consider the change to Iterator::sum, which (even though it specifies a default for the type parameter) can't be inferred everywhere.

@huonw
huonw May 6, 2015 Member

I suspect that is the same problem discussed below: https://github.com/rust-lang/rfcs/pull/1105/files#r29635875

@apasel422
Member

Should this mention changing an object-safe trait in such a way that it is no longer object-safe?

@theemathas

What about changing an unsafe function to a safe function?

@ruuda
ruuda commented May 7, 2015

There are two angles to versioning, and I think the difference is important:

  1. Given that the next release will not be major, can we include this change?
  2. Given that we are going to include this change, what should the version number for the next release be?

If I understand the release train model correctly, the version number is determined before changes that will be included in that version are made, so Rust is in scenario 1. If major bumps are going to be uncommon, then it makes sense to downplay some technically breaking changes as minor.

For Crates.io, I expect the vast majority of crates to be in scenario 2. I think the pragmatic approach is a good one; adding a public item without changing anything else certainly feels like a new feature, not like a backwards incompatible change. However, to play the devil’s advocate: there appears to be a hidden assumption that a major version bump is bad and should be avoided. I agree that backwards incompatible changes should be avoided if possible, but this RFC is about naming only. If a change is technically breaking and will be included in the next release, the only question is what the version number should be. Why downplay technically breaking changes as minor? To quote the sidebar rules:

5 . Chill out! A programming language version number is a silly thing to get upset over.

@kballard
Contributor
kballard commented May 7, 2015

Why downplay technically breaking changes as minor?

Because unnecessarily incrementing the major version number is bad. It requires all dependent crates to update their Cargo.toml or they won't get any of the bug fixes in this (or future) versions. And it can cause larger dependency trees to end up with multiple major version numbers of the same crate, which always has the potential to behave badly (for example, two crates that both work with types from a third crate, if they're using different versions of the third crate then the types they're working with aren't compatible; or alternatively, a crate that manages some static data, two versions of the crates means two separate instances of the static data). And of course there's the simpler reason which is that any time there's a major version bump, the authors of any dependent crates are going to have to do research to find out what backwards-incompatible changes were made, which is a burden we shouldn't put on developers unnecessarily.

@glaebhoerl
Contributor

I think this seems like the right policy, pragmatically speaking. Essentially, to re-summarize: patch-level version bumps are bugfixes only, backwards and forwards compatible; minor versions can cause minor, rare, non-cascading and backwards-compatibly fixable1 compile failures due to additions or generalizations; and major versions are for everything else.

1 These two are very important! At the outset, the idea that "it turns out you can't actually change anything in Rust without the potential for breaking downstreams, but admitting that would be too awkward, so let's just ignore it and pretend" had me rather skeptical, but these are actually very substantial distinctions and guarantees that I'm not sure I could have thought of myself, and they have me convinced that this is the right resolution. So, good work!

The 'elaboration' idea is very clever. I think it could be taken even further:

  • We could also use it to detect the cases where code continues to compile, but a name/method resolves to a different entity than it did before (which I think is much worse than if the code had just stopped compiling - guarding against this kind of thing is what a type system is supposed to be for!). If we have the elaborated source from the previous version, we can just check whether it's consistent with the new one.
  • Similarly, we could use this to automatically fix the kind of "minor breakage" that a minor version can introduce. If, e.g., the resolution of a name is ambiguous with the new version, just replace it with the fully-resolved name from the previous elaborated source.

(I'm not saying this would be easy - but it seems possible. And possibly worthwhile.)

@aturon aturon self-assigned this May 7, 2015
@pnkfelix pnkfelix commented on an outdated diff May 11, 2015
text/0000-api-evolution.md
+
+### Principles of the policy
+
+The basic design of the policy is that **minor changes should require at most a
+few local *annotations* to the code you are developing, and in principle no
+changes to your dependencies.**
+
+In more detail:
+
+* Minor changes should require at most minor amounts of work upon upgrade. For
+ example, changes that may require occasional type annotations or use of UFCS
+ to disambiguate are not automatically "major" changes. (But in such cases, one
+ must evaluate how widespread these "minor" changes are).
+
+* In principle, it should be possible to produce a version of the code for any
+ dependencies that *will not break* when upgrading to a new minor
@pnkfelix
pnkfelix May 11, 2015 Member

@aturon you already said you're planning to rephrase this text, but I just wanted to point out: the wording here is awkward, because the word "that" in "that will not break" is ambiguous as to whether it refers to "the code" or the "any dependencies".

@pnkfelix pnkfelix and 2 others commented on an outdated diff May 11, 2015
text/0000-api-evolution.md
+
+Most of the policy is simplest to lay out with reference to specific language
+features and the way that APIs using them can, and cannot, evolve in a minor
+release.
+
+**Breaking changes are assumed to be major changes unless otherwise stated**.
+The RFC covers many, but not all breaking changes that are major; it covers
+*all* breaking changes that are considered minor.
+
+### Crates
+
+#### Major change: introducing `#[feature]` for the first time.
+
+Changing a crate from working on stable Rust to *requiring* a nightly is
+considered a breaking change. Crate authors should consider using Cargo
+"features" for their crate to make such use opt-in.
@pnkfelix
pnkfelix May 11, 2015 Member

can you provide a pointer to the relevant cargo documentation on these Cargo "features" ?

@Gankro
Gankro May 11, 2015 Contributor

http://doc.crates.io/manifest.html#the-%5Bfeatures%5D-section

To be clear: this is not that there are some vague set of features that cargo itself provides, but rather that crates can register features of themselves to be optionally compiled.

@aturon
aturon May 11, 2015 Contributor

Ha yes, sorry about that ambiguity. Thanks for the link, @Gankro

@pnkfelix pnkfelix commented on an outdated diff May 11, 2015
text/0000-api-evolution.md
+deprecated items from the docs, and could even consider putting out a major
+version solely as a kind of "garbage collection" for long-deprecated APIs.
+
+#### Minor change: adding new public items.
+
+Note that adding new public items is currently a breaking change, due to glob
+imports. For example, the following snippet of code will break if the `foo`
+module introduces a public item called `bar`:
+
+```rust
+use foo::*;
+fn bar() { ... }
+```
+
+The problem here is that glob imports currently do not allow any of their
+imports to be shadowed by an explicitly-define item.
@pnkfelix
pnkfelix May 11, 2015 Member

typo: "explicitly-defined"

@pnkfelix pnkfelix commented on an outdated diff May 11, 2015
text/0000-api-evolution.md
+```rust
+use foo::*;
+fn bar() { ... }
+```
+
+The problem here is that glob imports currently do not allow any of their
+imports to be shadowed by an explicitly-define item.
+
+There are two reasons this is considered a minor change by this RFC:
+
+1. The RFC also suggests permitting shadowing of a glob import by any explicit
+ item. This has been the intended semantics of globs, but has not been
+ implemented. The details are left to a future RFC, however.
+
+2. Even if that change were made, though, there is still the case where two glob
+ imports conflict without any explicit definition "covering" them. This is
@pnkfelix
pnkfelix May 11, 2015 Member

nit: should say, for precision, "where two glob imports conflict with each other"

@pnkfelix pnkfelix commented on an outdated diff May 11, 2015
text/0000-api-evolution.md
+fn bar() { ... }
+```
+
+The problem here is that glob imports currently do not allow any of their
+imports to be shadowed by an explicitly-define item.
+
+There are two reasons this is considered a minor change by this RFC:
+
+1. The RFC also suggests permitting shadowing of a glob import by any explicit
+ item. This has been the intended semantics of globs, but has not been
+ implemented. The details are left to a future RFC, however.
+
+2. Even if that change were made, though, there is still the case where two glob
+ imports conflict without any explicit definition "covering" them. This is
+ permitted to produce an error under the principles of this RFC because the
+ glob imports could have been written as more explicit (expanded) `use`
@pnkfelix
pnkfelix May 11, 2015 Member

The reasoning being applied here (namely that the principles of this RFC say that these glob imports could have been written in expanded form), don't they equally apply to item (1.) above?

  • I don't object to the reasoning of item (1.) that globs were intended to allow shadowing via explicit items; but if we can appeal to something even more fundamental such as the reasoning being applied here, then I would like that noted in item (1.) as well.
  • And of course, if I am incorrect and the reasoning applied here does not apply to item (1.), then perhaps a clarification as to the distinction between the cases is warranted.
@pnkfelix pnkfelix and 1 other commented on an outdated diff May 11, 2015
text/0000-api-evolution.md
+### Structs
+
+See "[Signatures in type definitions](#signatures-in-type-definitions)" for some
+general remarks about changes to the actual types in a `struct` definition.
+
+#### Major change: adding a private field when all current fields are public.
+
+This change has the effect of making external struct literals impossible to
+write, which can break code irreparably.
+
+#### Major change: adding a public field when no private field exists.
+
+This change retains the ability to use struct literals, but it breaks existing
+uses of such literals; it likewise breaks exhaustive matches against the struct.
+
+#### Minor change: adding or removing private fields when at least one already exists.
@pnkfelix
pnkfelix May 11, 2015 Member

nit: "at least one already exists prior to the change and also after the change."

(that is, as written it sounds like removing the sole private field is a minor change, when it is in fact a major change.)

@pnkfelix
pnkfelix May 11, 2015 Member

also, why is it a minor change? Because it breaks ABI? (It doesn't seem to qualify as a feature addition on its own; I can imagine bug-fixes that work via an added private field...)

@aturon
aturon May 12, 2015 Contributor

also, why is it a minor change? Because it breaks ABI? (It doesn't seem to qualify as a feature addition on its own; I can imagine bug-fixes that work via an added private field...)

The labeling of "minor" here is in opposition to major, but is also implicitly allowed in a patch release.

@pnkfelix pnkfelix commented on the diff May 11, 2015
text/0000-api-evolution.md
+Adding any item without a default will immediately break all trait implementations.
+
+It's possible that in the future we will allow some kind of
+"[sealing](#sealed-traits)" to say that a trait can only be used as a bound, not
+to provide new implementations; such a trait *would* allow arbitrary items to be
+added.
+
+#### Major change: any non-trivial change to item signatures.
+
+Because traits have both implementors and consumers, any change to the signature
+of e.g. a method will affect at least one of the two parties. So, for example,
+abstracting a concrete method to use generics instead might work fine for
+clients of the trait, but would break existing implementors. (Note, as above,
+the potential for "sealed" traits to alter this dynamic.)
+
+#### Minor change: adding a defaulted item.
@pnkfelix
pnkfelix May 11, 2015 Member

Adding a defaulted method to a trait that has any associated type with a default type expression should probably be considered a major change.

In particular, due to Default handling in RFC 195, if I have an impl that overrides a defaulted associated type, then I will be obligated to now implement the associated method that the upstream code added, regardless of the fact that it carried a default. (At least, that's my memory of how that rule is to be interpreted.)

@pnkfelix
pnkfelix May 11, 2015 Member

Likewise, ... doesn't Default handling in RFC 195 also mean that adding a default type expression to an associated type that did not have one previously should also be considered a major change?

(That is ... I think adding an entirely new defaulted associated type is a minor change, but putting a default onto an associated type that did not have one previously should be major.)

@pnkfelix pnkfelix commented on the diff May 11, 2015
text/0000-api-evolution.md
+ fn foo(&self, x: u8) { ... }
+}
+```
+
+then crate B would no longer compile, since dispatch would prefer the inherent
+impl, which has the wrong type.
+
+Once more, this is considered a minor change, since UFCS can disambiguate (see
+"Adding a defaulted item" above).
+
+It's worth noting, however, that if the signatures *did* happen to match then
+the change would no longer cause a compilation error, but might silently change
+runtime behavior. The case where the same method for the same type has
+meaningfully different behavior is considered unlikely enough that the RFC is
+willing to permit it to be labeled as a minor change -- and otherwise, inherent
+methods could never be added after the fact.
@pnkfelix
pnkfelix May 11, 2015 Member

hmm ... I wonder if this could provide motivation for some way to use a type without bringing its inherent impls into scope.

@pnkfelix pnkfelix commented on the diff May 11, 2015
text/0000-api-evolution.md
+
+```rust
+// MAJOR CHANGE
+
+// Before
+struct Foo<A> { .. }
+
+// After
+struct Foo<A: Clone> { .. }
+```
+
+#### Minor change: loosening bounds.
+
+Loosening bounds, on the other hand, cannot break code because when you
+reference `Foo<A>`, you *do not learn anything about the bounds on `A`*. (This
+is why you have to repeat any relevant bounds in `impl` blocks for `Foo`, for
@pnkfelix
pnkfelix May 11, 2015 Member

are we committed to that semantics for impl blocks for the long term? At times I thought I've heard @nikomatsakis muse about various strategies to allow some inference to occur in such cases.

@aturon
aturon May 12, 2015 Contributor

It's not completely clear, but this RFC can be revised if or when implied bounds happen.

@pnkfelix pnkfelix commented on the diff May 11, 2015
text/0000-api-evolution.md
+a minor change:
+
+```rust
+// MINOR CHANGE
+
+// Before
+struct Foo { .. }
+
+// After
+struct Foo<A = u8> { .. }
+```
+
+#### Minor change: generalizing to generics.
+
+A struct or enum field can change from a concrete type to a generic type
+parameter, provided that the change results in an identical type for all
@pnkfelix
pnkfelix May 11, 2015 Member

nit: "to a generic type parameter with a default" (I guess this probably follows from the previous section, but better to be explicit, I think.)

@pnkfelix pnkfelix commented on the diff May 11, 2015
text/0000-api-evolution.md
+particular version of Rust, which might in turn be provided only through some
+`cfg`-like mechanism.
+
+Note that these strategies are not mutually exclusive. Rust's development
+processes involved a very steady, strong stream of breakage, and while we need
+to be very serious about stabilization, it is possible to take an iterative
+approach. The changes considered "major" by this RFC already move the bar *very
+significantly* from what was permitted pre-1.0. It may turn out that even the
+minor forms of breakage permitted here are, in the long run, too much to
+tolerate; at that point we could revise the policies here and explore some
+opt-in scheme, for example.
+
+# Unresolved questions
+
+## Behavioral issues
+
@pnkfelix
pnkfelix May 11, 2015 Member

Reading the first item below, I wondered: what does adding a #[must_use] annotation to a type qualify as? Seems like a major change to me -- or is that lint only warn-by-default?

And are there other attributes in this bucket, where adding them should be considered a major change? (E.g. changes to #[inline] directives are probably safe, but then there's stuff like #[unsafe_no_drop_flag] ... though I guess that is unstable anyway, right?)

@sfackler
sfackler May 11, 2015 Member

must_use is warn by default.

@theemathas theemathas commented on the diff May 12, 2015
text/0000-api-evolution.md
+
+Perhaps somewhat surprisingly, generalization applies to trait objects as well,
+given that every trait implements itself:
+
+```rust
+// MINOR CHANGE
+
+// Before
+fn foo(t: &Trait);
+
+// After
+fn foo<T: Trait + ?Sized>(t: &T);
+```
+
+(The use of `?Sized` is essential; otherwise you couldn't recover the original
+signature).
@theemathas
theemathas May 12, 2015

Is it OK to go the other way around?

// Before
fn foo<T: Trait + ?Sized>(x: Box<T>);

// After
fn foo(x: Box<Trait>);
@reem
reem May 12, 2015

No, since that is less general and can break users in non-fixable ways.

@theemathas
theemathas May 12, 2015

@reem Can you give an example?

@huonw
huonw May 12, 2015 Member

I suspect coercions save the day for that sort of example, but more complicated examples with nested types where coercions don't/can't work will certainly break:

fn foo<T: Trait + ?Sized>(x: &[Box<T>]) {}

can be called with &[Box<i8>] (if i8: Trait) but the &[Box<Trait>] version cannot.

@Kimundi
Kimundi May 12, 2015 Member

@theemathas: You can not create trait objects for unsized types, so if eg str implemented Trait you could put it in the "before" case, but not in the "after" case.

@pnkfelix
Member

One subject that the RFC should probably address but currently does not: How to classify changes to lifetime parameters and lifetime bounds ?

E.g. it seems like this rewrite is a patch-change (not even minor):

fn foo(&self) -> &int;
fn foo<'a>(&'a self) -> &'a int;

There is also this kind of rewrite:

fn foo<'a>(x: &'a Trait);
fn foo<'a>(x: &'a (Trait + 'a));

and then there is this one, the validity of which may change based on ongoing discussion:

fn foo<'a>(x: &'a Box<Trait>);
fn foo<'a>(x: &'a Box<Trait+'a>);
@aturon
Contributor
aturon commented May 12, 2015

Thanks everyone for the feedback! I've pushed a slew of updates to the RFC addressing most of the points that have been raised. I'll also respond to a few general comments below, and do another round of revisions soon.

@nikomatsakis

I'll go over in detail, but one thought I had is that it might be useful to elaborate a kind of "best practices for forwards compatibility". That's probably not part of this RFC, but it might help illuminate nonetheless.

That sounds interesting, but I'm not entirely sure what you have in mind. Could you give a couple examples?

@BurntSushi

I very much love this RFC. Nice work @aturon!

Thanks!!

@theemathas

What happens if someone finds another way to cause unsafty using safe code and the standard library? We already had thread::scoped and Vec::drain.

@nikomatsakis is working on a separate RFC to talk about soundness issues, where the policy is somewhat different than in this RFC.

@apasel422

Should this mention changing an object-safe trait in such a way that it is no longer object-safe?

Good point! I'm adding text to that effect.

@theemathas

What about changing an unsafe function to a safe function?

Good question. Right now, this is a breaking change due to lack of subtyping. I'm not 100% sure what the right policy is here.

@glaebhoerl

I agree completely with your analysis. This RFC isn't digging too deeply into the specifics of elaboration, but tooling along the lines you're suggesting sounds very interesting indeed.

@pnkfelix

I've done an initial round addressing some of your comments, but need to revisit others. I'll let you know when I think I've addressed everything.

@pnkfelix
Member

Changes within macros are also a potential source of code breakage.

While the language strived to make macro_rules! hygienic, there are places where it simply it is not. (Some of these cases might get fixed in the future, I am not sure how many we can fix in a backwards-compatible fashion, though.)

For example:

My point in listing the above examples is that if a crate or the stdlib provides a macro_rules! style macro, then as long as the bugs above persist, one must be very careful in what changes one might make to the content of that macro. Transformations like supposed "alpha-renaming" that in principle are semantics preserving may not actually apply.

(Now, whether such things get classified as "major" or "minor" changes is another matter...)

@bstrie
Contributor
bstrie commented May 15, 2015

This is looking great! An incredible effort. ❤️

@bstrie
Contributor
bstrie commented May 15, 2015

Would it be worthwhile to go through and study the regression reports from the past four weeks and characterize each occurrence of breakage according to the categories presented here?

@bstrie bstrie commented on the diff May 15, 2015
text/0000-api-evolution.md
+ to disambiguate are not automatically "major" changes. (But in such cases, one
+ must evaluate how widespread these "minor" changes are).
+
+* In principle, it should be possible to produce a version of dependency code
+ that *will not break* when upgrading other dependencies, or Rust itself, to a
+ new minor revision. This goes hand-in-hand with the above bullet; as we will
+ see, it's possible to save a fully "elaborated" version of upstream code that
+ does not require any disambiguation. The "in principle" refers to the fact
+ that getting there may require some additional tooling or language support,
+ which this RFC outlines.
+
+That means that any breakage in a minor release must be very "shallow": it must
+always be possible to locally fix the problem through some kind of
+disambiguation *that could have been done in advance* (by using more explicit
+forms) or other annotation (like disabling a lint). It means that minor changes
+can never leave you in a state that requires breaking changes to your own code.
@bstrie
bstrie May 15, 2015 Contributor

The "could have been done in advance" rule is a lovely formulation. It makes communicating the scope of potential breakage easy and eases the fear of wanton flaunting of semver. Well done!

@bstrie bstrie commented on the diff May 15, 2015
text/0000-api-evolution.md
+## Mitigation for minor changes
+
+### The Crater tool
+
+@brson has been hard at work on a tool called "Crater" which can be used to
+exercise changes on the entire crates.io ecosystem, looking for
+regressions. This tool will be indispensable when weighing the costs of a minor
+change that might cause some breakage -- we can actually gauge what the breakage
+would look like in practice.
+
+While this would, of course, miss code not available publicly, the hope is that
+code on crates.io is a broadly representative sample, good enough to turn up
+problems.
+
+Any breaking, but minor change to the standard library must be evaluated through
+Crater before being committed.
@bstrie
bstrie May 15, 2015 Contributor

I don't think we want to commit to running Crater with every minor change. Is the current cadence of once-a-week reports too slow?

@bstrie bstrie commented on the diff May 15, 2015
text/0000-api-evolution.md
+particular, when methods happen to conflict across traits defined in separate
+crates, a user of the two traits could rename one of the methods out of the way.
+
+## Thoughts on possible language changes (unofficial)
+
+The following is just a quick sketch of some focused language changes that would
+help our API evolution story.
+
+**Glob semantics**
+
+As already mentioned, the fact that glob imports currently allow *no* shadowing
+is deeply problematic: in a technical sense, it means that the addition of *any*
+public item can break downstream code arbitrarily.
+
+It would be much better for API evolution (and for ergonomics and intuition) if
+explicitly-defined items trump glob imports. But this is left to a future RFC.
@bstrie
bstrie May 15, 2015 Contributor

Is there any possibility that the changes to name resolution from such an RFC would be a breaking change? Are we allowing ourselves that possibility because "we intended it to be this way"?

@bluss
bluss commented May 15, 2015

What about changing a crate from depending on Rust 1.0 to Rust 1.1 (and so on)? This is a non-breaking change I assume.

@bluss bluss commented on the diff May 15, 2015
text/0000-api-evolution.md
+crate wishes to define a closed set of types that implements a particular
+interface. Such an attribute would make it possible to evolve the interface
+without a major version bump (since no downstream implementors can exist).
+
+**Defaulted parameters**
+
+Also known as "optional arguments" -- an
+[oft-requested](https://github.com/rust-lang/rfcs/issues/323) feature. Allowing
+arguments to a function to be optional makes it possible to add new arguments
+after the fact without a major version bump.
+
+**Open-ended explicit type paramters**
+
+One hazard is that with today's explicit type parameter syntax, you must always
+specify *all* type parameters: `foo::<T, U>(x, y)`. That means that adding a new
+type parameter to `foo` can break code, even if a default is provided.
@bluss
bluss May 15, 2015

default type parameters for functions & methods aren't fully implemented, they don't seem to work really. So this can be addressed partially by just finishing that.

@Stebalien Stebalien commented on the diff May 17, 2015
text/0000-api-evolution.md
+
+### Structs
+
+See "[Signatures in type definitions](#signatures-in-type-definitions)" for some
+general remarks about changes to the actual types in a `struct` definition.
+
+#### Major change: adding a private field when all current fields are public.
+
+This change has the effect of making external struct literals impossible to
+write, which can break code irreparably.
+
+#### Major change: adding a public field when no private field exists.
+
+This change retains the ability to use struct literals, but it breaks existing
+uses of such literals; it likewise breaks exhaustive matches against the struct.
+
@Stebalien
Stebalien May 17, 2015 Contributor

I'd explicitly exclude the following design pattern from this restriction:

pub struct MyOptions {
    pub option_1: usize,
    pub opiton_2: &str,
    #[doc(hidden)]
    pub _non_exhaustive: (),
}
pub enum SomeError {
    A, B, C,
    #[doc(hidden)]
    _non_exhaustive,
}
@Stebalien Stebalien commented on the diff May 17, 2015
text/0000-api-evolution.md
+
+Adding any item without a default will immediately break all trait implementations.
+
+It's possible that in the future we will allow some kind of
+"[sealing](#thoughts-on-possible-language-changes-unofficial)" to say that a trait can only be used as a bound, not
+to provide new implementations; such a trait *would* allow arbitrary items to be
+added.
+
+#### Major change: any non-trivial change to item signatures.
+
+Because traits have both implementors and consumers, any change to the signature
+of e.g. a method will affect at least one of the two parties. So, for example,
+abstracting a concrete method to use generics instead might work fine for
+clients of the trait, but would break existing implementors. (Note, as above,
+the potential for "sealed" traits to alter this dynamic.)
+
@Stebalien
Stebalien May 17, 2015 Contributor

I believe removing a Self: Sized constraint from non-generic (i.e. otherwise object safe) functions can't break anything. Argument:

  1. impl<T> Trait for T where T: ... isn't possible outside of the crate that defines Trait so we never have to deal with the case where Self could be either Sized or unsized.
  2. Self is sized: Any explicit Self: Sized bounds on the function are redundant.
  3. Self is not sized: The function couldn't have been implemented before the Self: Sized bound was removed.

This argument does not generalize to T: Sized where T is an parameter type because 1 doesn't apply in this case.

See: rust-lang/rust#25471

@Stebalien
Contributor

It would be kind of nice to include a machine readable API change file in minor updates. Cargo could use this to at least prevent silent breakage. Theoretically, cargo could also use this to automatically disambiguate but I don't know if we really want to rewrite user code and take responsibility for any bugs that might result (we'd have to be REALLY careful and would probably want to prompt the user for every change).

@alexcrichton alexcrichton referenced this pull request in rust-lang/rust May 17, 2015
Merged

syntax: Remove unused `packed` attribute #25541

@aturon aturon added the T-libs label May 22, 2015
@aturon
Contributor
aturon commented May 28, 2015

Sorry for the delayed responses, I'm digging out from a bunch of stuff near the 1.0 release.

One general question that @wycats has raised offline with me is: do we need to require a full implementation of elaboration before we can fully embrace the policy here? Certainly this RFC is strengthened by various implementation work (glob import refinements probably being the most important), but my feeling is that we can adopt this basic framework even without working elaboration, but be especially judicious with checking changes against Crater for breakage. But I'd be curious to hear others' thoughts on this.

@pnkfelix

Regarding macros: this is definitely something we should think about, but I'm much less confident about what to say there at the moment, and we are also relatively conservative about the macros we export from std. I'm inclined to punt on these questions for now.

@bstrie

Would it be worthwhile to go through and study the regression reports from the past four weeks and characterize each occurrence of breakage according to the categories presented here?

Hm, I don't think so -- most of the breakage in the runup to 1.0 would fall into the "requires major version bump", I believe.

I don't think we want to commit to running Crater with every minor change. Is the current cadence of once-a-week reports too slow?

That's a good point. It may be worth further specifying which minor changes need a look on Crater. In particular, simply adding a public item shouldn't require it, even though it could break glob imports -- partly because we should make globs more robust, but partly because we wouldn't be deterred from making a change for that reason (I don't think). Whereas adding a defaulted method to a trait should definitely be tested on Crater for conflicts.

Regarding the existing regression reports: I was imagining this happening during prototyping/early implementation, rather than after landing in nightly. But maybe it's enough to land PRs directly to nightly as assess the breakage there, and then consider yanking breaking changes well before stabilization.

One other thing to keep in mind: a stable build still ships with all of the nightly APIs; it just lints on them. That means that basically all breaking changes would be immediately apparent on stable builds too; there's not a lot of margin for error here.

Is there any possibility that the changes to name resolution from such an RFC would be a breaking change? Are we allowing ourselves that possibility because "we intended it to be this way"?

Yes, it's possible, though @nrc took some pains to future-proof resolution for exactly this reason. And yes, I think these can largely be considered "bug fixes". But we should try to do them soon.

@bluss

What about changing a crate from depending on Rust 1.0 to Rust 1.1 (and so on)? This is a non-breaking change I assume.

Yes, I would agree -- in particular, if we assume that upgrading from 1.0 to 1.1 is itself not breaking (in a major way), then requiring that upgrade shouldn't count either.

default type parameters for functions & methods aren't fully implemented, they don't seem to work really. So this can be addressed partially by just finishing that.

Yep, we definitely need some implementation work in this area to make this RFC fully fly.

@Stebalien

I'd explicitly exclude the following design pattern from this restriction ...

Seems fair, but even better would be to land the equivalent first-class language features. :)

@comex
comex commented May 29, 2015

An idea:

The original version of #1122 proposed having a Rust version attribute in each crate's Cargo.toml. That has since been removed in favor of a more minimal PR, but suppose it ends up happening. What if rustc automatically versioned the standard library based on that number, preventing minor changes from actually breaking anything (in a better way than elaborated source, see below)? All #![stable] declarations in the standard library already have an associated Rust version number; this wouldn't be quite sufficient to take care of all ambiguities, but it'd be close. Specifically:

  • When resolving dispatch ambiguity for a method call, if some of the possible methods are stable in a version later than the caller crate's (or unstable), and others aren't, discard the former.
  • Ditto for glob imports: while having explicit items shadow glob-imported ones avoids problems in most use cases, there is still the case of conflict between multiple glob imports, which can be resolved in the same way.
  • Optionally: rather than using version comparison to resolve conflict, simply forbid newer items from being used by older crates altogether, in the same way as unstable items. This makes it harder to accidentally break compatibility with older Rust versions, and provides a carrot for libraries to upgrade and properly resolve the breaks. Also might make the below a bit easier to implement.
  • New template parameters: add a separate stability attribute for template parameters and allow older crates to omit them without .... Or just allow that in all cases, might make things simpler.
  • Introducing generics: this is the ugliest one, but compatibility can be solved by having a mechanism for the standard library to specify the old version of the signature, then having type inference use that version for older code, while still checking afterward that the resolved parameters match the new signature.

The last two sound a bit ugly, but I think it should be fairly rare for such changes to existing items to be made in the first place. Implementation of the type inference change sounds like it could be a challenge.

Edit: (...Perhaps a better solution for those would be to just allow entirely separate declarations that apply to versions < and >= X, rather than any messing with type inference or template parameters.)

So why go to this effort, if all minor changes are designed to break little code in the first place? A few reasons:

  • A rustfix that was fully automated would have to do the same work (including the ugly bits with type inference), unless it resorted to full elaboration (i.e. sticking types in everywhere whether needed or not), which would work as a quick fix but be pretty useless as an upgrade path. Intermediate solutions may be possible, though.
  • Elaborated source would require either having an already built copy of your library dependency or downloading a specific old version of rustc. This means that while cargo could do the conversion automatically, it would be a major usability hassle. In contrast, by maintaining the required information in the current source as above, users would not suffer if libraries just sat on older versions forever - which sounds gross, but is going to happen one way or another, because code often gets abandoned by its authors.
  • With such a robust upgrade path, it would be possible to be slightly more liberal than this RFC proposes in making small breaking changes. Of course, there are many huge downsides to breaking things, period - examples on the Internet, upgrade hassle, etc. - and I don't have any specific suggestion about what sorts of changes could be allowed - but especially combined with Crater, there is at least a bit more wiggle room.
  • And if it turns out to be desirable to release a Rust 2.0 in 1 year or 10, the same mechanisms could be used to avoid a Python 3-like flag day. (Language changes would probably make this trickier than usual, but that's a matter for the other RFC - it's my opinion that enough languages have successfully pulled this off that Rust should be able to, too.)

Edit: The same mechanisms could be adapted for use by independent crates. They ought to have equivalent ways to mark things stable or not anyway... nothing special about the standard library in its desire to introduce experimental features without committing to them.

@alexcrichton
Member

This RFC is now entering the week-long final-comment period.

@huonw
Member
huonw commented Jun 3, 2015

I'm in favour, but I think that we will definitely want to be "quick" to make amendments (to the process, even if we don't literally change the text here) in future as we gain more experience, and as the ecosystem/community changes.

(I'm also a little nervous about #1105 (comment) .)

@comex comex referenced this pull request Jun 5, 2015
Closed

Target Version #1147

@alexcrichton
Member

The consensus of the library subteam is to merge this RFC. We may tweak the specifics here and there over time, but there is broad consensus among the core ideas behind this RFC and minor updates are always fine to make later!

@alexcrichton alexcrichton merged commit 384e749 into rust-lang:master Jun 10, 2015
@aturon
Contributor
aturon commented Sep 25, 2015

It occurred to me today that this RFC did not take into account the impact of "OIBIT" traits (which have a .. impl). In particular, these traits can introduce downstream sensitivity to every aspect of a data type's representation, even if that representation is private.

Effectively, OIBITs make it possible for downstream crates to make promises on behalf of upstream crates that can easily be broken.

I don't think this should change anything about the policy itself, but it'd be worth amending the RFC to discuss it.

RFC issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment