Method resolution error

RustyYato commented 3 years ago

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

```rust 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(pub T); | ----- supplied 1 argument | note: associated function defined here --> src/lib.rs:6:8 | 6 | fn field(&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(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(self) where Self: Sized { | ^^^^^^^^^^^^^^^^^ help: function arguments must have a statically known size, borrowed types always have a known size | 14 | fn take_field(&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 commented 3 years ago

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 commented 3 years ago

searched nightlies: from nightly-2020-12-25 to nightly-2021-01-20 regressed nightly: nightly-2021-01-18 searched commits: from https://github.com/rust-lang/rust/commit/8a6518427e11e6dd13d6f39663b82eb4f810ca05 to https://github.com/rust-lang/rust/commit/4253153db205251f72ea4493687a31e04a2a8ca0 regressed commit: https://github.com/rust-lang/rust/commit/edeb631ad0cd6fdf31e2e31ec90e105d1768be28

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu Reproduce with: ```bash cargo bisect-rustc --preserve --start=2020-12-25 ```

SNCPlay42 commented 3 years ago

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 commented 3 years ago

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

cc @petrochenkov

petrochenkov commented 3 years ago

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 https://github.com/rust-lang/rust/pull/80765.

If https://github.com/rust-lang/rust/pull/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 commented 3 years ago

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

apiraino commented 3 years ago

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

pnkfelix commented 3 years ago

(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 commented 3 years ago

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 commented 3 years ago

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

```rust // 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(pub T); // On all of v1+v2+v3, stable+beta+nightly have same behavior. pub struct Foo_v1(pub T); pub struct Foo_v2(pub T); pub struct Foo_v3(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 fmt::Debug for Foo_v1 { 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 fmt::Debug for Foo_v2 { 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 fmt::Debug for Foo_v3 { 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 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 commented 3 years ago

@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 commented 3 years ago

:ok_hand: Experiment pr-81211 created and queued. :mag: You can check out the queue and this experiment's details.

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

Mark-Simulacrum commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

:construction: Experiment pr-81211 is now running

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

craterbot commented 3 years ago

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

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

pietroalbini commented 3 years ago

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

@craterbot retry p=2

craterbot commented 3 years ago

:rotating_light: Error: failed to parse the command

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

pietroalbini commented 3 years ago

@craterbot retry @craterbot p=3

craterbot commented 3 years ago

:hammer_and_wrench: Experiment pr-81211 queued again.

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

pietroalbini commented 3 years ago

@craterbot p=2

craterbot commented 3 years ago

:memo: Configuration of the pr-81211 experiment changed.

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

craterbot commented 3 years ago

:construction: Experiment pr-81211 is now running

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

craterbot commented 3 years ago

:tada: Experiment pr-81211 is completed! :bar_chart: 18 regressed and 13 fixed (142244 total) :newspaper: Open the full report.

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

Mark-Simulacrum commented 3 years ago

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 commented 2 years ago

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

pnkfelix commented 1 year ago

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

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 https://github.com/rust-lang/reference/issues/1308 for the latter). But I think the issue described here is resolved and has corresponding tests.

rust-lang / rust

Method resolution error #81211