Skip to content

Commit 5bdc36e

Browse files
vitek-karasvitek-karas
authored
Implements Requires*Attribute on class behavior for NativeAOT (#83417)
Implements most of the missing pieces to get Requires on class working correctly in NativeAOT. Major changes: * Detect Requires mismatch between derived and base class * Warn on field access if the owning class has Requires * Changes to reflection marking to warn on more cases (instance methods on Requires classes for example) Supportive changes: * The helpers to detect Requires attributes now return the found attribute view out parameter Fixes #81158 Still two missing pieces - tracked by #82447: * Requires on attributes - NativeAOT doesn't handle this at all yet, part of it is Requires on the attribute class * Avoid warning when DAM marking an override method which has Requires (or its class has) - this avoids lot of noise, NativeAOT currently generates these warnings in full
1 parent 2d82829 commit 5bdc36e

File tree

13 files changed

+340
-113
lines changed

13 files changed

+340
-113
lines changed

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DiagnosticUtilities.cs‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System.Diagnostics;
54
using System.Diagnostics.CodeAnalysis;
65
using System.Reflection.Metadata;
76
using Internal.TypeSystem;
@@ -37,7 +36,7 @@ internal static string GetGenericParameterDeclaringMemberDisplayName(GenericPara
3736
internal static bool TryGetRequiresAttribute(TypeSystemEntity member, string requiresAttributeName, [NotNullWhen(returnValue: true)] out CustomAttributeValue<TypeDesc>? attribute)
3837
{
3938
attribute = default;
40-
CustomAttributeValue<TypeDesc>? decoded = default;
39+
CustomAttributeValue<TypeDesc>? decoded;
4140
switch (member)
4241
{
4342
case MethodDesc method:
@@ -59,8 +58,9 @@ internal static bool TryGetRequiresAttribute(TypeSystemEntity member, string req
5958
decoded = @event.GetDecodedCustomAttribute("System.Diagnostics.CodeAnalysis", requiresAttributeName);
6059
break;
6160
default:
62-
Debug.Fail("Trying to operate with unsupported TypeSystemEntity " + member.GetType().ToString());
63-
break;
61+
// This can happen for a compiler generated method, for example if mark methods on array for reflection (through DAM)
62+
// There are several different types which can occur here, but none should ever have any of Requires* attributes.
63+
return false;
6464
}
6565
if (!decoded.HasValue)
6666
return false;
@@ -92,21 +92,21 @@ internal static string GetRequiresAttributeUrl(CustomAttributeValue<TypeDesc> at
9292
/// <remarks>Unlike <see cref="DoesMemberRequire(TypeSystemEntity, string, out CustomAttributeValue{TypeDesc}?)"/>
9393
/// if a declaring type has Requires, all methods in that type are considered "in scope" of that Requires. So this includes also
9494
/// instance methods (not just statics and .ctors).</remarks>
95-
internal static bool IsInRequiresScope(this MethodDesc method, string requiresAttribute) =>
96-
method.IsInRequiresScope(requiresAttribute, true);
95+
internal static bool IsInRequiresScope(this MethodDesc method, string requiresAttribute)
96+
=> IsInRequiresScope(method, requiresAttribute, out _);
9797

98-
private static bool IsInRequiresScope(this MethodDesc method, string requiresAttribute, bool checkAssociatedSymbol)
98+
internal static bool IsInRequiresScope(this MethodDesc method, string requiresAttribute, [NotNullWhen(returnValue: true)] out CustomAttributeValue<TypeDesc>? attribute)
9999
{
100-
if (method.HasCustomAttribute("System.Diagnostics.CodeAnalysis", requiresAttribute) && !method.IsStaticConstructor)
100+
if (TryGetRequiresAttribute(method, requiresAttribute, out attribute) && !method.IsStaticConstructor)
101101
return true;
102102

103-
if (method.OwningType is TypeDesc type && TryGetRequiresAttribute(type, requiresAttribute, out _))
103+
if (method.OwningType is TypeDesc type && TryGetRequiresAttribute(type, requiresAttribute, out attribute))
104104
return true;
105105

106-
if (checkAssociatedSymbol && method.GetPropertyForAccessor() is PropertyPseudoDesc property && TryGetRequiresAttribute(property, requiresAttribute, out _))
106+
if (method.GetPropertyForAccessor() is PropertyPseudoDesc property && TryGetRequiresAttribute(property, requiresAttribute, out attribute))
107107
return true;
108108

109-
if (checkAssociatedSymbol && method.GetEventForAccessor() is EventPseudoDesc @event && TryGetRequiresAttribute(@event, requiresAttribute, out _))
109+
if (method.GetEventForAccessor() is EventPseudoDesc @event && TryGetRequiresAttribute(@event, requiresAttribute, out attribute))
110110
return true;
111111

112112
return false;
@@ -153,6 +153,9 @@ internal static bool DoesPropertyRequire(this PropertyPseudoDesc property, strin
153153
internal static bool DoesEventRequire(this EventPseudoDesc @event, string requiresAttribute, [NotNullWhen(returnValue: true)] out CustomAttributeValue<TypeDesc>? attribute) =>
154154
TryGetRequiresAttribute(@event, requiresAttribute, out attribute);
155155

156+
internal static bool DoesTypeRequire(this TypeDesc type, string requiresAttribute, [NotNullWhen(returnValue: true)] out CustomAttributeValue<TypeDesc>? attribute) =>
157+
TryGetRequiresAttribute(type, requiresAttribute, out attribute);
158+
156159
/// <summary>
157160
/// Determines if member requires (and thus any usage of such method should be warned about).
158161
/// </summary>

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,11 @@ internal void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystem
203203
if (!_enabled)
204204
return;
205205

206-
if (entity.DoesMemberRequire(DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out CustomAttributeValue<TypeDesc>? requiresAttribute))
206+
// Note that we're using `ShouldSuppressAnalysisWarningsForRequires` instead of `DoesMemberRequire`.
207+
// This is because reflection access is actually problematic on all members which are in a "requires" scope
208+
// so for example even instance methods. See for example https://github.com/dotnet/linker/issues/3140 - it's possible
209+
// to call a method on a "null" instance via reflection.
210+
if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out CustomAttributeValue<TypeDesc>? requiresAttribute))
207211
{
208212
if (_typeHierarchyDataFlowOrigin is not null)
209213
{
@@ -215,11 +219,11 @@ internal void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystem
215219
}
216220
else
217221
{
218-
ReportRequires(origin, entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute);
222+
ReportRequires(origin, entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, requiresAttribute.Value);
219223
}
220224
}
221225

222-
if (entity.DoesMemberRequire(DiagnosticUtilities.RequiresAssemblyFilesAttribute, out _))
226+
if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresAssemblyFilesAttribute, out requiresAttribute))
223227
{
224228
if (_typeHierarchyDataFlowOrigin is not null)
225229
{
@@ -229,11 +233,11 @@ internal void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystem
229233
}
230234
else
231235
{
232-
ReportRequires(origin, entity, DiagnosticUtilities.RequiresAssemblyFilesAttribute);
236+
ReportRequires(origin, entity, DiagnosticUtilities.RequiresAssemblyFilesAttribute, requiresAttribute.Value);
233237
}
234238
}
235239

236-
if (entity.DoesMemberRequire(DiagnosticUtilities.RequiresDynamicCodeAttribute, out _))
240+
if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresDynamicCodeAttribute, out requiresAttribute))
237241
{
238242
if (_typeHierarchyDataFlowOrigin is not null)
239243
{
@@ -243,7 +247,7 @@ internal void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystem
243247
}
244248
else
245249
{
246-
ReportRequires(origin, entity, DiagnosticUtilities.RequiresDynamicCodeAttribute);
250+
ReportRequires(origin, entity, DiagnosticUtilities.RequiresDynamicCodeAttribute, requiresAttribute.Value);
247251
}
248252
}
249253

@@ -277,7 +281,7 @@ internal void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystem
277281
}
278282
}
279283

