Skip to content

Add more tests for the offset_of macro #111665

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 20, 2023
Merged

Conversation

est31
Copy link
Member

@est31 est31 commented May 17, 2023

Implements what I suggested in the tracking issue, plus some further improvements:

  • ensuring that offset_of!(Self, ...) works iff inside an impl block
  • ensuring that the output type is usize and doesn't coerce. this can be changed in the future, but if it is done, it should be a conscious decision
  • improving the privacy checking test
  • ensuring that generics don't let you escape the unsized check

r? @WaffleLapkin

@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. labels May 17, 2023
@est31 est31 force-pushed the offset_of_tests branch from ed84277 to d4be511 Compare May 17, 2023 03:53
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a test for privacy with Self? There recently were some issues with it, so a test that Self doesn't allow to refer to private fields would be nice.

Something like

mod module {
    pub struct Named { v: u8 }
}

trait Trait {
    fn f() -> usize {}
}

impl Trait for module::Named {
    fn f() -> usize {
        offset_of!(Self, v) //~ error: field is private or whatever
    }
}

(and similarly for a tuple struct)

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2023
@est31 est31 force-pushed the offset_of_tests branch from d4be511 to d8ac2af Compare May 17, 2023 09:01
@est31
Copy link
Member Author

est31 commented May 17, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2023
@est31 est31 force-pushed the offset_of_tests branch 2 times, most recently from 480d06c to f100c3c Compare May 17, 2023 09:45
@est31 est31 changed the title Add more tests for offset_of macro Add more tests for the offset_of macro May 17, 2023
@rust-log-analyzer

This comment has been minimized.

@est31
Copy link
Member Author

est31 commented May 17, 2023

error: internal compiler error: compiler/rustc_hir_typeck/src/writeback.rs:697:17: writeback: `Delta<dyn Trait>` has inference variables
  --> fake-test-src-base/offset-of/offset-of-dst-field.rs:44:5
   |
LL |     offset_of!(Delta<dyn Trait>, z); //~ ERROR the size for values of type
   |
   = note: this error: internal compiler error originates in the macro `offset_of` (in Nightly builds, run with -Z macro-backtrace for more info)

Hmm this is interesting. It's also interesting that I am not getting this ICE locally. Are there some checks running in CI that aren't running locally?

@WaffleLapkin
Copy link
Member

@est31 do you have debug assertions enabled? this is the check:

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

@est31 est31 force-pushed the offset_of_tests branch from f100c3c to 7e14b4f Compare May 17, 2023 10:43
@est31
Copy link
Member Author

est31 commented May 17, 2023

@WaffleLapkin I haven't. So I guess this is a bug then if you enable debug assertions in the compiler. Good that we are doing these tests 😆

@rust-log-analyzer

This comment has been minimized.

@est31
Copy link
Member Author

est31 commented May 17, 2023

Alright it still ICEs after having rebased this PR onto #111661 , so it means that one didn't fix it. I also can reproduce it locally if I enable debug assertions. Will remove the particular test from this PR and then file a separate issue.

@est31
Copy link
Member Author

est31 commented May 17, 2023

I've changed it to use Extern instead of dyn Trait. If you use [u8] then the error is deduplicated with the one about Alpha. I've also filed #111678.

