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

vitek-karas · 2023-04-11T10:56:16Z

Marking an assembly as "library" will mark all public APIs. If some of the marked members have RUC on them we decided we don't want to produce warnings, since there's nothing especially wrong with the library. The callers of those members would still get the right warnings.

Currently linker implements this correctly for methods and properties, but it fails for fields and events and it still produces some warnings, especially if the RUC is on the parent type.

This change fixes those cases and adds a special test for the library marked assembly in combination with several RUCs.

Fixes #81864

ghost · 2023-04-11T10:56:33Z

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

Issue Details

Marking an assembly as "library" will mark all public APIs. If some of the marked members have RUC on them we decided we don't want to produce warnings, since there's nothing especially wrong with the library. The callers of those members would still get the right warnings.

Currently linker implements this correctly for methods and properties, but it fails for fields and events and it still produces some warnings, especially if the RUC is on the parent type.

This change fixes those cases and adds a special test for the library marked assembly in combination with several RUCs.

Fixes #81864

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

ghost · 2023-04-11T10:56:47Z

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

Marking an assembly as "library" will mark all public APIs. If some of the marked members have RUC on them we decided we don't want to produce warnings, since there's nothing especially wrong with the library. The callers of those members would still get the right warnings.

Currently linker implements this correctly for methods and properties, but it fails for fields and events and it still produces some warnings, especially if the RUC is on the parent type.

This change fixes those cases and adds a special test for the library marked assembly in combination with several RUCs.

Fixes #81864

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	`linkable-framework`, `area-Tools-ILLink`
Milestone:	8.0.0

lewing · 2023-04-11T13:58:24Z

@thaystg

C:\helix\work\workitem\e>dotnet.exe test DebuggerTestSuite/DebuggerTestSuite.dll "-l:trx;LogFileName=testResults.trx" "-l:console;Verbosity=normal" --results-directory "C:\helix\work\workitem\uploads\xharness-output\logs" --filter FullyQualifiedName~DebuggerTests.EvaluateOnCallFrameTests -v diag  

Welcome to .NET 8.0!
---------------------
SDK Version: 8.0.100-preview.1.23115.2

----------------
Installed an ASP.NET Core HTTPS development certificate.
To trust the certificate run 'dotnet dev-certs https --trust' (Windows and macOS only).
Learn about HTTPS: https://aka.ms/dotnet-https
----------------
Write your first app: https://aka.ms/dotnet-hello-world
Find out what's new: https://aka.ms/dotnet-whats-new
Explore documentation: https://aka.ms/dotnet-docs
Report issues and find source on GitHub: https://github.com/dotnet/core
Use 'dotnet --help' to see available commands or visit: https://aka.ms/dotnet-cli
--------------------------------------------------------------------------------------
Microsoft (R) Test Execution Command Line Tool Version 17.6.0-preview-20230126-02 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:49:47.14]     DebuggerTests.EvaluateOnCallFrameTests2.EvaluateBrowsableNone(outerClassName: "EvaluateBrowsableClassStatic", className: "TestEvaluateFieldsNone", localVarName: "testFieldsNone", breakLine: 10, allMembersAreProperties: False) [FAIL]
  Failed DebuggerTests.EvaluateOnCallFrameTests2.EvaluateBrowsableNone(outerClassName: "EvaluateBrowsableClassStatic", className: "TestEvaluateFieldsNone", localVarName: "testFieldsNone", breakLine: 10, allMembersAreProperties: False) [1 ms]
  Error Message:
   System.ArgumentException : {
  "reason": "target_closed",
  "__forMethod": "Inspector.detached"
}
  Stack Trace:
     at DebuggerTests.Inspector.OpenSessionAsync(Func`3 getInitCmds, String urlToInspect, TimeSpan span) in /_/src/mono/wasm/debugger/DebuggerTestSuite/Inspector.cs:line 377
   at DebuggerTests.DebuggerTestBase.InitializeAsync() in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 171

...
[EXECUTION TIMED OUT]
Exit Code:-3Executor timed out after 3000 seconds and was killed

['chrome-DebuggerTests.EvaluateOnCallFrameTests' END OF WORK ITEM LOG: Command timed out, and was killed]

lewing · 2023-04-11T14:01:27Z

browser failures are unrelated

eerhardt · 2023-04-11T16:20:54Z

src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresInLibraryAssembly.cs

+
+	[ExpectedNoWarnings]
+	[RequiresUnreferencedCode ("--ClassWithRequires--")]
+	public sealed class ClassWithRequires


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

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.

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

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)

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

eerhardt · 2023-04-11T16:21:22Z

src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresInLibraryAssembly.cs

+	[RequiresUnreferencedCode ("--ClassWithRequires--")]
+	public sealed class ClassWithRequires
+	{
+		public static int Field;


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

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

sbomer · 2023-04-11T18:21:43Z

src/tools/illink/src/linker/Linker.Steps/MarkStep.cs

@@ -3270,6 +3279,20 @@ protected virtual void DoAdditionalMethodProcessing (MethodDefinition method)
 		{
 		}

+		static DependencyKind PropagateDependencyKindToAccessors(DependencyKind parentDependencyKind, DependencyKind kind)


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.

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.

sbomer · 2023-04-11T18:30:16Z

src/tools/illink/src/linker/Linker.Steps/MarkStep.cs

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


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?

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

sbomer

Thanks!

vitek-karas added 3 commits April 10, 2023 12:49

First test

95cbdfa

Implementation

f268167

Fix tests

fef344d

vitek-karas added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Apr 11, 2023

vitek-karas added this to the 8.0.0 milestone Apr 11, 2023

vitek-karas requested a review from sbomer April 11, 2023 10:56

vitek-karas requested a review from marek-safar as a code owner April 11, 2023 10:56

vitek-karas self-assigned this Apr 11, 2023

ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Apr 11, 2023

lewing mentioned this pull request Apr 11, 2023

WasmTestOnBrowser-System.Text.Json.Tests.WorkItemExecution timing out #84434

Closed

eerhardt reviewed Apr 11, 2023

View reviewed changes

sbomer reviewed Apr 11, 2023

View reviewed changes

sbomer approved these changes Apr 11, 2023

View reviewed changes

PR feedback

6634d33

build-analysis bot mentioned this pull request Apr 11, 2023

IOException running NuGet-Migrations during tests in dotnet CLI first run #80619

Closed

Update test

92d106c

vitek-karas merged commit 7a0c3a3 into dotnet:main Apr 12, 2023

vitek-karas deleted the LibraryWithRUCOnType branch April 12, 2023 13:59

ghost locked as resolved and limited conversation to collaborators May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vitek-karas commented Apr 11, 2023

ghost commented Apr 11, 2023

ghost commented Apr 11, 2023

lewing commented Apr 11, 2023

lewing commented Apr 11, 2023

eerhardt Apr 11, 2023

vitek-karas Apr 11, 2023

eerhardt Apr 11, 2023

vitek-karas Apr 11, 2023

eerhardt Apr 11, 2023 •

edited

Loading

eerhardt Apr 11, 2023

vitek-karas Apr 11, 2023

sbomer Apr 11, 2023

vitek-karas Apr 11, 2023

sbomer Apr 11, 2023

vitek-karas Apr 11, 2023

sbomer left a comment

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

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

Conversation

vitek-karas commented Apr 11, 2023

ghost commented Apr 11, 2023

ghost commented Apr 11, 2023

lewing commented Apr 11, 2023

lewing commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer left a comment

Choose a reason for hiding this comment

eerhardt Apr 11, 2023 •

edited

Loading