Avoid follow-up errors and ICEs after missing lifetime errors on data structures #127311
Conversation
|
rustbot has assigned @compiler-errors. Use |
This comment has been minimized.
This comment has been minimized.
|
nice :D |
| if let Err(guar) = value.error_reported() { | ||
| self.set_tainted_by_errors(guar); | ||
| } |
There was a problem hiding this comment.
While this could be done in a more targeted location to just fix this one specific kind of ICE, as long as there is no perf impact, this seems like a good place to check a lot of types for errors
f3e56de to
2d5b2d8
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Avoid follow-up errors and ICEs after missing lifetime errors on data structures Tuple struct constructors are functions, so when we call them typeck will use the signature tuple struct constructor function to provide type hints. Since typeck mostly ignores and erases lifetimes, we end up never seeing the error lifetime in writeback, thus not tainting the typeck result. Now, we eagerly taint typeck results by tainting from `resolve_vars_if_possible`, which is called all over the place. I did not carry over all the `crashes` test suite tests, as they are really all the same cause (missing or unknown lifetime names in tuple struct definitions or generic arg lists). fixes rust-lang#124262 fixes rust-lang#124083 fixes rust-lang#125155 fixes rust-lang#125888 fixes rust-lang#125992 fixes rust-lang#126666 fixes rust-lang#126648 fixes rust-lang#127268
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7b804d2): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (secondary 0.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 7.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 718.904s -> 724.442s (0.77%) |
|
While instruction counts go up, cycles do not (deeply nested multi is probably just noise in mono collection) @rustbot ready |
|
sure why not @bors r+ |
…ler-errors Avoid follow-up errors and ICEs after missing lifetime errors on data structures Tuple struct constructors are functions, so when we call them typeck will use the signature tuple struct constructor function to provide type hints. Since typeck mostly ignores and erases lifetimes, we end up never seeing the error lifetime in writeback, thus not tainting the typeck result. Now, we eagerly taint typeck results by tainting from `resolve_vars_if_possible`, which is called all over the place. I did not carry over all the `crashes` test suite tests, as they are really all the same cause (missing or unknown lifetime names in tuple struct definitions or generic arg lists). fixes rust-lang#124262 fixes rust-lang#124083 fixes rust-lang#125155 fixes rust-lang#125888 fixes rust-lang#125992 fixes rust-lang#126666 fixes rust-lang#126648 fixes rust-lang#127268
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
|
☔ The latest upstream changes (presumably #127358) made this pull request unmergeable. Please resolve the merge conflicts. |
|
-.- more crashes tests were added in the mean time |
2d5b2d8 to
dce98c5
Compare
|
@bors r=compiler-errors |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (c92a8e4): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 703.866s -> 704.358s (0.07%) |
Seems like the performance after merge largely mirrors the pre-merge perf run so I think @oli-obk's analysis stands. In any case, this is a correctness fix so it seems like a small perf hit would be acceptable in any case. @rustbot label: +perf-regression-triaged |
Tuple struct constructors are functions, so when we call them typeck will use the signature tuple struct constructor function to provide type hints. Since typeck mostly ignores and erases lifetimes, we end up never seeing the error lifetime in writeback, thus not tainting the typeck result.
Now, we eagerly taint typeck results by tainting from
resolve_vars_if_possible, which is called all over the place.I did not carry over all the
crashestest suite tests, as they are really all the same cause (missing or unknown lifetime names in tuple struct definitions or generic arg lists).fixes #124262
fixes #124083
fixes #125155
fixes #125888
fixes #125992
fixes #126666
fixes #126648
fixes #127268
fixes #127266
fixes #127304