Should sub-protocols (for i.e. MTLAllocation) turn on the parent feature?

[MarijnS95]

Hi!

We recently patched our objc2-metal crate to the latest version in this repository. In 6442cbe the new MTLAllocation protocol and feature flag were added. The effect is that, when we merely turn on features like MTLHeap and MTLResource, the code now compiles with "imports not found" for the respective types, because they're guarded behind MTLHeap and MTLAllocation.

While this is fixed by also turning on the MTLAllocation feature, doesn't it make more sense to have MTLHeap = ["MTLAllocation"] so that the feature is automatically enabled, for user convenience? Or if not, is there a thought-out reason why this should not be done?

Specifically, it took some digging through the new source code to understand all the places where these protocols were guarded, before realizing that it was now hidden by the new MTLAllocation feature.

[MarijnS95]

An unrelated side-issue to this is our "workaround" in Traverse-Research/gpu-allocator@6a2f521 (and we later pushed a different workaround: Traverse-Research/gpu-allocator@95b79c6). We have a codebase that uses gpu-allocator, where we patch objc2-metal and friends. We enable the new MTLAllocation feature there, and expect objc2-metal to be compiled just once for gpu-allocator and our internal use of the crate, yet gpu-allocator still fails to compile, as if rustc is able to track the origin of who requested a feature flag?

error[E0432]: unresolved imports `objc2_metal::MTLAccelerationStructure`, `objc2_metal::MTLBuffer`, `objc2_metal::MTLHeap`, `objc2_metal::MTLResource`, `objc2_metal::MTLTexture`
  --> .cargo/git/checkouts/gpu-allocator-a4a57137f341fd22/222b3d8/src/metal/mod.rs:12:5
   |
12 |     MTLAccelerationStructure, MTLBuffer, MTLCPUCacheMode, MTLDevice, MTLHeap, MTLHeapDescriptor,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^                              ^^^^^^^ no `MTLHeap` in the root
   |     |                         |
   |     |                         no `MTLBuffer` in the root
   |     no `MTLAccelerationStructure` in the root
   |     help: a similar name exists in the module: `MTLAccelerationStructureUsage`
13 |     MTLHeapType, MTLResource, MTLResourceOptions, MTLStorageMode, MTLTexture, MTLTextureDescriptor,
   |                  ^^^^^^^^^^^ no `MTLResource` in the root         ^^^^^^^^^^ no `MTLTexture` in the root

[madsmtm]

is there a thought-out reason why this should not be done?

Actually, header-translator originally did this for classes (e.g. enabling the NSView feature also enabled the NSResponder feature), but I've since reworked features to be based on the filename that something is declared in; and then it really doesn't make sense to do this sort of auto-enabling.

