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

RFC: Keyword unreservations (pure, sizeof, alignof, offsetof) #2421

Merged
merged 10 commits into from May 27, 2018

Conversation

@Centril
Copy link
Contributor

Centril commented Apr 26, 2018

πŸ–ΌοΈ Rendered

⏭ Tracking issue

πŸ“ Summary

We unreserve:

  • pure
  • unsized
  • sizeof
  • alignof
  • offsetof
  • priv

🎡 Note

If you have a particular objection and use case for some keyword this RFC wants to unreserve, please speak up and leave a comment.

*"go look at `std::mem::size_of` instead"*. However, we believe it is better
to allow users the freedom to use these keywords instead.

## Rationale for `priv`

This comment has been minimized.

@Centril

Centril Apr 26, 2018

Author Contributor

cc @withoutboats - You wanted to keep this?

This comment has been minimized.

@petrochenkov

petrochenkov Apr 26, 2018

Contributor

I wanted to keep this!

This comment has been minimized.

@Centril

Centril Apr 26, 2018

Author Contributor

@petrochenkov Could you elaborate a lil bit on possible use cases you foresee?

This comment has been minimized.

@joshtriplett

joshtriplett Apr 26, 2018

Member

Considering how much activity we have around module proposals, including visibility, I think holding onto this one seems reasonable for now.

This comment has been minimized.

@Centril

Centril Apr 26, 2018

Author Contributor

Aight. As we discussed on the lang team meeting; I'll scratch this one of the list in this round of unreservations. We can always revisit later.

In both 1. and 2., `pure` can be contextual.
We also don't think that the drawbacks are significant for `pure`.

## Rationale for `unsize`

This comment has been minimized.

@Centril

Centril Apr 26, 2018

Author Contributor

cc @eddyb - Double checking: This is fine?

This comment has been minimized.

@eddyb

This comment has been minimized.

@mikeyhew

mikeyhew Apr 26, 2018

unsized may be useful for custom DST, so I think it's worth holding onto for now

This comment has been minimized.

@Centril

Centril Apr 26, 2018

Author Contributor

@mikeyhew Can you elaborate a bit? (possibly with a code example?)

This comment has been minimized.

@kennytm

kennytm Apr 26, 2018

Member

I'd use dyn instead of unsize for DST.

This comment has been minimized.

@Centril

Centril Apr 27, 2018

Author Contributor

@mikeyhew No specific use case; your comment helps me refine the RFC tho :) But @kennytm's idea seems nice? If this discussion doesn't develop I'll strike unsized in a bit with your motivation.

This comment has been minimized.

@eddyb

eddyb Apr 27, 2018

Member

I'd use dyn for dynamic existentials, not dynamic sizes, e.g. you could imagine [T] expanding to dyn<n: usize> [T; n] and dyn Trait to dyn<T: Trait> T.

This comment has been minimized.

@Centril

Centril Apr 28, 2018

Author Contributor

@eddyb But what about let vla = [T; dyn size];?

This comment has been minimized.

@Centril

Centril Apr 28, 2018

Author Contributor

I've now scratched unsized given @mikeyhew's motivation and that unreserving it violates:

[..] we are sure that we have no intention of using the keyword in the future [..]

This comment has been minimized.

@eddyb

eddyb Apr 30, 2018

Member

@Centril That's not a size, that's a length. Dynamic existential shorthands (when each bound type/value name is used only once, in an unambiguous way) could be dyn Trait and [T; dyn usize] (or just dyn, since the type can be inferred or specified elsewhere).

@Centril Centril referenced this pull request Apr 26, 2018

Merged

RFC: Unreserve `proc` #2420

@nikomatsakis

This comment was marked as resolved.

Copy link
Contributor

nikomatsakis commented Apr 26, 2018

@withoutboats definitely did want to keep priv, which seems fine.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Apr 26, 2018

I don't want to keep offsetof as a keyword, but I do want us to have a macro offsetof! (or offset_of!) someday. If anyone is interested in an RFC for that, I'd be happy to work with them.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 26, 2018

@joshtriplett I dislike having macros that expand to builtins that cannot be written with other syntax, so I'm curious which approach you're looking for. I guess using an union and taking addresses into it via let $StructName { ref $field_name, .. } = uninit.data; could work without being builtin?

EDIT: I've marked the remaining offsetof discussion as offtopic as per #2421 (comment) - there's a link to an internals post in #2421 (comment) if you wish to continue the discussion there.

@joshtriplett

This comment was marked as off-topic.

