rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

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

Open yaahc opened this issue 3 years ago • 35 comments


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.

yaahc avatar Mar 04 '22 22:03 yaahc

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

programmerjake avatar Mar 04 '22 22:03 programmerjake

This proposes adding an edition field to rustc_stable, so I'll mention the spiritual opposite RFC https://github.com/rust-lang/rfcs/pull/3088 , which proposed to add an edition field to rustc_deprecated.

bstrie avatar Mar 04 '22 23:03 bstrie

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.

clarfonthey avatar Mar 07 '22 19:03 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.

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^1 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.

yaahc avatar Mar 07 '22 19:03 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.

m-ou-se avatar Mar 09 '22 14:03 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.

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.

yaahc avatar Mar 09 '22 16:03 yaahc

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.

m-ou-se avatar Mar 09 '22 17:03 m-ou-se

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!

yoshuawuyts avatar Mar 09 '22 18:03 yoshuawuyts

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.

yaahc avatar Mar 09 '22 18:03 yaahc

@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.

joshtriplett avatar Apr 05 '22 04:04 joshtriplett

I'm concerned that this is tying methods to editions. I'm also concerned that resolving the ambiguities still requires manual intervention. This means that breakage in a dependency requires patching that dependency to make your code compile.

An alternative is tying method resolution to the MSRV. Let's assume a crate specifies a MSRV of 1.59. If Rust runs into a name conflict (say, the library calls Foo::bar, but this method exists both in the standard library and in a third-party crate), then Rust should check if it already existed in the standard library version 1.59. If it did, it's an error as usual. If not, the method from the third-party crate is used and a "future incompatibility" warning is shown.

This could even be generalized to always compare the since field with the MSRV when calling a method, so you get an error when a method isn't supported by the MSRV.

Aloso avatar Apr 06 '22 08:04 Aloso

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.

I'm not sure how impactful that is, though, since it's exactly the same kind of breakage that you can get by upgrading any of your dependencies, when they add methods. And if we do let crates use #[stable(since = "...")] one day, that parallel would be even better, as you'd get warned (or more) about using anything not in your specified version, and wouldn't get broken any more for method ambiguity just from upgrading the version of the crate in your lock file.

To me, the critical thing is that people can upgrade their toolchain without breakage. (Or, analogously, that a binary can compile your crate with a newer version of the library than the one in your cargo.toml without failing due to method call ambiguity.) And basing the disambiguation on MSRV would work just as well for that, with the advantage that it rolls so much more often than editions do.

scottmcm avatar Apr 06 '22 10:04 scottmcm

I'm concerned that this is tying methods to editions.

I clearly need to find a way to be more clear about this because this is a confusion that keeps popping up, but this does not tie methods to editions. It only turns currently allowed breakages between editions into a breakage at an edition boundary, if the breakage wouldn't have occured the method is still callable. This only changes behavior when there is an ambiguity.

An alternative is tying method resolution to the MSRV. Let's assume a crate specifies a MSRV of 1.59. If Rust runs into a name conflict (say, the library calls Foo::bar, but this method exists both in the standard library and in a third-party crate), then Rust should check if it already existed in the standard library version 1.59. If it did, it's an error as usual. If not, the method from the third-party crate is used and a "future incompatibility" warning is shown.

This could even be generalized to always compare the since field with the MSRV when calling a method, so you get an error when a method isn't supported by the MSRV.

This is the idea that @m-ou-se suggested as well and I think it is a good idea, and something we should do along side this RFC, but it is not a sufficient fix on its own. If we only resolve this by tying these breakages to MSRV we turn bumping MSRV into a breaking change. This would be counter to our stability policy. We have a mechanism for introducing breaking changes, Editions, and they should be used where possible to introduce breaking changes in an opt in manner.

We talked about this some more in the lang team triage meeting yesterday and I'm going to be updating the alternatives section to more thoroughly document the rust-version based approach and clarify that it isn't actually an alternative but rather a complementary feature that I believe should get its own RFC.

