Method resolution error

[RustyYato]

I tried this code:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=5ce8c7b1f8fdb39762765a7bf1576cfb

#[derive(Debug)]
pub struct Foo<T>(pub T);

impl<T> Access for T {}
pub trait Access {
    fn field<N, T, Name: 'static>(&self) -> &T {
        todo!()
    }

    fn field_mut<N, T, Name: 'static>(&mut self) -> &mut T {
        todo!()
    }

    fn take_field<N, T, Name>(self) {
        todo!()
    }
}

I expected it to compile, but it doesn't because the Debug macro expands to something like foo.field("field_name", &value), which incorrectly resolves to Access::field instead of DebugTuple::field (which is an inherent method).

error message

error[E0061]: this function takes 0 arguments but 1 argument was supplied
 --> src/lib.rs:1:10
  |
1 | #[derive(Debug)]
  |          ^^^^^ expected 0 arguments
2 | pub struct Foo<T>(pub T);
  |                   ----- supplied 1 argument
  |
note: associated function defined here
 --> src/lib.rs:6:8
  |
6 |     fn field<N, T, Name: 'static>(&self) -> &T {
  |        ^^^^^                      -----
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the size for values of type `Self` cannot be known at compilation time
  --> src/lib.rs:14:31
   |
14 |     fn take_field<N, T, Name>(self) {
   |                               ^^^^ doesn't have a size known at compile-time
   |
   = help: unsized fn params are gated as an unstable feature
help: consider further restricting `Self`
   |
14 |     fn take_field<N, T, Name>(self) where Self: Sized {
   |                                     ^^^^^^^^^^^^^^^^^
help: function arguments must have a statically known size, borrowed types always have a known size
   |
14 |     fn take_field<N, T, Name>(&self) {
   |                               ^

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0061, E0277.
For more information about an error, try `rustc --explain E0061`.
error: could not compile `playground`

To learn more, run the command again with --verbose.

[SNCPlay42]

This works on stable and beta (as in, it compiles without error if you add a : Sized supertrait to Access) , but not on nightly.

@rustbot label regression-from-stable-to-nightly

[SNCPlay42]

searched nightlies: from nightly-2020-12-25 to nightly-2021-01-20
regressed nightly: nightly-2021-01-18
searched commits: from 8a65184 to 4253153
regressed commit: edeb631

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --preserve --start=2020-12-25 

[SNCPlay42]

I think part of the problem is that f.field(...) looks for methods on &DebugTuple and sees the <DebugTuple as Access>::field(&self) before it looks for methods on &mut DebugTuple and finds DebugTuple::field(&mut self, ...). This issue with autoref/autoderef interacting poorly with inherent method's precedence over trait methods has cropped up before, e.g. in #39232 (where the compiler finds <T as BorrowMut>::borrow_mut(self) before trying autoref and finding RefCell::borrow_mut(&self)). But I don't have an explanation for why this started being a problem recently.

On another note, I notice that #[derive(Debug)] appears to be the only built-in derive that ever uses . method calls instead of UFCS with fully qualified paths, is there a reason for that? This issue could be fixed by changing #[derive(Debug)] to not use . calls, but I'm concerned that the root cause might have broken something else.

[SNCPlay42]

Looking at the rollup that this regressed in, #80765 seems like it might be the cause.

cc @petrochenkov

[petrochenkov]

Minimized:

#[derive(Debug)]
struct Foo(u8);

pub trait Access {
    fn field(&self) {}
}

impl<T> Access for T {}

derive(Debug) should certainly use UFCS instead of method calls, regardless of #80765.

If #80765 causes other issues we'll find out sooner or later, in theory it shouldn't change any behavior unless macros 2.0 (which are unstable) or built-in macros (which we can fix) are involved.

[petrochenkov]

(I'll leave the fix itself to someone else, unassigning myself.)

[apiraino]

Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[pnkfelix]

(Regarding the assignments here: i am going to make derive(Debug) use UFCS. @Mark-Simulacrum is going to investigate if there is other fallout from PR #80765, I think via a directed crater run.)

[pnkfelix]

I have something put together that seems to fix the problem for the cases listed in this bug, namely field, and presumably finish though I need to double-check that one.

However, I also tried applying my solution to the debug_struct and debug_tuple calls that we make on the Formatter argument, and in those cases, the UFCS code I generated did not compile. I'm still working that out (and also working out whether the injection of an alternative field method illustrated in the description of this bug #81211 even applies to the debug_struct and debug_tuple methods as well).

[pnkfelix]

Okay, so it seems to me like there's some oddity in how we resolve method calls that had left us somewhat robust against injections of new methods on fmt::Formatter and fmt::DebugTuple, etc, all this time, and that #81211 removed some of that accidental robustness, but not all of it.

Here is the result of my experiments in the space (play):

Click here to see the code. Or just go to the playpen link above; there are comments embedded in the code

// run-pass
#![allow(warnings)]
use std::fmt;

// On 1.49.0 stable and 1.50.0 beta, this prints:
//
// ```
// thread 'main' panicked at 'got into field on "v1"', derive-Debug-use-ufcs-tuple-expanded.rs:115:9
// note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
// thread 'main' panicked at 'got into debug_tuple on "Foo_v3"', derive-Debug-use-ufcs-tuple-expanded.rs:111:9
// ```
//
// (I.e. the v0 and v2 assertions pass, and the v1 and v3 Debug::fmt attempts
// are overriden by different methods in the Access trait below.)
//
// On rustc 1.51.0-nightly (22ddcd1a1 2021-01-22), this prints:
//
// ```
// thread 'main' panicked at 'got into field on "v0"', derive-Debug-use-ufcs-tuple-expanded.rs:115:9
// note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
// thread 'main' panicked at 'got into field on "v1"', derive-Debug-use-ufcs-tuple-expanded.rs:115:9
// thread 'main' panicked at 'got into debug_tuple on "Foo_v3"', derive-Debug-use-ufcs-tuple-expanded.rs:111:9
// ```
//
// (I.e. the v2 assertion passes, and the v0, v1, and v3 Debug::fmts are all
// overriden by the Access trait below.)

fn main() {
    let foo0 = Foo_v0("v0");
    let foo1 = Foo_v1("v1");
    let foo2 = Foo_v2("v2");
    let foo3 = Foo_v3("v3");

    std::panic::catch_unwind(|| assert_eq!("Foo_v0(\"v0\")", format!("{:?}", foo0)));
    std::panic::catch_unwind(|| assert_eq!("Foo_v1(\"v1\")", format!("{:?}", foo1)));
    std::panic::catch_unwind(|| assert_eq!("Foo_v2(\"v2\")", format!("{:?}", foo2)));
    std::panic::catch_unwind(|| assert_eq!("Foo_v3(\"v3\")", format!("{:?}", foo3)));
}


// This is where behavior is changing between stable and nightly
#[derive(Debug)]
pub struct Foo_v0<T>(pub T);

// On all of v1+v2+v3, stable+beta+nightly have same behavior.

pub struct Foo_v1<T>(pub T);
pub struct Foo_v2<T>(pub T);
pub struct Foo_v3<T>(pub T);

// This, v1, most closely matches the first part of the current expansion of
// `derive(Debug)` that we see in v0.
//
// The weird thing is: Why do we see different behavior here than for Foo_v0 on
// stable? (On *nightly*, v0 and v1 have the same behavior.)
impl <T: fmt::Debug> fmt::Debug for Foo_v1<T> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Foo_v1(ref __self_0_0) => {
                let mut debug_trait_builder = f.debug_tuple("Foo_v1");
                let _ = debug_trait_builder.field(&&(*__self_0_0));
                debug_trait_builder.finish()
            }
        }
    }
}

// This adds a `&mut` borrow of the DebugTuple returned by the formatter, which
// effectively stops it from being accidentally overriden by Access trait below.
//
// (I.e. it seems to be using the same mechanism that `&mut fmt::Formatter` uses
//  to achieve robustness in the face of such accidental method collisions from
//  traits.)
impl <T: fmt::Debug> fmt::Debug for Foo_v2<T> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Foo_v2(ref __self_0_0) => {
                let mut debug_trait_builder = f.debug_tuple("Foo_v2");
                let _ = (&mut debug_trait_builder).field(&&(*__self_0_0));
                debug_trait_builder.finish()
            }
        }
    }
}