Copy link
Member

joshtriplett commented Apr 26, 2018

I dislike having macros that expand to builtins that cannot be written with other syntax

I hope that proc macros one day become powerful enough to implement this. :)

Until that time, I don't have a problem with builtin macros that can't today be written with other syntax.

@Centril Centril removed the I-nominated label Apr 26, 2018

@eddyb

This comment was marked as off-topic.

Copy link
Member

eddyb commented Apr 26, 2018

I hope that proc macros one day become powerful enough to implement this. :)

I don't see how that could possibly be the case, as "offset of" requires layout, which is strictly staged after syntax expansion. OTOH, this should work, if we put it in libcore:

macro_rules! offset_of {
    ($Struct:path, $field:ident) => ({
        // Using a separate function to minimize unhygienic hazards
        // (e.g. unsafety of #[repr(packed)] field borrows).
        // Uncomment `const` when `const fn`s can juggle pointers.
        /*const*/ fn offset() -> usize {
            let u = $crate::mem::MaybeUninit::<$Struct>::uninit();
            // Use pattern-matching to avoid accidentally going through Deref.
            let &$Struct { $field: ref f, .. } = unsafe { &*u.as_ptr() };
            let o = (f as *const _ as usize).wrapping_sub(&u as *const _ as usize);
            // Triple check that we are within `u` still.
            assert!((0..=$crate::mem::size_of_val(&u)).contains(&o));
            o
        }
        offset()
    })
}
@joshtriplett

This comment was marked as off-topic.

Copy link
Member

joshtriplett commented Apr 26, 2018

@eddyb
First of all, I think we should take that to somewhere else; is there an issue somewhere for implementation of offset_of that we could discuss that on? For this thread, I think it suffices to say that I don't think we need offsetof as a keyword.

Two other questions: 1) Will that optimize down to a compile-time constant? 2) Does that still hold true if you use mem::uninitialized() (sidestepping the whole ongoing union semantics discussion)? (Also, cc @RalfJung)

## Rationale for `sizeof`, `alignof`, and `offsetof`

We already have [`std::mem::size_of`](https://doc.rust-lang.org/nightly/std/mem/fn.size_of.html) and similar which
are `const fn`s or can be.

This comment has been minimized.

@rkruppe

rkruppe Apr 26, 2018

Member

offsetof can't be a const fn, at least not without some sort of static reflection being added to the language – which is a huge feature and AFAIK nothing specific in this direction has ever been proposed. I agree it could be unreserved (and implemented as a macro, as @eddyb showed), but this rationale here only applies to sizeof and alignof, not offsetof.

This comment has been minimized.

@eddyb

eddyb Apr 26, 2018

Member

Note that I would prefer if the macro generated a const fn, so you could use it anywhere a constant value was expected. Most of it will soon become possible, with the exception of the assert, although that's mostly a sanity check and shouldn't be required for soundness.

This comment has been minimized.

@joshtriplett

joshtriplett Apr 26, 2018

Member

@rkruppe In theory we could know the delta between fields of the same structure (or from the start of the structure to one field) statically at compile time, without needing full static reflection. And many uses of offsetof in C want it in a compile-time-constant context..

This comment has been minimized.

@rkruppe

rkruppe Apr 26, 2018

Member

@joshtriplett What you describe sounds more like a special keyword than a const fn! I don't see a nice, generic (i.e., which enables more than just const fn offsetof) extension to current Rust that allows one to write const fn offsetof in a library. Well, nothing that's smaller/simpler than the beginnings of a reflection system. Once you start introducing things like "reifying a field into a value and passing it to a function" (which seems necessary to even define the interface of const fn offsetof, let alone its implementation) you're already in reflection land.

(I agree that offset calculations need to work in constant contexts, but as @eddyb said, the macro is perfectly compatible with that!)

This comment has been minimized.

@Centril

Centril Apr 26, 2018

Author Contributor

@rkruppe This is true; I'll clarify.

@eddyb

This comment was marked as off-topic.

Copy link
Member

eddyb commented Apr 26, 2018

@joshtriplett mem::uninitialized() can cause UB so it's useless in the generic case, hence unions.

@joshtriplett

This comment was marked as off-topic.

Copy link
Member

joshtriplett commented Apr 26, 2018

@eddyb Only with ! AFAIK, and unions and many other kinds of unsafe code can cause UB too. :)

In this case, I just wanted to know if it affected code generation. And my concern was that that particular union approach presumes a certain semantics of unions, which is still under active discussion.

