Unnecessary Trim analysis warnings for internal static fields of a RequiresUnreferencedCode class

[eerhardt]

Description

In dotnet/aspnetcore#46518 I am trying to mark some ASP.NET classes as RequiresUnreferencedCode. However, in doing that, it is causing the ASP.NET LinkabilityChecker to fail with some unexpected warnings:

ILLink : Trim analysis warning IL2026: Microsoft.AspNetCore.Http.ParameterBindingMethodCache: Using member 'Microsoft.AspNetCore.Http.ParameterBindingMethodCache.TempSourceStringExpr' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Uses unbounded Reflection to inspect property types. [C:\git\aspnetcore2\src\Tools\LinkabilityChecker\LinkabilityChecker.csproj]
ILLink : Trim analysis warning IL2026: Microsoft.AspNetCore.Http.ParameterBindingMethodCache: Using member 'Microsoft.AspNetCore.Http.ParameterBindingMethodCache.HttpContextExpr' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Uses unbounded Reflection to inspect property types. [C:\git\aspnetcore2\src\Tools\LinkabilityChecker\LinkabilityChecker.csproj]

These fields are only used by other RequiresUnreferencedCode code, so I don't expect the trimmer to warn about them.

Reproduction Steps

Using https://github.com/vitek-karas/illinkrepro, I was able to get a repro of this issue. Run illink with the rsp file in the repro.zip.

repro.zip

Expected behavior

There should be no ILLink warnings when running the linker.

Actual behavior

The linker reports 2 warnings for these internal static fields on a class annotated as RequiresUnreferencedCode.

Regression?

??

Known Workarounds

A workaround is to move the static fields to a separate type that isn't annotated with RequiresUnreferencedCode.

Configuration

No response

Other information

cc @sbomer @vitek-karas

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

In dotnet/aspnetcore#46518 I am trying to mark some ASP.NET classes as RequiresUnreferencedCode. However, in doing that, it is causing the ASP.NET LinkabilityChecker to fail with some unexpected warnings:

ILLink : Trim analysis warning IL2026: Microsoft.AspNetCore.Http.ParameterBindingMethodCache: Using member 'Microsoft.AspNetCore.Http.ParameterBindingMethodCache.TempSourceStringExpr' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Uses unbounded Reflection to inspect property types. [C:\git\aspnetcore2\src\Tools\LinkabilityChecker\LinkabilityChecker.csproj]
ILLink : Trim analysis warning IL2026: Microsoft.AspNetCore.Http.ParameterBindingMethodCache: Using member 'Microsoft.AspNetCore.Http.ParameterBindingMethodCache.HttpContextExpr' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Uses unbounded Reflection to inspect property types. [C:\git\aspnetcore2\src\Tools\LinkabilityChecker\LinkabilityChecker.csproj]

Reproduction Steps

Using https://github.com/vitek-karas/illinkrepro, I was able to get a repro of this issue. Run illink with the rsp file in the repro.zip.

repro.zip

Expected behavior

There should be no ILLink warnings when running the linker.

Actual behavior

The linker reports 2 warnings for these internal static fields on a class annotated as RequiresUnreferencedCode.

Regression?

??

Known Workarounds

A workaround is to move the static fields to a separate type that isn't annotated with RequiresUnreferencedCode.

Configuration

No response

Other information

cc @sbomer @vitek-karas

Author:	eerhardt
Assignees:	-
Labels:	area-AssemblyLoader-coreclr
Milestone:	-

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

In dotnet/aspnetcore#46518 I am trying to mark some ASP.NET classes as RequiresUnreferencedCode. However, in doing that, it is causing the ASP.NET LinkabilityChecker to fail with some unexpected warnings:

ILLink : Trim analysis warning IL2026: Microsoft.AspNetCore.Http.ParameterBindingMethodCache: Using member 'Microsoft.AspNetCore.Http.ParameterBindingMethodCache.TempSourceStringExpr' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Uses unbounded Reflection to inspect property types. [C:\git\aspnetcore2\src\Tools\LinkabilityChecker\LinkabilityChecker.csproj]
ILLink : Trim analysis warning IL2026: Microsoft.AspNetCore.Http.ParameterBindingMethodCache: Using member 'Microsoft.AspNetCore.Http.ParameterBindingMethodCache.HttpContextExpr' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Uses unbounded Reflection to inspect property types. [C:\git\aspnetcore2\src\Tools\LinkabilityChecker\LinkabilityChecker.csproj]

