check_expr_struct_fields: taint context with errors if struct definition is malformed #125947
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
| let mut error_happened = if variant.fields.len() > remaining_fields.len() { | ||
| // Some field is defined more than once. Make sure we don't try to | ||
| // instantiate this struct in static/const context. | ||
| let guar = self.dcx().span_delayed_bug(expr.span, "struct fields have non-unique names"); |
There was a problem hiding this comment.
No need to delay a bug here. Please taint the infcx below when we emit FieldMultiplySpecifiedInInitializer or report_unknown_field.
There was a problem hiding this comment.
Here? I'm not sure this else branch is executed.
rust/compiler/rustc_hir_typeck/src/expr.rs
Lines 1703 to 1723 in e2c12b4
Should I move my checking and tainting code to this line after the for loop?
rust/compiler/rustc_hir_typeck/src/expr.rs
Lines 1742 to 1744 in e2c12b4
There was a problem hiding this comment.
Actually, I guess I misread the issue. You probably want to do this instead on line 1874 where we call report_missing_fields?
There was a problem hiding this comment.
I think none of the checks in check_expr_struct_fields really catch what this ICE is about. remaining_fields is a map that has field names as keys and duplicated fields just get overwritten. If we try to compile code given as an example in this PR, at 1873 remaining_fields will be empty and this branch will not be taken as well.
rust/compiler/rustc_hir_typeck/src/expr.rs
Lines 1873 to 1874 in e2c12b4
There was a problem hiding this comment.
I could make remaining_fields distinguish between fields with the same name. I didn't feel confident in changing that much just to cover this, because error about struct definition is always there anyway.
This comment has been minimized.
This comment has been minimized.
|
possibly related #123917 |
| let mut error_happened = if variant.fields.len() > remaining_fields.len() { | ||
| // Some field is defined more than once. Make sure we don't try to | ||
| // instantiate this struct in static/const context. | ||
| let guar = | ||
| self.dcx().span_delayed_bug(expr.span, "struct fields have non-unique names"); | ||
| self.set_tainted_by_errors(guar); | ||
|
|
||
| true | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
Can you split this up like so:
| let mut error_happened = if variant.fields.len() > remaining_fields.len() { | |
| // Some field is defined more than once. Make sure we don't try to | |
| // instantiate this struct in static/const context. | |
| let guar = | |
| self.dcx().span_delayed_bug(expr.span, "struct fields have non-unique names"); | |
| self.set_tainted_by_errors(guar); | |
| true | |
| } else { | |
| false | |
| }; | |
| let mut error_happened = false; | |
| if variant.fields.len() != remaining_fields.len() { | |
| // Some field is defined more than once. Make sure we don't try to | |
| // instantiate this struct in static/const context. | |
| let guar = | |
| self.dcx().span_delayed_bug(expr.span, "struct fields have non-unique names"); | |
| self.set_tainted_by_errors(guar); | |
| error_happened = true; | |
| } |
rustbot
added
has-merge-commits
|
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged. You can start a rebase with the following commands: The following commits are merge commits: |
This comment has been minimized.
This comment has been minimized.
rustbot
added
A-testsuite
rustbot
removed
has-merge-commits
check_expr_struct_fields: taint context with errors if struct definit…
Taint errors while checking `struct { field: 1 }` below if struct definition has duplicated fields so that we don't pass it to const eval.
fixes rust-lang#125842, fixes rust-lang#124464, fixes rust-lang#124552
```rust
struct Struct {
field: Option<u8>,
field: u8,
}
static STATIC: Struct = Struct {
field: 1,
};
pub fn main() {}
```
(This was rust-lang#125947 but i messed something up, sorry)
r? `@compiler-errors`
check_expr_struct_fields: taint context with errors if struct definit…
Taint errors while checking `struct { field: 1 }` below if struct definition has duplicated fields so that we don't pass it to const eval.
fixes rust-lang#125842, fixes rust-lang#124464, fixes rust-lang#124552
```rust
struct Struct {
field: Option<u8>,
field: u8,
}
static STATIC: Struct = Struct {
field: 1,
};
pub fn main() {}
```
(This was rust-lang#125947 but i messed something up, sorry)
r? ``@compiler-errors``
Rollup merge of rust-lang#126045 - olafes:master, r=compiler-errors check_expr_struct_fields: taint context with errors if struct definit… Taint errors while checking `struct { field: 1 }` below if struct definition has duplicated fields so that we don't pass it to const eval. fixes rust-lang#125842, fixes rust-lang#124464, fixes rust-lang#124552 ```rust struct Struct { field: Option<u8>, field: u8, } static STATIC: Struct = Struct { field: 1, }; pub fn main() {} ``` (This was rust-lang#125947 but i messed something up, sorry) r? ``@compiler-errors``
Make sure we don't create instances of structs with ambiguous layout in const/static context, as this can lead to ICE's. Fragment below is a reduced version of both #125842 and #124464. With this change, the body of static initializer is tainted and we don't evaluate it at compile-time.
fixes #125842, fixes #124464, fixes #124552