Edition Based Method Disambiguation: Preventing inference ambiguity breakages with extension trait methods

[yaahc]

• Rendered

---

This RFC proposes introducing an edition field to stability attributes on APIs in the standard libraries and using these edition fields to prevent breakage when newly introduced standard library methods share the same name with pre-existing methods on extension traits defined in downstream libraries.

cc @rust-lang/libs-api and @rust-lang/compiler since this RFC will likely need signoff from both teams. Libs API from the perspective of whether or not this is an acceptable solution to these breakages and Compiler from the perspective of whether this can be reasonably implemented in the compiler.

[programmerjake]

similar idea, but goes further with separate per-edition symbols that are user accessible from all editions (allowing edition migrated code to use the old function still): https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Effect.20of.20fma.20disabled/near/274199236

[bstrie]

This proposes adding an edition field to rustc_stable, so I'll mention the spiritual opposite RFC #3088 , which proposed to add an edition field to rustc_deprecated.

[clarfonthey]

So, the alternatives section mentions that supertrait method shadowing wouldn't solve all cases, but is there actually any data suggesting how regular those cases are in the wild? The specific case of intersperse, for example, would be fixed by that.

If there's data suggesting that these cases aren't very prevalent, I would personally feel like edition-based method resolution is pretty much a nuclear approach, and should be avoided whenever possible. There is already precedent of this occurring with array IntoIterator, but that feels more like a one-off case that shouldn't happen very often.

[yaahc]

So, the alternatives section mentions that supertrait method shadowing wouldn't solve all cases, but is there actually any data suggesting how regular those cases are in the wild? The specific case of intersperse, for example, would be fixed by that.

If there's data suggesting that these cases aren't very prevalent, I would personally feel like edition-based method resolution is pretty much a nuclear approach, and should be avoided whenever possible. There is already precedent of this occurring with array IntoIterator, but that feels more like a one-off case that shouldn't happen very often.

I don't have any specific data but I know that the second example I gave in the RFC, the one for adding a Display impl to ErrorKind¹ would not be fixed by the supertrait item shadowing approach, so we've already seen examples of the shortcomings of that approach.

This does raise the point that I don't think these should be seen as alternatives, but rather as complements. We can fix a lot of these with super trait item shadowing and absolutely should, but I don't want to leave holes in the system where we can't prevent breakage, so we shouldn't use super trait item shadowing as a reason for not accepting this PR.

Footnotes

1. https://github.com/rust-lang/rust/issues/94507 ↩

[m-ou-se]

Have we investigated how often this is actually a correct assumption? If I remember correctly, quite a few times we've stabilized something that's slightly different from something that already exists in the ecosystem. For example, std's new version might return things by reference where the version from the ecosystem might return by value.

[yaahc]

I have not but that seems like a good thing to check. I wasn't super confident about suggesting this behavior for rustfix so if it turns out we should only have the cross edition one and have users disambiguate manually for these cases that seems like a reasonable outcome as well. I'll try to find as many examples of these breakages as I can and start keeping a list.

(tossing in this comment for now, will move later)

• Tracking Issue for int_roundings rust#88581

[m-ou-se]

I'm not sure if it makes sense to tie this to editions. A period of three years is reasonable for language changes, but I don't think that's acceptable for library changes.

[m-ou-se]

Why do we need a separate edition and since field? Would we ever have e.g. a since = "1.56.0" that's not editon = "2021"? Or a since = "1.55.0" that's not edition = "2018"?

[yaahc]

I had initially considered not even having an edition field and having it be derived from the compiler version, but decided against it based on some advice from @Manishearth. His thinking if I recall correctly was that we want the ability to leave out the edition field for APIs that otherwise couldn't have caused breakage, because otherwise we might add a new type with some new methods and then people could add conflicting methods and get warnings, when really that conflicting method should have been an error from the start. We may very well want to enforce correctness by cross referencing the edition and since fields to ensure consistency, but I think we do want both of them.

[m-ou-se]

Ah, I think that makes sense. Then the only question left is whether we'll actually be able to judge properly if a change can cause breakage. And would use this only for known cases of breakage, or for all things that can theoretically cause breakage?

[yaahc]

