Change some notes into suggestions #42033
Conversation
|
meta: It's hard to read diagnostic differences in a test if they are done in the same commit that turns the test from |
src/test/ui/issue-35675.stderr
Outdated
| | ^^ not a type | ||
| | | ||
| = help: there is an enum variant `std::prelude::v1::Ok`,did you mean to use `std::prelude::v1`? | ||
| = help: there is an enum variant `std::prelude::v1::Result::Ok`,did you mean to use `std::prelude::v1::Result`? |
There was a problem hiding this comment.
IIRC, this test was moved to compile-fail due to instability in the order of suggestions (see #40775), so this will probably have to do be reverted, unless you want to fix the instability.
There was a problem hiding this comment.
I'll fix the instability
| @@ -2,7 +2,7 @@ error[E0425]: cannot find function `baz` in this scope | |||
| --> $DIR/issue-14254.rs:29:9 | |||
| | | |||
| 29 | baz(); | |||
| | ^^^ did you mean `self.baz(...)`? | |||
| | ^^^ help: did you mean `self.baz` | |||
There was a problem hiding this comment.
Are the "did you mean" questions... rhetoric?
Is it ok to omit question marks in this context, linguistically?
There was a problem hiding this comment.
The issue is that we have no idea whether the suggestion author wrote try or did you mean, so we can't generate a questionmark out of thin air. We could cover the common cases like did you mean, but have you tried won't get caught or any other textual changes
There was a problem hiding this comment.
I think it's ok to add a heuristic catching "did you"/"have you" and appending the question mark.
All the messages are controlled by us after all, so they can always be tweaked if there are false positives/negatives.
There was a problem hiding this comment.
(I'm assuming adding an extra argument to span_suggestion would be cumbersome, but it's a variant too.)
There was a problem hiding this comment.
We could allow a placeholder in the message. So if you write did you mean {}? it will produce
did you mean `foobar`?
An in case it's not displayed inline, but as a help, the placeholder is replaced by "the following":
did you mean the following?
There was a problem hiding this comment.
That would probably be the most convenient (I can live with a bit of home-grown formatting).
There was a problem hiding this comment.
Ok. I implemented this, but the code complexity goes way up. I need to find the correct {}. I'll also need to allow {{ to escape opening brackets. I need to allow some way to specify a replacement like "the following", ....
How about my first variant suggested here?
add a new enum so the lint author can choose a message
enum SuggestionMsg {
DidYouMean,
Try,
Custom {
label: String, // msg to display as label
help: String, // msg to display as help
},
}cc @nrc this touches on prepending help: before suggestions displayed as labels
There was a problem hiding this comment.
I went through all suggestions' messages in rustc:
- 5 "did you mean {the following}"
- "try casting to a reference instead:"
- "try casting to a
Boxinstead:" - "possible candidates found in another module, you can import them into scope:"
- "did you mean to use the variant's enum"
- "try adding parentheses:"
- "try parenthesizing the first index"
- "try placing this code inside a block"
- "make this visible only to module
{}within:" - really long message about
to_owned - "to access tuple elements, use"
- "to force the closure to take ownership of {} (and any other referenced variables), use the
movekeyword, as shown" - "consider using a reference instead"
I don't see a good way to get all of these under one hat.
There was a problem hiding this comment.
ISTM that the client should choose the punctuation here, which suggests that using {} is the best way. However, the extra complexity does seem a bit annoying. I think the current mix of template and free text is probably wrong - could we either allow totally free text or lock down the structure of the text more strictly (as an aside, I've never liked the "did you mean ..." phrasing, feels patronising, I'd prefer "you could try ...", but I don't care too much and this is a bit orthogonal.)
There was a problem hiding this comment.
OK. I'll create a commit with the enum variant to see how it looks. Then we can reiterate.
arielb1
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #41907) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
force-pushed
the
suggestions
branch
2 times, most recently
from
3a5d066
to
e1974c5
Compare
| /// * should not contain any parts like "the following", "as shown" | ||
| /// * may look like "to do xyz, use" or "to do xyz, use abc" | ||
| /// * may contain a name of a function, variable or type, but not whole expressions | ||
| /// |
There was a problem hiding this comment.
I added some guidelines for suggestion messages. This way we can keep appending the suggestion at the end. I like this solution, because it's easy to read on the command line, requires little effort from suggestion authors and works well with non-label suggestions and json output.
|
Is this ready for re-review? |
| @@ -35,46 +35,29 @@ macro_rules! attr_proc_mac { | |||
| } | |||
|
|
|||
| #[derive(FooWithLongNan)] | |||
| //~^ ERROR cannot find derive macro `FooWithLongNan` in this scope | |||
There was a problem hiding this comment.
I think it's better to keep these annotations.
UI and compile-fail tests are going to be unified and ERROR/WARN will be necessary again.
I'd vote for no " |
yes
fixed
done
As long as the message makes it clear that it's a suggestion... which all suggestions' messages should, but as discussed in the linked PR, this requires suggestion authors to take care when formulating the message. |
|
@bors r+
|
|
📌 Commit 72eb010 has been approved by |
I can create a PR that recognizes sentences starting with "try" or "you can try" and then leave out the "help:" |
Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458
Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458
arielb1
added
S-waiting-on-bors
|
I'm uncertain, but it looks like this caused the PR failure, but this is odd, since the checks here passed. It might be a conflict with another PR. Here's the relevant log: https://gist.github.com/Mark-Simulacrum/b5b140d6e27e99ace2598be544d6c405 which says that the file moved in this PR from compile-fail to ui tests failed UI testing. I haven't investigated beyond this. |
Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458
|
I need a decision whether it's fine to completely refactor error reporting in resolve by storing all the errors in a table and report them after resolve is done. |
|
@oli-obk |
|
I like fixing broken things. I'm sure this is not the only situation, just the only one we're testing. Also the paths reported are rather suboptimal, since no newcomer should have to see |
|
☔ The latest upstream changes (presumably #42850) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to mark this as being with the compiler team, cc @rust-lang/compiler, to get a response to
so this PR can progress in some way (either by moving the test back to |
aidanhs
added
S-waiting-on-team
|
Let's talk about this at the compiler meeting thursday? |
The produced paths aren't stable between builds, since reporting paths inside resolve, before resolve is finished might produce paths resolved to type aliases instead of the concrete type. Compile-fail tests can match just parts of messages, so they don't "suffer" from this issue. This is just a workaround, the instability should be fixed in the future.
|
I moved the test back to compile-fail and will produce a new PR in the future to fix the instability. |
|
@bors r+ |
|
📌 Commit eb7f429 has been approved by |
Change some notes into suggestions r? @petrochenkov since you commented on the same edits in #39458
| @@ -16,7 +16,7 @@ error[E0308]: mismatched types | |||
| 31 | if (x = x) { | |||
| | ^^^^^^^ | |||
| | | | |||
| | help: did you mean to compare equality? `x == x` | |||
| | help: did you mean to compare equality?: `x == x` | |||
There was a problem hiding this comment.
these look... funny to me now. I think it's just the juxtaposition of ? and :... can we end it with something like "did you mean to compare equality? Consider: x == x" instead?
There was a problem hiding this comment.
Hm... those came in after a rebase. I'll create a new PR fixing up all suggestions that came in after I wrote the guidelines into the docs.
carols10cents
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
Adjust new suggestions to the suggestion guidelines Addresses #42033 (comment) guidelines are https://github.com/rust-lang/rust/blob/master//src/librustc_errors/diagnostic.rs#L212-L224
resolve: Try to fix instability in import suggestions cc #42033 `lookup_import_candidates` walks module graph in DFS order and skips modules that were already visited (which is correct because there can be cycles). However it means that if we visited `std::prelude::v1::Result::Ok` first, we will never visit `std::result::Result::Ok` because `Result` will be skipped as already visited (note: enums are also modules here), and otherwise, if we visited `std::result::Result::Ok` first, we will never get to `std::prelude::v1::Result::Ok`. What child module of `std` (`prelude` or `result`) we will visit first, depends on randomized hashing, so we have instability in diagnostics. With this patch modules' children are visited in stable order in `lookup_import_candidates`, this should fix the issue, but let's see what Travis will say. r? @oli-obk
r? @petrochenkov since you commented on the same edits in #39458