Fix let keyword removal suggestion in structs
#102927
Conversation
rustbot
added
the
T-compiler
|
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| error: expected identifier, found keyword `let` | ||
| --> $DIR/removed-syntax-field-let-2.rs:2:5 | ||
| | | ||
| LL | let x: i32, |
There was a problem hiding this comment.
I'm actually not a fan of this 4-character wide removal span that overlaps (but isn't equal to) the let keyword span -- I feel like we don't usually care that spacing gets messed up by suggestions anyways, which is what this span is trying to account for.
I think we should just adjust the "remove this let keyword" span to remove just the let keyword, and the user can run rustfmt to fix the leftover spaces 😝 thoughts?
There was a problem hiding this comment.
The running rustfmt part might not be ideal :P
There was a problem hiding this comment.
Suggestions are not aware of global formatting rules, nor are they typically designed to handle cases where spacing, newlines, etc. are messed up because of code suggestions. At least, as long as the suggestion renders in a way that the user can understand, I feel like this is not the responsibility of the parser's error messages.
There was a problem hiding this comment.
Right, for just diagnostics that makes sense. I'm more worried about cargo-fix fixing this and resulting in non-formatted code.
There was a problem hiding this comment.
Given that rustfmt absolutely refuses to format a structlet ident: ty inside of it, I think formatting is a step that will need to be done regardless 😅
There was a problem hiding this comment.
I don't have a strong preference here - I'd probably bias towards keeping the four-character span. Feel free to change it if you want.
compiler-errors
changed the title
let keyword removal suggestion in structs
|
@bors r=davidtwco |
bors
added
S-waiting-on-bors
|
@bors rollup |
Rollup of 7 pull requests Successful merges: - rust-lang#102623 (translation: eager translation) - rust-lang#102719 (Enforce alphabetical sorting with tidy) - rust-lang#102830 (Unify `tcx.constness` query and param env constness checks) - rust-lang#102883 (Fix stabilization of `feature(half_open_range_patterns)`) - rust-lang#102927 (Fix `let` keyword removal suggestion in structs) - rust-lang#102936 (rustdoc: remove unused CSS `nav.sum`) - rust-lang#102940 (Update books) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fix `let` keyword removal suggestion in structs
(1.) Fixes a bug where, given this code:
```rust
struct Foo {
let x: i32,
}
```
We were parsing the field name as `let` instead of `x`, which causes issues later on in the type-checking phase.
(2.) Also, suggestions for `let: i32` as a field regressed, displaying this extra `help:` which is removed by this PR
```
help: remove the let, the `let` keyword is not allowed in struct field definitions
|
2 - let: i32,
2 + : i32,
```
(3.) Makes the suggestion text a bit more succinct, since we don't need to re-explain that `let` is not allowed in this position (since it's in a note that follows). This causes the suggestion to render inline as well.
cc `@gimbles,` this addresses a few nits I mentioned in your PR.
Rollup of 7 pull requests Successful merges: - rust-lang#102623 (translation: eager translation) - rust-lang#102719 (Enforce alphabetical sorting with tidy) - rust-lang#102830 (Unify `tcx.constness` query and param env constness checks) - rust-lang#102883 (Fix stabilization of `feature(half_open_range_patterns)`) - rust-lang#102927 (Fix `let` keyword removal suggestion in structs) - rust-lang#102936 (rustdoc: remove unused CSS `nav.sum`) - rust-lang#102940 (Update books) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
(1.) Fixes a bug where, given this code:
We were parsing the field name as
letinstead ofx, which causes issues later on in the type-checking phase.(2.) Also, suggestions for
let: i32as a field regressed, displaying this extrahelp:which is removed by this PR(3.) Makes the suggestion text a bit more succinct, since we don't need to re-explain that
letis not allowed in this position (since it's in a note that follows). This causes the suggestion to render inline as well.cc @gimbles, this addresses a few nits I mentioned in your PR.