Propagate DynamicallyAccessedMemberTypes through Nullable<T> to T by jtschuster · Pull Request #2675 · dotnet/linker

jtschuster · 2022-03-04T01:03:59Z

This PR enables annotations placed on Nullable to propagate through to T in the linker. This does not work in the analyzer yet and will fail in CI until I update the analyzer to work with the changes.

This creates a new InstrinsicId value for Nullable.GetUnderlyingType it creates a new struct NullableSystemTypeValue that inherits from SystemTypeValue, and tracks the MultiValue of the wrapped type. It also creates NullableRuntimeTypeHandleValue which works similarly. In MethodBodyScanner, when previously a RuntimeTypeHandleValue was created in ScanLdtoken, it now will create a NullableRuntimeTypeHandleValue when the type of the handle is Nullable`1. Then when a typeof is called on that handle, it converts a NullableRuntimeTypeHandleValue to the NullableSystemTypeValue. Finally, if Nullable.GetUnderlyingType is called on that NullableSystemTypeValue, it propagates the annotations of each ValueWithDynamicallyAccessedMembers to the return value, or propagates the SystemTypeValue to the return value.

I think this covers all the areas where nullables need to be handled, but I might be missing something.

Fixes #2662

vitek-karas

Other than the comments (main worry there is handling of generic arguments), this is missing marking changes.

We need to add a test which validates that code like

[StructLayout(LayoutKind.Auto)]  // This is important since without this linker will not trim fields, but add a test for methods as well just in case
struct TestS
{
    [Kept]  // <--- this is the important bit. There's no reference to Field, it should be preserved because of DAM
    public int Field;
}

void Test()
{
    typeof(Nullable<StructS>).RequirePublicFields(); // Applies DAM PublicFields onto the type, this should propagate to the StructS.
}

Note that "marking" is not just a linker concept - in analyzer it means check all members of the "marked" type that they're not RUC (for example). So add tests which check for these warnings as well - those will be valid for both linker and analyzer.

But we do need the "Keep" tests for linker as well.

src/ILLink.Shared/TrimAnalysis/RuntimeTypeHandleValue.cs

src/ILLink.Shared/TrimAnalysis/SystemTypeValue.cs

src/linker/Linker.Dataflow/MethodBodyScanner.cs

test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

src/ILLink.Shared/TrimAnalysis/SystemTypeValue.cs

vitek-karas · 2022-03-08T12:42:15Z

src/ILLink.Shared/TrimAnalysis/RuntimeTypeHandleValue.cs

-	record RuntimeTypeHandleValue : SingleValue
+	sealed record RuntimeTypeHandleValue : SingleValue
 	{
 		public RuntimeTypeHandleValue (in TypeProxy representedType) => RepresentedType = representedType;


Maybe add an assert that the type is not Nullable<T>

When I added an assert I ran into cases where System.Nullable`1 wouldn't have a type argument, so it wouldn't make a NullableValue and would instantiate a SystemTypeValue or RuntimeTypeHandleValue. To me a SystemTypeValue makes sense since there is nothing extra to track in those situations, but would it make more sense to have the UnderlyingType of NullableValue's be optional?

src/ILLink.Shared/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Generates NullableValue's in a few missing places, and tracks NullValue.Instance's in TypeHandle intrinsics.

vitek-karas

I think we need to fix the MakeGenericType as it currently creates an analysis hole which could break apps.

src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs

src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs

Also adds ArrayCreationOperation visitors to create ArrayValue's. Also addresses some other PR comments.

jtschuster · 2022-03-10T18:47:12Z

src/ILLink.Shared/TrimAnalysis/NullableRuntimeTypeHandleWithGenericParameterValue.cs

