Winnow private method candidates instead of assuming any candidate of the right name will apply #125622

oli-obk · 2024-05-27T15:52:58Z

partially reverts #60721

My original motivation was just to avoid the delay_span_bug (by attempting to thread the ErrorGuaranteed through to here). But then I realized that the error message is wrong. It refers to the Foo<A>::foo instead of Foo::foo. This is almost invisible, because both functions are the same, but on different lines, so -Zui-testing makes it so the test is the same no matter which of these two functions is referenced.

But there's a much more obvious bug: If Foo does not have a foo method at all, but Foo<A> has a private foo method, then we'll refer to that one. This has now been fixed, and we report a normal method not found error.

The way this is done is by creating a list of all possible private functions (just like we create a list of the public functions that can actually be called), and then winnowing it by analyzing where bounds and Self types to see if any of the found methods can actually apply (again, just like with the list of public functions).

I wonder if there is room for doing the same thing with unstable functions instead of running all of method resolution twice.

r? @compiler-errors for method resolution stuff

compiler-errors · 2024-05-27T15:55:55Z

Can you please share what the motivation here is? Don't see a linked issue and there's no inline discussion, and there's also no test -- hard to understand how all these changes fit together without that.

@rustbot author

compiler-errors · 2024-05-27T15:57:25Z

compiler/rustc_hir_typeck/src/method/probe.rs

+    /// List of potential private candidates. Will be trimmed to ones that
+    /// actually apply and then the result inserted into `private_candidate`
+    private_candidates: Vec<Candidate<'tcx>>,
+
    /// Some(candidate) if there is a private candidate
-    private_candidate: Option<(DefKind, DefId)>,
+    private_candidate: Cell<Option<(DefKind, DefId)>>,


What is the difference between these lol

Or actually, better question: Why do we need two fields here?

We need the private_candidate field to communicate an actually found candidate out to the pick method. While we could just eagerly return the PrivateMatch error, that would mean we'd miss out on suggesting traits that could be imported to get access to another method of the same name.