yaahc avatar Apr 06 '22 16:04 yaahc

If we only resolve this by tying these breakages to MSRV we turn bumping MSRV into a breaking change.

It can only break your code if you have "future incompatibility" warnings and ignored them:

Rust should check if it already existed in the standard library version 1.59. If it did, it's an error as usual. If not, the method from the third-party crate is used and a "future incompatibility" warning is shown.

So the situation is actually very similar to editions: When migrating to a newer MSRV, you first need to fix the "future incompatibility" warnings (and this could in theory be automated with cargo fix --msrv). If you forget to fix the warnings, they become errors after changing the MSRV.

Aloso avatar Apr 07 '22 12:04 Aloso

If we only resolve this by tying these breakages to MSRV we turn bumping MSRV into a breaking change.

It can only break your code if you have "future incompatibility" warnings and ignored them:

That does not contradict my statement and I do not think this is an acceptable situation to put our users in. Bumping your compiler version and by extension your MSRV within the same edition without fixing warnings should never break your code.

So the situation is actually very similar to editions

As you describe it, I agree, which is the problem. MSRV should not be equivalent to a mini edition.

yaahc avatar Apr 07 '22 16:04 yaahc

(Apologies for the long comment.)

It might help to include some concrete hypothetical scenarios (intersperse is a good one) with specific edition numbers, focused on what the user's experience is in different ways. For example, it's not immediately clear to me how intersperse would be stabilized today. Would it be marked as 2021 or 2024? What would happen for a user using a previous edition, current edition, or future edition?

Can you clarify what happens when my edition is less than the edition where the new method is added? For example, let's say std's intersperse is marked 2021, and I'm using 2018 and using itertools. In that situation, is there a warning? The text seems to imply that the warning only happens if the edition is equal.

Similar to above, let's say I'm happily using 2018. I start using std's intersperse. Sometime later, I import itertools::Iterator. Does that silently switch to itertools' implementation, or is there a warning?

Just a heads up that changing the fix behavior based on whether or not it is doing an edition migration may be difficult to implement. I don't believe the current system can support that, and I don't know how that could be hooked up.

I think it would be good to include some clarity on what happens when the function signature is different. The current text seems to assume they are always the same.

Will the edition field be required for the #[stable] attribute? If not, what happens if it is not specified? Any particular reason this couldn't be inferred from the since version? If it isn't required, why would one not want to set it?

A vague observation: IIUC, this mostly addresses the scenario where std introduces something that conflicts with a pre-existing crate, but doesn't seem to address the reverse, correct? It'll still be a breaking change for a crate to introduce something the conflicts with an already stable std version. For example, I depend on foo 1.0.0, which depends on itertools 0.12.0. foo is edition 2024. I update itertools to 0.12.1 which introduces a new method that conflicts with std, and is a hard error. This means that it is still a semver hazard to add methods to a user's trait? Imagine that 0.12.1 was published long before the std method was introduced, so they couldn't have anticipated this.

I feel like this seems heavily focused on std, but doesn't help with conflicts between user crates. I've never felt like the story for allowing breakage due to method resolution is a fair interpretation of upgrades being painless, or that being a semver "compatible" change (with "compatible" not being well defined). I understand that is a very difficult problem, and I don't expect this RFC to solve it (and I'm very thankful for you to be working on this). I just feel like that is a big hole in Rust's compatibility story (along with every "possibly-breaking" change listed at https://doc.rust-lang.org/nightly/cargo/reference/semver.html), and it would be nice to have more general solutions. Unfortunately I don't have any constructive ideas here.

ehuss avatar Apr 16 '22 18:04 ehuss

It might help to include some concrete hypothetical scenarios (intersperse is a good one) with specific edition numbers, focused on what the user's experience is in different ways. For example, it's not immediately clear to me how intersperse would be stabilized today. Would it be marked as 2021 or 2024? What would happen for a user using a previous edition, current edition, or future edition?

