Avoid later diagnostics and ice from typeck for recovered struct variants #127502
Conversation
rustbot
added
PG-exploit-mitigations
|
Some changes occurred in compiler/rustc_sanitizers cc @rust-lang/project-exploit-mitigations, @rcvalle Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred in cc @BoxyUwU rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer Some changes occurred in match checking cc @Nadrieril Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
rustbot
added
T-compiler
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
This seems like a really invasive change, and there's basically no information about why this needs to be so complicated. Especially because I don't see why we need to change almost 100 files in the compiler to fix two UI tests.
For example, why don't we just fail parsing instead of recovering a struct and tainting all of its fields? I'd rather we not have to handle all of this all the way through type checking, imo.
| @@ -81,7 +81,7 @@ impl Attrs { | |||
| let krate = loc.parent.lookup(db).container.krate; | |||
| let item_tree = loc.id.item_tree(db); | |||
| let variant = &item_tree[loc.id.value]; | |||
| (variant.fields.clone(), item_tree, krate) | |||
| (variant.fields().clone(), item_tree, krate) | |||
There was a problem hiding this comment.
Was this a find-replace mistake? Rust analyzer shouldn't be changed here lol
|
|
||
| pub fn fields_checked(&self) -> Result<&IndexVec<FieldIdx, FieldDef>, ErrorGuaranteed> { | ||
| self.fields.as_ref().map_err(|e| *e) | ||
| } |
There was a problem hiding this comment.
I think if we do decide that we should go ahead with this strategy, we should still keep fields as public on VariantDef and expose some other way for hir_typeck to detect that the ADT/Variant has been tainted.
An idea:
- Store
tainted: Option<ErrorGuaranteed>in the ADT, revert the changes to the fields list - Expose
VariantDef::has_errors() -> Result<(), ErrorGuaranteed> - Make the
TyCtxt::type_ofquery returnTyKind::Errorfor ADTs that have recovered.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
yes, maybe we should not recover from this, since the struct fields would be empty, and it's nonsense to continue. I will have a try on this way. |
…ice, r=<try> Avoid no field error and ice no recovered struct variant Fixes rust-lang#126744 Fixes rust-lang#126344, a more general fix compared with rust-lang#127426 r? `@oli-obk` From `@compiler-errors` 's comment rust-lang#127502 (comment) Seems most of the ADTs don't have taint, so maybe it's not proper to change `TyCtxt::type_of` query.
…ice, r=compiler-errors Avoid "no field" error and ICE on recovered ADT variant Fixes rust-lang#126744 Fixes rust-lang#126344, a more general fix compared with rust-lang#127426 r? `@oli-obk` From `@compiler-errors` 's comment rust-lang#127502 (comment) Seems most of the ADTs don't have taint, so maybe it's not proper to change `TyCtxt::type_of` query.
Fixes #126744
Fixes #126344, a more general fix compared with #127426
r? @oli-obk