ILLink: FormatException when building a diagnostic message

[vitek-karas]

Repro:

dotnet new console

app.csproj:

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
        <PublishTrimmed>true</PublishTrimmed>
        <WarningsAsErrors>false</WarningsAsErrors>
        <NoWarn>NU1605;NU1603;NU1701</NoWarn>
        <TrimmerSingleWarn>false</TrimmerSingleWarn>
    </PropertyGroup>

    <ItemGroup>
        <TrimmerRootAssembly Include="AltV.Net.Shared" />

        <PackageReference Include="AltV.Net.Shared" Version="14.0.2-dev" />
    </ItemGroup>
</Project>

dotnet publish

Stacktrace from the build:

  Unhandled exception. System.FormatException: Index (zero based) must be greater than or equal to zero and less than t
  he size of the argument list.
     at System.Text.ValueStringBuilder.AppendFormatHelper(IFormatProvider provider, String format, ReadOnlySpan`1 args)
     at System.String.FormatHelper(IFormatProvider provider, String format, ReadOnlySpan`1 args)
     at System.String.Format(String format, Object[] args)
     at ILLink.Shared.DiagnosticString.GetMessage(String[] args)
     at Mono.Linker.MessageContainer..ctor(MessageCategory category, DiagnosticId id, String subcategory, Nullable`1 or
  igin, String[] args)
     at Mono.Linker.MessageContainer.CreateWarningMessageContainer(LinkContext context, MessageOrigin origin, Diagnosti
  cId id, WarnVersion version, String subcategory, String[] args)
     at Mono.Linker.MessageContainer.CreateWarningMessage(LinkContext context, MessageOrigin origin, DiagnosticId id, W
  arnVersion version, String[] args)
     at Mono.Linker.LinkContext.LogWarning(MessageOrigin origin, DiagnosticId id, String[] args)
     at ILLink.Shared.TrimAnalysis.DiagnosticContext.AddDiagnostic(DiagnosticId id, String[] args)
     at ILLink.Shared.TrimAnalysis.DiagnosticContext.AddDiagnostic(DiagnosticId id, ValueWithDynamicallyAccessedMembers
   actualValue, ValueWithDynamicallyAccessedMembers expectedAnnotationsValue, String[] args)
     at ILLink.Shared.TrimAnalysis.RequireDynamicallyAccessedMembersAction.Invoke(ValueSet`1& value, ValueWithDynamical
  lyAccessedMembers targetValue)

/cc @LakshanF

[ghost]

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

Issue Details

---

Repro:

dotnet new console

app.csproj:

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
        <PublishTrimmed>true</PublishTrimmed>
        <WarningsAsErrors>false</WarningsAsErrors>
        <NoWarn>NU1605;NU1603;NU1701</NoWarn>
        <TrimmerSingleWarn>false</TrimmerSingleWarn>
    </PropertyGroup>

    <ItemGroup>
        <TrimmerRootAssembly Include="AltV.Net.Shared" />

        <PackageReference Include="AltV.Net.Shared" Version="14.0.2-dev" />
    </ItemGroup>
</Project>

dotnet publish

Stacktrace from the build:

  Unhandled exception. System.FormatException: Index (zero based) must be greater than or equal to zero and less than t
  he size of the argument list.
     at System.Text.ValueStringBuilder.AppendFormatHelper(IFormatProvider provider, String format, ReadOnlySpan`1 args)
     at System.String.FormatHelper(IFormatProvider provider, String format, ReadOnlySpan`1 args)
     at System.String.Format(String format, Object[] args)
     at ILLink.Shared.DiagnosticString.GetMessage(String[] args)
     at Mono.Linker.MessageContainer..ctor(MessageCategory category, DiagnosticId id, String subcategory, Nullable`1 or
  igin, String[] args)
     at Mono.Linker.MessageContainer.CreateWarningMessageContainer(LinkContext context, MessageOrigin origin, Diagnosti
  cId id, WarnVersion version, String subcategory, String[] args)
     at Mono.Linker.MessageContainer.CreateWarningMessage(LinkContext context, MessageOrigin origin, DiagnosticId id, W
  arnVersion version, String[] args)
     at Mono.Linker.LinkContext.LogWarning(MessageOrigin origin, DiagnosticId id, String[] args)
     at ILLink.Shared.TrimAnalysis.DiagnosticContext.AddDiagnostic(DiagnosticId id, String[] args)
     at ILLink.Shared.TrimAnalysis.DiagnosticContext.AddDiagnostic(DiagnosticId id, ValueWithDynamicallyAccessedMembers
   actualValue, ValueWithDynamicallyAccessedMembers expectedAnnotationsValue, String[] args)
     at ILLink.Shared.TrimAnalysis.RequireDynamicallyAccessedMembersAction.Invoke(ValueSet`1& value, ValueWithDynamical
  lyAccessedMembers targetValue)