Ah, I think that makes sense. Then the only question left is whether we'll actually be able to judge properly if a change can cause breakage. And would use this only for known cases of breakage, or for all things that can theoretically cause breakage?

My expectation is that it's probably fine if we leave off the edition field when we're unsure, and if we run into reports of breakage or see them on crater we can always go back and add the edition field to the offending API to resolve that breakage immediately. At a minimum though I imagine we'd want to always put an edition field on stability attributes for new methods on existing types and traits, and on any new Display impls for types that already have Debug impls.

[yaahc]

I'm not sure if it makes sense to tie this to editions. A period of three years is reasonable for language changes, but I don't think that's acceptable for library changes.

This doesn't tie library changes to editions. With this it just resolves the rare cases of breakages of APIs we're already introducing. If there are no conflicts the new API will be available like normal.

[m-ou-se]

This doesn't tie library changes to editions. With this it just resolves the rare cases of breakages of APIs we're already introducing. If there are no conflicts the new API will be available like normal.

Oh, I think I accidentally missed this part in the RFC:

when we detect an ambiguity

Without that, it seemed to suggest that APIs would not be available through method resolution until the next edition.

Thanks for clarifying.

[yoshuawuyts]

Oh, I should've mentioned this before I pressed submit on my review: but this feature seems really cool, and I'm excited for what this might enable. For async Rust we've been wondering how to add methods to the traits provided by the stdlib without breaking the whole ecosystem, and this seems like a step in providing an answer to that!

[yaahc]

Building on @cuviper's third comment above about how this could relate to into_iter for arrays. If we are able to make this behave nicely around deref coersions I think this may also help with introducing new methods to smart pointer types like https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.try_unwrap where we might be able to introduce them with a self parameter and gate the deref behavior behind that edition.

[nikomatsakis]

Huh, this is somewhat counterintuitive to me. I understand where it's coming from, but the workflow seems surprising.

[nikomatsakis]

Seems plausible, but as noted elsewhere this reasoning would like only apply to one level of autoderef. An alternative might be:

• If a method with this "dependent" character exists and we are in the "compatibility" mode:
  • Run the method selection probe once without the method in the list.
  • If this gets a "no such method" error, run again WITH the method in the list.

This seems like it would be strictly non-breaking.

My hunch is that it makes sense -- at this RFC stage -- to leave this unspecified and instead to focus the RFC on enumerated interesting examples and how we would ideally like them to behave. I can come up with various patterns to think about. Then we worry about how to implement it.

[joshtriplett]

@nikomatsakis I feel like we shouldn't tie any kind of "you have to change your code" changes to updating rust-version; that would give people the (correct) impression of breakage when upgrading Rust version. Editions specifically do advertise "you might have to change your code when upgrading to a new edition".

You can still use methods from a later edition, you just have to remove the ambiguity in name resolution in order to use the std method, or you need to explicitly reference the std name with a path.

[yaahc]

Lock files are published if you have a binary in the project.

Aaah, I had assumed we were talking about library crates, I see the problem now. I'm happy to add a note about this, and part of me wants to avoid dealing with this problem as part of this RFC to keep it as simple as possible, but another part of me wants to solve it immediately. The first solution that comes to mind is to require updating your lock file whenever the edition field is incremented in your toml file.

[CAD97]

The lockfile is also packaged for libraries which have an example. cargo publish also supports both --locked and --frozen as well as of course --no-verify. Plus you also have -Zminimal-versions, which may be required in order to build on the MSRV.

I also want to directly offer an argument against

The rust-version approach would prevent many breakages by not ever resolving ambiguous method calls to new methods when those new methods are introduced in later versions than your MSRV, but it would not be a complete solution by itself since otherwise it would turn bumping MSRV into a breaking change. This is counter to our stability policy which promises to only introduce breaking changes at edition boundaries.

I agree with scottmcm that

the critical thing is that people can upgrade their toolchain without breakage.

The key important part to the stability guarantee is that if you have a crate today, a new version of rustc N years later will still be able to compile it.

It is desirable but not core that you can use new features without fixing new warnings. Until we added k#word, it was the case that edition2015 just couldn't use async.await.

So I think that this concern

Bumping your compiler version and by extension your MSRV within the same edition without fixing warnings should never break your code.

