Method resolution can be influenced in unexpected ways by external crates.

[steffahn]

I tried this code:

use std::cell::RefCell;

fn foo(x: &RefCell<Box<[u8]>>) {
    let y = x.borrow();
    let z = y.as_ref();

    println!("{:?}", z);
}

fn main() {
    foo(&RefCell::new(Box::new([1, 2, 3_u8])));
}

I expected to see this happen: Either the code should fail to compile or it should print [1, 2, 3] (the latter is what currently happens when the above is executed in the playground).

Instead, this happened: The code prints [21, 42, 63]

Context

What I didn’t tell you, my crate also contains this code

fn _unrelated_function() {
    some_dependency::useful_api()
}

other than that, there are no other traits in scope, etc; no potential for any surprises, really there’s nothing else; you’ve seen the whole source code of a crate that prints [21, 42, 63].

More context

Now, the dependency in question offers only a single public thing, a function

pub fn useful_api() {}

Of course it also has some

// private module!
mod dont_look_at_this {
    [DETAILS OMITTED]
}

but that clearly isn’t used in the useful_api implementation, so it’s probably some tests or whatever, entirely unrelated, and I don’t have to worry about, correct? Well apparently not correct.

Here’s the whole source code of some_dependency (click to expand).

pub fn useful_api() {}

// private module!
mod dont_look_at_this {
    mod really_nobody_should_care_about_this {
        pub struct Pwned;
        use std::{cell, fmt};
        impl fmt::Debug for Pwned {
            fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
                formatter.write_str("[21, 42, 63]")
            }
        }
        impl AsRef<Pwned> for cell::Ref<'_, Box<[u8]>> {
            fn as_ref(&self) -> &Pwned {
                &Pwned
            }
        }
    }
}

Full reproduction guide:

git clone https://github.com/steffahn/iffy_methods.git
cd iffy_methods
cargo run

---

Is this a known issue? Is this something that needs to be fixed? @rustbot label T-lang, T-compiler, A-traits, A-security

[steffahn]

It just occurred to me how it’s even possible to make

use std::cell::RefCell;

fn foo(x: &RefCell<Box<[u8]>>) {
    let y = x.borrow();
    let z: &[u8] = y.as_ref();

    println!("{:?}", z);
}

fn main() {
    foo(&RefCell::new(Box::new([1, 2, 3_u8])));
}

misbehave. (Note the explicit type signature z: &[u8]!)

See this branch for how it’s done.

[eddyb]

Is this a known issue? Is this something that needs to be fixed?

This doesn't look like method lookup to me (EDIT: okay, I guess autoderef would let .as_ref() do something else if it didn't hit the spooky impl, and autoderef is parth of method lookup).

It's long been known that the "only one remaining applicable impl" design of trait impl lookup could lead to subtle issues. Most of the focus over the years has been around the ability to introduce ambiguities and therefore inference failures, with a new impl of an existing trait.

What's worse here IMO is the design of the AsRef trait, which I've only had a vague displeasement with over the years, but this doesn't make me like it any better.

When you do x.as_ref(): &[T], you're not calling AsRef::<&[T]>::as_ref(&x), but rather AsRef::as_ref(&x): &[T], i.e. you're asking to coerce the result of x.as_ref(), not hint its type. It's only if inference has absolutely no idea what the type is, that you are eventually hinting the type, through the noop coercion.

