Generic attributes handling in CustomAttributeDecoder

[buyaa-n]

Restore reverted PR, if-def-ed generic attributes part from Mobile builds

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Restore reverted PR, if-def-ed generic attributes part from Mobile builds

Author:	buyaa-n
Assignees:	-
Labels:	area-System.Reflection.Metadata
Milestone:	-

[buyaa-n]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[buyaa-n]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MichalStrehovsky]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MichalStrehovsky]

@buyaa-n your previous attempt failed so I retried with TARGET_BROWSER that we use in libs partiton elsewhere but that didn't help either.

I'm afraid it's what I wrote in https://github.com/dotnet/runtime/issues/73666#issuecomment-1210126908 "I don't know if we can per-platform ifdef things for libraries that don't have a WASM-specific version (this all targets net7.0, not net7.0-windows/net7.0-browser, etc.). Probably not.".

We can't do ifdefs because the test doesn't build for WASM/Browser, it just builds for net7.0 and nothing platform-specific is defined when building it.

Two options left - check it in without the test, or disable the test on WASM at a spot I pointed to in https://github.com/dotnet/runtime/issues/73666#issuecomment-1210123262.

I would lean towards disabling the test on WASM. S.R.Metadata is not something people would use on Browser-WASM in general and there's no Browser specific logic in it to test anyway. It's just an assembly like any other. I don't think it makes a huge difference in the amount of coverage. I want to have the test available for NativeAOT testing - which as you pointed out is currently broken and requires upgrading to S.R.Metadata that has the fix. I need to be able to test that it's the only thing left.

The bug illinker is running into is 7.0-level so it will likely be fixed before we ship and the test can be re-enabled.

We need to keep testing the non-WASM runtimes because it's clear this is the only coverage of generic attributes we have in the whole repo. It gives a very good reason to keep the test.

Cc @radical

[buyaa-n]

Strange we have some tests with generic attributes in runtime https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System/Attributes.cs#L243-L323 and the Attributes.cs file is added in cross platform build, so I believe it is built for browser too, not sure how this generic attribute

runtime/src/libraries/System.Runtime/tests/System/Attributes.cs

Lines 931 to 934 in 85aa052

	[AttributeUsage(AttributeTargets.All, AllowMultiple = true)]
	public class GenericAttribute<T> : Attribute
	{
	}

over there is not causing this issue

[radical]

TARGET_BROWSER -> TARGETS_BROWSER

[buyaa-n]

I would lean towards disabling the test on WASM. S.R.Metadata is not something people would use on Browser-WASM in general and there's no Browser specific logic in it to test anyway.

Not sure how to do that

I think the issue is the linker trying to trim test files, seems it is not doing that in Runtime but in S.R.M. I don't know how to exclude the test file from trimming, so maybe I will just comment out the test with generics until it is fixed in linker

[MichalStrehovsky]

TARGET_BROWSER -> TARGETS_BROWSER

There's only one test that has the TARGETS_BROWSER string and that's System.Net.Http.Functional test. It defines it in the csproj:

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj

Line 18 in 85aa052

<DefineConstants Condition="'$(TargetPlatformIdentifier)' == 'Browser'">$(DefineConstants);TARGETS_BROWSER</DefineConstants>

So maybe we just need to add it to the csproj here too. Let me try.

[radical]

And add <DefineConstants Condition="'$(TargetPlatformIdentifier)' == 'Browser'">$(DefineConstants);TARGETS_BROWSER</DefineConstants> to the project file. Trying it locally to confirm

[MichalStrehovsky]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[radical]

I can build with this:

diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/Decoding/CustomAttributeDecoderTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/Decoding/CustomAttributeDecoderTests.cs
index 62944bef46d..2189b4b3176 100644
--- a/src/libraries/System.Reflection.Metadata/tests/Metadata/Decoding/CustomAttributeDecoderTests.cs
+++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/Decoding/CustomAttributeDecoderTests.cs
@@ -197,7 +197,7 @@ public void TestCustomAttributeDecoderUsingReflection()
             }
         }

-#if NETCOREAPP && !TARGET_BROWSER // Generic attribute is not supported on .NET Framework.
+#if NETCOREAPP && !TARGETS_BROWSER // Generic attribute is not supported on .NET Framework.
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.HasAssemblyFiles))]
         [ActiveIssue("https://github.com/dotnet/runtime/issues/60579", TestPlatforms.iOS | TestPlatforms.tvOS)]
         public void TestCustomAttributeDecoderGenericUsingReflection()
diff --git a/src/libraries/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj b/src/libraries/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj
index d611b766fc2..85f193fef68 100644
--- a/src/libraries/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj
+++ b/src/libraries/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj
@@ -7,6 +7,7 @@
     <NoWarn>$(NoWarn);436;SYSLIB0037</NoWarn>
     <TargetFrameworks>$(NetCoreAppCurrent);$(NetFrameworkMinimum)</TargetFrameworks>
     <EnableLibraryImportGenerator>true</EnableLibraryImportGenerator>
+    <DefineConstants Condition="'$(TargetOS)' == 'Browser'">$(DefineConstants);TARGETS_BROWSER</DefineConstants>
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="$(CommonTestPath)System\Security\Cryptography\SignatureSupport.cs"
@@ -147,4 +148,4 @@
   <ItemGroup Condition="'$(TargetOS)' == 'Browser'">
     <WasmFilesToIncludeFromPublishDir Include="$(AssemblyName).dll" />
   </ItemGroup>
-</Project>
\ No newline at end of file
+</Project>

[radical]

TargetPlatformIdentifier doesn't seem to be set.

[buyaa-n]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[radical]

The wasm debugger test failures were fixed by #68807 which was just merged.
The wasm Library test failures are #73721 .

[buyaa-n]

Thank you both @MichalStrehovsky @radical as failures unrelated and reported merging