Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't skip inference for type in offset_of! #111696

Merged
merged 1 commit into from
May 21, 2023

Conversation

lukas-code
Copy link
Member

@lukas-code lukas-code commented May 17, 2023

Fixes #111678 by no longer skipping inference on the type in offset_of!. Simply erasing the regions the during writeback isn't enough and can cause ICEs. A test case for this is included.

This reverts #111661, because it becomes redundant, since inference already erases the regions.

@rustbot
Copy link
Collaborator

rustbot commented May 17, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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. F-offset_of `#![feature(offset_of)]` labels May 17, 2023
@lukas-code
Copy link
Member Author

@rustbot blocked on #111665

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2023
@@ -3077,7 +3077,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fields: &[Ident],
expr: &'tcx hir::Expr<'tcx>,
) -> Ty<'tcx> {
let container = self.to_ty(container).normalized;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be erasing regions this early in typeck... Why not erase later, like during writeback?

Copy link
Member Author

@lukas-code lukas-code May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just replace this assert with the erase? Nevermind, this also checks for infer types and not just infer regions.

if cfg!(debug_assertions) && container.has_infer() {
span_bug!(
hir_id.to_span(self.fcx.tcx),
"writeback: `{:?}` has inference variables",
container
);
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we shouldn't be skipping inference here in the first place, because this leads to another error-to-ICE for this code:

#![feature(offset_of)]

struct Foo<T> {
    x: T,
}

fn main() {
    let _ = core::mem::offset_of!(Foo<_>, x);
}

offset_of!(Delta<Alpha>, z); //~ ERROR the size for values of type
offset_of!(Delta<Extern>, z); //~ ERROR the size for values of type
offset_of!(Delta<dyn Trait>, z); //~ ERROR the size for values of type
offset_of!(Delta<Box<dyn Trait>>, z); // compiles fine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR this might not actually ensure that the line compiles fine. I mean it does compile fine, but there is deduplication active for the "unsized" error, which means that if some type causes the error multiple times inside the same function, the error is only emitted once. That's why I moved the delta related stuff into its own function.

So maybe the best would be to move the Box<dyn Trait> stuff into its own function.

@lukas-code lukas-code force-pushed the offset-of-erase-regions-harder branch 3 times, most recently from dd2f636 to 6be4dc9 Compare May 18, 2023 13:18
@lukas-code lukas-code changed the title Erase regions of type in offset_of!, take 2 don't skip inference for type in offset_of! May 18, 2023
@compiler-errors
Copy link
Member

r=me when that dependency lands

r? @compiler-errors @bors delegate+

@rustbot rustbot assigned compiler-errors and unassigned TaKO8Ki May 18, 2023
@compiler-errors
Copy link
Member

bors wake up

@bors delegate+

@lukas-code lukas-code force-pushed the offset-of-erase-regions-harder branch from 6be4dc9 to 7cdb23b Compare May 20, 2023 13:21
@lukas-code
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented May 20, 2023

📌 Commit 7cdb23b has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 20, 2023
@bors
Copy link
Contributor

bors commented May 21, 2023

⌛ Testing commit 7cdb23b with merge a11235d...

@bors
Copy link
Contributor

bors commented May 21, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 21, 2023
@bors bors merged commit a11235d into rust-lang:master May 21, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 21, 2023
@lukas-code lukas-code deleted the offset-of-erase-regions-harder branch May 21, 2023 08:21
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a11235d): 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

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)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) - - 0

Cycles

Results

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.4% [-3.6%, -3.1%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 643.686s -> 644.164s (0.07%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-offset_of `#![feature(offset_of)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: has inference variables for offset_of with Box<dyn Trait>
7 participants