is not just about upgrading which compiler you are using (this does not imply bumping MSRV), but also utilizing new language features, which is updating your MSRV. So it should instead read

Upgrading your MSRV (e.g. to take advantage of new language features) without fixing warnings should never break your code.

This is a reasonable position to take, is potentially supported by the stability promise¹, and becomes even more desirable an axiom if the compiler were to start erroring on the use of features newer than rust-version.

For the rest of this post I am going to treat rust-version as a binding contract enforced by the compiler that you don't use newer features. This is the strictest possible interpretation and the effect in practice of actually conforming to the advertised MSRV.

But I argue that this isn't wanting for tying name resolution to editions, but instead for introducing a std-version key instead.

This makes std behave much more like any other library, and as a bonus makes how version-influenced name resolution would be extended to other libraries trivial, rather than requiring every library to introduce (and users to manage) their own edition train.

Reduced to absurdity, the argument that name resolution priority of the special std library should be tied to the language edition reduces to it should be possible to use new APIs in std without resolving name resolution conflicts introduced by new APIs.

The problem is that using lockfiles means that just because you don't currently have a name collision on new APIs doesn't mean that one isn't present in your updated dependency tree.

Assuming full due diligence from crate publishers — that is, never publishing a new version which adds a new name which could cause a new name resolution warning, or which could completely shadow a std name, and always testing with both current maximal and minimal version resolution — it seems possible² that tying ambiguous name resolution fallback to edition instead of rust-version could both allow the use of new library APIs in and not require said libraries to do their due diligence in resolving name ambiguity warnings.

But this seems really lopsided to me. In this scheme we're requiring a lot of open-ended due diligence on the part of the library authors to prove that they aren't introducing new potential for name resolution ambiguity¹... just so that they can avoid doing the simple, machine-enumerated task of resolving existing ambiguity before using new language features.

And for all that work, we still aren't solving the issue for local crates! It doesn't help my proposal that local crates probably aren't using rust-version³, but it hurts all schemes that they can be using arbitrarily old dependencies. Unlike with published crates, there's no way to enforce⁴ that they're using up-to-date dependencies, and ever requiring they do actively goes against stability goal of upgrading toolchains should require no further changes.

The failure case with a local crate is even more worrying if it doesn't cause a build error, and only causes a warning. The whole point of not requiring addressing warnings before using new toolchains / library features is that we don't want to require paying attention to warnings. But if a local crate updates their toolchain and takes advantage of a new std trait method, then gets around to updating a library dependency that also provides that trait method, their code now effectively silently — because we've told them they can upgrade without looking at warnings — has changed behavior.

---

To be tractable, any initiative for improving stability / decreasing the number of allowed breaking changes has to be explicit about goals and non-goals. For this RFC (and thus any competing proposal should meet the same goals or actively argue an unmet goal nondesirable), I believe we have

Goals

Given any compiling crate and resolved dependency tree,

• Upgrading the toolchain should never result in different method lookup⁵
• Upgrading the rust-version key should never result in different method lookup
• If all crates in the dependency tree are well-behaved⁶
  • Upgrading private-to-root dependency versions should never result in different method lookup

Non-goals

• Upgrading public-to-root dependency versions may result in different method lookup, even if all dependencies are well-behaved
  • e.g. a trait method got deprioritized in favor of a library version
• Upgrading dependency versions where any dependency is not well-behaved may result in different method lookup
• Additional caveat: even if all dependencies are well-behaved, upgrading private-to-root dependency versions can result in a method lookup conflict between crates, just not to std

---

I am actively re-counter-proposing tying method lookup deprioritization to rust-version, or a new std-version key if access to new language features separately is considered important, rather than edition. I argue that the provided limitation/downside does not apply

• the burden of resolving machine-enumerated method lookup ambiguities is significantly smaller than the other due diligence required for a well-behaved crate
  • and in fact is not required the crate doesn't care about being well-behaved
• the important part is that a new toolchain can compile existing code
  • not that old code can use new features without minor changes

and comes with measurable benefits

• crates can switch to using std functionality idiomatically immediately
• method deprioritization is trivially extendable to libraries annotating stabilization versions
  • significantly decreasing the effort required for a well-behaved library
  • using the std version as the resolution if the std version is included but the library version is not

