Skip to content

Avoid warnings due to RUC on type when marking assembly as library #84620

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

Merged
vitek-karas merged 5 commits into from
Apr 12, 2023
Merged

Avoid warnings due to RUC on type when marking assembly as library #84620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95cbdfa
First test
vitek-karas Apr 10, 2023
f268167
Implementation
vitek-karas Apr 11, 2023
fef344d
Fix tests
vitek-karas Apr 11, 2023
6634d33
PR feedback
vitek-karas Apr 11, 2023
92d106c
Update test
vitek-karas Apr 12, 2023
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
42 changes: 35 additions & 7 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1775,9 +1775,19 @@ void ProcessAnalysisAnnotationsForField (FieldDefinition field, DependencyKind d
(origin.Provider is MethodDefinition originMethod && CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (originMethod)));
}

if (dependencyKind == DependencyKind.DynamicallyAccessedMemberOnType) {
switch (dependencyKind) {
// Marked through things like descriptor - don't want to warn as it's intentional choice
case DependencyKind.AlreadyMarked:
case DependencyKind.TypePreserve:
Copy link
Member

@sbomer sbomer Apr 11, 2023

Choose a reason for hiding this comment

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

Why is this different than the logic in ProcessAnalysisAnnotationsForMethod which has an early return for Alreadymarked, TypePreserve, and PreservedMethod - should those be added here too?

Copy link
Member Author

@vitek-karas vitek-karas Apr 11, 2023

Choose a reason for hiding this comment

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

The other 2 can't ever happen for fields, but I'll unify it.

case DependencyKind.PreservedMethod:
return;

case DependencyKind.DynamicallyAccessedMemberOnType:
ReportWarningsForTypeHierarchyReflectionAccess (field, origin);
return;

default:
break;
}

if (Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (origin.Provider, out _))
Expand Down Expand Up @@ -3189,9 +3199,10 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
if (!Annotations.ProcessSatelliteAssemblies && KnownMembers.IsSatelliteAssemblyMarker (method))
Annotations.ProcessSatelliteAssemblies = true;
} else if (method.TryGetProperty (out PropertyDefinition? property))
MarkProperty (property, new DependencyInfo (DependencyKind.PropertyOfPropertyMethod, method));
else if (method.TryGetEvent (out EventDefinition? @event))
MarkEvent (@event, new DependencyInfo (DependencyKind.EventOfEventMethod, method));
MarkProperty (property, new DependencyInfo (PropagateDependencyKindToAccessors (reason.Kind, DependencyKind.PropertyOfPropertyMethod), method));
else if (method.TryGetEvent (out EventDefinition? @event)) {
MarkEvent (@event, new DependencyInfo (PropagateDependencyKindToAccessors(reason.Kind, DependencyKind.EventOfEventMethod), method));
}

if (method.HasMetadataParameters ()) {
#pragma warning disable RS0030 // MethodReference.Parameters is banned. It's easiest to leave the code as is for now
Expand Down Expand Up @@ -3270,6 +3281,20 @@ protected virtual void DoAdditionalMethodProcessing (MethodDefinition method)
{
}

static DependencyKind PropagateDependencyKindToAccessors(DependencyKind parentDependencyKind, DependencyKind kind)
Copy link
Member

@sbomer sbomer Apr 11, 2023

Choose a reason for hiding this comment

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

It's a little unfortunate that we are tweaking the dependency graph just to give the right warnings. This will make it look like properties/events got marked due to descriptors even if they were actually only marked by our internal logic that keeps them for marked accessors.

I think we already do this in a number of places and this is just continuing with the existing pattern - but I'll continue to bring up the concern as a general point. :) Eventually I'd love to get rid of any logic that changes warning behavior based on DependencyKind and use that solely for tracking.

Copy link
Member Author

@vitek-karas vitek-karas Apr 11, 2023

Choose a reason for hiding this comment

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

It's not that bad - the edge will be as is right now, just the "kind" on the edge will be different.
But I agree.

The long term goal -> do it the same way ILC does this. Meaning we don't actually generate any warnings for usage in MarkMethod, instead they're produced at the callsite. In short, everything should go through ReflectionMarker which will take care of these things.

{
switch (parentDependencyKind) {
// If the member is marked due to descriptor or similar, propagate the original reason to suppress some warnings correctly
case DependencyKind.AlreadyMarked:
case DependencyKind.TypePreserve:
case DependencyKind.PreservedMethod:
return parentDependencyKind;

default:
return kind;
}
}

void MarkImplicitlyUsedFields (TypeDefinition type)
{
if (type?.HasFields != true)
Expand Down Expand Up @@ -3506,9 +3531,12 @@ protected internal virtual void MarkEvent (EventDefinition evt, in DependencyInf
using var eventScope = ScopeStack.PushScope (new MessageOrigin (evt));

MarkCustomAttributes (evt, new DependencyInfo (DependencyKind.CustomAttribute, evt));
MarkMethodIfNotNull (evt.AddMethod, new DependencyInfo (DependencyKind.EventMethod, evt), ScopeStack.CurrentScope.Origin);
MarkMethodIfNotNull (evt.InvokeMethod, new DependencyInfo (DependencyKind.EventMethod, evt), ScopeStack.CurrentScope.Origin);
MarkMethodIfNotNull (evt.RemoveMethod, new DependencyInfo (DependencyKind.EventMethod, evt), ScopeStack.CurrentScope.Origin);

DependencyKind dependencyKind = PropagateDependencyKindToAccessors(reason.Kind, DependencyKind.EventMethod);
MarkMethodIfNotNull (evt.AddMethod, new DependencyInfo (dependencyKind, evt), ScopeStack.CurrentScope.Origin);
MarkMethodIfNotNull (evt.InvokeMethod, new DependencyInfo (dependencyKind, evt), ScopeStack.CurrentScope.Origin);
MarkMethodIfNotNull (evt.RemoveMethod, new DependencyInfo (dependencyKind, evt), ScopeStack.CurrentScope.Origin);

DoAdditionalEventProcessing (evt);
}

Expand Down
8 changes: 7 additions & 1 deletion src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresCapabilityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ public Task RequiresInCompilerGeneratedCodeRelease ()
return RunTest ();
}

[Fact]
public Task RequiresInLibraryAssembly ()
{
return RunTest ();
}

[Fact]
public Task RequiresOnAttribute ()
{
Expand Down Expand Up @@ -112,4 +118,4 @@ public Task SuppressRequires ()
return RunTest (nameof (SuppressRequires));
}
}
}
}
62 changes: 62 additions & 0 deletions ...tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresInLibraryAssembly.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Diagnostics.CodeAnalysis;
using Mono.Linker.Tests.Cases.Expectations.Assertions;
using Mono.Linker.Tests.Cases.Expectations.Metadata;

