Move various checks to typeck so them failing causes the typeck result to get tainted #96046

oli-obk · 2022-04-14T12:09:15Z

Fixes #69487
fixes #79047

cc @RalfJung this gets rid of the Transmute invalid program error variant

rust-highfive · 2022-04-14T12:09:17Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-04-14T12:09:19Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

RalfJung · 2022-04-14T12:26:42Z

compiler/rustc_middle/src/mir/interpret/error.rs

@@ -148,8 +148,6 @@ pub enum InvalidProgramInfo<'tcx> {
    /// (which unfortunately typeck does not reject).
    /// Not using `FnAbiError` as that contains a nested `LayoutError`.
    FnAbiAdjustForForeignAbi(call::AdjustForForeignAbiError),
-    /// An invalid transmute happened.
-    TransmuteSizeDiff(Ty<'tcx>, Ty<'tcx>),


That's great. :D So does this also fix #79047 ?

What about SizeOfUnsizedType, which seems similar to me?

similar, but when I looked into it it was a different beast that I'll tackle seperately

That's great. :D So does this also fix #79047 ?

yes, the corresponding test has been updated

We might want to keep the issue open for SizeOfUnsizedType though. Do we have a nice example of that (or an existing separate issue)?

I think I saw an example when I removed that variant ^^ but I don't remember where

What about SizeOfUnsizedType, which seems similar to me?

I have opened #97477 for this.

compiler/rustc_const_eval/src/interpret/place.rs

bors · 2022-04-16T07:39:46Z

