Skip to content

Detect likely . -> .. typo in method calls #105765

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

Merged
merged 3 commits into from
Dec 28, 2022
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
66 changes: 66 additions & 0 deletions compiler/rustc_hir_typeck/src/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.note_type_is_not_clone(err, expected, expr_ty, expr);
self.note_need_for_fn_pointer(err, expected, expr_ty);
self.note_internal_mutation_in_method(err, expr, expected, expr_ty);
self.check_for_range_as_method_call(err, expr, expr_ty, expected);
}

/// Requires that the two types unify, and prints an error message if
Expand Down Expand Up @@ -1448,4 +1449,69 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
_ => false,
}
}

/// Identify when the user has written `foo..bar()` instead of `foo.bar()`.
pub fn check_for_range_as_method_call(
&self,
err: &mut Diagnostic,
expr: &hir::Expr<'_>,
checked_ty: Ty<'tcx>,
expected_ty: Ty<'tcx>,
) {
if !hir::is_range_literal(expr) {
return;
}
let hir::ExprKind::Struct(
hir::QPath::LangItem(LangItem::Range, ..),
[start, end],
_,
) = expr.kind else { return; };
let parent = self.tcx.hir().get_parent_node(expr.hir_id);
if let Some(hir::Node::ExprField(_)) = self.tcx.hir().find(parent) {
// Ignore `Foo { field: a..Default::default() }`
return;
}
let mut expr = end.expr;
while let hir::ExprKind::MethodCall(_, rcvr, ..) = expr.kind {
// Getting to the root receiver and asserting it is a fn call let's us ignore cases in
// `src/test/ui/methods/issues/issue-90315.stderr`.
expr = rcvr;
}
let hir::ExprKind::Call(method_name, _) = expr.kind else { return; };
let ty::Adt(adt, _) = checked_ty.kind() else { return; };
if self.tcx.lang_items().range_struct() != Some(adt.did()) {
return;
}
if let ty::Adt(adt, _) = expected_ty.kind()
&& self.tcx.lang_items().range_struct() == Some(adt.did())
{
return;
}
// Check if start has method named end.
let hir::ExprKind::Path(hir::QPath::Resolved(None, p)) = method_name.kind else { return; };
let [hir::PathSegment { ident, .. }] = p.segments else { return; };
let self_ty = self.typeck_results.borrow().expr_ty(start.expr);
let Ok(_pick) = self.probe_for_name(
probe::Mode::MethodCall,
*ident,
probe::IsSuggestion(true),
self_ty,
expr.hir_id,
probe::ProbeScope::AllTraits,
) else { return; };
let mut sugg = ".";
let mut span = start.expr.span.between(end.expr.span);
if span.lo() + BytePos(2) == span.hi() {
// There's no space between the start, the range op and the end, suggest removal which
// will be more noticeable than the replacement of `..` with `.`.
span = span.with_lo(span.lo() + BytePos(1));
sugg = "";
}
err.span_suggestion_verbose(
span,
"you likely meant to write a method call instead of a range",
sugg,
Applicability::MachineApplicable,
);
}
}
40 changes: 33 additions & 7 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use rustc_ast::ptr::P;
use rustc_ast::visit::{self, AssocCtxt, BoundKind, FnCtxt, FnKind, Visitor};
use rustc_ast::*;
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap};
use rustc_errors::{DiagnosticArgValue, DiagnosticId, IntoDiagnosticArg};
use rustc_errors::{Applicability, DiagnosticArgValue, DiagnosticId, IntoDiagnosticArg};
use rustc_hir::def::Namespace::{self, *};
use rustc_hir::def::{self, CtorKind, DefKind, LifetimeRes, PartialRes, PerNS};
use rustc_hir::def_id::{DefId, LocalDefId, CRATE_DEF_ID, LOCAL_CRATE};
Expand Down Expand Up @@ -536,6 +536,9 @@ struct DiagnosticMetadata<'ast> {
in_assignment: Option<&'ast Expr>,
is_assign_rhs: bool,

/// Used to detect possible `.` -> `..` typo when calling methods.
in_range: Option<(&'ast Expr, &'ast Expr)>,

/// If we are currently in a trait object definition. Used to point at the bounds when
/// encountering a struct or enum.
current_trait_object: Option<&'ast [ast::GenericBound]>,
Expand Down Expand Up @@ -3320,17 +3323,14 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
);
}

