Migration lints for 2021 prelude

[nikomatsakis]

The 2021 Edition plans to have a new prelude, which will include a number of new traits. This means that new methods will be in scope which could introduce ambiguities into some code bases if they are using methods with the same name (but from different traits). We need a migration lint that detects such cases and suggests a rewrite into fully qualified form: e.g., x.foo() would become something like Trait::foo(&x).

[nikomatsakis]

@rustbot assign @djc

I believe @djc was planning to write mentoring instructions here, so I'm assigning to them. We discussed it a bit on zulip; my plan and @djc's somewhat simpler plan. Either of these seem great to me.

[djc]

The latest version of this is now in RFC 3114 (rendered).

[nikomatsakis]

Here are the instructions written in RFC 3114:

Migration Lint

As for all edition changes, we will implement a migration lint to detect cases where code would break in the new edition. It includes a MachineApplicable suggestion for an alternative that will work in both the current and next edition.

The migration lint will be implemented as follows:

• Find method calls matching the name of one of the newly added traits' methods.
  This can be done either by hardcoding these method names or by setting up some
  kind of registry through the use of an attribute on the relevant traits.
• After method resolution, if:
  • The method matches one of the newly added methods' names, and
  • The originating trait is not from core or std (and/or does not
    match the originating trait), and
  • The originating trait is also implemented for the receiver's type,
    • Suggest a rewrite to disambiguating syntax (such as foo.try_into() to TryInto::try_into(foo)). If necessary, additional levels of (de)referencing
      might be needed to match the implementing type of the target trait.

Currently, diagnostics for trait methods that are not in scope suggest importing the originating trait. For traits that have become part of the prelude in a newer edition, the diagnostics should be updated such that they suggest upgrading to the latest edition as an alternative to importing the relevant trait.

[nikomatsakis]

Here are some mentoring instructions for how to do this.

To start, I would go with hard-coding the names of the methods for now. It's easier. Those methods are the methods from TryFrom, TryInto, and FromIterator.

The method call resolution is is n the rustc_typeck::check::method module:

rust/compiler/rustc_typeck/src/check/method/mod.rs

Lines 170 to 192 in ef0ec30

	/// Performs method lookup. If lookup is successful, it will return the callee
	/// and store an appropriate adjustment for the self-expr. In some cases it may
	/// report an error (e.g., invoking the `drop` method).
	///
	/// # Arguments
	///
	/// Given a method call like `foo.bar::<T1,...Tn>(...)`:
	///
	/// * `self`: the surrounding `FnCtxt` (!)
	/// * `self_ty`: the (unadjusted) type of the self expression (`foo`)
	/// * `segment`: the name and generic arguments of the method (`bar::<T1, ...Tn>`)
	/// * `span`: the span for the method call
	/// * `call_expr`: the complete method call: (`foo.bar::<T1,...Tn>(...)`)
	/// * `self_expr`: the self expression (`foo`)
	#[instrument(level = "debug", skip(self, call_expr, self_expr))]
	pub fn lookup_method(
	&self,
	self_ty: Ty<'tcx>,
	segment: &hir::PathSegment<'_>,
	span: Span,
	call_expr: &'tcx hir::Expr<'tcx>,
	self_expr: &'tcx hir::Expr<'tcx>,
	) -> Result<MethodCallee<'tcx>, MethodError<'tcx>> {

The call to lookup_probe is what selects which method is being invoked:

rust/compiler/rustc_typeck/src/check/method/mod.rs

Lines 198 to 199 in ef0ec30

	let pick =
	self.lookup_probe(span, segment.ident, self_ty, call_expr, ProbeScope::TraitsInScope)?;

We want to check if the method name is in the desired set of methods. The method name can be found in segment.ident, it is an Ident. You can add the try_from, try_into idents etc in this table:

rust/compiler/rustc_span/src/symbol.rs

Lines 105 to 123 in ef0ec30

	// Pre-interned symbols that can be referred to with `rustc_span::sym::*`.
	//
	// The symbol is the stringified identifier unless otherwise specified, in
	// which case the name should mention the non-identifier punctuation.
	// E.g. `sym::proc_dash_macro` represents "proc-macro", and it shouldn't be
	// called `sym::proc_macro` because then it's easy to mistakenly think it
	// represents "proc_macro".
	//
	// As well as the symbols listed, there are symbols for the strings
	// "0", "1", ..., "9", which are accessible via `sym::integer`.
	//
	// The proc macro will abort if symbols are not in alphabetical order (as
	// defined by `impl Ord for str`) or if any symbols are duplicated. Vim
	// users can sort the list by selecting it and executing the command
	// `:'<,'>!LC_ALL=C sort`.
	//
	// There is currently no checking that all symbols are used; that would be
	// nice to have.
	Symbols {

similar to how we already have as_str etc.

You can then compare segment.ident against sym::try_from etc to see if the method has the desired name.

Assuming it does, we want to see if the method being invoked is from a trait in libstd/libcore. If it is, we don't have to give a warning, since we are already using the method we expect. Otherwise, we should.

To determine what method is being invoked, we want to check the PickResult which, if Ok, contains a Pick. This contains a field kind that has a PickKind -- PickKind::Trait corresponds to our case.

We then check the item field to see what is invoked. If the item.def_id field comes from libstd or libcore, then no warning is required. You can determine that by checking the krate field of the DefId. Get the cstore from the tcx or something. (I'm not actually 100% sure the best way to do this step.)

[nikomatsakis]

@rustbot release-assignment

[jam1garner]

@rustbot claim

[jam1garner]

Currently the big issue on the PR is that a majority of the breaking changes (by cases to handle, likely not by how common it is) look involve associated functions and thus something like this:

trait TryFromU8 {
    fn try_from(x: u8) -> Result<Self, !>;
}

impl TryFromU8 for u32 {
    fn try_from(x: u8) -> Result<Self, !> {
        x as u32
    }
}

let x = u32::try_from(3);

The part of typeck we were looking at only handles dot-call syntax and can only handle try_into (as it's the only method). When I get back to this I'm going to start looking for where associated functions are looked up for a given path during type checking, as that'd likely be a good place to target handling the associated function for the lint.

[nikomatsakis]

@jam1garner ah, good catch, I didn't consider this case. It should be relatively easy to manage this as well.

[m-ou-se]

Fixed by #85707 🎉