Footnotes

1. The counterargument is that the stability guarantee applies solely to the new compiler compiling your code, and not access to new features. ↩ ↩²

2. However, I am not willing to bet that there isn't still a hole allowing for a new name ambiguity to sneak in. ↩

3. Though we could tip the balance going forward by including rust-version in the cargo new template. ↩

4. Enforcing it for published libraries is still not necessarily possible, either. And in the extreme there're libraries that just fail to do their full due diligence; are we just blaming them if name inference fails? ↩

5. Caveat to the entire list: as currently defined, this only changes the behavior to meet the goals if the conflict is at the same autoderef level. This means only trait name conflicts on identical Self is covered. Inherent methods and new trait implementations are not covered. ↩

6. A dependency is considered well-behaved if it does not add any new methods to existing traits which clash with a stable-at-the-time method name on a non-mutually-exclusive std trait and was verified using the most up-to-date resolution of its dependency tree at time of publish. ↩

[joshtriplett]

@rfcbot merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

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

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[CAD97]

Despite being a loud proponent for an alternative solution, I'd like to formally state I have no current concerns about the acceptance of this RFC.

Note: [...] This RFC only changes behavior when there is an ambiguity to make things that are currently errors into warnings until the next edition boundary.

I think this may be the contentious point which is difficult to ensure is communicated entirely. The edition-sensitive name resolution rule as added by this RFC exclusively impacts the narrow case of default methods added to preexisting traits, and only serves as a disambiguation rule when method name resolution would otherwise fail because it resolved multiple applicable candidates from multiple traits in scope.