Is there already a proposal for offset_of that we could take this discussion to? If not, should we take it to an internals thread?

@eddyb

This comment was marked as off-topic.

Copy link
Member

eddyb commented Apr 26, 2018

@joshtriplett No, union will not cause the same kind of UB if the type happens to be uninhabited.
With mem::uninitialized, enum Void {} struct Foo { bar: Void } offset_of!(Foo, bar) is UB.

EDIT: both mem::uninitialized and MaybeUninit produce optimal LLVM IR.
I was expecting undef in the former case, but the UB present there didn't manifest itself...

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Apr 26, 2018

Update: I've removed priv from the list of unreservations and moved it to the "future work" section.

@Centril

This comment was marked as resolved.

Copy link
Contributor Author

Centril commented Apr 26, 2018

@joshtriplett Yeah; an internals thread would be best :)

@Centril Centril changed the title RFC: Keyword unreservations (pure, unsize, sizeof, alignof, offsetof, priv) RFC: Keyword unreservations (pure, sizeof, alignof, offsetof) Apr 30, 2018

@Amanieu

This comment was marked as off-topic.

Copy link
Contributor

Amanieu commented May 2, 2018

I think that we should aim for proper offset_of support in the compiler. The problem with the macro-based approach (which I am using in intrusive-collections) is that it doesn't return a constant value from the point of view of the language. This makes it unusable for many cases, such as global constants or passing an immediate operand to inline asm.

Returning a proper constant is not possible with a normal macro implementation, however we could make it a "magic" macro like asm! which generates a special OffsetOf node in MIR.

Show resolved Hide resolved text/0000-unreservations-2018.md Outdated
@eddyb

This comment was marked as off-topic.

Copy link
Member

eddyb commented May 2, 2018

Returning a proper constant is not possible with a normal macro implementation

You mean because stack addresses are converted to integers? IMO this shouldn't be a problem because miri could understand that (alloc + x) - (alloc + y) is x - y. cc @oli-obk @RalfJung

EDIT: For the record, I've had /*const*/ fn in #2421 (comment) from the start, so the value would be guaranteed to be constant, once we support that code. Also, if we put such a macro in libstd, users of it would not need #[feature(const_fn)] to be able to use it, even in a constant context.

@RalfJung

This comment was marked as off-topic.

Copy link
Member

RalfJung commented May 2, 2018

As long as the arithmetic is all in-bounds and there are no overflows and so on, this should work, yes. In fact this should already be implemented in miri.

@oli-obk

This comment was marked as off-topic.

Copy link
Contributor

oli-obk commented May 2, 2018

Yea you should be able to write offset_of on nightly/beta by using int to ptr casts

@eddyb

This comment was marked as off-topic.

Copy link
Member

eddyb commented May 2, 2018

@oli-obk AFAIK that's UB in the general case, #2421 (comment) uses a local union.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented May 2, 2018

Could we discuss the implementation detail / UB-ness of offset_of! somewhere else? This is off-topic for this RFC about whether to keep reserving the offsetof keyword.

As mentioned in #2421 (comment) and #2421 (comment), even if it turns out miri can't obtain the field offset, offset_of! can still be a built-in macro like asm!, and it isn't blocking freeing offsetof.

@oli-obk

This comment was marked as off-topic.

Copy link
Contributor

oli-obk commented May 2, 2018

Hmm. OK. maybe we need rust-lang/rust#49172 first. Then again, if you only do it in a constant, is it really UB? :D miri has very clear semantics for this.
Whoops sorry, mobile didn't update so I didn't see the post above

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented May 2, 2018

I made a topic for it: https://internals.rust-lang.org/t/discussion-on-offset-of/7440

Just to double check: Everyone is fine with unreserving offsetof?
Please mark this comment with πŸ‘ if yes, or πŸ‘Ž if not, and then explain why (if πŸ‘Ž).

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented May 3, 2018

Based on discussion in the lang team meeting, we're ready to unreserve the remaining keywords in this RFC.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented May 3, 2018

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented May 17, 2018

πŸ”” This is now entering its final comment period, as per the review above. πŸ””

Show resolved Hide resolved text/0000-unreservations-2018.md Outdated
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented May 27, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril Centril merged commit 7a2ce51 into rust-lang:master May 27, 2018

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented May 27, 2018

Huzzah! This RFC is merged!

Tracking issue: rust-lang/rust#51115

@Centril Centril deleted the Centril:rfc/unreservations-2018 branch May 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment