-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Record used assembly references for more scenarios. #40138
Record used assembly references for more scenarios. #40138
Conversation
- Merged extern aliases - Embeddable attributes - Usage of canonical definitions for NoPia embedded types - nameof of a method - Deconstruction - Collection initializers Related to dotnet#37768.
@dotnet/roslyn-compiler Please review. |
@@ -258,7 +258,7 @@ public override BoundNode VisitLocalFunctionStatement(BoundLocalFunctionStatemen | |||
var typeParameters = localFunction.TypeParameters; | |||
if (typeParameters.Any(typeParameter => typeParameter.HasUnmanagedTypeConstraint)) | |||
{ | |||
_factory.CompilationState.ModuleBuilderOpt?.EnsureIsUnmanagedAttributeExists(); | |||
_factory.CompilationState.ModuleBuilderOpt?.EnsureIsUnmanagedAttributeExists(recordUsage: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnsureIsUnmanagedAttributeExists(recordUsage: true) [](start = 60, length = 51)
do we ever pass false
to this API?
(same question for EnsureNullableAttributeExists
and EnsureIsReadOnlyAttributeExists
below) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we ever pass false to this API?
Not at the moment. However, since the API is defined in a different component, which participates in phases beyond lowering, I think there is a value in making consumers of the API aware of its side-effect and providing an option to opt-out.
In reply to: 354490448 [](ancestors = 354490448)
|
||
if (conversion.Kind == ConversionKind.Deconstruction) | ||
{ | ||
Visit(conversion.DeconstructionInfo.Invocation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conversion.DeconstructionInfo.Invocation [](start = 22, length = 40)
@333fred Are we rewriting this in Nullability rewriter?
@@ -110,6 +114,15 @@ public ImmutableArray<string> RecursiveAliasesOpt | |||
} | |||
} | |||
|
|||
public ImmutableArray<MetadataReference> MergedReferences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MergedReferences [](start = 53, length = 16)
nit: consider adding xml doc as neighboring properties #Pending
|
||
MergedAliases mergedProperties; | ||
if (propertyMapOpt != null && propertyMapOpt.TryGetValue(reference, out mergedProperties)) | ||
{ | ||
aliasesOpt = mergedProperties.AliasesOpt?.ToImmutableAndFree() ?? default(ImmutableArray<string>); | ||
recursiveAliasesOpt = mergedProperties.RecursiveAliasesOpt?.ToImmutableAndFree() ?? default(ImmutableArray<string>); | ||
|
||
if (mergedProperties.MergedReferencesOpt is object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is object) [](start = 57, length = 10)
This is a struct. Do you mean to check for .IsDefault
?
Oops, sorry, it's an ArrayBuilder, not an ImmutableArray #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 3)
@dotnet/roslyn-compiler Please review, need a second sign-off. |
Related to #37768.