Today, if we were to try to stabilize Iterator::intersperse again it would cause inference breakages for any crate that already calls Itertools::intersperse via method call syntax. If we had this RFC intersperse would be marked with edition = "2021". Users in the 2021 edition and earlier editions would instead see a warning when the inference occurs indicating that there was an ambiguity, but the compiler defaulted to Itertools::intersperse because Iterator::intersperse was introduced during the current / a later edition. In future editions this would be a hard error as it is today.

Similar to above, let's say I'm happily using 2018. I start using std's intersperse. Sometime later, I import itertools::Iterator. Does that silently switch to itertools' implementation, or is there a warning?

There is a warning.

Just a heads up that changing the fix behavior based on whether or not it is doing an edition migration may be difficult to implement. I don't believe the current system can support that, and I don't know how that could be hooked up.

Unfortunate, but good to know, thank you.

I think it would be good to include some clarity on what happens when the function signature is different. The current text seems to assume they are always the same.

I'm not sure how different function signatures interact with method resolution atm. I was working with the assumption that it only considers the self type and the method name, and that any other differences in the signature are essentially ignored, but if this isn't the case I will figure out if any of the suggestions here need to change as a result.

edit: some basic testing on the playground seems to confirm this, so I think my answer to this question is that this doesn't change method resolution rules beyond adding a tiebreak in situations where there are exactly two answers and one of them is considered a recent breaking change.

Will the edition field be required for the #[stable] attribute? If not, what happens if it is not specified? Any particular reason this couldn't be inferred from the since version? If it isn't required, why would one not want to set it?

I believe all of these questions are answered in https://github.com/rust-lang/rfcs/pull/3240#discussion_r822695917 but let me know any answers are missing.

A vague observation: IIUC, this mostly addresses the scenario where std introduces something that conflicts with a pre-existing crate, but doesn't seem to address the reverse, correct? It'll still be a breaking change for a crate to introduce something the conflicts with an already stable std version. For example, I depend on foo 1.0.0, which depends on itertools 0.12.0. foo is edition 2024. I update itertools to 0.12.1 which introduces a new method that conflicts with std, and is a hard error. This means that it is still a semver hazard to add methods to a user's trait? Imagine that 0.12.1 was published long before the std method was introduced, so they couldn't have anticipated this.

I'm having trouble processing this one. Is the idea that foo pinned their itertools version to 0.12.0 as part of their upgrade to the 2024 edition? If 0.12.1 was available before the 2024 edition I don't see how foo could have avoided depending on it during their edition upgrade in a way that allows the a user of foo to cause a breakage later.

I feel like this seems heavily focused on std, but doesn't help with conflicts between user crates. I've never felt like the story for allowing breakage due to method resolution is a fair interpretation of upgrades being painless, or that being a semver "compatible" change (with "compatible" not being well defined). I understand that is a very difficult problem, and I don't expect this RFC to solve it (and I'm very thankful for you to be working on this). I just feel like that is a big hole in Rust's compatibility story (along with every "possibly-breaking" change listed at https://doc.rust-lang.org/nightly/cargo/reference/semver.html), and it would be nice to have more general solutions. Unfortunately I don't have any constructive ideas here.

There's a bit of commentary on this in a thread between scottmcm and I earlier in the discussion which has been persisted a bit to the future possibilities. I agree tho that our current policy of allowing method inference breakages is definitely a hole in our stability story that needs to be patched up. I'm hoping that this can go most of the way towards solving the issue for std, but yea, extending it to all libraries in the ecosystem is definitely a much bigger lift, though hopefully not as necessary since they don't have the same stability story that we do.

yaahc avatar Apr 18 '22 00:04 yaahc

Thanks for the response, that is indeed helpful.

Regarding the function signature differences, I think it would be nice to acknowledgment its consequences in the RFC. The diagnostic suggestion probably can't be applied, so cargo fix won't work. I don't know how often that will be the case. I think the suggestion could prefer the pre-edition method if the signature is different.

Another option for the diagnostic suggestion is to provide both suggestions, and let the user decide which one to use (when the signatures match).

