compiler: suggest const _ for a misplaced const {}#128374
compiler: suggest const _ for a misplaced const {}#128374workingjubilee wants to merge 6 commits intorust-lang:mainfrom
const _ for a misplaced const {}#128374Conversation
| span, | ||
| "consider using `const` or `static` instead of `let` for global variables", | ||
| ); | ||
| } else if self.parse_const_block(span, false).is_ok() { |
There was a problem hiding this comment.
Using is_ok here is kinda hazardous since you need to cancel the diagnostic on the bad path.
There was a problem hiding this comment.
that is... definitely the part I was least certain about, yes. is there, like, a sample of Rust that will now elicit some disastrously bad diagnostic as a result of doing this, or...?
There was a problem hiding this comment.
No, if you drop a Diagnostic without emitting it the compiler will ICE.
Presumably you will have some code that looks like:
match self.parse_const_block(span, false) {
Err(e) => { e.cancel(); }
Ok(_) => { do the recovery code here }
}Which at that point we probably should do snapshotting too, so try this:
let snapshot = self.create_snapshot_for_diagnostic();
match self.parse_const_block(span, false) {
Err(e) => {
e.cancel();
self.recover_from_snapshot(snapshot);
}
Ok(_) => { do the recovery code here }
}There was a problem hiding this comment.
No, if you drop a Diagnostic without emitting it the compiler will ICE.
...oh lmao, drop bombs away.
There was a problem hiding this comment.
Example of the drop bomb in question -- this ICEs after your PR:
const { a: () = 1; };
| span, | ||
| "consider using `const` or `static` instead of `let` for global variables", | ||
| ); | ||
| } else if self.parse_const_block(span, false).is_ok() { |
There was a problem hiding this comment.
This logic should probably be moved to parse_item (well, one of its helpers like parse_item_common) since this is really an item recovery.
There was a problem hiding this comment.
should I just yeet all these suggestions that are secretly item recoveries over there?
There was a problem hiding this comment.
If you'd like to do so, please do.
There was a problem hiding this comment.
yeah, I think it's important that both of these suggestions continue to live next to each other so they get refactored together.
| err.span_label( | ||
| span, |
There was a problem hiding this comment.
Also please use a structured suggestion (with Applicability::MaybeIncorrect)
err.span_suggestion_verbose(
span.shrink_to_lo(),
/* ... */
)
There was a problem hiding this comment.
The suggestion above this one for the kw::Let case should also be using something like that, right?
There was a problem hiding this comment.
Yea, though that one's harder because it has two choices. You can try to use span_suggestions if you'd like.
This comment has been minimized.
This comment has been minimized.
|
oh THERE'S the broken tests lmao |
|
You should probably also check that the current token is |
|
fun times ahead, @rustbot author |
Co-authored-by: Christopher Serr <christopher.serr@gmail.com>
0cab476 to
7bfec85
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
|
genuinely not sure what is going on there with that parse error going away.... |
|
probably because these recoveries are in the wrong place, as discussed. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
It seems that the code for the entire "parse const item" arm inside |
Fixes #128338