// This adds an explicit deref of the formatter before invoking debug_tuple.
//
// Doing so makes the formatter vulnerable to the accidental method collision:
// It now resolves to Access::debug_tuple, below, on stable+beta+nightly.
//
// This variant is an attempt to explain the source of the robustness that we
// observe when using `&mut`: the presence of `&mut` forces the method resolver
// to use an extra deref when looking up `debug_tuple`, and that, for some
// reason, makes it favor the inherent `&mut self` method over the `&self`
// method in `Access::debug_tuple`.
impl <T: fmt::Debug> fmt::Debug for Foo_v3<T> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Foo_v3(ref __self_0_0) => {
                let mut debug_trait_builder = (*f).debug_tuple("Foo_v3");
                let _ = debug_trait_builder.field(&&(*__self_0_0));
                debug_trait_builder.finish()
            }
        }
    }
}

impl<T> Access for T {}
pub trait Access {
    fn debug_tuple(&self, x: &str) -> fmt::DebugTuple {
        panic!("got into debug_tuple on {:?}", x);
    }

    fn field(&self, x: impl fmt::Debug) {
        panic!("got into field on {:?}", x);
    }
}

As I note in the comments in the code, I'm a little confused about some of the deviations I'm seeing on stable/beta. It certainly seems like nightly is behaving more consistently overall, and has just exposed a latent weakness in the derive(Debug) expansion.

