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

Do not suggest unresolvable builder methods #125397

Merged
merged 1 commit into from
Jun 3, 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
1 change: 1 addition & 0 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Some(rcvr),
rcvr_t,
segment.ident,
expr.hir_id,
SelfSource::MethodCall(rcvr),
error,
Some(args),
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
None,
ty.normalized,
item_name,
hir_id,
SelfSource::QPath(qself),
error,
args,
Expand Down
26 changes: 22 additions & 4 deletions compiler/rustc_hir_typeck/src/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
rcvr_opt: Option<&'tcx hir::Expr<'tcx>>,
rcvr_ty: Ty<'tcx>,
item_name: Ident,
expr_id: hir::HirId,
Copy link
Member

Choose a reason for hiding this comment

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

Now that we're just passing in the expr id, I feel like rcvr_opt: Option<_>, SelfSource: _, args: Option<_> are all redundant, since they can all be derived from the expression pointed to by the expr_id.

Copy link
Member

Choose a reason for hiding this comment

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

And the span too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into it. Not much is derivable from expr_id.

These are the two calls to report_method_error:

if let Some(e) = self.report_method_error(
span,
None,
ty.normalized,
item_name,
SelfSource::QPath(qself),
error,
args,
Expectation::NoExpectation,
trait_missing_method && span.edition().at_least_rust_2021(), // emits missing method for trait only after edition 2021
) {
and
if let Some(err) = self.report_method_error(
span,
Some(rcvr),
rcvr_t,
segment.ident,
SelfSource::MethodCall(rcvr),
error,
Some(args),
expected,
false,
) {

As you can see rcvr_opt (second arg) is being explicitly passed as None in one case and Some(rcvr) in the other. Similarly rcvr_ty (third arg) is ty.normalized in one case and rcvr_ty in the other and both are obtained in different ways. source (fifth arg) too has different values with somewhat convoluted provenance and the same can be said of args (seventh arg).

So I don't feel we should change any of this.

Copy link
Member

Choose a reason for hiding this comment

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

SelfSource, rcvr_opt, and args are all a property of whether the expr_id (which really should be passed in as a whole hir::Expr) is ExprKind::MethodCall or ExprKind::Path.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, segment.ident should be derivable from that expression (it's directly stored in the MethodCall and is also as the final segment in ExprKind::Path, or something like that).

I don't think I said to touch rcvr_ty, which can continue to be passed in.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, span can be derived from the expression without much trouble as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, let me have another look at it

Copy link
Contributor Author

@gurry gurry May 24, 2024

Choose a reason for hiding this comment

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

SelfSource, rcvr_opt, and args are all a property of whether the expr_id (which really should be passed in as a whole hir::Expr) is ExprKind::MethodCall or ExprKind::Path.

@compiler-errors Turns out the expr_id isn't necessarily pointing to an Expr always. it could also be a Pat because check_pat also calls report_method_error (check_pat->resolve_ty_and_res_fully_qualified_call->report_method_error) . So if I were to derive all these bits from expr_id I'll have to take Pat into account in addition to Expr, which means I might have to repeat code like this from resolve_ty_and_res_fully_qualified_call:

let (ty, qself, item_segment) = match *qpath {
QPath::Resolved(ref opt_qself, path) => {
return (
path.res,
opt_qself.as_ref().map(|qself| self.lower_ty(qself)),
path.segments,
);
}
QPath::TypeRelative(ref qself, ref segment) => {
// Don't use `self.lower_ty`, since this will register a WF obligation.
// If we're trying to call a nonexistent method on a trait
// (e.g. `MyTrait::missing_method`), then resolution will
// give us a `QPath::TypeRelative` with a trait object as
// `qself`. In that case, we want to avoid registering a WF obligation
// for `dyn MyTrait`, since we don't actually need the trait
// to be object-safe.
// We manually call `register_wf_obligation` in the success path
// below.
let ty = self.lowerer().lower_ty_in_path(qself);
(LoweredTy::from_raw(self, span, ty), qself, segment)
}
QPath::LangItem(..) => {
bug!("`resolve_ty_and_res_fully_qualified_call` called on `LangItem`")
}

within report_method_error. I have a feeling that complicates things instead of making them simpler 😫

Copy link
Member

Choose a reason for hiding this comment

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

That didn't end up being too complicated, for the record: 2227c1f

source: SelfSource<'tcx>,
error: MethodError<'tcx>,
args: Option<&'tcx [hir::Expr<'tcx>]>,
Expand All @@ -216,6 +217,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
rcvr_opt,
rcvr_ty,
item_name,
expr_id,
source,
args,
sugg_span,
Expand Down Expand Up @@ -551,6 +553,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
rcvr_opt: Option<&'tcx hir::Expr<'tcx>>,
rcvr_ty: Ty<'tcx>,
item_name: Ident,
expr_id: hir::HirId,
source: SelfSource<'tcx>,
args: Option<&'tcx [hir::Expr<'tcx>]>,
sugg_span: Span,
Expand Down Expand Up @@ -683,7 +686,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

if matches!(source, SelfSource::QPath(_)) && args.is_some() {
self.find_builder_fn(&mut err, rcvr_ty);
self.find_builder_fn(&mut err, rcvr_ty, expr_id);
}

if tcx.ty_is_opaque_future(rcvr_ty) && item_name.name == sym::poll {
Expand Down Expand Up @@ -1944,7 +1947,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

/// Look at all the associated functions without receivers in the type's inherent impls
/// to look for builders that return `Self`, `Option<Self>` or `Result<Self, _>`.
fn find_builder_fn(&self, err: &mut Diag<'_>, rcvr_ty: Ty<'tcx>) {
fn find_builder_fn(&self, err: &mut Diag<'_>, rcvr_ty: Ty<'tcx>, expr_id: hir::HirId) {
let ty::Adt(adt_def, _) = rcvr_ty.kind() else {
return;
};
Expand All @@ -1953,8 +1956,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let mut items = impls
.iter()
.flat_map(|i| self.tcx.associated_items(i).in_definition_order())
// Only assoc fn with no receivers.
.filter(|item| matches!(item.kind, ty::AssocKind::Fn) && !item.fn_has_self_parameter)
// Only assoc fn with no receivers and only if
// they are resolvable
.filter(|item| {
matches!(item.kind, ty::AssocKind::Fn)
&& !item.fn_has_self_parameter
&& self
.probe_for_name(
Mode::Path,
item.ident(self.tcx),
None,
IsSuggestion(true),
rcvr_ty,
expr_id,
ProbeScope::TraitsInScope,
)
.is_ok()
})
.filter_map(|item| {
// Only assoc fns that return `Self`, `Option<Self>` or `Result<Self, _>`.
let ret_ty = self
Expand Down
5 changes: 0 additions & 5 deletions tests/ui/resolve/fn-new-doesnt-exist.rs

This file was deleted.

14 changes: 0 additions & 14 deletions tests/ui/resolve/fn-new-doesnt-exist.stderr

This file was deleted.

64 changes: 64 additions & 0 deletions tests/ui/resolve/suggest-builder-fn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Tests that we suggest the right alternatives when
// a builder method cannot be resolved

use std::net::TcpStream;

trait SomeTrait {}

struct Foo<T> {
v : T
}

impl<T: SomeTrait + Default> Foo<T> {
// Should not be suggested if constraint on the impl not met
fn new() -> Self {
Self { v: T::default() }
}
}

struct Bar;

impl Bar {
// Should be suggested
fn build() -> Self {
Self {}
}

// Method with self can't be a builder.
// Should not be suggested
fn build_with_self(&self) -> Self {
Self {}
}
}

mod SomeMod {
use Bar;

impl Bar {
// Public method. Should be suggested
pub fn build_public() -> Self {
Self {}
}

// Private method. Should not be suggested
fn build_private() -> Self {
Self {}
}
}
}

fn main() {
// `new` not found on `TcpStream` and `connect` should be suggested
let _stream = TcpStream::new();
//~^ ERROR no function or associated item named `new` found

// Although `new` is found on `<impl Foo<T>>` it should not be
// suggested because `u8` does not meet the `T: SomeTrait` constraint
let _foo = Foo::<u8>::new();
//~^ ERROR the function or associated item `new` exists for struct `Foo<u8>`, but its trait bounds were not satisfied

// Should suggest only `<impl Bar>::build()` and `SomeMod::<impl Bar>::build_public()`.
// Other methods should not suggested because they are private or are not a builder
let _bar = Bar::new();
//~^ ERROR no function or associated item named `new` found
}
51 changes: 51 additions & 0 deletions tests/ui/resolve/suggest-builder-fn.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
error[E0599]: no function or associated item named `new` found for struct `TcpStream` in the current scope
--> $DIR/suggest-builder-fn.rs:52:29
|
LL | let _stream = TcpStream::new();
| ^^^ function or associated item not found in `TcpStream`
|
note: if you're trying to build a new `TcpStream` consider using one of the following associated functions:
TcpStream::connect
TcpStream::connect_timeout
--> $SRC_DIR/std/src/net/tcp.rs:LL:COL

error[E0599]: the function or associated item `new` exists for struct `Foo<u8>`, but its trait bounds were not satisfied
--> $DIR/suggest-builder-fn.rs:57:27
|
LL | struct Foo<T> {
| ------------- function or associated item `new` not found for this struct
...
LL | let _foo = Foo::<u8>::new();
| ^^^ function or associated item cannot be called on `Foo<u8>` due to unsatisfied trait bounds
|
note: trait bound `u8: SomeTrait` was not satisfied
--> $DIR/suggest-builder-fn.rs:12:9
|
LL | impl<T: SomeTrait + Default> Foo<T> {
| ^^^^^^^^^ ------
| |
| unsatisfied trait bound introduced here

error[E0599]: no function or associated item named `new` found for struct `Bar` in the current scope
--> $DIR/suggest-builder-fn.rs:62:21
|
LL | struct Bar;
| ---------- function or associated item `new` not found for this struct
...
LL | let _bar = Bar::new();
| ^^^ function or associated item not found in `Bar`
|
note: if you're trying to build a new `Bar` consider using one of the following associated functions:
Bar::build
SomeMod::<impl Bar>::build_public
--> $DIR/suggest-builder-fn.rs:23:5
|
LL | fn build() -> Self {
| ^^^^^^^^^^^^^^^^^^
...
LL | pub fn build_public() -> Self {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0599`.
2 changes: 1 addition & 1 deletion tests/ui/suggestions/deref-path-method.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ note: if you're trying to build a new `Vec<_, _>` consider using one of the foll
Vec::<T>::with_capacity
Vec::<T>::try_with_capacity
Vec::<T>::from_raw_parts
and 6 others
and 4 others
--> $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
help: the function `contains` is implemented on `[_]`
|
Expand Down
1 change: 0 additions & 1 deletion tests/ui/suggestions/issue-109291.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ note: if you're trying to build a new `Backtrace` consider using one of the foll
Backtrace::capture
Backtrace::force_capture
Backtrace::disabled
Backtrace::create
--> $SRC_DIR/std/src/backtrace.rs:LL:COL
help: there is an associated function `force_capture` with a similar name
|
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/ufcs/bad-builder.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ note: if you're trying to build a new `Vec<Q>` consider using one of the followi
Vec::<T>::with_capacity
Vec::<T>::try_with_capacity
Vec::<T>::from_raw_parts
and 6 others
and 4 others
--> $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
help: there is an associated function `new` with a similar name
|
Expand Down
Loading