Inherit PlatformAttribute

[manfred-brands]

Fixes #5161
Fixes #4131

Replaces PR #5162 with renamed source branch and retargeted to 4.6

@stevenaw Same code, but with tests that utilize the inheritance of the PlatfromAttribute.

[stevenaw]

Thanks @manfred-brands I'll try to give this a look today before I head on vacation tomorrow.

[stevenaw]

Thanks @manfred-brands
You discovered in the other PR that in the event that the platform attribute is specified at multiple levels that it would only honour the "first one" found. Could we add a test for that please? I think that's an important new scenario to cover if we are allowing this to be inheritable.

Also, I wanted to double check with you and @OsirisTerje if that is actually what we want for this behaviour, or if both should be factored in. For example, a test suite like this may be a fairly common way to use inheritance for this attribute. Should LegacyWindowsSupport run for all 32-bit runs or just Windows 32-bit runs?

[Platform("Win")]
public abstract class WindowsTests {}

[Platform("32-Bit")]
public class LegacyWindowsSupport : WindowsTests  { }

[Platform("64-Bit")]
public class ModernWindowsSupport : WindowsTests  { }



[Platform("Linux")]
public abstract class LinuxTests {}

[Platform("32-Bit")]
public class LegacyLinuxSupport : LinuxTests { }

[Platform("64-Bit")]
public class ModernLinuxSupport : LinuxTests { }

I've left this as a "comment" instead of request for changes as I will be away soon, but I think these would be important behaviours to define and maybe even document as part of making this inheritable

[OsirisTerje]

@stevenaw Would this not be the same as having multiple Platform attributes?
We could implement that, but perhaps later. I believe most would use a base class to set the platform attribute. It is one step further to set it also on the derived class.
I haven't checked the code if this will work as is, but I would assume not. @manfred-brands implements this, so I assume he knows more.

I am actually a bit indifferent to this. We could document the behavior (or restrictions, if that is what this boils down to now) for now, and then consider if we should do something later.

[stevenaw]

@OsirisTerje
That's the discussion I'm looking to start.

I haven't tested the runtime behavior, but from my earlier experimentation that will compile fine since attributes are applied to the type and different levels of an inheritance hierarchy are different types. In other words, my understanding then is that Multiple=false combined with Inherit=true will still permit multiple attributes to potentially be at play, we would just be trading one form of it for another.

I'm also a bit indifferent as to what we want the behavior to be in that case. I'm mostly just asking to ensure we account for possible use cases and don't introduce unintended behaviors.

My understanding is that the example above may need the child classes to duplicate the "Win" in their definitions to be specific to Win32 or Win64. Ex:

-[Platform("32-bit")]
+[Platform("Win,32-bit")]

I'm also fine to not change that for now so long as we document it.

[stevenaw]

Having thought this through a bit more, I'm going to approve this as well. I suspect it's likely anyone seeking a fixture setup similar to the example is already likely duplicating the identifiers given the lack of Inherited support up until now.

I think it might still be helpful to add a test for where the attribute appears twice in a hierarchy in order to proof existing behavior. @manfred-brands I understand your limited bandwidth and I'd be happy to add it myself if it's still needed after I return from travels next week.

Thanks again for your change here

[OsirisTerje]

I added an issue #5168 to cover @stevenaw comments above.

I'll merge this now.

[stevenaw]

Thanks @OsirisTerje

[manfred-brands]

I haven't tested the runtime behavior, but from my earlier experimentation that will compile fine since attributes are applied to the type and different levels of an inheritance hierarchy are different types. In other words, my understanding then is that Multiple=false combined with Inherit=true will still permit multiple attributes to potentially be at play, we would just be trading one form of it for another.

From my tests I added for Timeout and CancelAfter, if found that when AllowMultiple=false the search stops at the first one it finds in the hierarchy.

In your example it would find 64-Bit but not the base class' Win.

[stevenaw]

From my tests I added for Timeout and CancelAfter, if found that when AllowMultiple=false the search stops at the first one it finds in the hierarchy.

Ah. Ok thanks for reconfirming @manfred-brands ! I had misunderstood earlier. Glad to hear that's the behavior with AllowMultiple= false 🙂