Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't emit analyzer warnings for MakeGeneric understood by ILC #100858

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Don't emit analyzer warnings for MakeGeneric understood by ILC #100858

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3a74c7
Don't emit analyzer warnings for MakeGeneric understood by ILC
MichalStrehovsky Apr 10, 2024
ddf26e5
More tests
MichalStrehovsky Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>;

namespace ILLink.RoslynAnalyzer
{
Expand Down Expand Up @@ -342,5 +343,14 @@ internal bool CheckAndCreateRequiresDiagnostic (
incompatibleMembers,
out diagnostic);
}

internal virtual bool IsIntrinsicallyHandled (
IMethodSymbol calledMethod,
MultiValue instance,
ImmutableArray<MultiValue> arguments
)
{
return false;
}
}
}
74 changes: 74 additions & 0 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresDynamicCodeAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@
using System.Collections.Immutable;
using System.Linq;
using ILLink.Shared;
using ILLink.Shared.TrimAnalysis;
using ILLink.Shared.TypeSystemProxy;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>;

namespace ILLink.RoslynAnalyzer
{
Expand Down Expand Up @@ -38,6 +41,77 @@ public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase
internal override bool IsAnalyzerEnabled (AnalyzerOptions options) =>
options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableAotAnalyzer);

internal override bool IsIntrinsicallyHandled (IMethodSymbol calledMethod, MultiValue instance, ImmutableArray<MultiValue> arguments) {
MethodProxy method = new (calledMethod);
var intrinsicId = Intrinsics.GetIntrinsicIdForMethod (method);

switch (intrinsicId) {
case IntrinsicId.Type_MakeGenericType: {
if (!instance.IsEmpty ()) {
foreach (var value in instance.AsEnumerable ()) {
if (value is SystemTypeValue) {
if (!IsKnownInstantiation (arguments[0])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also check that the instantiation length matches the method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the instantiation length doesn't match, we'd get an exception at runtime both before and after AOT compilation, so maybe we don't need an AOT warning. The code is wrong, but we don't concern ourselves with bugs that exist before and after publish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should this return true if it's not a known instantiation, but the length doesn't match? I think that's what ILC does if I read #99037 correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh never mind, it triggers the warning if TryGetMakeGenericInstantiation returns false. Probably not an important edge case anyway.

return false;
}
} else {
return false;
}
}
}
return true;
}
case IntrinsicId.MethodInfo_MakeGenericMethod: {
if (!instance.IsEmpty ()) {
foreach (var methodValue in instance.AsEnumerable ()) {
if (methodValue is SystemReflectionMethodBaseValue methodBaseValue) {
if (!IsKnownInstantiation (arguments[0])) {
return false;
}
} else {
return false;
}
}
}
return true;
}
}

return false;

static bool IsKnownInstantiation(MultiValue genericParametersArray) {
var typesValue = genericParametersArray.AsSingleValue ();
if (typesValue is NullValue) {
// This will fail at runtime but no warning needed
return true;
}

// Is this an array we model?
if (typesValue is not ArrayValue array) {
return false;
}

int? size = array.Size.AsConstInt ();
if (size == null) {
return false;
}

for (int i = 0; i < size.Value; i++) {
// Go over each element of the array. If the value is unknown, bail.
if (!array.TryGetValueByIndex (i, out MultiValue value)) {
return false;
}

var singleValue = value.AsSingleValue ();

if (singleValue is not SystemTypeValue and not GenericParameterValue and not NullableSystemTypeValue) {
return false;
}
}

return true;
}
}

private protected override bool IsRequiresCheck (IPropertySymbol propertySymbol, Compilation compilation) {
var runtimeFeaturesType = compilation.GetTypeByMetadataName ("System.Runtime.CompilerServices.RuntimeFeature");
if (runtimeFeaturesType == null)
Expand Down
7 changes: 5 additions & 2 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisMethodCallPattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,11 @@ public IEnumerable<Diagnostic> CollectDiagnostics (DataFlowAnalyzerContext conte

foreach (var requiresAnalyzer in context.EnabledRequiresAnalyzers)
{
if (requiresAnalyzer.CheckAndCreateRequiresDiagnostic(Operation, CalledMethod, OwningSymbol, context, FeatureContext, out Diagnostic? diag))
diagnosticContext.AddDiagnostic(diag);
if (requiresAnalyzer.CheckAndCreateRequiresDiagnostic (Operation, CalledMethod, OwningSymbol, context, FeatureContext, out Diagnostic? diag)
&& !requiresAnalyzer.IsIntrinsicallyHandled (CalledMethod, Instance, Arguments))
{
diagnosticContext.AddDiagnostic (diag);
}
}

return diagnosticContext.Diagnostics;
Expand Down
143 changes: 143 additions & 0 deletions src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresDynamicCodeAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using ILLink.Shared;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Testing;
using Microsoft.CodeAnalysis.Text;
using Xunit;
Expand Down Expand Up @@ -34,6 +36,19 @@ public RequiresDynamicCodeAttribute (string message)
}
}";