Reproduction Steps

Using https://github.com/vitek-karas/illinkrepro, I was able to get a repro of this issue. Run illink with the rsp file in the repro.zip.

repro.zip

Expected behavior

There should be no ILLink warnings when running the linker.

Actual behavior

The linker reports 2 warnings for these internal static fields on a class annotated as RequiresUnreferencedCode.

Regression?

??

Known Workarounds

A workaround is to move the static fields to a separate type that isn't annotated with RequiresUnreferencedCode.

Configuration

No response

Other information

cc @sbomer @vitek-karas

Author:	eerhardt
Assignees:	-
Labels:	untriaged, area-Tools-ILLink
Milestone:	-

[sbomer]

The command-line arguments include -a Microsoft.AspNetCore.Http.Extensions visible. The visible root mode also includes internals if the assembly has InternalsVisibleTo, which it does:

runtime/src/tools/illink/src/linker/Linker.Steps/RootAssemblyInputStep.cs

Lines 56 to 61 in 232fbce

	var preserve_visible = TypePreserveMembers.Visible;
	if (MarkInternalsVisibleTo (assembly))
	preserve_visible |= TypePreserveMembers.Internal;
	MarkAndPreserve (assembly, preserve_visible);
	break;

In my mind this is somewhat expected, since the input is asking to preserve things for unknown reasons, and RequiresUnreferencedCode on the type means that the internal fields could be unsafe to access via reflection. Passing this assembly as a root assembly would produce these warnings as well.

I think we made some exceptions for similar circumstances where members were referenced via XML, but I don't remember the details. Maybe @vitek-karas does.

[eerhardt]

In my mind this is somewhat expected, since the input is asking to preserve things for unknown reasons, and RequiresUnreferencedCode on the type means that the internal fields could be unsafe to access via reflection.

I think warning at the places that use these fields is reasonable, since the class is marked RUC. However, my understanding is the warnings are happening simply because they are fields on a RUC type being exposed. That doesn't really make sense to me. Of course these fields are unsafe - that's why they are in the class that is marked RUC. It doesn't mean the trimmer needs to warn simply because they are visible. There should only be warnings at the places using the fields.

[vitek-karas]

I "think" (didn't check the code) that we intentionally suppress RUC when caused by a direct reference from a descriptor XML. But otherwise we report it always.

As for this case, the -a assembly visible acts as if there's something in the app which directly references all visible APIs on the assembly. That's why it produces a warning, because the linker thinks that there's a real reference. This is very similar in behavior to what would happen if you added for example a DynamicDependency pointing to the fields in question. All of these avenues are basically telling linker "you don't understand this app... so trust me, you're going to need this API as well", and linker's response is "that's fine, but this API won't work when running in a trimmed app... that's probably bad".

The descriptor case should be similar, but I honestly don't remember why we chose not to warn in that case. My main guess would be: there are cases in corelib and similar where we ran into the warning and there's no good way to suppress it (since it doesn't originates in code).

For this specific use case in ASP.NET we have basically two options:

• Invest into linker fully supporting the "library" mode (and actually defined what it means, since currently it's more like a "This seems to work, so keep it". Currently it's not a supported feature in any sense - we maintain it for our repos only. This might include changing the behavior in this case to not warn - not sure, depends on what would be the desired behavior of the feature.
• Leave it as-is and ASP.NET will baseline these warnings. We already have a couple of places in runtime where we intentionally suppress warnings via XMLs for similar reasons.

[eerhardt]

The part that I still don't get is why static fields are treated specially here. We aren't warning for all the "visible" methods in the same class with RequiresUnreferencedCode on it. But they are just as "visible" as the static fields are.

For this specific use case in ASP.NET

I agree that this isn't critical to fix right now, especially considering this "library" mode isn't a fully supported feature. I was able to workaround it by moving the fields into a separate static class. dotnet/aspnetcore@a68e4c8

[vitek-karas]

We aren't warning for all the "visible" methods in the same class with RequiresUnreferencedCode on it.

That's a great point - and I missed this. We need to look into this some more. Whatever the behavior is (I could explain away both ;-)), it should be consistent.

[eerhardt]

FYI - I hit this again a couple times in #84468. It might make sense to fix this at the same time as #84433.

cc @agocke