Author:	vitek-karas
Assignees:	-
Labels:	area-Tools-ILLink
Milestone:	-

[vitek-karas]

Simple repro:

		static Type UnannotatedField;

		static void MakeGenericNullableOfUnannotatedFieldRequiresPublicProperties ()
		{
                        // Expected IL2087 (currently)
			typeof (Nullable<>).MakeGenericType (UnannotatedField).RequiresPublicProperties ();
		}

The bug is in

runtime/src/tools/illink/src/ILLink.Shared/Annotations.cs

Lines 114 to 118 in 7c8fff9

	(NullableValueWithDynamicallyAccessedMembers, MethodParameterValue maybeThis) when maybeThis.IsThisParameter () => DiagnosticId.DynamicallyAccessedMembersMismatchTypeArgumentTargetsThisParameter,
	(NullableValueWithDynamicallyAccessedMembers, MethodParameterValue) => DiagnosticId.DynamicallyAccessedMembersMismatchTypeArgumentTargetsParameter,
	(NullableValueWithDynamicallyAccessedMembers, MethodReturnValue) => DiagnosticId.DynamicallyAccessedMembersMismatchTypeArgumentTargetsMethodReturnType,
	(NullableValueWithDynamicallyAccessedMembers, FieldValue) => DiagnosticId.DynamicallyAccessedMembersMismatchTypeArgumentTargetsField,
	(NullableValueWithDynamicallyAccessedMembers, GenericParameterValue) => DiagnosticId.DynamicallyAccessedMembersMismatchTypeArgumentTargetsGenericParameter,

The code which maps the source/target pair values to the Diagnostic ID treats NullableValueWithDynamicallyAccessedMembers the same as GenericParameterValue - assigns the same diag IDs for them. The messages with these IDs expect that the source value will have 2 arguments for the formatted message, because GenericParameterValue has two (The name of the generic parameter and the containing symbol name).
But NullableValueWithDynamicallyAccessedMembers doesn't produce the arguments itself, it simply forwards the call to the underlying (non-nullable) value. Which can be pretty much anything. Most values do produce 2 arguments, but FieldValue doesn't, it only produces the full name of the field.
If this happens we fail with the above exception because we provide one less argument to the formatted message.

This is wrong because the formatted message will not make any sense. For example:

void DoIt(Type p1)
{
    typeof(Nullable<>).MakeGenericType(p1).GetProperties();
}

Will produce:

warning IL2090: 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperties()'. The generic parameter 'p1' of 'DoIt(Type)' does not have matching annotations.

Specifically The generic parameter 'p1' of 'DoIt(Type)' does not have matching annotations. makes no sense - p1 is not a generic parameter and the method is not generic either.

I think the correct behavior is to unwrap NullableValueWithDynamicallyAccessedMembers for the purposes of creating diag messages - the Nullable<> itself doesn't affect data flow, we intentionally treat it completely transparently, so the error comes from whatever it's wrapping - we should report it as such.

[vitek-karas]

The problem with the proposed fix is, that it would change warning codes reported for these cases. Which is a breaking change.
Alternatively, we could produce a warning with the old warning code but with the right message, but that comes with:

• Non-trivial changes to the code (we're not setup to lie about the warning code)
• Our docs page for the warning code would be wrong in this case.

I think we do need to fix this, as in, we must not crash the trimmer in this case.

One last alternative would be to special case this specific situation (nullable over field) and in that case produce a warning with a different code.

Current behavior:

• Trimmer - crashes as described in this issue
• NativeAOT - crashes with basically the same exception (the bug is in the shared code)
• Analyzer - doesn't crash, but produces unformatted message: 'this' argument does not satisfy {3} in call to '{0}'. The generic parameter '{1}' of '{2}' does not have matching annotations.

Because of this, the specific case with nullable over field is basically not used in 8 - The only way to get the warning is in analyzer, both trimmer and AOT won't produce it (they crash). So changing the warning code in this specific case might be acceptable, as it's basically a change from crash to working.

Unfortunately this would mean we leave the other cases with wrong messages (see above).

[vitek-karas]

Note that we've shipped this in 7.0 trimmer already, so rushing a real fix into 8.0 servicing would not reduce the potential impact of the breaking change.