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

Support enum variants in offset_of! #114208

Merged
merged 4 commits into from
Nov 1, 2023
Merged

Conversation

GKFX
Copy link
Contributor

@GKFX GKFX commented Jul 29, 2023

This MR implements support for navigating through enum variants in offset_of!, placing the enum variant name in the second argument to offset_of!. The RFC placed it in the first argument, but I think it interacts better with nested field access in the second, as you can then write things like

offset_of!(Type, field.Variant.field)

Alternatively, a syntactic distinction could be made between variants and fields (e.g. field::Variant.field) but I'm not convinced this would be helpful.

RFC 3308 # Enum Support
Tracking Issue #106655.

@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2023

r? @wesleywiser

(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. labels Jul 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2023

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

@GKFX
Copy link
Contributor Author

GKFX commented Jul 29, 2023

cc @thomcc as the RFC author.

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Jul 29, 2023

For MIR we are using (val as Variant).field as syntax for accessing variant fields. I don't think it works well with the offset_of!() syntax though.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 10, 2023

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

@llogiq
Copy link
Contributor

llogiq commented Aug 13, 2023

So once this is merged, the syntax to get the offset of the Some value from an Option would then be offset_of!(Option, Some.0), right? Edit: Yes, this was even in the error description example. Having some docs for this on the offset_of! macro itself would also be nice.

Also I dislike the rule of "last part must be field". I think we should rather specify it as "field must follow enum variant". Otherwise people may try SomeType.SomeVariant.InnerVariant.field and make it really unclear once SomeVariant gets another entry.

@est31
Copy link
Member

est31 commented Aug 13, 2023

I've wondered about possible syntaxes. It seems here we have an exception from the rule of having to import the variant via use before being able to use the enum variant without :: (like, say, in patterns).

Personally I think it's okay to not be required to do offset_of!(Type, Type.field.Enum::Variant.0), just being able to do offset_of!(Type, Type.field.Variant.0), without need for an use Enum::* or use Enum::Variant;. So I think the syntax this PR proposes is fine.

@scottmcm
Copy link
Member

scottmcm commented Aug 13, 2023

Musing on syntax:

Would it be worth having some syntactic distinction between variant parts and field parts? After all, offset_of!(Option, Some) doesn't work.

Spitballing, offset_of!(Option, Some::0) or something?

(Dunno that this would be necessary, though. Just Some.0 is certainly unambiguous, and thus would absolutely work.)

EDIT: Oh, you mentioned this in the OP. Dunno how to choose here.

I started a zulip thread to try to think about things more: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/.60offset_of!.60.20Syntax/near/384555143


I agree with @est31 that we don't need to type-qualify things, though, same as how field access in general isn't type-qualified.

@CAD97
Copy link
Contributor

CAD97 commented Aug 13, 2023

Some discussion w.r.t. syntax on Zulip: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/.60offset_of!.60.20Syntax

My proposed bikeshed color: offset_of!(Type, field, as Enum::Variant, field), with Enum::Variant being a name-resolved item path (like Type, unlike fields). This separates the items/types/variants from fields predictably.

macro_rules! matcher syntax
macro_rules! offset_of {
    ($Container:ty,
        $($($midfields:tt).+, as $Variant:path,)*
        $($fields:tt).+ $(,)?
    ) => { ... };
    // and for top-level enum types:
    ($Container:ty,
        as $TopVariant:path,
        $($($midfields:tt).+, as $Variant:path,)*
        $($fields:tt).+ $(,)?
    ) => { ... };
}

Requiring full item name lookup is tbf unnecessary since we do already know the type contextually, but the same applies to patterns and we don't use the contextual information there either. I'd expect _::Variant to work if/when it works for patterns.

The as could be dropped, but I think it's useful for clarity, as well as it allows spelling the inspection of a top-level enum offset_of!(Option::<T>, as Some, 0) instead of requiring something like perhaps offset_of!(Option::<T>, self, as Some, 0) to have a no-op field path (in order to distinguish between the item/type/variant name and field name(s)).

Whether the as Type segments are additionally permitted to assert the specific type at a point in the path in addition to traversing variants, it could go either way tbh. Maybe give every "type segment" of offset_of! a syntax of $($Container:ty)? $(as $Variant:ident)?,? E.g. offset_of!(Type, field, _ as Variant, field) and offset_of!(Option::<T> as Some, 0).

My opinion changes if the basic syntax changes. If offset_of!(Type.field) were permitted, I'd be fully on board with using offset_of!(Type.field.Variant.field). It's because the initial type is comma separated that I feel later type variants should also be comma separated.

@llogiq
Copy link
Contributor

llogiq commented Aug 14, 2023

I'm personally not a fan of as Variant. While I understand the idea of distinguishing variants from fields (and we don't want to rely on upper- vs. lowercase here, they aren't mandatory after all), as is currently used only in conjunction with a type, not a variant.

So we either use ::Variant (but then should also allow e.g. offset_of!(Option<T>::Some.0) or rely on the fact that the type can only ever have variants or fields but not both at the same level (as per this PR).

@scottmcm
Copy link
Member

For MIR we are using (val as Variant).field as syntax for accessing variant fields. I don't think it works well with the offset_of!() syntax though.

Yeah, that basically only works in MIR because of the RValue::Cast vs ProjectionElem::Downcast distinction.

In anything that would go in surface-like Rust, I don't think we can make that so cleanly. Any use of as would probably end up needing to be a fully-resolved type -- even with enum variant types -- and this wants something more like field access that wouldn't need to fully scope it. (Like this wants to be able to just say Bar because of the context, rather than needing to either use MyType::Bar; or have to say MyType::Bar.)

@llogiq
Copy link
Contributor

llogiq commented Aug 15, 2023

Yeah, doing offset_of!(Type, comma-separated list of field or Variant::field) would probably minimize the implementation effort, be perfectly clear and fit into { $ty, $( $opt_var_field:path ),+ }.

@the8472
Copy link
Member

the8472 commented Aug 15, 2023

The feature is unstable, so can we pick something and then bikeshed the stable syntax separately?

@llogiq
Copy link
Contributor

llogiq commented Aug 15, 2023

I stand corrected, paths don't accept Variant::0. So the easiest way would be offset_of!(Type, Variant.field, field) (using a . instead of :: via an expr parser).

@est31
Copy link
Member

est31 commented Aug 15, 2023

and fit into { $ty, $( $opt_var_field:path ),+ }.

I don't think that path would work here, as we want to support tuple fields, and 0 isn't a valid path (nor is it an ident). edit: expr might work.

@llogiq
Copy link
Contributor

llogiq commented Aug 15, 2023

@est31 I agree. When I checked it in the playground, it didn't work. Trying other options brought up the expr matcher in conjunction with . separating the field from the variant.

The nice thing is that ideally we want a (VariantIdx, FieldIdx) tuple per lookup, and using this structure means we can use the comma delimiter and then just parse (Ident | Number | Ident "." Number), using VariantIdx::FIRST_VARIANT if we don't have a given variant.

With that said, I'd be totally OK with unstably merging any syntax that makes it work and handling the other stuff later.

@scottmcm
Copy link
Member

I agree with @the8472 in #114208 (comment) -- let's get this landed in nightly under a feature gate, and continue the syntax picking conversation on zulip.

I would vote for picking something as simple as possible in the macro that's consistent with offset_of!(Type, field_name) from the RFC, so maybe just allow offset_of!(Type, a, b, c, d), and keep the code you already have here for figuring out based on types which levels are variants. (I'd also be fine with merging the variant and field into one element of the list, but since the code already exists for token-at-a-time, might as well just use that unless a consensus evolves for something else.)

(I have some compiler code comments, but feel free to do or ignore those depending on the advice of the actual compiler reviewers here.)

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GKFX
Copy link
Contributor Author

GKFX commented Nov 1, 2023

@wesleywiser @est31 This is now rebased and review comments are resolved.

@wesleywiser
Copy link
Member

Thanks @GKFX!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 1, 2023

📌 Commit e742f80 has been approved by wesleywiser

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 Nov 1, 2023
@bors
Copy link
Contributor

bors commented Nov 1, 2023

⌛ Testing commit e742f80 with merge 146dafa...

@bors
Copy link
Contributor

bors commented Nov 1, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 146dafa to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 1, 2023
@bors bors merged commit 146dafa into rust-lang:master Nov 1, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 1, 2023
@GKFX GKFX deleted the offset_of_enum branch November 1, 2023 16:18
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (146dafa): 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)
-2.5% [-2.8%, -2.2%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.5% [-2.8%, -2.2%] 2

Cycles

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

Binary size

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

Bootstrap: 635.994s -> 638.45s (0.39%)
Artifact size: 304.49 MiB -> 304.48 MiB (-0.01%)

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 2, 2023
80: Automated pull from upstream `master` r=tshepang a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117498
  * rust-lang/rust#117488
  * rust-lang/rust#117441
  * rust-lang/rust#117373
  * rust-lang/rust#117298
* rust-lang/rust#117029
* rust-lang/rust#117289
* rust-lang/rust#117307
* rust-lang/rust#114208
* rust-lang/rust#117482
  * rust-lang/rust#117475
  * rust-lang/rust#117401
  * rust-lang/rust#117397
  * rust-lang/rust#115626
* rust-lang/rust#117436
* rust-lang/rust#115356
* rust-lang/rust#117422
* rust-lang/rust#116692



Co-authored-by: David CARLIER <devnexen@gmail.com>
Co-authored-by: Taiki Endo <te316e89@gmail.com>
Co-authored-by: ltdk <usr@ltdk.xyz>
Co-authored-by: Ryan Mehri <ryan.mehri1@gmail.com>
zhassan-aws added a commit to model-checking/kani that referenced this pull request Nov 6, 2023
Update to the latest Rust toolchain (2023-11-06).

The relevant changes are:
- rust-lang/rust#117507: this required changing
the import of `Span` from `rustc_span::source_map::Span` to
`rustc_span::Span`.
- rust-lang/rust#114208: this changed the data
field for the `OffsetOf` variant of `NullOp` from `List<FieldIdx>` to
`List<(VariantIdx, FieldIdx)>`, which required updating the relevant
code in `rvalue.rs`.
- rust-lang/rust#115626: the unchecked shift
operators have been separated from the `unchecked_math` feature, so this
required changing the feature annotation in
`tests/ui/should-panic-attribute/unexpected-failures/test.rs`
- Some rustc change (not sure which one) result in a line in
`tests/coverage/unreachable/variant/main.rs` getting optimized out. To
maintain what this test is testing, I changed the `match` to make it a
bit less-prone to optimization.
- A change in `cargo` (rust-lang/cargo#12779)
resulted in an update to Kani's workspace `Cargo.toml` when `cargo add`
is executed inside `tests/script-based-pre/build-cache-bin`. This is
apparently intended behavior, so I had to make the `exclude` in the
`Cargo.toml` more specific to make sure this doesn't happen (I tried
using a glob, but that didn't work, apparently because of
rust-lang/cargo#6009.

Resolves #2848 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2023
…trochenkov

Remove option_payload_ptr; redundant to offset_of

The `option_payload_ptr` intrinsic is no longer required as `offset_of` supports traversing enums (rust-lang#114208). This PR removes it in order to dogfood offset_of (as suggested at rust-lang#106655 (comment)). However, it will not build until those changes reach beta (which I think is within the next 8 days?) so I've opened it as a draft.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.