Of course we could just eagerly report (not return) an error from method selection and return that no methods were found, which will subsequently return an error containing the list of traits that should be imported. But then method selection is reporting errors, which is also not be desirable (and probably wrong in various cases where we'll return a better error later).

oli-obk · 2024-05-27T20:23:51Z

there's also no test

🤦 there is a test, I've just been looking at it all day working on this, and the end result was "no change", which I wanted.

Though I just remembered how to write a test that has user visible changes. Will update docs and tests tomorrow

compiler-errors · 2024-05-27T20:27:46Z

@oli-obk: I don't need a test per se, but eitehr a test or motivation for why you want this change otherwise would be useful. I just don't know how to review this change w/o any information, since it doesn't seem like a cleanup by itself.

compiler-errors · 2024-06-04T00:22:44Z

@bors r+

bors · 2024-06-04T00:22:47Z

📌 Commit 575c86f has been approved by compiler-errors

It is now in the queue for this repository.

…ompiler-errors Winnow private method candidates instead of assuming any candidate of the right name will apply partially reverts rust-lang#60721 My original motivation was just to avoid the `delay_span_bug` (by attempting to thread the `ErrorGuaranteed` through to here). But then I realized that the error message is wrong. It refers to the `Foo<A>::foo` instead of `Foo::foo`. This is almost invisible, because both functions are the same, but on different lines, so `-Zui-testing` makes it so the test is the same no matter which of these two functions is referenced. But there's a much more obvious bug: If `Foo` does not have a `foo` method at all, but `Foo<A>` has a private `foo` method, then we'll refer to that one. This has now been fixed, and we report a normal `method not found` error. The way this is done is by creating a list of all possible private functions (just like we create a list of the public functions that can actually be called), and then winnowing it by analyzing where bounds and `Self` types to see if any of the found methods can actually apply (again, just like with the list of public functions). I wonder if there is room for doing the same thing with unstable functions instead of running all of method resolution twice. r? `@compiler-errors` for method resolution stuff

compiler-errors · 2024-06-04T14:24:44Z

@bors r- #125975 (comment)

double check tidy perhaps?

…ndidate

… candidates instead of assuming any candidate of the right name will apply.

oli-obk · 2024-06-04T15:34:33Z

rebased and adjusted tidy limits

@bors r=compiler-errors

bors · 2024-06-04T15:34:35Z

📌 Commit ffb1b2c has been approved by compiler-errors

It is now in the queue for this repository.

…ompiler-errors Winnow private method candidates instead of assuming any candidate of the right name will apply partially reverts rust-lang#60721 My original motivation was just to avoid the `delay_span_bug` (by attempting to thread the `ErrorGuaranteed` through to here). But then I realized that the error message is wrong. It refers to the `Foo<A>::foo` instead of `Foo::foo`. This is almost invisible, because both functions are the same, but on different lines, so `-Zui-testing` makes it so the test is the same no matter which of these two functions is referenced. But there's a much more obvious bug: If `Foo` does not have a `foo` method at all, but `Foo<A>` has a private `foo` method, then we'll refer to that one. This has now been fixed, and we report a normal `method not found` error. The way this is done is by creating a list of all possible private functions (just like we create a list of the public functions that can actually be called), and then winnowing it by analyzing where bounds and `Self` types to see if any of the found methods can actually apply (again, just like with the list of public functions). I wonder if there is room for doing the same thing with unstable functions instead of running all of method resolution twice. r? `@compiler-errors` for method resolution stuff

Rollup of 7 pull requests Successful merges: - rust-lang#122192 (Do not try to reveal hidden types when trying to prove Freeze in the defining scope) - rust-lang#124840 (resolve: mark it undetermined if single import is not has any bindings) - rust-lang#125622 (Winnow private method candidates instead of assuming any candidate of the right name will apply) - rust-lang#125871 (Orphanck[old solver]: Consider opaque types to never cover type parameters) - rust-lang#125893 (Handle all GVN binops in a single place.) - rust-lang#125911 (delete bootstrap build before switching to bumped rustc) - rust-lang#125918 (Revert: create const block bodies in typeck via query feeding) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#124840 (resolve: mark it undetermined if single import is not has any bindings) - rust-lang#125622 (Winnow private method candidates instead of assuming any candidate of the right name will apply) - rust-lang#125648 (Remove unused(?) `~/rustsrc` folder from docker script) - rust-lang#125672 (Add more ABI test cases to miri (RFC 3391)) - rust-lang#125800 (Fix `mut` static task queue in SGX target) - rust-lang#125871 (Orphanck[old solver]: Consider opaque types to never cover type parameters) - rust-lang#125893 (Handle all GVN binops in a single place.) - rust-lang#126008 (Port `tests/run-make-fulldeps/issue-19371` to ui-fulldeps) - rust-lang#126032 (Update description of the `IsTerminal` example) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#125622 - oli-obk:define_opaque_types15, r=compiler-errors Winnow private method candidates instead of assuming any candidate of the right name will apply partially reverts rust-lang#60721 My original motivation was just to avoid the `delay_span_bug` (by attempting to thread the `ErrorGuaranteed` through to here). But then I realized that the error message is wrong. It refers to the `Foo<A>::foo` instead of `Foo::foo`. This is almost invisible, because both functions are the same, but on different lines, so `-Zui-testing` makes it so the test is the same no matter which of these two functions is referenced. But there's a much more obvious bug: If `Foo` does not have a `foo` method at all, but `Foo<A>` has a private `foo` method, then we'll refer to that one. This has now been fixed, and we report a normal `method not found` error. The way this is done is by creating a list of all possible private functions (just like we create a list of the public functions that can actually be called), and then winnowing it by analyzing where bounds and `Self` types to see if any of the found methods can actually apply (again, just like with the list of public functions). I wonder if there is room for doing the same thing with unstable functions instead of running all of method resolution twice. r? ``@compiler-errors`` for method resolution stuff

fmease · 2024-06-06T01:39:02Z

bors sleepy @bors r-

rustbot assigned compiler-errors May 27, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 27, 2024

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2024

compiler-errors reviewed May 27, 2024

View reviewed changes

oli-obk force-pushed the define_opaque_types15 branch from 81ca8e3 to 08914ca Compare May 28, 2024 09:36

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 28, 2024

oli-obk force-pushed the define_opaque_types15 branch from 08914ca to 575c86f Compare May 28, 2024 10:18

oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2024

compiler-errors mentioned this pull request Jun 4, 2024

Rollup of 9 pull requests #125975

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 4, 2024

oli-obk added 6 commits June 4, 2024 15:28

Manual rustfmt

5e8df95

Show that it will pick up the entirely wrong function as a private ca…

14f9c63

…ndidate

Turn a delayed bug back into a normal bug by winnowing private method…

7d151fa

… candidates instead of assuming any candidate of the right name will apply.

Move tests to a more appropriate directory

7894a11

Give test a more useful name

8189506

Add test description

ffb1b2c

oli-obk force-pushed the define_opaque_types15 branch from 575c86f to ffb1b2c Compare June 4, 2024 15:34

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2024

fmease mentioned this pull request Jun 4, 2024

Rollup of 7 pull requests #126001

Closed

matthiaskrgr mentioned this pull request Jun 5, 2024

Rollup of 9 pull requests #126038

Merged

bors merged commit 9abf8b1 into rust-lang:master Jun 5, 2024
6 checks passed

rustbot added this to the 1.80.0 milestone Jun 5, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 6, 2024

oli-obk deleted the define_opaque_types15 branch June 6, 2024 02:53

matthiaskrgr mentioned this pull request Jun 6, 2024

ICE: instantiate_value_path: (UFCS) Fail<T/#0> was a subtype of Fail<i32> but now is not? #126062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Winnow private method candidates instead of assuming any candidate of the right name will apply #125622

Winnow private method candidates instead of assuming any candidate of the right name will apply #125622

oli-obk commented May 27, 2024 •

edited

Loading

compiler-errors commented May 27, 2024

compiler-errors May 27, 2024

compiler-errors May 27, 2024

oli-obk May 28, 2024 •

edited

Loading

oli-obk commented May 27, 2024

compiler-errors commented May 27, 2024

compiler-errors commented Jun 4, 2024

bors commented Jun 4, 2024

compiler-errors commented Jun 4, 2024

oli-obk commented Jun 4, 2024

bors commented Jun 4, 2024

fmease commented Jun 6, 2024

Winnow private method candidates instead of assuming any candidate of the right name will apply #125622

Winnow private method candidates instead of assuming any candidate of the right name will apply #125622

Conversation

oli-obk commented May 27, 2024 • edited Loading

compiler-errors commented May 27, 2024

compiler-errors May 27, 2024

Choose a reason for hiding this comment

compiler-errors May 27, 2024

Choose a reason for hiding this comment

oli-obk May 28, 2024 • edited Loading

Choose a reason for hiding this comment

oli-obk commented May 27, 2024

compiler-errors commented May 27, 2024

compiler-errors commented Jun 4, 2024

bors commented Jun 4, 2024

compiler-errors commented Jun 4, 2024

oli-obk commented Jun 4, 2024

bors commented Jun 4, 2024

fmease commented Jun 6, 2024

oli-obk commented May 27, 2024 •

edited

Loading

oli-obk May 28, 2024 •

edited

Loading