static async Task VerifyRequiresDynamicCodeAnalyzer (
string source,
params DiagnosticResult[] expected)
{

await VerifyCS.VerifyAnalyzerAsync (
source,
consoleApplication: false,
TestCaseUtils.UseMSBuildProperties (MSBuildPropertyOptionNames.EnableAotAnalyzer),
Array.Empty<MetadataReference> (),
expected);
}

static Task VerifyRequiresDynamicCodeCodeFix (
string source,
string fixedSource,
Expand Down Expand Up @@ -332,5 +347,133 @@ private int M2 {
};
return VerifyRequiresDynamicCodeCodeFix (src, fix, diag, Array.Empty<DiagnosticResult> ());
}

[Fact]
public Task MakeGenericTypeWithAllKnownTypes ()
{
const string src = $$"""
class C
{
public void M() => typeof(Gen<>).MakeGenericType(typeof(object));
}
class Gen<T> { }
""";

return VerifyRequiresDynamicCodeAnalyzer (src);
}

[Fact]
public Task MakeGenericTypeWithAllKnownTypesInGenericContext ()
{
const string src = $$"""
class C
{
public void M<T>() => typeof(Gen<>).MakeGenericType(typeof(T));
}
class Gen<T> { }
""";

return VerifyRequiresDynamicCodeAnalyzer (src);
}

[Fact]
public Task MakeGenericTypeWithUnknownDefinition ()
{
const string src = $$"""
using System;
class C
{
public void M() => GetDefinition().MakeGenericType(typeof(object));
static Type GetDefinition() => typeof(Gen<>);
}
class Gen<T> { }
""";

return VerifyRequiresDynamicCodeAnalyzer (src,
// (4,21): warning IL3050: Using member 'System.Type.MakeGenericType(params Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.
VerifyCS.Diagnostic (DiagnosticId.RequiresDynamicCode).WithSpan (4, 21, 4, 68).WithArguments ("System.Type.MakeGenericType(params Type[])", " The native code for this instantiation might not be available at runtime.", ""));
}

[Fact]
public Task MakeGenericTypeWithUnknownArgument ()
{
const string src = $$"""
using System;
class C
{
public void M() => typeof(Gen<>).MakeGenericType(GetObject());
static Type GetObject() => typeof(object);
}
class Gen<T> { }
""";

return VerifyRequiresDynamicCodeAnalyzer (src,
// (4,21): warning IL3050: Using member 'System.Type.MakeGenericType(params Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.
VerifyCS.Diagnostic (DiagnosticId.RequiresDynamicCode).WithSpan (4, 21, 4, 63).WithArguments ("System.Type.MakeGenericType(params Type[])", " The native code for this instantiation might not be available at runtime.", ""));
}

[Fact]
public Task MakeGenericMethodWithAllKnownTypes ()
{
const string src = $$"""
class C
{
public void M() => typeof(C).GetMethod(nameof(N)).MakeGenericMethod(typeof(object));
public void N<T>() { }
}
""";

return VerifyRequiresDynamicCodeAnalyzer (src);
}

[Fact]
public Task MakeGenericMethodWithAllKnownTypesInGenericContext ()
{
const string src = $$"""
class C
{
public void M<T>() => typeof(C).GetMethod(nameof(N)).MakeGenericMethod(typeof(T));
public void N<T>() { }
}
""";

return VerifyRequiresDynamicCodeAnalyzer (src);
}

[Fact]
public Task MakeGenericMethodWithUnknownDefinition ()
{
const string src = $$"""
using System.Reflection;
class C
{
public void M() => GetMethodInfo().MakeGenericMethod(typeof(object));
public void N<T>() { }
public MethodInfo GetMethodInfo() => typeof(C).GetMethod(nameof(N));
}
""";

return VerifyRequiresDynamicCodeAnalyzer (src,
// (4,21): warning IL3050: Using member 'System.Reflection.MethodInfo.MakeGenericMethod(params Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.
VerifyCS.Diagnostic (DiagnosticId.RequiresDynamicCode).WithSpan (4, 21, 4, 70).WithArguments ("System.Reflection.MethodInfo.MakeGenericMethod(params Type[])", " The native code for this instantiation might not be available at runtime.", ""));
}

[Fact]
public Task MakeGenericMethodWithUnknownArgument ()
{
const string src = $$"""
using System;
class C
{
public void M() => typeof(C).GetMethod(nameof(N)).MakeGenericMethod(GetObject());
public void N<T>() { }
static Type GetObject() => typeof(object);
}
""";

return VerifyRequiresDynamicCodeAnalyzer (src,
// (4,21): warning IL3050: Using member 'System.Reflection.MethodInfo.MakeGenericMethod(params Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.
VerifyCS.Diagnostic (DiagnosticId.RequiresDynamicCode).WithSpan (4, 21, 4, 82).WithArguments ("System.Reflection.MethodInfo.MakeGenericMethod(params Type[])", " The native code for this instantiation might not be available at runtime.", ""));
}
}
}
Loading