Rust: RFC: limit suggestions to a white-list of functions or to `as_`, `to_`, and `into_` methods

[nikomatsakis]

The "here are some functions that may meet your needs" tips are rather broad. I think we should impose some limits. As a spin-off from #42764, consider this (same) example:

fn main() {
    let _: Option<i32> = 42i32;
}

Right now this gives:

error[E0308]: mismatched types
 --> <anon>:2:26
  |
2 |     let _: Option<i32> = 42i32;
  |                          ^^^^^ expected enum `std::option::Option`, found i32
  |
  = note: expected type `std::option::Option<i32>`
             found type `i32`
  = help: here are some functions which might fulfill your needs:
          - .checked_abs()
          - .checked_neg()

checked_abs() and checked_neg() seem like very silly suggestions to me. Similarly, #42746 points out that suggesting unwrap() is sort of inappropriate. I think it would be better if used a whitelist whitelist of approved conversion functions, or else if we limited them to the prefixes as_, to_, or into_. The latter is sort of codifying an existing convention -- I personally see no problem with that, since this is just a compiler hint. It has the plus and minus that it would apply rather broadly (i.e., to functions outside the stdlib), where a whitelist obviously wouldn't unless we standardized it.

[oli-obk]

I actually liked these verbose suggestions. Couldn't we just order the suggestions better? I mean place all the methods fulfilling a whitelist of prefixes on the front. In the command line diagnostics, only a few suggestions show up anyway, so most weird ones should end up being hidden. Tools using the json output could get all the suggestions and users could browse them.

I agree that we should blacklist anything like expect or unwrap.

[ollie27]

In my opinion these suggestions should primarily come from conversion traits like From, Into, AsRef, AsMut, TryFrom, TryInto, Borrow, BorrowMut, ToOwned, Deref, DerefMut maybe even thing like ToString and Index etc. For example the suggestions for let s: String = "foo"; are the following:

  = help: here are some functions which might fulfill your needs:
          - .escape_debug()
          - .escape_default()
          - .escape_unicode()
          - .to_lowercase()
          - .to_uppercase()

No mention of String::from, .to_owned() or .to_string(). Even .to_lowercase() and .to_uppercase() are a bit silly so putting them at the top of the list doesn't help.

[nikomatsakis]

My intution is that we should only be suggesting "identity" conversions of various kinds. Things like checked_neg or to_lowercase kind of alters the semantics of the program.

[nikomatsakis]

Also, do we not suggest things from traits? If so, that's a pretty serious shortcoming. =) That was, indeed, sort of the whole idea here. Certainly let s: String = "foo" was meant to suggest to_string etc.

[zackmdavis]

I believe I've made some progress on this.

We currently filter suggestions from probe_for_return_type, which calls further probing machinery with a TraitsInScope option: if we use AllTraits instead, we do get suggestions from the desired conversion traits.

A question: does anyone know how we could look up the DefIDs of the specific traits that we want to whitelist suggestions from (From, Into, ToString, _&c._)? My current implementation looks at the method name and whether it comes from a trait, which is a _pretty good_ heuristic, but it's not _perfect_ (_e.g._, it has no way of ruling out the semantic-changing .to_degrees() and .to_radians() methods converting a Box(f64) to an f64).

[zackmdavis]

(illustration of work-in-progress discussed in previous comment)

Before:

Extending the current suggestion list with methods from traits (zackmdavis/rust@9df1ed91b6):

Using the structured diagnostic suggestions, only using suggestions from traits (zackmdavis/rust@3b1ab62363):

[nikomatsakis]

Structured suggestions look cool, but I think the biggest problem we have is that the suggestions are often nonsensical. For example, I feel like suggestions to_degrees or to_radians feels -- to me -- mildly inappropriate. I would think we only want 'identity'-like conversions, such as as_slice or as_mut, not something that has semantic meaning.

I'm also pretty nervous about suggesting people invoke borrow_mut and as_mut, neither of which is meant to be used in ordinary code (and the fact that people do so is a common source of breakage when new impls are added).

[arielb1]

@zackmdavis

Maybe we want to add an attribute on the methods that we want to be used in suggestions, and only put it on the desirable methods?

[estebank]

@arielb1 I recall that being suggested before, but don't think anyone has created a ticket or gone ahead and implemented that. I think that's gonna end up being the only reasonable option we'll have.

[estebank]

@nikomatsakis I think that if we added an attribute for "safe" methods, then it'd be reasonable to use the suggestion output, otherwise I'd just stick to using the list output.

[zackmdavis]

Attributes are looking promising (zackmdavis/rust@b3f1b4a7), but can't PR yet because the UI test diffs show us overzealously (not to mention mysteriously) suggesting .into() in places where it doesn't actually work. Research to follow on some other day.