use help rather than span note for no method error; a slight rephrasing

[zackmdavis]

The use of the span_note diagnostic method resulted in error messages
that some users (including the present author) found very confusing: the
code snippet and highlighted span displayed with the "did you mean" note
was easy to interpret as a suggested edit (as if it had been set by
span_suggestion), but was in fact identical to the snippet/span
displayed just above, indicating the error in the original code, making
it look as if the compiler was suggesting a no-op change.

To remedy this, we use help instead of span_note, and reword one of
the affected messages to emphasize the concept of accessing a field
being different from calling a method (when the message just said, "did
you mean to write self.foo", it was not immediately obvious that this
was meant in contrast to self.foo(), with method-call parens).

Resolves #38321.

---

Addendum (1 Feb): screenshot by reviewer request—

[rust-highfive]

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

[GuillaumeGomez]

Can you add a screenshot of actual and of new output please?

[oli-obk]

how about moving to a suggestion entirely?

[zackmdavis]

@GuillaumeGomez Screenshot added.

@oli-obk Respectfully, I disagree: I'd rather conserve vertical terminal space and have the correction in a one-line help note, rather than spend five lines rendering a suggestion span that's almost identical to the error span but with a single paren-pair added or removed.

---

CI failure is spurious, as if our friend Travis has been sick lately.

[nrc]

This does feel like a suggestion, rather than a help message because it is giving you a specific suggestion for fixing the code. On the other hand, I believe we tend to use suggestions when we are more sure that the suggestion is correct (here they might have meant something completely different). The advantage of using suggestion rather than help is that it allows automatic fixing by tools.

OTOH, this PR is clearly an improvement and I do prefer the 'after' screenshot, so I don't object to this.

cc @jonathandturner

[GuillaumeGomez]

I prefer the after screenshot as well. Waiting for @jonathandturner's confirmation.

[oli-obk]

rather than spend five lines rendering a suggestion span that's almost identical to the error span but with a single paren-pair added or removed.

Actually, rustc only prints out one line.

error[E0178]: expected a path on the left-hand side of `+`, not `&Copy`
 --> <anon>:2:12
  |
2 |     let _: &Copy + 'static;
  |            ^^^^^ expected a path
  |
help: try adding parentheses (per RFC 438):
  |     let _: &(Copy + 'static);

On the other hand, I believe we tend to use suggestions when we are more sure that the suggestion is correct (here they might have meant something completely different).

see #39458 for distinguishing between guesses and suggestions

Note: don't block this PR on account of that. It's definitely an improvement, there's no reason not to merge this and revisit making it a suggestion/guess later.

[sophiajt]

My current thinking is that if the suggestions are small and directly for the underlined text, we should say them.

For example:

  |   w.wrap.not_closure();
  |          ^^^^^^^^^^^^^ did you mean the field `.not_closure`?

The reason being that the faster we can get the person reading the error to know what to do to fix the issue, the better. In some cases, the suggestion is enough to jog your memory of "oh yeah, that's a field, not a method" Rather than taking up space to give the error and the suggestion, for simple suggestions, it's easier to just make them a label.

For suggestions that don't easily fall into that category, like this one:

error[E0178]: expected a path on the left-hand side of `+`, not `&Copy`
 --> <anon>:2:12
  |
2 |     let _: &Copy + 'static;
  |            ^^^^^ expected a path
  |
help: try adding parentheses (per RFC 438):
  |     let _: &(Copy + 'static);

It's better to have a separate suggestion because it's a different span than the error (here, we're showing a larger expression surrounded by parentheses).

There isn't a hard-fast rule or anything. I think we're still trying to figure out what is best. But my general rule is "less is more". If we can show less text to the user and enable to fix the issue is a similar amount of time, that's a good change.

[oli-obk]

great idea. When the suggestion span and the error span are exactly the same, we can use the shorthand.

[estebank]

  |   w.wrap.not_closure();
  |          ^^^^^^^^^^^^^ did you mean the field `.not_closure`?

@jonathandturner part of the problem where is that the actual span is instead

  |   w.wrap.not_closure();
  |          ^^^^^^^^^^^ did you mean the field `.not_closure`?

which is leads to the confusion.

Agree with the rest of the points.

[sophiajt]

@estebank - I wonder if we can fix the span that's giving the error, since the span should probably cover the whole function call piece rather than just the symbol name.

Curious what @nikomatsakis thinks.

[nikomatsakis]

@jonathandturner that seems eminently doable... we're probably working hard to only underline the method name right now, versus the whole call.

[nikomatsakis]

yeah so the code like this:

                    self.report_method_error(method_name.span,
                                             expr_t,
                                             method_name.node,
                                             Some(rcvr),
                                             error,
                                             Some(args));

is passing method_name.span specifically. However, I am somewhat skeptical that we should switch to underlining the whole call -- consider that it may stretch over multiple lines, etc.

[mrhota]

@nikomatsakis @jonathandturner what's the disposition here?

CC @zackmdavis @oli-obk

[bors]

☔ The latest upstream changes (presumably #40911) made this pull request unmergeable. Please resolve the merge conflicts.

[zackmdavis]

@mrhota Thanks for checking in! (I'm still generally interested in rustc development, but have been very busy with other projects lately.) As luck would have it, it looks like this just got scooped by #40816. Closing.

[estebank]

@zackmdavis sorry about that, I completely forgot that there was already a PR for this.