Support tuples in offset_of

[AndrewGaspar]

This change enables offset_of! for tuple-types. I'd like this capability for a derive macro. This should not be merged as-is - it has to disable _memoffset__field_check to do this.

Any advice on how I can support tuples in addition to the field checks?

[adamreichold]

Any advice on how I can support tuples in addition to the field checks?

Maybe an additional entry point for tuples/tuple structs will be necessary? At least the deref coercion issue should not apply to numerically indexed fields, should it?

[AndrewGaspar]

Unfortunately it does: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=54d5c03cae645a8def401c2f10a962e2

[AndrewGaspar]

Yeah, thinking about this more, I'm not quite sure how you'd do the check for tuples and tuple structs... maybe not possible?

We don't have this problem in our derive macro, btw - we get the precise structure layout. So no room for a deref coercion to hide.

[adamreichold]

We don't have this problem in our derive macro, btw - we get the precise structure layout. So no room for a deref coercion to hide.

So a separate unsafe entry point for the tuple case?

[adamreichold]

Doing something like

let (_, _, ..): (u32, f32, u8);

seems to work and

let (_, _, ..): Box<(u32, f32, u8)>;

fails.

[RalfJung]

So maybe we can use let (_, ..) to check if this is a tuple type? Is there a way for a macro to distinguish field names from tuple indices to do the appropriate check for each?

[AndrewGaspar]

Ah, yeah, it makes sense why that would work.

[AndrewGaspar]

Maybe ident for named fields vs. literal for tuple fields?

https://doc.rust-lang.org/reference/macros-by-example.html`

[AndrewGaspar]

You'd also need to distinguish between tuple structs and regular structs.

[AndrewGaspar]

Not sure how you could possibly do that...

[AndrewGaspar]

I have done it: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a958f17d6d9d6e290512cff36fb9b7c2

[AndrewGaspar]

Of course these selection modes aren't precise, but they don't have to be if alternative inputs would otherwise fail the field/tuple check.

[Amanieu]

memoffset already supports tuple structs. You can use the existing field check with them. This works because Rust accepts let Foo { 0: _, ..}; when Foo is a tuple struct.

[AndrewGaspar]

Cool! Didn't know that.

[AndrewGaspar]

This adds support for tuples. Weirdly, I couldn't just apply the tuple implementation of raw_field directly - it wouldn't match correctly when used from offset_of for some reason. Unfortunately, this doesn't support type aliases. I think that's probably an unsolvable problem unless we add some sort of offset_of_unchecked or something.

[AndrewGaspar]

Hm... 1.19.0 still broken. Are you guys OK with increasing MSRV?

[adamreichold]

Hm... 1.19.0 still broken. Are you guys OK with increasing MSRV?

I think it would be prefferrable to limit tuple support to newer compiler releases by putting it behind a feature flag and detecting the necessary minimum Rust version in the build script as is already done for e.g. MaybeUninit.

[adamreichold]

@AndrewGaspar Is it sufficient to just skip the test or should we disable the functionality itself on Rust 1.19 and earlier? I guess it will build but fail when you try to use the macro? Personally, I think removing the functionality and documenting the minimum Rust version would be better to avoid people trying to figure out why it does not build for them.

[RalfJung]

I agree about clearly documenting this. What would removing the functionality look like, though? The memoffset crate itself still builds on old rustc and works as before for structs, so all seems fine there.

[adamreichold]

I agree about clearly documenting this. What would removing the functionality look like, though? The memoffset crate itself still builds on old rustc and works as before for structs, so all seems fine there.

I am sorry, I was still under the impression that we need a separate "entry point", i.e. exported macro, but we don't as this is now an internal branch. I am sorry for the confusion.

One thing I do wonder is whether we should remove the internal branch based on #[cfg(tuple_ty)] to avoid making the error messages harder to understand? Especially since :tt could match a lot of things yield more incomprehensible error message further down the macro "pattern matching chain".

[RalfJung]

IMO that's a question of how much effort this is on our side. We already have a lot of branching for different macro variants, so we risk making the code too messy.

[adamreichold]

IMO that's a question of how much effort this is on our side. We already have a lot of branching for different macro variants, so we risk making the code too messy.

Indeed, since 1.19 is pretty old it might not be worth the effort.

[AndrewGaspar]

Let me know if you don't want the crate version to change here - I changed it to reflect the impact of increasing MSRV.

[RalfJung]

Indeed, since 1.19 is pretty old it might not be worth the effort.

Wait, what? No, nothing I said is an argument for increasing MSRV. If just MSRV was the problem there'd be better ways. My understanding was that macros just don't support branching on the token type once an outermost macro made used a particular category?

I am sorry for not being clear enough; I was not talking about bumping the MSRV as a possible alternative here.

[adamreichold]