As a bit of a contrived example, NSViewFullScreenModeOptionKey is also cfg-gated behind the NSView feature (because it's also declared in AppKit/NSView.h), but can and should be usable without also enabling NSResponder.

Similar logic applies to protocols, if we auto-enabled the features for sub-protocols, other items defined in the same files would require those features as well even though they don't need them.

Specifically, it took some digging through the new source code to understand all the places where these protocols were guarded, before realizing that it was now hidden by the new MTLAllocation feature.

Yeah, I get that that must have been confusing. It is noted in the changelog, but probably not prominently enough, I understand that it is long and perhaps overly verbose.

I think it may be mitigated once v0.3 is actually released? Since then the online docs would state fairly prominently "Available on crate features MTLResource and MTLAllocation only".

as if rustc is able to track the origin of who requested a feature flag?

Don't know much about that, Cargo's crate and feature unification is weird.

[MarijnS95]

In hindsight, let's not call this "auto-enabling" but rather a regular dependency chain. If I request MTLResource to be enabled via a feature flag, I expect it to configure anything else that it needs.

---

I had never expected this to be filename-based which makes things extra complicated, when there appears to be a mostly 1:1 mapping between filenames and the main type that it defines.

As a bit of a contrived example, NSViewFullScreenModeOptionKey is also cfg-gated behind the NSView feature (because it's also declared in AppKit/NSView.h), but can and should be usable without also enabling NSResponder.

Is this debunked by stating that the type is only used in functions on NSView? https://developer.apple.com/documentation/appkit/nsviewfullscreenmodeoptionkey

If NSResponder is required to have NSView, then having NSViewFullScreenModeOptionKey without NSView and intrinsically without NSresponder seems useless?

---

We previously discussed that this "parent trait" relation is purely for the type system and only matters when calling a function on the available protocol - which is a requirement for retroactively introducing a "parent protocol" in the hierarchy in newer versions. This means that old code without any notion of the MTLAllocation should still function correctly on whichever version of MacOS/iOS/.. . Hence, how bad of an idea would it be to guard just the trait bound with a cfg()? There's currently:

extern_protocol!(
    #[cfg(feature = "MTLAllocation")]
    pub unsafe trait MTLResource: MTLAllocation {

But if we have something like the following (perhaps with {} wrapping) the MTLResource type continues to be usable without the MTLAllocation feature and type:

extern_protocol!(
    pub unsafe trait MTLResource:
        #[cfg(feature = "MTLAllocation")] MTLAllocation
    {

[madsmtm]

I had never expected this to be filename-based which makes things extra complicated, when there appears to be a mostly 1:1 mapping between filenames and the main type that it defines.

I agree that it's kinda confusing (though documented here, open to input on how to make it clearer), but the only consistent choices were basically between file-based or one-feature-per-item.

And given that these feature flags are intended for compilation performance optimization, I found that one-feature-per-item just doesn't make sense, since a lot of time is spent just parsing the files and expanding macros.

As a bit of a contrived example, NSViewFullScreenModeOptionKey is also cfg-gated behind the NSView feature (because it's also declared in AppKit/NSView.h), but can and should be usable without also enabling NSResponder.

Is this debunked by stating that the type is only used in functions on NSView? https://developer.apple.com/documentation/appkit/nsviewfullscreenmodeoptionkey

Yes, which is why I said "contrived" ;)

A more realistic case might be MTLRenderCommandEncoder.h - This file contains for example MTLPrimitiveType, which is used elsewhere, but where MTLCommandEncoder.h probably doesn't need to be activated (?)

(My confidence level here is low, I'm not sure which choice is the correct one here)

We previously discussed that this "parent trait" relation is purely for the type system and only matters when calling a function on the available protocol - which is a requirement for retroactively introducing a "parent protocol" in the hierarchy in newer versions. This means that old code without any notion of the MTLAllocation should still function correctly on whichever version of MacOS/iOS/.. . Hence, how bad of an idea would it be to guard just the trait bound with a cfg()?

It's not a bad idea, and would be workable in Metal (where you rarely want to implement these protocols), but I think it would interfere with implementing the protocols yourself; e.g. if a library implementer does unsafe impl MTLResource for CustomClass { ... }, but doesn't implement MTLAllocation, then that would fail to compile if the end user activated the MTLAllocation feature.

[MarijnS95]

Yes, which is why I said "contrived" ;)

A more realistic case might be MTLRenderCommandEncoder.h - This file contains for example MTLPrimitiveType, which is used elsewhere, but where MTLCommandEncoder.h probably doesn't need to be activated (?)

I expected and am glad to see a counter-example. You're right that MTLPrimitiveType is referenced from MTLIndirectCommandEncoder (and MTKSubmesh) which should be usable independently from MTLRenderCommandEncoder.

---

but I think it would interfere with implementing the protocols yourself; e.g. if a library implementer does unsafe impl MTLResource for CustomClass { ... }, but doesn't implement MTLAllocation, then that would fail to compile if the end user activated the MTLAllocation feature.

Yikes, that would be very annoying and I don't have a solution around this.