Avoid "no field" error and ICE on recovered ADT variant #127575

chenyukang · 2024-07-10T14:41:21Z

Fixes #126744
Fixes #126344, a more general fix compared with #127426

From @compiler-errors 's comment #127502 (comment)
Seems most of the ADTs don't have taint, so maybe it's not proper to change TyCtxt::type_of query.

compiler/rustc_middle/src/ty/mod.rs

oli-obk · 2024-07-10T14:44:46Z

@bors try @rust-timer queue

compiler/rustc_hir_typeck/src/expr.rs

bors · 2024-07-10T14:45:57Z

⌛ Trying commit 17651e3 with merge 83d5b81...

…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.

oli-obk · 2024-07-10T14:46:54Z

Seems most of the ADTs don't have taint, so maybe it's not proper to change TyCtxt::type_of query.

If we changed the type_of query, does the ICE get fixed without having to add extra checks in typeck?

oli-obk · 2024-07-10T14:49:50Z

lol we could make https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Ty.html#method.new_adt return an error type if the AdtDef is tainted. It may require some additional adjustments of other code that relies on getting an ty::Adt when explicitly asking for one, but it can be done.

compiler/rustc_hir_typeck/src/expr.rs

compiler-errors · 2024-07-10T15:00:17Z

@oli-obk:

lol we could make https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Ty.html#method.new_adt return an error type if the AdtDef is tainted. It may require some additional adjustments of other code that relies on getting an ty::Adt when explicitly asking for one, but it can be done.

I really don't think we should do this unless there's a compelling reason to do so. These constructor functions shouldn't be lying about their output.

It's one thing to change the type_of query, which already has precedent to return Error when the query has issues with resolving an item's type, but it's another thing to change the constructors themselves. I will probably reject such a change.

compiler-errors

You also need to delete a test from the crashes directory I think

compiler/rustc_hir_analysis/src/collect.rs

compiler-errors · 2024-07-10T15:18:57Z

compiler/rustc_hir_typeck/src/expr.rs

@@ -2547,6 +2544,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
            "ban_nonexisting_field: field={:?}, base={:?}, expr={:?}, base_ty={:?}",
            ident, base, expr, base_ty
        );
+
+        for (ty, _) in self.autoderef(expr.span, base_ty) {


I'd kinda rather you handle this in the autoderef loop in check_field.

You mean like this?
20cc0e1

I also thought about it, I was afraid to add extra perf since we need to track whether it's tainted in normal path.

or even we return Ty::new_error(self.tcx(), guar) when there is an error.

compiler/rustc_hir_typeck/src/pat.rs

compiler-errors · 2024-07-10T15:57:32Z

compiler/rustc_hir_typeck/src/expr.rs

+                    if let Err(guard) = adt_def.non_enum_variant().has_errors() {
+                        recovered_variant = Some(guard);
+                        break;
+                    }


Suggested change

if let Err(guard) = adt_def.non_enum_variant().has_errors() {

recovered_variant = Some(guard);

break;

}

if let Err(guar) = adt_def.non_enum_variant().has_errors() {

return Ty::new_error(self.tcx, guar);

}

compiler-errors

also side note: s/guard/guar, since "guar" is short for "guarantee"

compiler-errors · 2024-07-10T15:58:05Z

Also can you squash this PR lol

compiler-errors

One last thing -- can you leave a short comment next to all of these error-shortcuts you added saying something along the lines of "we don't care to report errors for a struct if the struct itself is tainted" or something

compiler-errors · 2024-07-10T16:10:28Z

r=me after

chenyukang · 2024-07-10T16:34:55Z

@bors r=compiler-errors

bors · 2024-07-10T16:34:57Z

📌 Commit 07e6dd9 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-07-11T03:12:42Z

⌛ Testing commit 07e6dd9 with merge 8c39ac9...

bors · 2024-07-11T05:26:23Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 8c39ac9 to master...

rust-timer · 2024-07-11T06:43:28Z

Finished benchmarking commit (8c39ac9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary -3.0%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -1.2%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.3%, -1.0%]	2
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%, secondary 0.1%)

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.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	40
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	21
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	40

Bootstrap: 705.402s -> 703.504s (-0.27%)
Artifact size: 328.86 MiB -> 328.69 MiB (-0.05%)

rustbot assigned oli-obk Jul 10, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 10, 2024

oli-obk reviewed Jul 10, 2024

View reviewed changes

compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 10, 2024

oli-obk reviewed Jul 10, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/expr.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Jul 10, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/expr.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Jul 10, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

compiler-errors reviewed Jul 10, 2024

View reviewed changes

chenyukang force-pushed the yukang-fix-struct-fields-ice branch 3 times, most recently from 3d09cfa to c5afe4d Compare July 10, 2024 16:07

compiler-errors reviewed Jul 10, 2024

View reviewed changes

chenyukang force-pushed the yukang-fix-struct-fields-ice branch from c5afe4d to 3341966 Compare July 10, 2024 16:14

compiler-errors changed the title ~~Avoid no field error and ice no recovered struct variant~~ Avoid "no field" error and ICE on recovered ADT variant Jul 10, 2024

report pat no field error no recoverd struct variant

07e6dd9

chenyukang force-pushed the yukang-fix-struct-fields-ice branch from 3341966 to 07e6dd9 Compare July 10, 2024 16:19

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 10, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 11, 2024

bors merged commit 8c39ac9 into rust-lang:master Jul 11, 2024

rustbot added this to the 1.81.0 milestone Jul 11, 2024

Avoid "no field" error and ICE on recovered ADT variant #127575

Avoid "no field" error and ICE on recovered ADT variant #127575

Uh oh!

Conversation

chenyukang commented Jul 10, 2024

Uh oh!

Uh oh!

oli-obk commented Jul 10, 2024

Uh oh!

This comment has been minimized.

Uh oh!

bors commented Jul 10, 2024

Uh oh!

oli-obk commented Jul 10, 2024

Uh oh!

oli-obk commented Jul 10, 2024

Uh oh!

Uh oh!

compiler-errors commented Jul 10, 2024

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

compiler-errors Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

chenyukang Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenyukang Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

compiler-errors Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Jul 10, 2024

Uh oh!

compiler-errors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Jul 10, 2024

Uh oh!

chenyukang commented Jul 10, 2024

Uh oh!

bors commented Jul 10, 2024

Uh oh!

bors commented Jul 11, 2024

Uh oh!

bors commented Jul 11, 2024

Uh oh!

rust-timer commented Jul 11, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Uh oh!

chenyukang Jul 10, 2024 •

edited

Loading

compiler-errors left a comment •

edited

Loading