I am sorry for not being clear enough; I was not talking about bumping the MSRV as a possible alternative here.

Indeed, neither was I. AFAIU, we were just discussing how much effort to put into fully removing the tuple matching "branches" when a compiler older than 1.20 is used with the possible conclusion of not removing them if everything still compiles even though the error messages when trying to use the macro might be gibberish.

So my personal take here is that:

• A single entry point offset_of! that supports both structs and tuples would be nice to able to use it generically. (I am thinking of rsmpi's derive proc macro here. @AndrewGaspar I think a separate entry point would complicate things downstream, right?)
• Since Rust 1.19 is a bit long in the tooth, I think going the this-crates-builds-and-works-fine-but-using-it-with-tuples-will-yield-a-unintelligble-error way would be warranted.

AFAIU, we could do that without bumping MSRV?

[RalfJung]

A single entry point offset_of! that supports both structs and tuples would be nice to able to use it generically. (I am thinking of rsmpi's derive proc macro here. @AndrewGaspar I think a separate entry point would complicate things downstream, right?)

AFAIK this is just not possible though, I thought that was the conclusion of the experiments so far?

[adamreichold]

AFAIK this is just not possible though, I thought that was the conclusion of the experiments so far?

Now I am confused: Isn't commit 397ae745df6d504d79b19f855d157c310d3be868 exactly that, i.e. only the test is deactivated before 1.20, but the generic functionality is in place without a MSRV bump? The only other issue I had on file for that revision was the internal code duplication.

EDIT: Fixed the commit hash I wanted to refer to.

[RalfJung]

The only other issue I had on file for that revision was the internal code duplication.

This demonstrates however that the common entry point does not work when people are using offset_of! in a macro themselves, doesn't it? Just like offset_of! cannot use a "common entry point" version of raw_field!.

[adamreichold]

The only other issue I had on file for that revision was the internal code duplication.

This demonstrates however that the common entry point does not work when people are using offset_of! in a macro themselves, doesn't it? Just like offset_of! cannot use a "common entry point" version of raw_field!.

This is restricted to the case of pattern-based macro_rules!, isn't it? At least @AndrewGaspar and me came here from creating a proc marco which should be unaffected.

However, I do agree that it is a significant limitation. Would having a separate public macro for tuples improve the situation? I guess it would avoid cryptic error messages during pattern matching? Avoiding distinct handling for structs and tuples in the proc macro would be nice but is almost surely not required to be able to implement that proc macro.

Considering all the confusion and the complications of using nested marco_rules!, I guess having a completely separate API with offset_of_tuple and raw_field_tuple that is available only on Rust 1.20+ would be preferable for now as it has a much simpler compatibility story than changing the existing public macros. :-\

[RalfJung]

Considering all the confusion and the complications of using nested marco_rules!, I guess having a completely separate API with offset_of_tuple and raw_field_tuple that is available only on Rust 1.20+ would be preferable for now as it has a much simpler compatibility story than changing the existing public macros. :-\

That is exactly my proposal as well.

Note that even if we were willing to bump the MSRV, we still would have to provide these separate entry points for the benefit of users that have their own macros! Fundamentally what we are doing here is "branching on whether the type is an ident", and it seems like that that can only be done at the top-level macro (or with a proc macro), and our macros here might not be the top-level macros.

[AndrewGaspar]

Wait, what? No, nothing I said is an argument for increasing MSRV. If just MSRV was the problem there'd be better ways. My understanding was that macros just don't support branching on the token type once an outermost macro made used a particular category?

Sorry, had mis-understood this - will try to fix this evening.

[AndrewGaspar]

This is restricted to the case of pattern-based macro_rules!, isn't it? At least @AndrewGaspar and me came here from creating a proc marco which should be unaffected.

In my testing, this occurred with the rsmpi custom derive proc macro, too.

[AndrewGaspar]

I guess having a completely separate API with offset_of_tuple and raw_field_tuple that is available only on Rust 1.20+ would be preferable for now as it has a much simpler compatibility story than changing the existing public macros

OK, I'll get rid of the branching.

[AndrewGaspar]

Should be good now.

[RalfJung]

LGTM, aside from the test. :) Also, please squash the commits together.

Thanks for the patience and endurance while working out the API!

[AndrewGaspar]

Thanks for reviewing! Could you cut a new release with this change?

[RalfJung]

Thanks for reviewing! Could you cut a new release with this change?

@Gilnaa is usually doing the releases. It'd be great if you could experiment with the new macros by directly using the git version in your crate, to let us know if it really works as intended. Once it does, yes we can make a release. :)

[Gilnaa]

Thank you for the contribution! I will make a release in a short while

[AndrewGaspar]

It'd be great if you could experiment with the new macros by directly using the git version in your crate, to let us know if it really works as intended.

Yeah, I've done that. It's working as intended.