Adding or removing an item should not silently change behavior of downstream code

[glaebhoerl]

If you add or remove a trait impl, or anonymous impl, or any other item in upstream code, this should have one of three effects on downstream code:

1. Nothing.
2. Where it previously compiled, now it fails to.
3. Where it previously failed to compile, now it succeeds.

What should not happen is

   4. It continues to compile, but its runtime behavior is now different.

This property is currently known to be violated by method call autoderef combined with trait impls on reference types (#11818). For instance:

mod up1 {
    pub trait Stringify {
        fn stringify(&self) -> ~str;
    }
    impl<'a, T> Stringify for &'a T {
        fn stringify(&self) -> ~str { ~"Hi, I'm a borrowed reference!" }
    }
}

mod up2 {
    use up1::Stringify;
    pub struct Foo;

    #[cfg(with_foo_impl)]
    impl Stringify for Foo {
        fn stringify(&self) -> ~str { ~"This is Foo" }
    }
}

mod down {
    use up1::Stringify;
    use up2::Foo;
    pub fn some_string() -> ~str {
        (&Foo).stringify()
    }
}

fn main() {
    println!("{}", down::some_string())
}

When compiled with --cfg with_foo_impl, this code will output "This is Foo". Otherwise it will say "Hi, I'm a borrowed reference!".

Why this is important: it seriously impairs the ability to reason about interface stability and compatibility. In today's situation, upstreams cannot add or remove impls for their types without having to worry about introducing silent breakage somewhere downstream. If this property held, then the worst case would be that downstream would be alerted to the change by a compile failure, which is much, much preferable to silent breakage. (This is exactly the kind of thing static type systems are supposed to ensure!)

The provided example is only one instance I know of where the principle is violated, I don't know whether there might be others.

[thestinger]

As far as I know, fixing this requires removing the auto-reference and auto-dereference features used for .. I don't expect it to happen at this point.

[glaebhoerl]

Don't expect is quite different from shouldn't. And if we go down that path, we should at least do so with eyes open to the consequences, which are serious. (This is the same "feature" which earned OverlappingInstances its massive unpopularity in Haskell circles.)

[pnkfelix]

@glaebhoerl perhaps you should start by adding a -Z flag that disables auto-deref, get some firsthand experience on how painful that might be?

[glaebhoerl]

I don't think simply yanking out autoderef is the only possible, or best, solution (though if it were, I would favor it!). I can think of at least two others:

1. Adopt C++'s solution to disambiguating between methods of the pointer versus methods of the pointee: p.pointer_method() versus p->pointee_method(). I like this personally, but am not so optimistic about others' enthusiasm for it.
2. Keep autoderef, but if there is ambiguity between a method of the pointer and a method of the pointee, it is an error. (It's not clear how you might disambiguate this in favor of the pointer, or what the precise definition of ambiguity might be.) But this doesn't protect against one upstream adding an instance and the other removing it, so I'm not sure it's satisfactory.

There might be other options: anyone else see one? I'd be happy to try implementing one or the other of these behind an experimental flag if it'd help move the ball forward.

[thestinger]

Adopt C++'s solution to disambiguating between methods of the pointer versus methods of the pointee

It's not really much better than using (*x).foo and doesn't scale to multiple levels of indirection. I think removing it without adding something like this would be better.

Keep autoderef, but if there is ambiguity between a method of the pointer and a method of the pointee, it is an error. (It's not clear how you might disambiguate this in favor of the pointer, or what the precise definition of ambiguity might be.) But this doesn't protect against one upstream adding an instance and the other removing it, so I'm not sure it's satisfactory.

A Clone implementation for &T is a necessity, as is a Clone implementation for types that aren't references.

[glaebhoerl]

It's not really much better than using (*x).foo

I think it's a lot more ergonomic, especially when chaining: (*(*x).y).foo versus x->y->foo, which is rather common. It's not an accident that C and C++ added it.

and doesn't scale to multiple levels of indirection

True. Again, we could follow C++ and have it resolve to the most-indirect method. Having pointers-to-pointers is not so common, and wanting to call a method on a pointer in the middle even less so, which can still be done explicitly ((*outer_pointer).inner_pointer_method()).

A Clone implementation for &T is a necessity, as is a Clone implementation for types that aren't references.

This is more general than either Clone (there are many other traits) or &T (we will have library smart pointer types with autoderef). Requiring the "cooperation" of multiple upstreams for the problem to manifest would make it something like an order of magnitude less frequent, but the question is whether we find that acceptable. We don't find it acceptable for issues of type system soundness to be merely very unlikely to be triggered, for example.

EDIT: Another solution might be almost, but not quite, the inverse of the -> solution: have . itself resolve unconditionally to a method on the most-indirected type, and add some new syntax to call methods on the least-indirect type (the outermost pointer). I have no idea what would be a good choice there, but just for the sake of illustration I'll go with <-: then not_a_pointer.method() would resolve to a method of not_a_pointer, is_a_pointer.method() would resolve unconditionally to a method of the innermost pointee of is_a_pointer (so like ->), and is_a_pointer<-method() would resolve to a method of is_a_pointer. But I'm not sure if this is any good, nor if it's worth trying to deviate from C/C++ like this. Probably not.

[glaebhoerl]

Before delving too deeply into potential solutions it might be wise to determine the scope of the problem. For instance, is this an issue with both auto_de_ref and autoref? (Once I have an answer I'll edit it in, but feel free to beat me to it.) Are there any other parts of the language which might have similar issues? I've updated the title to be more general.

[thestinger]

It's an issue for auto-dereferencing for sure. It currently doesn't find an implementation for &T when looking up the method for T but I doubt this is well-defined. It probably just works as it does now due to implementation quirks.

[pnkfelix]

Accepted for 1.0, P-backcompat-lang.

(We need to discuss ways to deal with this, but I think an approach we could solve this would be to make "ambiguous" cases an error that need to be explicitly resolved, e.g. via a function-call syntax where you are explicit about the trait and impl.)

[brson]

UFCS: #11938

[glaebhoerl]

statics in patterns is another instance where the property is violated, though at least we have some lints. Someone bring me up to speed: what's the motivation for this feature? Does it have any advantage over using a pattern guard and == besides brevity?

[pnkfelix]

@glaebhoerl I agree that statics in patterns is another instance of this hypothetical property, but I would prefer that we not fork the comment thread to discuss the motivation for that here, since sharing that property is AFAICT the only way that the two topics are related.

[huonw]

(Re statics: don't they have to be explicitly imported to be used in patterns? So the only way code can change behaviour accidentally is if is importing via globs. (Sorry for continuing the fork.))

[glaebhoerl]

That's true I think. Still, other items don't have this kind of effect (and rightly so). If someone adds a struct and you import it with a glob, your code won't end up doing different things than it did before.

It also depends on how narrowly you define upstream/downstream. Even within the same module, having to worry about whether any code in that module has a pattern with the same name before you can safely add a static is not nice. So it's the same general issue.

@pnkfelix I was intending this ticket to be about the general property, so far impls and statics as patterns exhibit known violations of it. I was thinking this is the right place to discuss whether/what could be done, and if anything specific is decided that should be done, a new issue would be opened for it. How do you think it should work instead?

[pnkfelix]

@glaebhoerl oh you are correct, I misread the title of the bug as saying "adding or removing an impl should not ...", but you are correct, it says the more general "adding or removing an item should not ..."

[glaebhoerl]

@pnkfelix Not totally surprising, that was the original title. :) I renamed it as I noted in an earlier comment. (We could have also separate issues for statics and impls and make a metabug or something, if you think that's better (though there's already some open issues for statics).)

[huonw]

That's true I think. Still, other items don't have this kind of effect (and rightly so). If someone adds a struct and you import it with a glob, your code won't end up doing different things than it did before.

No, they do have this effect.

use somemod::{Foo, bar};
use othermod::*;

fn main() {
     bar(Foo { x: 1 })
}

if othermod adds bar and/or Foo to othermod, they will shadow the explicit imports and (in edge cases) change behavior.

Although, iirc, the unused import warning will pick this up, but I guess the first import could be use somemod::*; or the shadowing glob could be inside another block, with the explicit imports used elsewhere, i.e. something like

use somemod::bar;

fn qux() { bar() }

fn main() {
    use othermod::*;

    qux();
    bar(); // supposed to be somemod's bar
    something_in_othermod();    
}

Rather than shadowing, we could have a warning for when an name is ambiguous (i.e. two imports of the same name).

[glaebhoerl]

Interesting, thanks, this is good to know.

Rather than shadowing, we could have a warning for when an name is ambiguous (i.e. two imports of the same name).

Yes, that's how I naively assumed it already worked. :) Local items should shadow imported ones, but either two local items, or two imported items with the same name (and namespace) should be ambiguous, and cause an error (not warning) to be raised at the point of attempted use.

Your second example is tougher. You don't even need somemod: if the outer module defines bar(), the same thing happens. Yet I think local items and imports shadowing ones from outer scopes feels intuitive and correct. Perhaps the right solution here would be a warning/error specifically if a glob import ends up shadowing something from an outer scope, as that's the only case where it can be unintended.

[nikomatsakis]

cc me (I am not sure that this is a bug)

[pcwalton]

I propose closing this. Autoderef and autoref are pretty fundamental, and globs can cause hazards in many ways. Nominating for closing.

[pnkfelix]

Majority of team believes that support for autoderef and autoref is more fundamental/important than the hypothetical "Adding or removing an item should not silently change behavior of downstream code" philosophy espoused in the title, and thus the example in the description is not itself actionable.

At least, not without finding some solution to the problem that does not involve removing autoref/autoderef.

But this ticket, as described, is not actionable, since it does not propose such a solution.

[pnkfelix]

(closing as unactionable, see above comment.)