namespace Mono.Linker.Tests.Cases.RequiresCapability
{
[SetupLinkerArgument ("-a", "test.exe", "library")]

[SkipKeptItemsValidation]
[ExpectedNoWarnings]
public class RequiresInLibraryAssembly
{
public static void Main ()
{
}

[RequiresDynamicCode ("--MethodWhichRequires--")]
public static void MethodWhichRequires () { }

[RequiresDynamicCode ("--InstanceMethodWhichRequires--")]
public void InstanceMethodWhichRequires () { }
}

[ExpectedNoWarnings]
public sealed class ClassWithDAMAnnotatedMembers
{
public static void Method ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type) { }

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
public static Type Field;
}

[ExpectedNoWarnings]
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
public sealed class ClassWithDAMAnnotation
{
public void Method () { }
}

[ExpectedNoWarnings]
[RequiresUnreferencedCode ("--ClassWithRequires--")]
public sealed class ClassWithRequires
Copy link
Member

@eerhardt eerhardt Apr 11, 2023

Choose a reason for hiding this comment

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

Does the visibility of the class/member matter at all? All of the places I hit this were not public.

Copy link
Member Author

@vitek-karas vitek-karas Apr 11, 2023

Choose a reason for hiding this comment

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

It can also be internal, but then the assembly must have InternalsVisibleTo - so I intentionally avoided that. It should not matter at all to the warning behavior.

Copy link
Member

@eerhardt eerhardt Apr 11, 2023

Choose a reason for hiding this comment

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

You don't need InternalsVisibleTo in order to get the trimmer to emit the bad warning. I hit it here:

https://github.com/dotnet/runtime/pull/84468/files#diff-0feadd0bb2789c2621b7be1a6efe5807907d218fe60d62fd89cc7c4578dbe08cR10

image

Copy link
Member Author

@vitek-karas vitek-karas Apr 11, 2023

Choose a reason for hiding this comment

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

Sort of - it needs IVT if the type is internal... but I guess fields are taken as a whole (probably because of base classes, in which case we should still be able to exclude statics... eventually)

Copy link
Member

@eerhardt eerhardt Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So in dotnet/aspnetcore#46518, this bug was encountered only because the assembly has IVT to tests?

{
public static int Field;
Copy link
Member

@eerhardt eerhardt Apr 11, 2023

Choose a reason for hiding this comment

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

I've also seen this warning for private const string. Should we add that test as well?

Copy link
Member Author

@vitek-karas vitek-karas Apr 11, 2023

Choose a reason for hiding this comment

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

I will try with a private field, again visibility should not really matter, but worth a try.


internal static int InternalField;

private static int PrivateField;

public static void Method () { }

public void InstanceMethod () { }

public static int Property { get; set; }

public static event EventHandler PropertyChanged;
}
}
2 changes: 0 additions & 2 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresViaXml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ namespace Mono.Linker.Tests.Cases.RequiresCapability
[SetupLinkerDescriptorFile ("RequiresViaXml.descriptor.xml")]
[SkipKeptItemsValidation]
[ExpectedNoWarnings]
// [LogContains ("--RequiresOnlyViaDescriptor--")] // https://github.com/dotnet/linker/issues/2103
[ExpectedWarning ("IL2026", "RequiresOnFieldOnlyViaDescriptor.Field", FileName = "RequiresViaXml.descriptor.xml", ProducedBy = Tool.Trimmer)]
class RequiresViaXml
{

Expand Down