Recover from incorrect replacement of & for types and expressions - #162376
KevinA-cpu wants to merge 1 commit into
Conversation
|
The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
- Suggest from `T&` to `&T` - Suggest from `mut&` to `&mut` - Suggest from `mut &'a u8` to `&'a mut u8` - Suggest from `expr&` to `&expr` - Suggest from `expr &mut` to `&mut expr` Guards: - check for may_recover() to avoid breaking macros - can_begin_type/can_begin_expr to avoid breaking `u8 & u8` and tests/ui/consts/closure-type-error-during-const-eval-66706.rs - Binary/Cast guards are in-place to prevent making wrong suggestions in complicated `lhs` Does not touch the following areas: - const& T: overlapping with open issue 146122 - i32&&: C++'s T&& is an rvalue reference and Rust doesn't have the same thing currently - u8 &mut - x as u8&
bba988b to
bea1b70
Compare
There was a problem hiding this comment.
We're in an expression context here, so "reference types" isn't correct terminology. You're looking for "borrow expression".
However, stepping back, expr& is not a thing in C/C++, so I'm not so sure if it's worth trying to recover from it. Do you think that anybody is really going to accidentally type expr& when they meant &expr? Re. expr &mut I'm 99.9% confident nobody has ever written that by accident.
There was a problem hiding this comment.
Hi @fmease , thanks for your review, since the issue wants recovery in the expr &mut case, would it be acceptable to remove recovery for only expr& cases, or would you prefer to drop both entirely and revisit the issue?
There was a problem hiding this comment.
It is ok to not address every case raised in the issue when in doubt.
There was a problem hiding this comment.
I'm wondering whether we should say &mut /* Type */ instead to make it crystal clear what is a placeholder and what isn't.
There was a problem hiding this comment.
Hi @fmease do you mean literal &mut /* Type */ or extracting the type and place it directly in the message, i.e &mut i32?
coming from C# with Generics I would definitely prefer /* Type */ over just T
There was a problem hiding this comment.
I believe he means in the message instead of T
There was a problem hiding this comment.
Thanks for making sure this is well-tested. However, personally I think it's a tinge excessive for such a niche recovery 🤷
There was a problem hiding this comment.
Hi @fmease , since I'm only starting to contribute to Rust I want to be thorough at the very least 😅 , would you prefer that I drop some cases?
There was a problem hiding this comment.
The only ones that might feel a bit redundant are the ones that only differ on whitespace, but suggestions can absolutely be dependent on whitespace, so it is ok to be defensive. Better to have too many than too few, I'd say.
|
r? @fmease |
|
☔ The latest upstream changes (presumably #162269) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
There was a problem hiding this comment.
| help: write the `&` before the type |
There was a problem hiding this comment.
I believe he means in the message instead of T
There was a problem hiding this comment.
The only ones that might feel a bit redundant are the ones that only differ on whitespace, but suggestions can absolutely be dependent on whitespace, so it is ok to be defensive. Better to have too many than too few, I'd say.
There was a problem hiding this comment.
It is ok to not address every case raised in the issue when in doubt.
There was a problem hiding this comment.
In cases where we have less certainty, it makes sense to mention as many cases that we could anticipate realistically being the problem.
| error: unexpected `&` at end of expression | |
| --> $DIR/c-style-reference-exprs-issue-101487.rs:22:16 | |
| | | |
| LL | let _ptr = x&; | |
| | ^ | |
| | | |
| help: reference expressions must be written as `&/* expr */` | |
| | | |
| LL - let _ptr = x&; | |
| LL + let _ptr = &x; | |
| | | |
| help: you might be missing the right-hand side expression of a "binary and" (`&`) expression | |
| | | |
| LL | let _ptr = x& /* expr */; | |
| | ++++++++++ |
There was a problem hiding this comment.
I'm going to be mean and mention two more cases you'd want to take into consideration :)
fn main() {
let r#mut = 0;
let x = 4 & mut;
println!("{x:b}")
}
The correct code was supposed to be
fn main() {
let r#mut = 0;
let x = 4 & r#mut;
println!("{x:b}")
}
fn main() {
let mu = 0;
let x = 4 & mut;
println!("{x:b}")
}
the correct code should have been
fn main() {
let mu = 0;
let x = 4 & mu;
println!("{x:b}")
}
Neither of these cases is great today, and can be dealt with in a different PR. They need to be accounted for not only in your new code, but also in the code that already triggers today (likely one of the "expected/found" methods):
error: expected expression, found keyword `mut`
--> src/main.rs:3:17
|
3 | let x = 4 & mut;
| ^^^ expected expression
Because this happens during parsing, it is harder to architect this so that it relies on name resolution. We can either collect local binding names in the parser (potentially only the ones that are r#) or delay the parse error so it can be emitted after name res. Neither of these cases need to be addressed in this PR (beyond maybe mentioning in the diagnostic that may be you meant to write something else instead of mut) :)
There was a problem hiding this comment.
Because of when you check for the trailing &, this fails at expression context and not type context (as it would if you were incorrectly eager while parsing the type of the as), but it might make sense to handle as expressions explicitly and make this output closer to
error: unexpected trailing `&`
--> $DIR/c-style-reference-no-sugg-issue-101487.rs:27:14
|
LL | let _d = x as u8&;
| ^
help: reference types must be written as `&expr`
|
LL | let _d = &(x as u8);
| + +
help: you might be missing a right-hand side expression for a "binary and" (`&`) operation
|
LL | let _d = x as u8& /* expr */;
| ++++++++++
T&to&Tmut&to&mutmut &'a u8to&'a mut u8expr&to&exprexpr &mutto&mut exprGuards:
u8 & u8and tests/ui/consts/closure-type-error-during-const-eval-66706.rslhsDoes not touch the following areas:
#101487