Making the inner type public seems fine, but please don't deprecate as_raw/from_raw.

@aloucks just like my PR description above you will have to justify your comment otherwise there's no way for me to even consider honouring your request.

In other words: why?

The newtype `.0` member is now directly available and preferred

What makes this preferred? It wouldn't be my preference. I guess this is subjective, but requiring newtype syntax in a public API is unappealing to me. as_raw hints at what you're getting and is more descriptive than simply .0.

Removing the methods and making the inner component public is a step in the direction towards making them a simple typedef. I think we can all agree is not the right choice. Why do we want to move in this direction?

Furthermore, with this change, the way you construct a bitflag type and a handle type from a raw value now uses different API, where as without this PR both use a common from_raw/as_raw API. It's also possible to break a bunch of code for no real gain.

My feeling is that it's a pretty minor cosmetic difference to be spending buildtime on, but I also don't feel too strongly either way.

Do you actually have a bunch of code calling these methods? I'd have thought conversions to/from raw form would tend to be isolated.

The newtype `.0` member is now directly available and preferred

What makes this preferred? It wouldn't be my preference. I guess this is subjective, but requiring newtype syntax in a public API is unappealing to me. as_raw hints at what you're getting and is more descriptive than simply .0.

I am preferring it because it looks more natural to write pub const RGBA: Self = Self(Self::R.0 | Self::G.0 | Self::B.0 | Self::A.0); than doing the same with from/as_raw: #549 (comment). We can re-evaluate if public newtypes are discouraged globally across crates.

By the way I can't help but notice that I've been writing it "wrongly" as to_raw instead of Ash's as_raw all along. The Rust naming conventions are pretty clear that Ash's as_ prefix should only be used for reference->reference conversion, whereas these enums and bitflags are actually owned copy types. They should use to_ which is probably something we should fix regardless as yet another breaking change.

Removing the methods and making the inner component public is a step in the direction towards making them a simple typedef. I think we can all agree is not the right choice. Why do we want to move in this direction?

Not at all. A newtype is still fundamentally different from a typedef no matter the inner value being pub. With typedefs you can easily and accidentally pass any enum or bitflag constant into any other, whereas the newtype still requires explicit construction of said newtype. We can only protect against users accidentally passing the wrong type, not against explicitly constructing the right type from the wrong constant value.

Agreed though, dropping the raw here in favour of direct newtype construction and unwrapping does make that less explicit.

Furthermore, with this change, the way you construct a bitflag type and a handle type from a raw value now uses different API, where as without this PR both use a common from_raw/as_raw API. It's also possible to break a bunch of code for no real gain.

Bitflags are still different from handles, especially the dispatchable one that performs a cast between a raw u64 and holding a *mut u8 internally.

Either way this PR was really only sparked as a means to accommodate the const case from #549: as @Ralith mentioned uses of from/as_raw should be far if nonexistent in most programs, the newtype can be retained everywhere in the API thanks to all the ops:: implementations.
Together with an almost negligible compile-time improvement of 0.1s this is perhaps the wrong decision to make? (Again: I'm willing to break as_raw and name it to_raw properly.) Do we instead prefer the const construction functions originally found in #549 to cater to trivial ops:: not being const and having no alternative?

They should use to_ which is probably something we should fix regardless as yet another breaking change.

That's not entirely correct either. The to_ convention is to convey that it is an expensive operation, where as the as_ convection is to in indicate that it is free. The most correct convention in this case would be into_ .

Conversions prefixed as_ and into_ typically decrease abstraction, either exposing a view into the underlying representation (as) or deconstructing data into its underlying representation (into). Conversions prefixed to_, on the other hand, typically stay at the same level of abstraction but do some work to change from one representation to another.

https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

Prefix Cost Ownership
as_ Free borrowed -> borrowed
to_ Expensive borrowed -> borrowed, borrowed -> owned (non-Copy types), owned -> owned (Copy types)
into_ Variable owned -> owned (non-Copy types)

These are very much Copy types, though. Unfortunately the guidelines don't seem to cover this case.

@aloucks Fair enough, I didn't read my own link 🤦

Not sure what else it should be then though, if everything in that table is wrong... For the bitflags we could/should definitely match bitflags! fn bits() and fn from_bits_unchecked() (can't do fn from_bits_truncate() unless we introduce the machinery to produce a proper fn all() this time). It seems some suggest to use the name of the field as if it were a getter (without the get_ prefix of course) but there's no such name in newtypes.

Anyway, should we leave this for a future discussion/PR and solely focus on whether to keep these functions in the first place / make the inner type public, per the initial intent of the PR? At this point I'm feeling more and more indifferent to it all.

Works for me. Decisions are always easier one at a time.

Honestly I have no strong preference here. I'd keep the as_raw functions though, and I think as_ is the correct pattern here because it is a free conversion.

Having both as_raw and the inner field public is a bit weird though.

Honestly I have no strong preference here. I'd keep the as_raw functions though, and I think as_ is the correct pattern here because it is a free conversion.

as_ seems to match best, it's not fully correct but with no better alternative it's not worth the API churn to change to anything else.

Having both as_raw and the inner field public is a bit weird though.

Of course they're marked deprecated now, but we can also remove them immediately as the next breaking release is imminent anyway.

On that note, is this still something we want in? Repeating what I said before: this PR is mainly to smoothen the const side of things since Ash itself has all the Ops implemented for non-const and regular API usage doesn't require these functions anyway.

Given that IMO const use is niche and already possible, and the compile time impact is very marginal, I don't feel like this is a big win.

Closing this as being unclear and not worth the trouble.

I don't think we should reopen #549 then either. While using type-safe functions is preferred this is probably not worth the trouble for a few const cases. If anything Rust should find a solution for const ops upstream.