There are no changes made to name resolution by this RFC; solely a new step of name selection when multiple resolution candidates are present is added. If a name has higher resolution priority (e.g. it's an inherent method or earlier in the autoref lookup order), it's still always chosen.

---

I do think it is fair to say this "ties methods to editions;" it does deliberately make the new method selection (not resolution) edition-dependent. But I no longer think this is problematic, due to the following observation:

Even in its most generalized form, edition-sensitive name resolution isn't actually about upgrading the used standard library version; it's about upgrading the toolchain version without upgrading the standard library API.

The stability promise of the Rust toolchain is that if your project compiles with the 1.N toolchain, then it will also compile and run without changes¹ on toolchain 1.M for any M ≥ N, if you compile the exact same code the same way.

This isn't fully true today, because there's no way to upgrade the toolchain/compiler version without also upgrading the standard library version, and library upgrades are allowed to cause inference breakage².

The real generalized form of this feature is std adaptively downgrading its API such that user code always gets the version of the API it was written against, even without the code being written in fully elaborated form. The edition-sensitive part of it just puts a cap on how far std will downgrade its API, such that std functionality can eventually actually be used without requiring the fully elaborated form.

To this interpretation: it could make sense to also consider rust-version as a lower bound for this name deprioritization. The goal imho isn't that my crate should be able to raise its manifest rust-version without encountering minor breaking changes; it's that the full package tree can raise its toolchain version without breaking packages unaware of the toolchain upgrade. This is not asking for strict rust-version enforcement; such would supersede adaptive downgrading if enabled, but is in fact a distinct problem.

Doing so may make the purpose of the name deprioritization more obvious, and would probably be necessary for more extensive application.

I am explicitly not proposing such a scheme. I merely present it as a potential generalization of the RFC's behavior and potential model of what the behavior is accomplishing.

Footnotes

1. Presuming no UB, of course. ↩

2. We say a library change is only a "major" breaking change if it breaks fully elaborated downstream code. We consider name inference breakage (here this includes name shadowing / higher priority resolution for the same name lookup) as allowed "minor" breaking changes because adding any new functionality can cause name inference breakage. ↩

[pnkfelix]

The text should clarify what the word "matches" means above, or use different phrasing. (When I first read it, I thought "matches" meant "equals" and thought that the text was only covering the cases of {stdlib-method < crate, stdlib-method = crate} and not stdlib-method > crate.

(At this point I think the intent is that "matches" is meant to be read as stdlib-method >= crate, as in "the edition field of the standard library method is greater than or equal to the current crate's edition". I'll post a separate comment about that supposed intent.)

[pnkfelix]

I want to echo a comment made from @ehuss above: The RFC text, as written, does not give me a clear picture of the expected workflow.

Part of the intent of the RFC process is that the feedback in the comment threads on the RFC PR's be fed back into the RFC text, to improve the presentation there. We should not require people to read both the RFC text and the RFC PR comment thread in order to understand what an RFC is proposing; instead, the RFC text itself should be updated with clarifying remarks.

---

Let me spell out my own specific confusion that has led me to make the above comment.

It took me longer than I expected to infer the basis for the semantics described in the Reference-level explanation, and I am still not confident that I understand it correctly.

The proposed semantics (in the presence of a potential resolution to both a pre-edition method and a stdlib-method) is:

• if stdlib-method edition < crate edition, then signal ambiguity error
• if stdlib-method edition >= crate edition, then warn and ignore the stdlib-method (i.e. resolve to the pre-edition method).

I found the above confusing at first, because I didn't understand the basis for the handling of the case where the editions were equal. (I can understand treating stdlib-method edition > crate edition as an obvious signal that the crate predates that version of the standard library, and thus it is a good idea to bias towards ignoring the stdlib-method. And likewise, I can understand treating stdlib-method edition < crate edition as an obvious signal that the crate postdates that version of the standard library where the method was introduced, and thus it is a good idea in that case to bias towards signalling an ambiguity error. But: what to do when the stdlib-method edition = crate edition? Why is the proposed semantics the correct one?)

In short: I did not have a good mental model for how the use of the edition tags would relate to the release of a given edition, and I built up some flawed mental models.

I now realize that the RFC's expectation is that methods will be gradually introduced into Rust after edition E has been established (just as they are today), and all such methods will be tagged with edition=E, with the expectation that you will not actually start signalling ambiguity errors until the successor edition to E (because you have to account for crates developed at the outset of edition E, which hit later ambiguities with methods introduced during the evolution of the stdlib).

The latter is exactly what the RFC's semantics implies. But I don't think it is clearly explained.

I think spelling out the an example timeline of how you expect new methods to be rolled out, and how various client crates will hit the cases described, could help clarify this for people reading the RFC on its own.

---

(Having said that, this feature is entirely aimed at stdlib developers, who probably do have the above mental model of incremental development well ingrained into their brains, so its possible that they entirely sidestepped the flawed mental models that plagued my own understanding of this proposal. That is why I'm not going so far as to register a concern about my own confusions in reading this RFC text.)

[pnkfelix]

I took a swing at trying to diagram what I think the intended workflow is: https://hackmd.io/TvFfyYbpQaK5L6kseXZheg

[Manishearth]

A thing I want to mention as a potential additional thing on this feature is that we could add a <Foo as Self>::method() syntax for unambiguously calling inherent methods: we haven't needed it yet, but this would allow people to call stdlib methods in cases where there is ambiguity.

(Though I suppose that can also be handled by being careful about how and where you import traits)

[pnkfelix]

@rustbot label: +I-libs-api-nominated

I just wanted to double check with the libs team about whether they are happy with this proposal as an interface for solving this problem as stdlib developers.

(My motivation is that I don't want to put pressure on T-lang to invest time looking at this until after we have more concrete evidence that T-libs-api is happy with the proposal.)

[rustbot]

Error: This repository is not enabled to use triagebot.
Add a triagebot.toml in the root of the default branch to enable it.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[m-ou-se]

This doesn't seem to be close to consensus (yet), so I'm cancelling the proposed FCP for now.

@rfcbot cancel

[rfcbot]

@m-ou-se proposal cancelled.

[ijackson]

I think spelling out the an example timeline of how you expect new methods to be rolled out, and how various client crates will hit the cases described, could help clarify this for people reading the RFC on its own.

It's not just a clarification. The expected rollout plan for new methods is part of the design underlying the behaviour. Writing down these aspects of the design is important so that we can check that the design does in fact meet its goals. It often happens that properly describing something reveals previously-overlooked lacunae.

[yaahc]

I should mention that I'm not currently interested in continuing to push for this change. I still do believe it is worth doing, but I'm prioritizing other work I'm more interested in over this. If anyone wants to take the torch and complete this work, I'm happy to answer questions and provide clarifications about the RFC; just message me in Zulip.