New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Supertrait item shadowing v2 #3624
base: master
Are you sure you want to change the base?
Conversation
# Summary | ||
[summary]: #summary | ||
|
||
When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select that method instead of reporting an ambiguity error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select that method instead of reporting an ambiguity error. | |
When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select the sub-trait method instead of reporting an ambiguity error. |
I'd like to know how this would interact with the previous RFC #2845 . Is this meant as a complete replacement or as an extension? I deliberately did not address It reads as a replacement, but fundamentally the same issue also occurs with generic trait bounds, which I was trying to address in the previous RFC. What do you propose in the case where someone has written a generic bound for |
This is intended as a replacement for #2845 since it is more general: this RFC effectively proposes one of the alternatives listed in #2845 where we always prefer the sub-trait no matter what generic bounds are in use (and also applying to the more general case of
This RFC would always resolve |
# Rationale and alternatives | ||
[rationale-and-alternatives]: #rationale-and-alternatives | ||
|
||
If we choose not to accept this RFC then there doesn't seem to be a reasonable path for adding new methods to the `Iterator` trait if such methods are already provided by `itertools` without a lot of ecosystem churn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is picking a different name not a reasonable path? The unavoidable bikeshedding around a new name is annoying, but it seems to me like a small, one-time cost compared to the permanent additional language complexity of this feature.
I also wonder how many times we anticipate to run into this problem in the future. Are there more examples aside from Itertools::intersperse
? If we only ran into this problem once within 9 years of Rust being stable, the benefit of this feature seems very limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picking a new name also has disadvantages other than than the time taken to pick it, users then have to learn the new method name and that it is semantically identical to the Itertools::intersperse()
yet has a different name. This is not a one-time cost as it will effect all future users when, for example, searching docs for this method. This would be the case for all methods stabilized from itertools
to std
which might be quite a few if we had a reliable way to do such a migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
users then have to learn the new method name
I don't see the problem. People who are using intersperse
from itertools
shouldn't expect the standard library to use the exact same name. And I assume few people rely on the unstable version in std
for production code, it would be unwise to opt into breaking changes for what amounts to a small ergonomics improvement (which a library can also provide).
For regular Rust users, the function doesn't exist right now. In the future it may - its name is an open question.
This would be the case for all methods stabilized from
itertools
tostd
which might be quite a few
I agree that this is something to consider, we don't want to have to pick the "second best" name for many functions in std
. But first we should think concretely about which methods from itertools
might actually make the jump into std
. itertools has been around for a long time and I'm not aware of a big push to upstream many of its methods. Which indicates to me, there isn't that much need. But I'm happy to be convinced otherwise. Examples from other libraries besides itertools count as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that because itertools is used widely, std cannot upstream methods from itertools without causing lots of breakage in all crates currently using itertools. This is a perverse incentive which forces the "second best" issue you mentioned, and it's not limited to itertools, it happens whenever std lacks a function, someone implements an extension trait to add it in a crate (as they should), and everyone picks it up because it is really useful (as they should). The exact sequence of events which leads to strong evidence that something should be in std is also the sequence of events that blocks it from being added to std.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. The reason I'm pushing back is that to me, the feature doesn't seem to align with Rust's design principles. This new implicit default doesn't seem obviously correct to me. If I call a method that has two implementations, I would generally prefer the compiler yell at me rather than pick one without telling me. That's why I think we should be certain that we'll make good use of this feature before adding it.
But I'll admit this is a theoretical objection. In practice, the problem may never show up and then it's fine to add the feature. Pragmatism comes first. I guess I just agree with this comment. We should think about edge cases where this could go wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would generally prefer the compiler yell at me rather than pick one without telling me
The rfc mentions this:
This behavior can be surprising: adding a method to a sub-trait can change which function is called in unrelated code. A lint could be emitted to warn users about the potential ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, a lint would make sense if the migration-situation with intersperse is the only use case we expect this feature to be used in practice. But maybe some users will use the shadowing to implement a form of specialization? Or any other unrelated use case I can't think of right now. If that happens, it might lead to a discussion about whether the lint should be enabled by default or not. And if it ends up allow-by-default, it loses much of its value.
How about an alternative approach for stabilization where This way we don't have to add any language complexity, while still being able to migrate any methods we like, at the expense that it takes quite a while to do so. |
Then we could then add clippy lints for these renamed methods to encourage people to switch back to the stabilized |
Instead of relying on trait dependence, maybe we can rely on crate dependence? That is, if one crate crate is a dependency of the other use its method. |
(not directed at anyone in particular) Keep in mind that this RFC is generally applicable, not specific to |
It seems to me like we're adding a feature to the language not because we think it's a good feature on its own, but because it happens to have a convenient side effect for the work of the project. It feels wrong, but I'll always agree to go the pragmatic route. I'll throw in another idea for an alternative: hard-code the method resolution for those exact methods we want to uplift into The advantage I see would be that there is no seemingly useless "feature" in the langauge we have to support forever. There is no user-facing change. We may at some point run crater again and if use of I have no idea how complex this would be in terms of the implementation. This is certainly not worth paying for with much complexity. |
We should clarify the problematic scenario here: We've a crate There are several safer alternatives:
I'd think 4 or similar provides the safest solution. We'd stabalize the shadow attribute so that ecosystem crates could benefit too, but only under edition upgrades. |
@senekor, I wonder whether the project would be OK with the implications this has for other (non- I agree that complex features like this must be considered carefully, especially if they can introduce unexpected results. However, hardcoding a list of workarounds for method resolution could easily be "just as confusing", IMHO. |
Yes, that's a good point. It's not an impossible problem to solve, but I'm not fond of the idea myself.
Thank you, that is the kind of situation I was worried about. Although this seems hard to exploit in practice, I think it shows that the language feature isn't very desirable on its own. I like the idea of picking the supertrait method iff the supertrait opted into this shadowing behavior. It makes it clear that this feature is meant for our exact migration use case only. I'm not sure if gating on a specific edition and stabilization are necessary, it seems like those things could be added later as well, if they turn out to be needed. |
"A lint could be emitted to warn users about the potential ambiguity." So Perhaps there should also be another lint, that detects when the sub-trait defined that shared the exact same name with the super-trait, to address the "dependents" case. BTW I'm very confused why some comments here keep saying this RFC is a "complex feature". The whole feature is literally described in 4 sentences. The implementation is most likely localized to the probe phase. There isn't much complexity added compared with many other RFCs out there. |
At least for my comment, "complex feature" means a feature introducing language complexity. Many RFCs have more complex implementation details, but this RFC can still be complex language-wise if it makes the code harder to understand. In this case, writing new code using the standard library implementation while old code continues to use the Some programming languages are centred around keeping language complexity to a minimum, even if it comes at the cost of convenience. Now, Rust isn't Zig or Go, and the language complexity introduced by this feature is likely manageable, as this form of method resolution "feels natural" in most cases. As such, I personally don't think the complexity should block this RFC. |
@kennytm I am also thinking about langauge complexity, specifically for people learning Rust. I recently taught a Rust workshop and traits were a difficult topic for the participants. I'm wondering if in the future, I'll have to teach people about which trait method will be implicitly selected under which circumstances. Now, it's likely that the average Rust dev will never come into contact with this feature in the first place (meaning I don't have to teach it), because it only ever comes up in such migration situations. In that case, I'm perfectly fine with it. I'm just not sure of it yet. I'm trying to think of ways this feature could be (ab)used in ways that newcomers will have to know about. No success so far, which is a good sign. What would be the downside of always resolving the implementation to the supertrait? For the intended use case, it doesn't matter, because they are the same. It's also closer to what we're trying to achieve in the first place: Move away from the subtrait toward the supertrait. It seems like less potential for spooky-action-at-a-distance to me. Adding or removing a |
adding |
# Summary | ||
[summary]: #summary | ||
|
||
When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select that method instead of reporting an ambiguity error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's implicit in the statement of "encounters an ambiguity" (because having an ambiguity is dependent on actually having two methods in scope), but I would appreciate if the RFC mentions that we won't automatically select a subtrait method if it's not in scope.
In practice, for the Itertools
example, this RFC's new rules only apply if we actually use itertools::Itertools
so it's actually in scope (and therefore it's able to create an ambiguity at all).
I think "complexity" does not capture the concerns here, really neither does "intuitiveness". We do not need a single word that captures the concerns though. Wow @programmerjake that's really nasty. Any idea why that happens? I'd thought inherent methods always won out in resolution? Can we lint against this behavior? Or does std require it somewhere?
It's not "hard to exploit" in the same sense in which secureity people say "hard to exploit". It's quite easy. An exploit requires work of course, but that's common. Accedental changes cause some concern too here, like when using traits at VM boundaries. @kennytm Cool. Yeah, I figured some lint would appear eventually, but..
As denying should become common, we should ask what solution really makes the most sense here? Around my 3. Afaik, there is no good reason to favor the subtrait method over the supertrait here. It's clear either direction could break agreement between static methods and trait object methods, but we'd break agreement less if we favor supertraits over subtraits. Again, this does not solve the problem I mentioned either, but again it's less bad this way. Around my 4. We want shadowing to remain rare, so op-in attributes should create less work than any op out solution. Yes, op-in is delicate too, but several designs make sense, so they should be seriously discussed. I thought up op-ins that exploit edition boundaries, but maybe other flavors make sense. |
That's probably because the method candidate |
I'll single out the simplest seeming solution: Add a Implementation should be similar to this RFC, except the shadowing would be reversed, and turnned off by default. Any errors must originate from the rare excplicit references to There are no impacts on how you read or audit code here: We only have supertraits shadowing subtraits, which sounds safer, for both trait objects and supply chain attacks, but even this shadowing only occurs when explicitly required by the supertrait. Am I missing anything? |
Consider this situation:
In this general case, the reason this cannot be resolved in favor of the supertrait is that the method signatures are not necessarily compatible. #![allow(unused)]
mod a {
pub trait Int {
// fn call(&self) -> u32 {
// 0
// }
}
impl Int for () {}
}
mod b {
pub trait Int: super::a::Int {
fn call(&self) -> u8 {
0
}
}
impl Int for () {}
}
use a::Int as _;
use b::Int as _;
fn main() {
let val = ().call();
println!("{}", std::any::type_name_of_val(&val));
} Resolving in favor of @burdges Nothing about this RFC is specific to the standard library, so I don't see how you envision an edition fitting into this. |
As far as I can tell, regardless of the outcome of this RFC, we still have to view introduction of trait methods as introducing ambiguity, e.g. for the The main benefit I can see of this RFC is that it allows me as a library maintainer to refactor methods into supertaits without breaking explicit usage of I also think this RFC proposes an idea that feels consistent with both Deref shadowing and also #3245, so the additional complexity doesn't seem high. |
@archer-321 @senekor However, item shadowing is already happening with inherent vs trait methods, so that "complexity" already existed. (Not to mention the auto-ref based method resolution mentioned #3624 (comment) which was exploited for like Generalized Autoref-Based Specialization. This is the emergent complexity.). Explaining this RFC is as simple as For each step of auto-deref/auto-ref,
1. Pick the inherent item `S::name` if it existed
2. Find all traits `Tn` that `S` implements, and check every trait that `Tn::name` existed.
If there is a single unique trait `T0` defining `T0::name`
- and no other traits do,
+ which is a subtrait of every other traits that do,
pick `T0::name`, otherwise (if there are >1 such candidates) fail for ambiguity error. |
Also in my proposal.
An edition change provides a flag that downstream code upgraded. There is no reason other code cannot use this flag independently of the standard library, but major version upgrades provide a similar flag.
Sure. We'd exploit the edition boundary in my proposal precisely because itertools has an incompatible method signature, since itertools' methods are not In general, subtraits being favored brings a similar problem too. We could build examples that this RFC breaks too: Initially a crate has both At a high level, all this seems extremely rare, so it's much worse to make auditing hard across the whole ecosystem. If you ship this RFC as proposed, then supply chain attacks should eventually make In fact, we'll definitely have future legislation about supply chain attacks in many jurisdictions, especially for government contractors. It's likely legislation cares about upgrades, but does not care about build breakage, which could make |
Thanks for the explanations all, my opposition to this is dwindling. I agree with @burdges proposal to make this behavior opt-in from the supertrait side. So far, there hasn't been a use case discussed where it would be desirable for this shadowing to happen without the knowledge / consent of the supertrait author. So I don't see a reason to extend this implicit behavior beyond its intended use case. |
In the
This would mean that
An attribute could be an interesting idea, but in a different way: we could explicitly mark specific methods on the |
At least technically, those concerns wind up fairly symetric, as shown above in #3624 (comment)
It'll always require an edition to add itertools methods to
Yeah, there are no problems if shadowing logic were never stablized. All my comments only concern stable shadowing. As argued in #3624 (comment) though, if stabalized then this RFC worsens supply chain attacks, so crates should disable it, which makes this RFC counter productive. If you want stable shadowing, then just pick some shadowing scheme which does not worsen supply chain attacks. Imho #3624 (comment) looks clean-ish. I think supertrait favored shadowing fits trait objects better too, but there exist safe approaches to subtrait favored shadowing too: Idea 1. An attribute could explicitly name the shadower:
Idea 2. Afaik we always need edition upgrades anyways, so
|
That's entirely contrived. If you're using crates A and B and dependencies, you must already implicitly trust them fully, since they can do arbitrary things both at runtime and at build time. The answer to "my dependency added bad code in an update" is always "vet your dependencies and their updates". This RFC changes nothing in that regard. One issue I haven't noticed in either the RFC or the existing discussion is that the proposed name resolution change works both ways. It is a way to add methods to supertraits in a backwards-compatible way, yes. And that's valuable, because any trait can be subtraited in downstream crates. But also, this RFC proposes a legal mechanism to override functionality in upstream traits! A downstream trait can always just add a method with the same name as in supertrait in order to shadow it for its consumers. This has interesting implications. It's similar to the mechanism of overriding methods in subclasses in OOP languages, including C++ and Java. Notably, those languages ended up adding explicit annotations to overriding methods: People will certainly use this RFC both ways, to add methods backwards-compatibly and to override existing functionality. If the latter part is deemed undesirable for the language, this may require a change to the RFC. If it's deemed ok, perhaps we should at least simultaneously add an explicit |
@afetisov One difference between this and OOP-overriding (as I understand it), is that with this, the supertrait method still "exists" and is callable, either by just not importing the subtrait, or by using UFCS. trait Supertrait { fn method(&self); }
trait Subtrait: Supertrait { fn method(&self); }
// ...
let x: X = ...; // where X implements Supertrait and Subtrait
x.method(); // calls <X as Supertrait>::method if Subtrait is not in scope.
<X as Supertrait>::method(&x); // calls <X as Supertrait>::method regardless whereas in Java/OOP-overriding, the superclass method effectively "doesn't exist anymore" (outside of Superclass instance = new Subclass();
instance.method(); // If Subclass overrides method(), then this will call Subclass::method.
// There is no way to call Superclass::method on a Subclass instance
// (except using `super.method()` inside Subclass itself). (example) As such, I don't think an Footnotes |
@afetisov wrote:
To extend on that, Java also has a trait Iterator {
fn next ...
open fn intersperse ...
}
trait IteratorExt: Iterator {
fn next ... // ERROR: next cannot be overridden
fn intersperse ... // OK
} Counter-argument: In Java/Kotlin, overriding methods introduces dynamic dispatch. One reason to disallow it is to improve performance, but this is not an issue in Rust. Another reason is to be certain what methods are invoked; this is impossible when the receiver might or might not be a subclass, which is not (as big of) an issue in Rust. |
I don't think the comparison with Java/Kotlin/C++ is fair since they allow overloading i.e. they allow multiple methods sharing the same name as long as they have different arguments. public class Parent {
public final void a(int a) {}
public void b(int b) {}
}
public class Child extends Parent {
// public void a(int a) {} // a(int) in Child cannot override a(int) in Parent
public void a(double a) {} // ok! a(double) and a(int) are different, no @Override even needed
@Override
public void b(int b) {}
}
Child child = new Child();
child.a(64.0); // always invoke Child's `a`
child.a(32); // always invoke Parent's `a` This is not the same in Rust — pub trait Parent {
fn a(&self, a: i32) {}
fn b(&self, b: i32) {}
}
pub trait Child : Parent {
fn a(&self, a: f64) {}
fn b(&self, b: i32) {}
}
struct S;
impl Parent for S {}
impl Child for S {}
S.a(64.0); // error[E0034]: multiple applicable items in scope
S.a(32); // error[E0034]: multiple applicable items in scope With this RFC, both calls will resolve to |
This is an alternative to #2845 which aims to resolve the issues surrounding the stabilization of
Iterator::intersperse
.Summary
When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select that method instead of reporting an ambiguity error.
Rendered