+	/// </summary>
+	sealed record NullableRuntimeTypeWithDamHandleValue : SingleValue
+	{
+		public NullableRuntimeTypeWithDamHandleValue (in TypeProxy nullableType, in SingleValue underlyingTypeValue)


`UnderlyingTypeValue needs to be SingleValue here because in MethodBodyScanner, a RuntimeTypeHandleForGenericParameterValue is passed as the underlying type.

* wip * Fixes * Improve array creation - correctly initialize size and support multi-value for size

eng/Version.Details.xml

src/ILLink.RoslynAnalyzer/TrimAnalysis/ArrayValue.cs

src/ILLink.RoslynAnalyzer/TrimAnalysis/SingleValueExtensions.cs

src/ILLink.Shared/TrimAnalysis/NullableRuntimeSystemTypeHandleValue.cs

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

src/ILLink.Shared/TrimAnalysis/NullableSystemTypeValue.cs

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

src/ILLink.RoslynAnalyzer/TrimAnalysis/ArrayValue.cs

src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

src/ILLink.RoslynAnalyzer/TrimAnalysis/SingleValueExtensions.cs

src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs

src/ILLink.Shared/DataFlow/ValueSet.cs

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

sbomer · 2022-03-14T21:09:06Z

src/ILLink.Shared/DataFlow/IDeepCopyValue.cs

+namespace ILLink.Shared.DataFlow
+{
+	// Adds ability to deep copy a value
+	public interface IDeepCopyValue<TSingleValue>


I think ideally this would be required by every type used as a value in a lattice, but we could do that in a separate change.

Yeah - I think it makes sense to require it everywhere - this change does require it on SingleValue. Other values - I would also prefer a different PR>

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

sbomer · 2022-03-14T21:38:10Z

src/ILLink.Shared/TrimAnalysis/NullableSystemTypeValue.cs

+namespace ILLink.Shared.TrimAnalysis
+{
+	/// <summary>
+	/// This represents a Nullable<T> where T is a known SystemTypeValue.


If this is true, should we enforce at the type system level that the underlying type is a SystemTypeValue?

I think I considered that, but Vitek had a reason to leave it as SingleValue. I can't remember exactly why, but having it different makes sure that we handle it separately from SystemTypeValue instead of accidentally matching NullableSystemTypeValue to the SystemTypeValue case.

I think it's OK to change the underlying type to SystemTypeValue if it's always that.
The NullableSystemTypeValue must be separate from SystemTypeValue - it does act very similarly, but it adds the "this is Nullable flag" to the type.

Now I remember we also wanted to keep SystemTypeValue sealed to make type checking faster. I think it might be best to leave it as inheriting from SingleValue.

Sorry - what I meant was, can we make the underlyingType a SystemTypeValue (not that NullableSystemTypeValue should itself be a SystemTypeValue)? I think this is more consistent with how the underlying type of a NullableValueWithDynamicallyAccessedMembers is also a dataflow value, rather than a TypeProxy.

Same question for RuntimeTypeHandleForNullableSystemTypeValue.

src/ILLink.Shared/TrimAnalysis/NullableRuntimeSystemTypeHandleValue.cs

src/ILLink.Shared/TrimAnalysis/NullableRuntimeTypeHandleWithGenericParameterValue.cs

Adds tests to validate dataflow in multidimensional arrays, adds a temporary nullable MethodReturnValue variable to differentiate intentional Top values and unassigned return values. Moves the static MultiValueLattice field to the shared ArrayValue definition.

… nullables

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs

…onal arrays

vitek-karas

Looks good. Let's wait on Sven for another opinion.

sbomer

A few more comments, overall LGTM. Thanks a lot!

sbomer · 2022-03-18T00:26:59Z

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

+								AddReturnValue (new NullableValueWithDynamicallyAccessedMembers (typeValue.RepresentedType, damValue));
+								break;
+							// Don't warn on Nullable<T>, it will throw instead
+							case NullableSystemTypeValue:


nit: maybe put this and NullableValueWithDynamicallyAccessedMembers next to each other and have one fall through - since they aren't tracked for the same reason.

sbomer · 2022-03-18T00:31:18Z

src/ILLink.Shared/TrimAnalysis/NullableSystemTypeValue.cs

+namespace ILLink.Shared.TrimAnalysis
+{
+	/// <summary>
+	/// This represents a Nullable<T> where T is a known SystemTypeValue.


Sorry - what I meant was, can we make the underlyingType a SystemTypeValue (not that NullableSystemTypeValue should itself be a SystemTypeValue)? I think this is more consistent with how the underlying type of a NullableValueWithDynamicallyAccessedMembers is also a dataflow value, rather than a TypeProxy.

sbomer · 2022-03-18T00:34:34Z

src/ILLink.Shared/TrimAnalysis/NullableSystemTypeValue.cs

+namespace ILLink.Shared.TrimAnalysis
+{
+	/// <summary>
+	/// This represents a Nullable<T> where T is a known SystemTypeValue.


Same question for RuntimeTypeHandleForNullableSystemTypeValue.

sbomer · 2022-03-18T00:40:32Z

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

+							new GenericParameterValue (gp, _context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (gp)));
+
+					case TypeReference underlyingType:
+						return new NullableSystemTypeValue (genericArgumentType, ResolveToTypeDefinition (underlyingType)!);


Probably uncommon, but I think it's possible for ResolveToTypeDefinition to fail - maybe this should return an UnknownValue if it resolves to null?

sbomer · 2022-03-18T00:41:32Z

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

+											case ValueWithDynamicallyAccessedMembers damValue:
+												AddReturnValue (new NullableValueWithDynamicallyAccessedMembers (typeValue.RepresentedType, damValue));
+												break;
+											// Don't warn on Nullable<T>, it will throw instead


Same nit here, I would combine these two cases.

sbomer · 2022-03-18T00:43:08Z

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs


 			return true;
+
+			void AddReturnValue (MultiValue value)


Thanks for making this more consistent with the shared code!

src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

jtschuster · 2022-03-18T01:18:28Z

@sbomer
Would you mind opening an issue to track making the analyzer consistent with the linker on this?

Unless I'm misunderstanding what you're talking about, it looks like #2680 tracks the overall discrepancy between the analyzer and linker for arrays. I'll make a note about multidimensional arrays though.

…tnet/linker#2675) Add intrinsic support for Nullable.GetUnderlyingType and support for MakeGenericType with Nullables Adds ArrayCreationOperation visitors to create ArrayValue's in the analyzer, and adds start of dataflow analysis for array values. Adds tests to validate dataflow in Arrays. Co-authored-by: vitek-karas <[email protected]> Commit migrated from dotnet/linker@ed8b22a

jtschuster added 4 commits March 1, 2022 17:15

Add instrinsic support for Nullable.GetUnderlyingType

cb97c35

passing first test

7504ed6

cleaned up to remove TypeProxy changes

c21a803

Formatting

6ac243b

jtschuster requested review from MichalStrehovsky, sbomer and vitek-karas March 4, 2022 01:04

Remove usings

95e6955

vitek-karas reviewed Mar 4, 2022

View reviewed changes

Adds new tests, works in linker when TestClass is a struct

82f3b89

vitek-karas reviewed Mar 8, 2022

View reviewed changes

src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Mar 8, 2022

View reviewed changes

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs Show resolved Hide resolved

vitek-karas reviewed Mar 8, 2022

View reviewed changes

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs Show resolved Hide resolved

vitek-karas reviewed Mar 8, 2022

View reviewed changes

src/ILLink.Shared/TrimAnalysis/SystemTypeValue.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Mar 8, 2022

View reviewed changes

src/ILLink.Shared/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs Show resolved Hide resolved

vitek-karas reviewed Mar 8, 2022

View reviewed changes

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs Outdated Show resolved Hide resolved

jtschuster added 5 commits March 8, 2022 11:26

Generate NullableValues in Analyzer and track NullValue

6a8e088

Generates NullableValue's in a few missing places, and tracks NullValue.Instance's in TypeHandle intrinsics.

Formatting

cc16aa0

Add missing ExpectedWarning

11e949b

Formatting

3cf3dd9

More formatting

d107a60

jtschuster marked this pull request as ready for review March 8, 2022 22:00

jtschuster requested a review from marek-safar as a code owner March 8, 2022 22:00

vitek-karas requested changes Mar 9, 2022

View reviewed changes

jtschuster and others added 3 commits March 9, 2022 16:50

Support for MakeGenericType with Nullables

dfc5644

Also adds ArrayCreationOperation visitors to create ArrayValue's. Also addresses some other PR comments.

Share Type.Base intrinsic

9be2701

Add support for Nullable in MakeGenericType

02c0d8e

jtschuster commented Mar 10, 2022

View reviewed changes

Remove redundant tests and format

412dc67

jtschuster and others added 6 commits March 10, 2022 17:11

Merge branch 'nullables' into MakeGenericTypeFix

4cd2d12

Copy ArrayValue on Meet

ca200f4

formatting

0770bca

mostly formatting

37ebdb9

Nullable T improvements (#1)

782da4a

* wip * Fixes * Improve array creation - correctly initialize size and support multi-value for size

Remove unnecessary type checks in ValueSet.Clone()

e1fe616

vitek-karas reviewed Mar 14, 2022

View reviewed changes

jtschuster added 3 commits March 14, 2022 10:31

Reset eng/ to match main

0864704

reset all changes to eng/ and global.json to main at branch

6fc8dfe

Merge branch 'main' into nullables

bc7e2f4

sbomer reviewed Mar 14, 2022

View reviewed changes

sbomer reviewed Mar 15, 2022

View reviewed changes

src/ILLink.Shared/TrimAnalysis/NullableRuntimeTypeHandleWithGenericParameterValue.cs Outdated Show resolved Hide resolved

src/ILLink.Shared/TrimAnalysis/NullableRuntimeTypeHandleWithGenericParameterValue.cs Show resolved Hide resolved

jtschuster added 4 commits March 15, 2022 10:24

wip

38dd521

Merge branch 'nullables' of https://github.com/jtschuster/linker into…

9caa741

… nullables

vitek-karas reviewed Mar 16, 2022

View reviewed changes

src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Mar 16, 2022

View reviewed changes

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Mar 16, 2022

View reviewed changes

test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs Outdated Show resolved Hide resolved

jtschuster added 4 commits March 16, 2022 15:21

Rename TypeHandles, default to warn on MakeGenericType

57050f5

Update comment to reflect that we do not plan to support multidimensi…

fa8e8d6

…onal arrays

formatting

f8c25be

Remove extra parentheses

9f6a1a9

vitek-karas approved these changes Mar 17, 2022

View reviewed changes

sbomer approved these changes Mar 18, 2022

View reviewed changes

Change underlyingType to SingleValue, fix feedback

426b8cd

jtschuster merged commit ed8b22a into dotnet:main Mar 18, 2022

Conversation

jtschuster commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtschuster Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtschuster commented Mar 4, 2022 •

edited

Loading

jtschuster Mar 8, 2022 •

edited

Loading