Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty::ImplContainer => {
if segments.len() == 1 {
// `<T>::assoc` will end up here, and so
// can `T::assoc`. It this came from an
// can `T::assoc`. If this came from an
// inherent impl, we need to record the
// `T` for posterity (see `UserSelfTy` for
// details).
Expand Down Expand Up @@ -1416,11 +1416,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
) {
Ok(ok) => self.register_infer_ok_obligations(ok),
Err(_) => {
self.dcx().span_delayed_bug(
self.dcx().span_bug(
span,
format!(
"instantiate_value_path: (UFCS) {self_ty:?} was a subtype of {impl_ty:?} but now is not?",
),
"instantiate_value_path: (UFCS) {self_ty:?} was a subtype of {impl_ty:?} but now is not?",
),
);
}
}
Expand Down
28 changes: 21 additions & 7 deletions compiler/rustc_hir_typeck/src/method/probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ use rustc_trait_selection::traits::query::method_autoderef::{
use rustc_trait_selection::traits::query::CanonicalTyGoal;
use rustc_trait_selection::traits::ObligationCtxt;
use rustc_trait_selection::traits::{self, ObligationCause};
use std::cell::Cell;
use std::cell::RefCell;
use std::cmp::max;
use std::iter;
Expand Down Expand Up @@ -76,8 +77,12 @@ pub(crate) struct ProbeContext<'a, 'tcx> {
/// requested name (by edit distance)
allow_similar_names: bool,

/// 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)>>,
Comment on lines +80 to +85
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between these lol

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@oli-obk oli-obk May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


/// Collects near misses when the candidate functions are missing a `self` keyword and is only
/// used for error reporting
Expand Down Expand Up @@ -581,7 +586,8 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
orig_steps_var_values,
steps,
allow_similar_names: false,
private_candidate: None,
private_candidates: Vec::new(),
private_candidate: Cell::new(None),
static_candidates: RefCell::new(Vec::new()),
unsatisfied_predicates: RefCell::new(Vec::new()),
scope_expr_id,
Expand All @@ -593,7 +599,8 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
self.inherent_candidates.clear();
self.extension_candidates.clear();
self.impl_dups.clear();
self.private_candidate = None;
self.private_candidates.clear();
self.private_candidate.set(None);
self.static_candidates.borrow_mut().clear();
self.unsatisfied_predicates.borrow_mut().clear();
}
Expand All @@ -617,9 +624,8 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
} else {
self.extension_candidates.push(candidate);
}
} else if self.private_candidate.is_none() {
self.private_candidate =
Some((candidate.item.kind.as_def_kind(), candidate.item.def_id));
} else {
self.private_candidates.push(candidate);
}
}

Expand Down Expand Up @@ -1171,7 +1177,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
let mut possibly_unsatisfied_predicates = Vec::new();

for (kind, candidates) in
&[("inherent", &self.inherent_candidates), ("extension", &self.extension_candidates)]
[("inherent", &self.inherent_candidates), ("extension", &self.extension_candidates)]
{
debug!("searching {} candidates", kind);
let res = self.consider_candidates(
Expand All @@ -1185,6 +1191,14 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
}
}

if self.private_candidate.get().is_none() {
if let Some(Ok(pick)) =
self.consider_candidates(self_ty, &self.private_candidates, &mut vec![], None)
{
self.private_candidate.set(Some((pick.item.kind.as_def_kind(), pick.item.def_id)));
}
}

// `pick_method` may be called twice for the same self_ty if no stable methods
// match. Only extend once.
if unstable_candidates.is_some() {
Expand Down
1 change: 0 additions & 1 deletion src/tools/tidy/src/issues.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2445,7 +2445,6 @@ ui/issues/issue-53300.rs
ui/issues/issue-53333.rs
ui/issues/issue-53348.rs
ui/issues/issue-53419.rs
ui/issues/issue-53498.rs
ui/issues/issue-53568.rs
ui/issues/issue-5358-1.rs
ui/issues/issue-53728.rs
Expand Down
2 changes: 1 addition & 1 deletion src/tools/tidy/src/ui_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::path::{Path, PathBuf};
const ENTRY_LIMIT: u32 = 900;
// FIXME: The following limits should be reduced eventually.

const ISSUES_ENTRY_LIMIT: u32 = 1674;
const ISSUES_ENTRY_LIMIT: u32 = 1672;

const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
"rs", // test source files
Expand Down
17 changes: 0 additions & 17 deletions tests/ui/issues/issue-53498.rs

This file was deleted.

15 changes: 15 additions & 0 deletions tests/ui/privacy/ufc-method-call.different_name.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0599]: no function or associated item named `foo` found for struct `Foo<B>` in the current scope
--> $DIR/ufc-method-call.rs:27:27
|
LL | pub struct Foo<T>(T);
| ----------------- function or associated item `foo` not found for this struct
...
LL | test::Foo::<test::B>::foo();
| ^^^ function or associated item not found in `Foo<B>`
|
= note: the function or associated item was found for
- `Foo<A>`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0599`.
30 changes: 30 additions & 0 deletions tests/ui/privacy/ufc-method-call.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
//! This test used to report that the method call cannot
//! call the private method `Foo<A>::foo`, even though the user
//! explicitly selected `Foo<B>::foo`. This is because we only
//! looked for methods of the right name, without properly checking
//! the `Self` type

//@ revisions: same_name different_name

pub mod test {
pub struct A;
pub struct B;
pub struct Foo<T>(T);

impl Foo<A> {
fn foo() {}
}

impl Foo<B> {
#[cfg(same_name)]
fn foo() {}
#[cfg(different_name)]
fn bar() {}
}
}

fn main() {
test::Foo::<test::B>::foo();
//[same_name]~^ ERROR associated function `foo` is private
//[different_name]~^^ ERROR no function or associated item named `foo` found for struct `Foo<B>`
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0624]: associated function `foo` is private
--> $DIR/issue-53498.rs:16:27
--> $DIR/ufc-method-call.rs:27:27
|
LL | fn foo() {}
| -------- private associated function defined here
Expand Down
Loading