280-
private void ReportRequires(in MessageOrigin origin, TypeSystemEntity entity, string requiresAttributeName)
284+
private void ReportRequires(in MessageOrigin origin, TypeSystemEntity entity, string requiresAttributeName, in CustomAttributeValue<TypeDesc> requiresAttribute)
281285
{
282286
var diagnosticContext = new DiagnosticContext(
283287
origin,
@@ -286,7 +290,7 @@ private void ReportRequires(in MessageOrigin origin, TypeSystemEntity entity, st
286290
_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresAssemblyFilesAttribute),
287291
_logger);
288292

289-
ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, entity, requiresAttributeName);
293+
ReflectionMethodBodyScanner.ReportRequires(diagnosticContext, entity, requiresAttributeName, requiresAttribute);
290294
}
291295
}
292296
}

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Diagnostics;
77
using System.Diagnostics.CodeAnalysis;
88
using System.Linq;
9+
using System.Reflection.Metadata;
910
using ILCompiler.Logging;
1011
using ILLink.Shared;
1112
using ILLink.Shared.TrimAnalysis;
@@ -61,6 +62,11 @@ internal static void CheckAndReportRequires(in DiagnosticContext diagnosticConte
6162
if (!calledMember.DoesMemberRequire(requiresAttributeName, out var requiresAttribute))
6263
return;
6364

65+
ReportRequires(diagnosticContext, calledMember, requiresAttributeName, requiresAttribute.Value);
66+
}
67+
68+
internal static void ReportRequires(in DiagnosticContext diagnosticContext, TypeSystemEntity calledMember, string requiresAttributeName, in CustomAttributeValue<TypeDesc> requiresAttribute)
69+
{
6470
DiagnosticId diagnosticId = requiresAttributeName switch
6571
{
6672
DiagnosticUtilities.RequiresUnreferencedCodeAttribute => DiagnosticId.RequiresUnreferencedCode,
@@ -69,8 +75,8 @@ internal static void CheckAndReportRequires(in DiagnosticContext diagnosticConte
6975
_ => throw new NotImplementedException($"{requiresAttributeName} is not a valid supported Requires attribute"),
7076
};
7177

72-
string arg1 = MessageFormat.FormatRequiresAttributeMessageArg(DiagnosticUtilities.GetRequiresAttributeMessage(requiresAttribute.Value));
73-
string arg2 = MessageFormat.FormatRequiresAttributeUrlArg(DiagnosticUtilities.GetRequiresAttributeUrl(requiresAttribute.Value));
78+
string arg1 = MessageFormat.FormatRequiresAttributeMessageArg(DiagnosticUtilities.GetRequiresAttributeMessage(requiresAttribute));
79+
string arg2 = MessageFormat.FormatRequiresAttributeUrlArg(DiagnosticUtilities.GetRequiresAttributeUrl(requiresAttribute));
7480

7581
diagnosticContext.AddDiagnostic(diagnosticId, calledMember.GetDisplayName(), arg1, arg2);
7682
}
@@ -152,15 +158,19 @@ protected override MultiValue HandleGetField(MethodIL methodBody, int offset, Fi
152158
{
153159
_origin = _origin.WithInstructionOffset(methodBody, offset);
154160

161+
if (field.DoesFieldRequire(DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out _) ||
162+
field.DoesFieldRequire(DiagnosticUtilities.RequiresDynamicCodeAttribute, out _) ||
163+
field.DoesFieldRequire(DiagnosticUtilities.RequiresAssemblyFilesAttribute, out _))
164+
TrimAnalysisPatterns.Add(new TrimAnalysisFieldAccessPattern(field, _origin));
165+
155166
ProcessGenericArgumentDataFlow(field);
156167

157168
return _annotations.GetFieldValue(field);
158169
}
159170

160171
private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, int offset, ValueWithDynamicallyAccessedMembers targetValue, MultiValue sourceValue, string reason)
161172
{
162-
// We must record all field accesses since we need to check RUC/RDC/RAF attributes on them regardless of annotations
163-
if (targetValue.DynamicallyAccessedMemberTypes != 0 || targetValue is FieldValue)
173+
if (targetValue.DynamicallyAccessedMemberTypes != 0)
164174
{
165175
_origin = _origin.WithInstructionOffset(methodBody, offset);
166176
HandleAssignmentPattern(_origin, sourceValue, targetValue, reason);

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -52,16 +52,6 @@ public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker, Logger
5252
{
5353
foreach (var targetValue in Target)
5454
{
55-
if (targetValue is FieldValue fieldValue)
56-
{
57-
// Once this is removed, please also cleanup ReflectionMethodBodyScanner.HandleStoreValueWithDynamicallyAccessedMembers
58-
// which has to special case FieldValue right now, should not be needed after removal of this
59-
ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, fieldValue.Field, DiagnosticUtilities.RequiresUnreferencedCodeAttribute);
60-
ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, fieldValue.Field, DiagnosticUtilities.RequiresDynamicCodeAttribute);
61-
// ?? Should this be enabled (was not so far)
62-
//ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, fieldValue.Field, DiagnosticUtilities.RequiresAssemblyFilesAttribute);
63-
}
64-
6555
if (targetValue is not ValueWithDynamicallyAccessedMembers targetWithDynamicallyAccessedMembers)
6656
throw new NotImplementedException();
6757

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisFieldAccessPattern.cs‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using ILCompiler.Logging;
5+
using ILLink.Shared.TrimAnalysis;
6+
using Internal.TypeSystem;
7+
8+
namespace ILCompiler.Dataflow
9+
{
10+
public readonly record struct TrimAnalysisFieldAccessPattern
11+
{
12+
public FieldDesc Field { init; get; }
13+
public MessageOrigin Origin { init; get; }
14+
15+
public TrimAnalysisFieldAccessPattern(FieldDesc field, MessageOrigin origin)
16+
{
17+
Field = field;
18+
Origin = origin;
19+
}
20+
21+
// No Merge - there's nothing to merge since this pattern is uniquely identified by both the origin and the entity
22+
// and there's only one way to "access" a field.
23+
24+
public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker, Logger logger)
25+
{
26+
var diagnosticContext = new DiagnosticContext(
27+
Origin,
28+
logger.ShouldSuppressAnalysisWarningsForRequires(Origin.MemberDefinition, DiagnosticUtilities.RequiresUnreferencedCodeAttribute),
29+
logger.ShouldSuppressAnalysisWarningsForRequires(Origin.MemberDefinition, DiagnosticUtilities.RequiresDynamicCodeAttribute),
30+
logger.ShouldSuppressAnalysisWarningsForRequires(Origin.MemberDefinition, DiagnosticUtilities.RequiresAssemblyFilesAttribute),
31+
logger);
32+
33+
ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, Field, DiagnosticUtilities.RequiresUnreferencedCodeAttribute);
34+
ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, Field, DiagnosticUtilities.RequiresDynamicCodeAttribute);
35+
ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, Field, DiagnosticUtilities.RequiresAssemblyFilesAttribute);
36+
}
37+
}
38+
}

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ public readonly struct TrimAnalysisPatternStore
1717
private readonly Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern> MethodCallPatterns;
1818
private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisReflectionAccessPattern> ReflectionAccessPatterns;
1919
private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern> GenericInstantiations;
20+
private readonly Dictionary<(MessageOrigin, FieldDesc), TrimAnalysisFieldAccessPattern> FieldAccessPatterns;
2021
private readonly ValueSetLattice<SingleValue> Lattice;
2122
private readonly Logger _logger;
2223