Regarding the semver compatibility issue, imagine the following series of events:

  1. itertools 0.12.0 is published
  2. foo 1.0.0 is published, using edition 2021 and also importing Itertools from 0.12.0.
  3. itertools 0.12.1 is published adding cool_new_method to its iterator trait
  4. std introduces cool_new_method on Iterator with edition=2021
  5. Edition 2024 is stabilized
  6. foo 2.0.0 is published, using edition 2024, using cool_new_method from std, and still has Itertools imported from 0.12.0. foo developer hasn't updated Cargo.lock in a while. There's no warning for foo because this older version of itertools doesn't have the conflict.
  7. foo developer runs cargo update -p itertools and picks up 0.12.1
  8. foo no longer builds because it now has a conflict on the cool_new_method

This is the kind of scenario where a supposedly semver-compatible release causes the build to fail, even with this RFC's mitigation.

FWIW, I'm not advocating this RFC needs to be a perfect solution to the compatibility problem, but I think it would be good to acknowledge there are still some scenarios that cause semver-breakage when adding methods.

ehuss avatar Apr 19 '22 00:04 ehuss

6. foo 2.0.0 is published, using edition 2024, using cool_new_method from std, and still has Itertools imported from 0.12.0. foo developer hasn't updated Cargo.lock in a while. There's no warning for foo because this older version of itertools doesn't have the conflict.

Does publish respect lock files? I would have expected this to fail because when you publish a new version it does a full build which I hope and assume ignores lock files because lock files are not published with crates.

yaahc avatar Apr 19 '22 00:04 yaahc

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

ehuss avatar Apr 19 '22 01:04 ehuss

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.

yaahc avatar Apr 19 '22 01:04 yaahc

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[^stability], and becomes even more desirable an axiom if the compiler were to start erroring on the use of features newer than rust-version.

[^stability]: The counterargument is that the stability guarantee applies solely to the new compiler compiling your code, and not access to new features.

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[^but] 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]: However, I am not willing to bet that there isn't still a hole allowing for a new name ambiguity to sneak in.

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[^stability]... just so that they can avoid doing the simple, machine-enumerated task of resolving existing ambiguity before using new language features.

[^stability]: A further problem is that the more due diligence expectation we put onto crate authors (e.g. when adding a new defaulted method to a trait, I must make sure that it doesn't have the same name as any std trait method, because a downstream crate could have both implemented on a type), the more this feels like pushing the responsibility for the stability guarantee onto library authors, rather than offering a solution.

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[^new], but it hurts all schemes that they can be using arbitrarily old dependencies. Unlike with published crates, there's no way to enforce[^publish] 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.

[^new]: Though we could tip the balance going forward by including rust-version in the cargo new template.

[^publish]: 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?

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[^caveat]
  • Upgrading the rust-version key should never result in different method lookup
  • If all crates in the dependency tree are well-behaved[^well-behaved]
    • Upgrading private-to-root dependency versions should never result in different method lookup

[^caveat]: 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.

[^well-behaved]: 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.

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

CAD97 avatar Jul 10 '22 07:07 CAD97

@rfcbot merge

joshtriplett avatar Jan 10 '23 19:01 joshtriplett

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

  • [ ] @Amanieu
  • [ ] @BurntSushi
  • [ ] @dtolnay
  • [x] @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.

rfcbot avatar Jan 10 '23 19:01 rfcbot

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[^1] on toolchain 1.M for any M ≥ N, if you compile the exact same code the same way.

[^1]: Presuming no UB, of course.

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[^2].

[^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.

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.

CAD97 avatar Jan 10 '23 21:01 CAD97

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 avatar Jan 11 '23 03:01 pnkfelix

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

pnkfelix avatar Jan 11 '23 04:01 pnkfelix

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)

Manishearth avatar Jan 11 '23 05:01 Manishearth

@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.)

pnkfelix avatar Feb 21 '23 19:02 pnkfelix

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.

rustbot avatar Feb 21 '23 19:02 rustbot