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.

[MarijnS95]

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.

@madsmtm I've recently been thinking about this, and wouldn't it be possible with:

extern_protocol!(
    pub unsafe trait MTLResource: NSObjectProtocol {
        // ...
    }
);

#[cfg(feature = "MTLAllocation")]
unsafe impl MTLAllocation for dyn MTLResource {}

Granted, every consumer of the MTLResource protocol now has to add such a line to re-expose the "underlying" MTLAllocation protocol:

#[cfg(feature = "MTLAllocation")]
unsafe impl MTLAllocation for dyn MTLTexture {}

Consequently, this approach probably applies to every sub-protocol (IIRC we discussed that all protocols are optional), i.e. MTLTexture: MTLResource will also be pulled apart into an unsafe impl MTLResource for dyn MTLTexure {}.

I've tried quickly with:

#[cfg(feature = "MTLAllocation")]
unsafe impl<T: MTLResource> MTLAllocation for T {}

But that results in conflicting implementation for `objc2::runtime::ProtocolObject<_>`.

[madsmtm]

Hmm, I think it would be doable, though I gotta admit I don't initially particularly like it, but I guess part of that is because I don't really see this as a problem to solve, though another part is that I suspect it will make non-Metal code more verbose? Idk., I'd have to implement it in header-translator to really know for sure.

[MarijnS95]

@madsmtm it's not too bad in Traverse-Research/saxaboom#31, but we don't really rely on the allocatedSize() getter there (which is also provided by MTLResource already).

Looking forward to your experiments with this 👌

[cyber-excel10]

I am encountering two issues:

When enabling the MTLHeap feature, I get "imports not found" errors for the respective types. It seems these types are guarded behind both MTLHeap and MTLAllocation. Should the MTLHeap feature automatically enable the MTLAllocation feature for convenience, or is there a specific reason it doesn't? This has caused some confusion and required digging through the source code to understand the dependencies.

While compiling objc2 v0.6.0, I receive the following error:

Code
Compiling objc2 v0.6.0
error: A runtime must be selected
--> C:\Users\DELL.cargo\registry\src\index.crates.io-6f17d22bba15001f\objc2-0.6.0\src/lib.rs:206:1
|
206 | compile_error!("A runtime must be selected");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile objc2 (lib) due to 1 previous error
Additionally, when running cargo run, I encounter this issue:

Code
error: failed to select a version for objc2.
... required by package wifi_secured_Cli v0.1.0 (C:\Users\DELL\projects\hello_world\Excel2024_calculatorProject\secured_wifi_cli)
versions that meet the requirements ^0.6.0 (locked to 0.6.0) are: 0.6.0

the package wifi_secured_Cli depends on objc2, with features: tokio but objc2 does not have these features.

failed to select a version for objc2 which could resolve this conflict
How should I proceed to resolve these issues?

[MarijnS95]

@cyber-excel10 the first portion is relevant here, and solutions along what you mention are already proposed and disputed for various sensible reasons.

The second issue is unrelated here, and the error already tells you what the problems are. You specify a tokio feature which this crate doesn't have, and you need to select a runtime which is the default when compiling on or for an apple target:

objc2/crates/objc2/src/lib.rs

Lines 197 to 206 in 2794ebf

	#[cfg(all(
	not(docsrs),
	not(any(
	target_vendor = "apple",
	feature = "unstable-compiler-rt",
	feature = "gnustep-1-7",
	feature = "unstable-objfw",
	))
	))]
	compile_error!("A runtime must be selected");

Are you perhaps not specifying a --target somthing-apple-something when cross-compiling?