☔ The latest upstream changes (presumably #94468) made this pull request unmergeable. Please resolve the merge conflicts.

jackh726 · 2022-04-22T18:41:21Z

This looks fine to me, but I'm not really familiar with this code, so maybe @RalfJung wants to give the final say?

RalfJung · 2022-04-23T08:48:56Z

typeck is outside my purview, sorry -- I can't really help with this PR, other than that I like the end-to-end effect of turning that one check in the interpreter into an assertion.

oli-obk · 2022-04-23T09:00:10Z

Rerolling the reviewer dice

r? rust-lang/compiler

oli-obk · 2022-05-05T08:01:36Z

r? rust-lang/compiler

bors · 2022-05-07T04:26:59Z

☔ The latest upstream changes (presumably #96531) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2022-05-10T19:00:26Z

r? rust-lang/compiler

I won't be able to get to reviewing things for the upcoming next couple weeks.

bors · 2022-05-17T09:53:02Z

☔ The latest upstream changes (presumably #96825) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot

I can't really review the typeck algorithm, but left a few questions where comments could be appropriate.

compiler/rustc_typeck/src/check/expr.rs

compiler/rustc_typeck/src/check/intrinsicck.rs

cjgillot · 2022-05-22T08:36:32Z

compiler/rustc_typeck/src/check/mod.rs

+            fcx.check_transmutes();
+        }
+
+        fcx.check_asms();


Why the difference between check_transmutes and check_asms?

I wanted to replicate the previous diagnostics. For transmute that meant not to check if there were other errors already happening, as that would just cause useless errors in the transmute checks

cjgillot · 2022-05-22T08:38:44Z

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -274,6 +274,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
        error: &SelectionError<'tcx>,
        fallback_has_occurred: bool,
    ) {
+        self.set_tainted_by_errors();


Does this change anything?

yes. It avoids ICEs and bogus errors in CTFE.

… const eval can avoid running at all

…irst error

cjgillot · 2022-05-26T06:53:11Z

@bors r+

bors · 2022-05-26T06:53:13Z

📌 Commit 6ba8da6 has been approved by cjgillot

bors · 2022-05-26T08:27:05Z

⌛ Testing commit 6ba8da6 with merge d3e7c4b8bb35907bab7d8714b84b21e7bc39fda0...

bors · 2022-05-26T08:50:27Z

💔 Test failed - checks-actions

oli-obk · 2022-05-27T09:18:45Z

@bors r=cjgillot

bors · 2022-05-27T09:18:46Z

📌 Commit 4332c2f has been approved by cjgillot

bors · 2022-05-27T11:31:40Z

⌛ Testing commit 4332c2f with merge 56fd680...

bors · 2022-05-27T14:12:20Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 56fd680 to master...

rust-timer · 2022-05-27T15:29:46Z

Finished benchmarking commit (56fd680): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.9%	1.1%	4
Improvements 🎉 (primary)	-0.3%	-0.4%	4
Improvements 🎉 (secondary)	-0.3%	-0.3%	24
All 😿🎉 (primary)	-0.3%	-0.4%	4

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.5%	2.5%	1
Improvements 🎉 (primary)	-2.4%	-2.8%	2
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.4%	-2.8%	2

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.8%	-3.2%	2
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rust-highfive assigned jackh726 Apr 14, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 14, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2022

RalfJung reviewed Apr 14, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/place.rs Show resolved Hide resolved

rust-highfive assigned cjgillot and unassigned jackh726 Apr 23, 2022

oli-obk force-pushed the const_typeck branch from 727a836 to 9a90263 Compare May 5, 2022 08:01

rust-highfive assigned nagisa and unassigned cjgillot May 5, 2022

oli-obk force-pushed the const_typeck branch from 9a90263 to 289861e Compare May 10, 2022 14:10

rust-highfive assigned compiler-errors and unassigned nagisa May 10, 2022

cjgillot reviewed May 22, 2022

View reviewed changes

oli-obk added 6 commits May 24, 2022 15:37

trait selection errors should poison the typeck results, too, so that…

31e0bf7

… const eval can avoid running at all

move intrinsicck to typeck

cb886cc

Remove the check_mod_intrinsics query

0fdaaad

Add the transmute and asm checks to typeck as deferred checks

0d88631

Remove outdated comment

ac6b708

Make a test independent of 64/32 bit

d5ffe11

Update messages

46a2342

oli-obk force-pushed the const_typeck branch from 289861e to 46a2342 Compare May 24, 2022 16:33

Fall through to check other arguments instead of bailing out on the f…

6ba8da6

…irst error

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 May 26, 2022

This comment has been minimized.

Sign in to view

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

Update tests on aarch64

4332c2f

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 May 27, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2022

bors merged commit 56fd680 into rust-lang:master May 27, 2022

rustbot added this to the 1.63.0 milestone May 27, 2022

RalfJung mentioned this pull request May 28, 2022

Constant gets evaluated despite unsatisfied where clauses #97477

Open

oli-obk deleted the const_typeck branch May 28, 2022 12:32

JohnTitor mentioned this pull request May 28, 2022

ICE: rustdoc on transmute-size-mismatch-before-typeck.rs: size-changing transmute, should have been caught by transmute checking #79494

Closed

jruderman mentioned this pull request Nov 10, 2022

Hang with weird trait requirement #104225

Open

jruderman mentioned this pull request Nov 19, 2022

ICE involving type inference during check_transmute #104609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move various checks to typeck so them failing causes the typeck result to get tainted #96046

Move various checks to typeck so them failing causes the typeck result to get tainted #96046

oli-obk commented Apr 14, 2022 •

edited

Loading

rust-highfive commented Apr 14, 2022

rust-highfive commented Apr 14, 2022

RalfJung Apr 14, 2022

oli-obk Apr 14, 2022

oli-obk Apr 14, 2022

RalfJung Apr 14, 2022 •

edited

Loading

oli-obk Apr 14, 2022

RalfJung May 28, 2022

bors commented Apr 16, 2022

jackh726 commented Apr 22, 2022

RalfJung commented Apr 23, 2022

oli-obk commented Apr 23, 2022

oli-obk commented May 5, 2022

bors commented May 7, 2022

nagisa commented May 10, 2022

bors commented May 17, 2022

cjgillot left a comment

cjgillot May 22, 2022

oli-obk May 22, 2022

cjgillot May 22, 2022

oli-obk May 25, 2022

cjgillot commented May 26, 2022

bors commented May 26, 2022

bors commented May 26, 2022

This comment has been minimized.

bors commented May 26, 2022

oli-obk commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

rust-timer commented May 27, 2022

Move various checks to typeck so them failing causes the typeck result to get tainted #96046

Move various checks to typeck so them failing causes the typeck result to get tainted #96046

Conversation

oli-obk commented Apr 14, 2022 • edited Loading

rust-highfive commented Apr 14, 2022

rust-highfive commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 16, 2022

jackh726 commented Apr 22, 2022

RalfJung commented Apr 23, 2022

oli-obk commented Apr 23, 2022

oli-obk commented May 5, 2022

bors commented May 7, 2022

nagisa commented May 10, 2022

bors commented May 17, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented May 26, 2022

bors commented May 26, 2022

bors commented May 26, 2022

This comment has been minimized.

bors commented May 26, 2022

oli-obk commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

rust-timer commented May 27, 2022

Footnotes

oli-obk commented Apr 14, 2022 •

edited

Loading

RalfJung Apr 14, 2022 •

edited

Loading