I think the resolution is still generally consistent with the rules given in https://doc.rust-lang.org/reference/expressions/method-call-expr.html. I am not 100% sure of that claim because I would have thought an &self method would still take precedence over an &mut self method if resolve had to go through the same number of derefs to reach either one (and I think we are going through the same number of derefs in both paths here...?), but the fact that adding &mut borrows side-steps the method collision from the trait Access shows that either my thinking is wrong, or there is another bug lying in wait here.

• Further exploration on a separate test led me to realize what is probably happening here: obviously you can get to a &self method when you need to on a value of type &mut Foo: just deref and autoref back. But I think that is interpreted as a longer path than just passing the &mut Foo directly to a &mut self method if one is present, and thus that is probably when a &mut self method can take precedence over a &self method in a parallel trait. Still: Tricky stuff.

[Mark-Simulacrum]

@craterbot run start=master#7d3818152d8ab5649d2e5cc6d7851ed7c03055fe end=master#edeb631ad0cd6fdf31e2e31ec90e105d1768be28 mode=check-only

This should hopefully give us some sense of the breakage caused by the rollup here, hopefully the majority of which is down to #80765.

[craterbot]

👌 Experiment pr-81211 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Mark-Simulacrum]

@pnkfelix -- one thing that this makes me think would be an excellent outcome is to eventually write up, perhaps on that reference page, an explicit hierarchy of what is checked/reached first, at least for 2-3 autoref/deref steps or so.

[pnkfelix]

@pnkfelix -- one thing that this makes me think would be an excellent outcome is to eventually write up, perhaps on that reference page, an explicit hierarchy of what is checked/reached first, at least for 2-3 autoref/deref steps or so.

Hey @Mark-Simulacrum, do you mean like the test that I wrote in PR #81294? Is that an example of the kind of thing you think should be added, in some fashion, to the documentation?

[Mark-Simulacrum]

Yeah, perhaps as something like https://en.wikipedia.org/wiki/Partially_ordered_set#/media/File:Hasse_diagram_of_powerset_of_3.svg but I think not a Hasse diagram - basically, showing which is considered first and so forth. I guess we'd want multiple such graphs for different types of calls (e.g., if you call via method whether original type is &T, &mut T, T, or impl Deref<target= T>, &impl Deref<target= T>, &mut impl Deref<target= T> etc)

[craterbot]

🚧 Experiment pr-81211 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚨 Experiment pr-81211 has encountered an error: some threads returned an error
🛠️ If the error is fixed use the retry command.

🆘 Can someone from the infra team check in on this? @rust-lang/infra
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[pietroalbini]

Sorry about that. A new crater deployment broke a thing. Should be fixed now.

@craterbot retry p=2

[craterbot]

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[pietroalbini]

@craterbot retry
@craterbot p=3

[craterbot]

🛠️ Experiment pr-81211 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[pietroalbini]

@craterbot p=2

[craterbot]

📝 Configuration of the pr-81211 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-81211 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-81211 is completed!
📊 18 regressed and 13 fixed (142244 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Mark-Simulacrum]

No one ended up triaging this crater run, but I think the only possible additional breakage is in hlist - https://crater-reports.s3.amazonaws.com/pr-81211/master%23edeb631ad0cd6fdf31e2e31ec90e105d1768be28/reg/typsy-0.1.0/log.txt - and I'd guess the fix would be similar there. Ultimately this is 'just' a form of inference breakage in some sense, so I think it's OK to let this slip. It seems to affect only 2 crates in the crater run, so breakage is very minor.

[jackh726]

This is fixed on current nightly. Removing priority and marking as E-needs-test.

[pnkfelix]

I actually added tests, I think, in #81294. Namely:

• https://github.com/rust-lang/rust/blob/master/src/test/ui/derives/derive-Debug-use-ufcs-struct.rs
• https://github.com/rust-lang/rust/blob/master/src/test/ui/derives/derive-Debug-use-ufcs-tuple.rs

From reviewing the comment thread here, I know there were lots of various concerns noted, and ideas for enhancements to the language documentation (I've filed rust-lang/reference#1308 for the latter). But I think the issue described here is resolved and has corresponding tests.