@@ -26,6 +27,7 @@ public TrimAnalysisPatternStore(ValueSetLattice<SingleValue> lattice, Logger log
2627
MethodCallPatterns = new Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern>();
2728
ReflectionAccessPatterns = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisReflectionAccessPattern>();
2829
GenericInstantiations = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern>();
30+
FieldAccessPatterns = new Dictionary<(MessageOrigin, FieldDesc), TrimAnalysisFieldAccessPattern>();
2931
Lattice = lattice;
3032
_logger = logger;
3133
}
@@ -74,6 +76,14 @@ public void Add(TrimAnalysisGenericInstantiationAccessPattern pattern)
7476
// and there's only one way to "access" a generic instantiation.
7577
}
7678

79+
public void Add(TrimAnalysisFieldAccessPattern pattern)
80+
{
81+
FieldAccessPatterns.TryAdd((pattern.Origin, pattern.Field), pattern);
82+
83+
// No Merge - there's nothing to merge since this pattern is uniquely identified by both the origin and the entity
84+
// and there's only one way to "access" a field.
85+
}
86+
7787
public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker)
7888
{
7989
foreach (var pattern in AssignmentPatterns.Values)
@@ -87,6 +97,9 @@ public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker)
8797

8898
foreach (var pattern in GenericInstantiations.Values)
8999
pattern.MarkAndProduceDiagnostics(reflectionMarker, _logger);
100+
101+
foreach (var pattern in FieldAccessPatterns.Values)
102+
pattern.MarkAndProduceDiagnostics(reflectionMarker, _logger);
90103
}
91104
}
92105
}

0 commit comments

Comments
 (0)