#[instrument(level = "debug", skip(self))]
fn smart_resolve_path_fragment(
&mut self,
qself: &Option<P<QSelf>>,
path: &[Segment],
source: PathSource<'ast>,
finalize: Finalize,
) -> PartialRes {
debug!(
"smart_resolve_path_fragment(qself={:?}, path={:?}, finalize={:?})",
qself, path, finalize,
);
let ns = source.namespace();

let Finalize { node_id, path_span, .. } = finalize;
Expand All @@ -3341,8 +3341,28 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {

let def_id = this.parent_scope.module.nearest_parent_mod();
let instead = res.is_some();
let suggestion =
if res.is_none() { this.report_missing_type_error(path) } else { None };
let suggestion = if let Some((start, end)) = this.diagnostic_metadata.in_range
&& path[0].ident.span.lo() == end.span.lo()
Copy link
Member

@compiler-errors compiler-errors Dec 16, 2022

Choose a reason for hiding this comment

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

I didn't look too much into this, but does this check that this is a method?

We should also probably double check this is a path with one segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, but during resolve we don't have enough data to do that (although now that I'm tracking start I might be able to cobble something together).

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'm comfortable with the weasel wording on this one suggestion: it is a guess, and we'd have to perform more extensive surgery on resolve to track more state, and it is already quite a beast.

Copy link
Member

Choose a reason for hiding this comment

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

My only concern is that this triggers when we have any resolution error on the RHS of a range, e.g.

fn foo() {
  a..b;
}

Results in:

error[E0425]: cannot find value `a` in this scope
 --> /home/ubuntu/test.rs:2:5
  |
2 |     a..b;
  |     ^ not found in this scope

error[E0425]: cannot find value `b` in this scope
 --> /home/ubuntu/test.rs:2:8
  |
2 |     a..b;
  |        ^ not found in this scope
  |
help: you might have meant to write a method call instead of a range
  |
2 -     a..b;
2 +     a.b;

I'd much prefer if the wording was made to not mention methods in specific, but focus on the . itself -- something like "you might've meant to write the . operator instead of a range"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of "you might have meant to write . instead of .."

Copy link
Member

@compiler-errors compiler-errors Dec 27, 2022

Choose a reason for hiding this comment

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

Yeah, that works fine.

Just would prefer it to not mention "method" unless we can verify it's actually a method, but since this code is in resolve, I totally understand how that's hard, haha.

{
let mut sugg = ".";
let mut span = start.span.between(end.span);
if span.lo() + BytePos(2) == span.hi() {
// There's no space between the start, the range op and the end, suggest
// removal which will look better.
span = span.with_lo(span.lo() + BytePos(1));
sugg = "";
}
Some((
span,
"you might have meant to write `.` instead of `..`",
sugg.to_string(),
Applicability::MaybeIncorrect,
))
} else if res.is_none() {
this.report_missing_type_error(path)
} else {
None
};

this.r.use_injections.push(UseError {
err,
Expand Down Expand Up @@ -4005,6 +4025,12 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
self.visit_expr(rhs);
self.diagnostic_metadata.is_assign_rhs = false;
}
ExprKind::Range(Some(ref start), Some(ref end), RangeLimits::HalfOpen) => {
self.diagnostic_metadata.in_range = Some((start, end));
self.resolve_expr(start, Some(expr));
self.resolve_expr(end, Some(expr));
self.diagnostic_metadata.in_range = None;
}
_ => {
visit::walk_expr(self, expr);
}
Expand Down
30 changes: 30 additions & 0 deletions src/test/ui/suggestions/method-access-to-range-literal-typo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
fn as_ref() -> Option<Vec<u8>> {
None
}
struct Type {
option: Option<Vec<u8>>
}
trait Trait {
fn foo(&self) -> Vec<u8>;
}
impl Trait for Option<Vec<u8>> {
fn foo(&self) -> Vec<u8> {
vec![1, 2, 3]
}
}

impl Type {
fn method(&self) -> Option<Vec<u8>> {
self.option..as_ref().map(|x| x)
//~^ ERROR E0308
}
fn method2(&self) -> &u8 {
self.option..foo().get(0)
//~^ ERROR E0425
//~| ERROR E0308
}
}

fn main() {
let _ = Type { option: None }.method();
}
48 changes: 48 additions & 0 deletions src/test/ui/suggestions/method-access-to-range-literal-typo.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
error[E0425]: cannot find function `foo` in this scope
--> $DIR/method-access-to-range-literal-typo.rs:22:22
|
LL | self.option..foo().get(0)
| ^^^ not found in this scope
|
help: you might have meant to write `.` instead of `..`
|
LL - self.option..foo().get(0)
LL + self.option.foo().get(0)
|

error[E0308]: mismatched types
--> $DIR/method-access-to-range-literal-typo.rs:18:9
|
LL | fn method(&self) -> Option<Vec<u8>> {
| --------------- expected `Option<Vec<u8>>` because of return type
LL | self.option..as_ref().map(|x| x)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `Option`, found struct `Range`
|
= note: expected enum `Option<_>`
found struct `std::ops::Range<Option<_>>`
help: you likely meant to write a method call instead of a range
|
LL - self.option..as_ref().map(|x| x)
LL + self.option.as_ref().map(|x| x)
|

error[E0308]: mismatched types
--> $DIR/method-access-to-range-literal-typo.rs:22:9
|
LL | fn method2(&self) -> &u8 {
| --- expected `&u8` because of return type
LL | self.option..foo().get(0)
| ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&u8`, found struct `Range`
|
= note: expected reference `&u8`
found struct `std::ops::Range<Option<Vec<u8>>>`
help: you likely meant to write a method call instead of a range
|
LL - self.option..foo().get(0)
LL + self.option.foo().get(0)
|

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0308, E0425.
For more information about an error, try `rustc --explain E0308`.