A "correct" design would require writing something like x.as_ref::<&[T]>() to call it (since we can't have something as nice as x.as_ref(&[T])), and the method you need to implement wouldn't be called as_ref (or at least wouldn't take method-call self).

(EDIT: something I forgot to include here is that T: AsRef<U> doesn't have a blanket impl for T == U the same way From/Into do, which both decreases ergonomics for certain usecases, but also allows inference to succeed when only the source type is known, but the target type isn't - sadly we can't fix this now)

I know this kind of thing is not that helpful but if I were to make a suggestion (for e.g. lints) is that AsRef should only used in generic functions, i.e. x.as_ref() should only be called when x's type is a type parameter with a single AsRef bound.

(Something similar applies to .into(), I suppose - I tend to prefer e.g. u32::from(x) instead of x.into())

Oh and currently, _: Trait<Bar> doesn't solve even with a single impl, but Foo<_>: Trait<Bar> does, which is IMO part of why this kind of behavior is not as obvious to users as it could be - I suspect Chalk may make the problem worse, without intentionally special-casing "Self param is an inference variable" on top of it.

[steffahn]

I guess the main thing that bugs me here is that when calling a standard library trait method (as_ref from the AsRef trait; no other as_ref methods of other traits in scope) on a standard library type (&RefCell<Box<[u8]>>) type inference can decide to fill in the details to call into a third-party implementation, as long as it’s somewhere in one of your (transitive) dependencies.

I would probably prefer if it’d be made necessary to somehow mention the Pwned type explicitly. For a minimal improvement perhaps a warning that triggers whenever type inference fills in the type parameter of a trait impl that allowed that impl to exist in the first place w.r.t. orphan rules. I.e. the AsRef<Pwned> for cell::Ref<'_, Box<[u8]>> is only allowed because Pwned is local to some_dependency; but IMO the caller of this method also should only be getting this implementation by somehow explicitly requiring that a Pwned value is really what they want. (Passing the result to a function expecting &Pwned, or assigning to a &Pwned variable should be enough though...)

The other thing that bothers me is that method inference auto-derefs Ref<'_, T> to T when looking up as_ref in the first place: Even when Ref<'_, T> doesn’t implement AsRef<Foo> for any type Foo, downstream crates can add such an impl, so why is the compiler allowed to determine that there is no as_ref method?

Both things listed above can have the unfortunate outcome that code changes behavior depending on what dependencies a crate has. If it merely would be possible that code stops compiling successfully due to an additional dependency with additional impls, that would be fine IMO, it’s the behavior change that bothers me.

---

@eddyb Note that this issue is not really about as_ref in particular but instead about the interaction of orphan rules + type inference. I only picked as_ref because it’s a possible example of the pattern using standard library types and traits.

I’m aware of possible inference failures that can be introduced by new trait implementations, this issue is mainly about behavior change i.e. successfully compiling code turns into successfully compiling code with different behavior.

---

I do agree with the assertion that as_ref (and similarly into, etc) would be better with a type parameter on the method, so much so that in-fact I’ve even created a proof-of-concept crate that adds a version of .into that can accept a type argument. (It even has an unpolished/unpublished version of the trait for AsRef.)

[eddyb]

I guess I largely agree with you on most of those points.

I do not have a lot of confidence in us being able to restrict behavior here, but it would be interesting to do a crater run where we compare the crates mentioned in TypeckResults (via e.g. TyKind::Adt) vs the crates mentioned in the types of explicit paths in the body (but the naive approach might not complain if foo happened to call some_dependency::useful_api() - so a non-trivial notion of "type reachability" might be needed ugh).

It's definitely weird to have a crate "show up out of nowhere", but I don't have any good ideas for a predicate during trait solving (or even coherence), because of the "global" nature of trait solving - the best I can come up with is detecting the surprising situation after the fact and emitting a lint warning.

[arnauorriols]

Here's another case I believe related to this issue, or at least another case derived from the "only one remaining applicable impl" desig: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7c1a2e56a4ccb316bd7480494c44225c

[Dylan-DPC]

This code now returns what the user expects

[WaffleLapkin]

@Dylan-DPC I don't think anything changed here. Are you maybe missing the "malicious" code?

; cargo +nightly run
   Compiling some_dependency v0.1.0 (/home/waffle/projects/repos/iffy_methods/some_dependency)
   Compiling main_crate v0.1.0 (/home/waffle/projects/repos/iffy_methods/main_crate)
    Finished dev [unoptimized + debuginfo] target(s) in 1.84s
     Running `target/debug/main_crate`
[21, 42, 63]
; cargo +nightly --version
cargo 1.71.0-nightly (ac8401032 2023-05-02)

[Dylan-DPC]

Ah nvm, my bad missed something in the setup :P