Should be ready for review.

}
impl super::n::T {
fn v_offs_self() -> usize {
offset_of!(Self, v) //~ ERROR field `v` of struct
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
offset_of!(Self, v) //~ ERROR field `v` of struct
offset_of!(Self, v) //~ ERROR field `v` of struct `T` is private

fn main() {
offset_of!(Alpha, z); //~ ERROR the size for values of type
offset_of!(Beta, z); //~ ERROR the size for values of type
offset_of!(Gamma, z); //~ ERROR the size for values of type
}

fn delta() {
offset_of!(Delta<u8>, 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.

Maybe we should move fine-compiling parts to separate files, to ensure they really compile? (besides, I think there already are test that have the same compiling code, so maybe this and generic_with_sized_type are just redundant)

Copy link
Member Author

@est31 est31 May 18, 2023

Choose a reason for hiding this comment

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

The only other tests with generics I could find were the one with Self that are added by this PR as well as the test added in #111661. edit: nvm, there are other tests, e.g. tests/ui/lint/dead-code/offset-of-correct-param-env.rs or tests/ui/lint/dead-code/offset-of.rs.

I will move the passing tests though, at least the ones in this file.

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2023
@est31 est31 force-pushed the offset_of_tests branch from dd7c128 to 412105d Compare May 18, 2023 11:14
* ensuring that offset_of!(Self, ...) works iff inside an impl block
* ensuring that the output type is usize and doesn't coerce. this can be
  changed in the future, but if it is done, it should be a conscious descision
* improving the privacy checking test
* ensuring that generics don't let you escape the unsized check
@est31 est31 force-pushed the offset_of_tests branch from 412105d to 30c0e4e Compare May 18, 2023 11:16
@est31
Copy link
Member Author

est31 commented May 18, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2023
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

Thanks!

@WaffleLapkin
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 18, 2023

📌 Commit 30c0e4e has been approved by WaffleLapkin

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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 18, 2023
Add more tests for the offset_of macro

Implements what I [suggested in the tracking issue](rust-lang#106655 (comment)), plus some further improvements:

* ensuring that offset_of!(Self, ...) works iff inside an impl block
* ensuring that the output type is usize and doesn't coerce. this can be changed in the future, but if it is done, it should be a conscious decision
* improving the privacy checking test
* ensuring that generics don't let you escape the unsized check

r? `@WaffleLapkin`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 19, 2023
Add more tests for the offset_of macro

Implements what I [suggested in the tracking issue](rust-lang#106655 (comment)), plus some further improvements:

* ensuring that offset_of!(Self, ...) works iff inside an impl block
* ensuring that the output type is usize and doesn't coerce. this can be changed in the future, but if it is done, it should be a conscious decision
* improving the privacy checking test
* ensuring that generics don't let you escape the unsized check

r? ``@WaffleLapkin``
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 19, 2023
Add more tests for the offset_of macro

Implements what I [suggested in the tracking issue](rust-lang#106655 (comment)), plus some further improvements:

* ensuring that offset_of!(Self, ...) works iff inside an impl block
* ensuring that the output type is usize and doesn't coerce. this can be changed in the future, but if it is done, it should be a conscious decision
* improving the privacy checking test
* ensuring that generics don't let you escape the unsized check

r? ```@WaffleLapkin```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 19, 2023
Add more tests for the offset_of macro

Implements what I [suggested in the tracking issue](rust-lang#106655 (comment)), plus some further improvements:

* ensuring that offset_of!(Self, ...) works iff inside an impl block
* ensuring that the output type is usize and doesn't coerce. this can be changed in the future, but if it is done, it should be a conscious decision
* improving the privacy checking test
* ensuring that generics don't let you escape the unsized check

r? ````@WaffleLapkin````
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2023
Rollup of 10 pull requests

Successful merges:

 - rust-lang#111491 (Dont check `must_use` on nested `impl Future` from fn)
 - rust-lang#111606 (very minor cleanups)
 - rust-lang#111619 (Add timings for MIR passes to profiling report)
 - rust-lang#111652 (Better diagnostic for `use Self::..`)
 - rust-lang#111665 (Add more tests for the offset_of macro)
 - rust-lang#111708 (Give a more useful location for where a span_bug was delayed)
 - rust-lang#111715 (Fix doc comment for `ConstParamTy` derive)
 - rust-lang#111723 (style: do not overwrite obligations)
 - rust-lang#111743 (Improve cgu merging debug output)
 - rust-lang#111762 (fix: emit error when fragment is `MethodReceiverExpr` and items is empty)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e892e32 into rust-lang:master May 20, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 20, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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.

5 participants