Add option to emit nullable metadata for public members only#36398
Add option to emit nullable metadata for public members only#36398cston merged 16 commits intodotnet:masterfrom
Conversation
docs/features/nullable-metadata.md
Outdated
There was a problem hiding this comment.
attributes for private #Resolved
There was a problem hiding this comment.
You might consider putting a link the nullable attribute spec here, so it's easier to understand the logic #Resolved
|
LGTM, Modulo the failing tests for expression evaluation |
| @@ -0,0 +1,191 @@ | |||
| Nullable Metadata | |||
There was a problem hiding this comment.
nit: FWIW, I'd prefer if we merged this doc into the main design doc. #Closed
There was a problem hiding this comment.
I'd prefer to see this as a separate document because the metadata spec is now longer and can stand alone.
In reply to: 293550340 [](ancestors = 293550340)
| [System.AttributeUsage( | ||
| AttributeTargets.Module | | ||
| AttributeTargets.Class | | ||
| AttributeTargets.Delegate | |
There was a problem hiding this comment.
Delegate [](start = 25, length = 8)
Why allow on Delegate? #Resolved
There was a problem hiding this comment.
Why allow on Delegate?
I think, with respect to this attribute, delegates should be valid targets as other classes
In reply to: 293552115 [](ancestors = 293552115)
docs/features/nullable-metadata.md
Outdated
| ```C# | ||
| namespace System.Runtime.CompilerServices | ||
| { | ||
| public enum NullableMembers |
There was a problem hiding this comment.
NullableMembers [](start = 16, length = 15)
Out-of-date from our discussion today #Closed
docs/features/nullable-metadata.md
Outdated
| without explicit `NullableAttribute` attributes. | ||
|
|
||
| To reduce the size of metadata, the C#8 compiler will not emit attributes for `private` members, | ||
| and the compiler will only emit attributes for `internal` members if the assembly contains |
There was a problem hiding this comment.
assembly [](start = 73, length = 8)
module? #Closed
There was a problem hiding this comment.
InternalsVisibleToAttribute is an assembly-level attribute.
In reply to: 293554207 [](ancestors = 293554207)
docs/features/nullable-metadata.md
Outdated
|
|
||
| _If necessary, a compiler option could be added in a future release to override the default behavior and | ||
| explicitly emit or drop nullable attributes for `private` or `internal` members._ | ||
|
|
| _If necessary, a compiler option could be added in a future release to override the default behavior and | ||
| explicitly emit or drop nullable attributes for `private` or `internal` members._ | ||
|
|
||
| ## Compatibility |
There was a problem hiding this comment.
Compatibility [](start = 3, length = 13)
Not related to this PR: when making the compaction change, let's add an entry to the breaking changes doc, even though this is only a break from previous previews.
| internal static readonly AttributeDescription AssemblyAlgorithmIdAttribute = new AttributeDescription("System.Reflection", "AssemblyAlgorithmIdAttribute", s_signaturesOfAssemblyAlgorithmIdAttribute); | ||
| internal static readonly AttributeDescription DeprecatedAttribute = new AttributeDescription("Windows.Foundation.Metadata", "DeprecatedAttribute", s_signaturesOfDeprecatedAttribute); | ||
| internal static readonly AttributeDescription NullableAttribute = new AttributeDescription("System.Runtime.CompilerServices", "NullableAttribute", s_signaturesOfNullableAttribute); | ||
| internal static readonly AttributeDescription NullableMembersAttribute = new AttributeDescription("System.Runtime.CompilerServices", "NullableMembersAttribute", s_signaturesOfNullableMembersAttribute); |
There was a problem hiding this comment.
NullableMembersAttribute [](start = 54, length = 24)
It doesn't looks like we pay attention to this attribute on import. Are we using this description anywhere? Perhaps we should disallow an explicit attribute application in source. #Closed
There was a problem hiding this comment.
This AttributeDescription is used by PEAssemblyBuilder in determining whether to synthesize the type.
In reply to: 293917589 [](ancestors = 293917589,293911855,293557364)
| /// (public, protected, and if any [InternalsVisibleTo] attributes, internal members). | ||
| /// If false, attributes are emitted for all members regardless of visibility. | ||
| /// </summary> | ||
| internal bool EmitPublicNullableMetadataOnly { get; private set; } |
There was a problem hiding this comment.
EmitPublicNullableMetadataOnly [](start = 22, length = 30)
From discussion today, we'll use a feature flag for now. #Resolved
| return IsSymbolAccessibleCore(symbol, within, throughTypeOpt, out failedThroughTypeCheck, within.DeclaringCompilation, ref useSiteDiagnostics, basesBeingResolved); | ||
| } | ||
|
|
||
| internal static bool IsPublicOrInternal(Symbol symbol, out bool isInternal) |
There was a problem hiding this comment.
IsPublicOrInternal [](start = 29, length = 18)
nit: xml doc would be useful here #Closed
| // Note: we don't need to warn on annotations used without NonNullTypes context for local functions, as this is handled in binding already | ||
| var compilation = DeclaringCompilation; | ||
| ParameterHelpers.EnsureIsReadOnlyAttributeExists(compilation, parameters, diagnostics, modifyCompilation: false); | ||
| ParameterHelpers.EnsureNullableAttributeExists(compilation, this, parameters, diagnostics, modifyCompilation: false); |
There was a problem hiding this comment.
EnsureNullableAttributeExists [](start = 29, length = 29)
Should we guard this call to Never mind, EnsureNullableAttributeExists with an accessibility check?ParameterHelpers does that #Closed
| ParameterHelpers.EnsureNullableAttributeExists(Parameters, diagnostics, modifyCompilation: true); | ||
| var compilation = DeclaringCompilation; | ||
| ParameterHelpers.EnsureIsReadOnlyAttributeExists(compilation, Parameters, diagnostics, modifyCompilation: true); | ||
| ParameterHelpers.EnsureNullableAttributeExists(compilation, this, Parameters, diagnostics, modifyCompilation: true); |
There was a problem hiding this comment.
EnsureNullableAttributeExists [](start = 29, length = 29)
Same question here (should we guard with an access check?) Never mind, ParameterHelpers does that #Closed
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.Symbols | ||
| { | ||
| internal sealed class SynthesizedEmbeddedNullableMembersAttributeSymbol : SynthesizedEmbeddedAttributeSymbolBase |
There was a problem hiding this comment.
SynthesizedEmbeddedNullableMembersAttributeSymbol [](start = 26, length = 49)
I'll skip reviewing this file in this iteration, since changes are planned #Closed
| base.AddSynthesizedAttributes(moduleBuilder, ref attributes); | ||
|
|
||
| CSharpCompilation compilation = this.DeclaringCompilation; | ||
| var type = this.TypeWithAnnotations; |
There was a problem hiding this comment.
type [](start = 16, length = 4)
nit: typeWithAnnotations would be better name to avoid confusion with this.Type below. Consider declaring a local var type = typeWithAnnotations.Type to replace this.Type. #Closed
| } | ||
| "; | ||
|
|
||
| protected const string NullableMembersAttributeDefinition = @" |
There was a problem hiding this comment.
NullableMembersAttributeDefinition [](start = 31, length = 34)
seems unused at the moment #Closed
| VisitList(@namespace.GetMembers()); | ||
| } | ||
|
|
||
| public override void VisitNamedType(NamedTypeSymbol type) |
There was a problem hiding this comment.
VisitNamedType [](start = 29, length = 14)
Should we visit constraints? (same comment for VisitMethod) #Closed
There was a problem hiding this comment.
Visiting constraints doesn't fit easily into this model since we're assuming all symbols are definitions.
In reply to: 293585452 [](ancestors = 293585452)
| return new string(' ', level * 4); | ||
| } | ||
|
|
||
| private static readonly SymbolDisplayFormat _displayFormat = |
There was a problem hiding this comment.
_displayFormat [](start = 52, length = 14)
Assuming this is a variant of an existing format, could it be expressed as ExistingFormat.With(change) instead? I think that is easier to maintain when we add new options. #Closed
|
|
||
| private void ReportSymbol(Symbol symbol, bool includeAlways = false) | ||
| { | ||
| var attributes = (symbol.Kind == SymbolKind.Method) ? ((MethodSymbol)symbol).GetReturnTypeAttributes() : symbol.GetAttributes(); |
There was a problem hiding this comment.
(symbol.Kind == SymbolKind.Method) [](start = 29, length = 34)
nit: consider a type test symbol is MethodSymbol method ? method. ... #Closed
| return; | ||
| } | ||
| ReportContainingSymbols(symbol.ContainingSymbol); | ||
| ReportSymbol(symbol, includeAlways: true); |
There was a problem hiding this comment.
, includeAlways: true [](start = 31, length = 21)
I didn't understand this part. We always want to report containing symbols? #Closed
There was a problem hiding this comment.
When we need to report a specific symbol, we first report all ancestors, from the outermost type down.
In reply to: 293589349 [](ancestors = 293589349)
|
|
||
| symbol = symbol.ContainingType; | ||
| } | ||
| while (!(symbol is null)); |
There was a problem hiding this comment.
!(symbol is null) [](start = 19, length = 17)
symbol is object? #Closed
| /// Returns true if the symbol is effectively public or internal based on | ||
| /// the declared accessibility of the symbol and any containing symbols. | ||
| /// </summary> | ||
| internal static bool IsPublicOrInternal(Symbol symbol, out bool isInternal) |
There was a problem hiding this comment.
IsPublicOrInternal [](start = 29, length = 18)
Consider including "effectively" in the name for clarity. Something like "IsEffectivelyPublicOrInternal". #Closed
| Debug.Assert(symbol.IsDefinition); | ||
|
|
||
| var sourceAssembly = SourceAssembly; | ||
| if (symbol.ContainingAssembly != sourceAssembly) |
There was a problem hiding this comment.
if (symbol.ContainingAssembly != sourceAssembly) [](start = 12, length = 48)
Is this check for just in case, or do we have a scenario when it is significant? Consider switching to SourceModule comparison instead. #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
We hit this in the EE.
Then comparing the module is the right check instead, I think.
In reply to: 293924981 [](ancestors = 293924981,293924012)
| { | ||
| if (returnType.NeedsNullableAttribute()) | ||
| if (compilation.ShouldEmitNullableAttributes(lambdaSymbol) && | ||
| returnType.NeedsNullableAttribute()) |
There was a problem hiding this comment.
returnType.NeedsNullableAttribute() [](start = 20, length = 35)
Should we stop emitting nullable attributes for any synthesized code not meant for direct consumption?
| foreach (var parameter in parameters) | ||
| { | ||
| if (parameter.TypeWithAnnotations.NeedsNullableAttribute()) | ||
| if (compilation.ShouldEmitNullableAttributes(container) && parameter.TypeWithAnnotations.NeedsNullableAttribute()) |
There was a problem hiding this comment.
compilation.ShouldEmitNullableAttributes(container) [](start = 20, length = 51)
It feel like this condition can be taken out of the loop. If we don't want to evaluate this value for an empty parameters array, we could check that too. #Closed
|
|
||
| if ((needsAttributes & EmbeddedAttributes.NullablePublicOnlyAttribute) != 0) | ||
| { | ||
| CreateEmbeddedAttributeItselfIfNeeded(diagnostics); |
There was a problem hiding this comment.
CreateEmbeddedAttributeItselfIfNeeded(diagnostics) [](start = 16, length = 50)
Minor, I guess now we can take this out of all the '''ifs''' and call this once before them if needsAttributes is not 0. #Closed
There was a problem hiding this comment.
| } | ||
|
|
||
| internal void EnsureIsReadOnlyAttributeExists(DiagnosticBag diagnostics, Location location, bool modifyCompilation) | ||
| private void EnsureEmbeddedAttributeExists(EmbeddedAttributes attribute, DiagnosticBag diagnostics, Location location, bool modifyCompilation) |
There was a problem hiding this comment.
EnsureEmbeddedAttributeExists [](start = 21, length = 29)
The name is confusing:
- We have an EmbeddedAttribute
- The attributes are not always embedded.
Consider renaming to something like "EnsureEmbeddableAttributeExists" #Closed
| namespace Microsoft.CodeAnalysis.CSharp | ||
| { | ||
| [Flags] | ||
| internal enum EmbeddedAttributes |
There was a problem hiding this comment.
EmbeddedAttributes [](start = 18, length = 18)
"EmbeddableAttributes"? #Closed
There was a problem hiding this comment.
| } | ||
|
|
||
| internal void EnsureIsReadOnlyAttributeExists() | ||
| private void EnsureEmbeddedAttributeExists(EmbeddedAttributes attribute) |
There was a problem hiding this comment.
EnsureEmbeddedAttributeExists [](start = 21, length = 29)
"EnsureEmbeddableAttributeExists"? #Closed
|
|
||
| bool constraintsNeedNullableAttribute = typeParameters.Any( | ||
| typeParameter => ((SourceTypeParameterSymbolBase)typeParameter).ConstraintsNeedNullableAttribute()); | ||
| if (_factory.CompilationState.Compilation.ShouldEmitNullableAttributes(localFunction)) |
There was a problem hiding this comment.
if (_factory.CompilationState.Compilation.ShouldEmitNullableAttributes(localFunction)) [](start = 12, length = 86)
Similar suggestion as for lambdas.
| } | ||
|
|
||
| if (returnType.NeedsNullableAttribute()) | ||
| if (compilation?.ShouldEmitNullableAttributes(this) == true && |
There was a problem hiding this comment.
compilation? [](start = 16, length = 12)
Can compilation ever be null and why other places do not guard? #Closed
There was a problem hiding this comment.
| { | ||
| var compilation = DeclaringCompilation; | ||
| return compilation.EmitNullablePublicOnly && | ||
| compilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes); |
There was a problem hiding this comment.
compilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes) [](start = 20, length = 73)
Having this property on the Module symbol as well feels confusing, especially that it has extra conditions. Effectively, there is an inconsistency between other users of compilation.EmitNullablePublicOnly and this code. Consider incorporating the feature check into the compilation's property and getting rid of this property. #Closed
There was a problem hiding this comment.
Alternatively, consider renaming this property to EmitNullablePublicOnlyAttribute
In reply to: 293957455 [](ancestors = 293957455)
| { | ||
| if (!_lazyEmitNullablePublicOnly.HasValue()) | ||
| { | ||
| bool value = SyntaxTrees.FirstOrDefault()?.Options?.Features?.ContainsKey("nullablePublicOnly") == true; |
There was a problem hiding this comment.
SyntaxTrees.FirstOrDefault()?.Options?.Features?.ContainsKey("nullablePublicOnly") == true; [](start = 33, length = 91)
We may already have a way of checking feature flags. I'm surprised we'd need to do this "by hand" here.
Also, should we be checking for LangVersion? #Closed
There was a problem hiding this comment.
I couldn't find an appropriate helper method for checking Features.
Regarding language version, is it necessary to add that restriction here? Presumably language version will produce the necessary warnings and errors during binding.
In reply to: 294014629 [](ancestors = 294014629)
There was a problem hiding this comment.
You're right, we don't have a helper. IOperations did the same...
internal override bool IsIOperationFeatureEnabled()
{
var options = (CSharpParseOptions)this.SyntaxTrees.FirstOrDefault()?.Options;
return options?.IsFeatureEnabled(MessageID.IDS_FeatureIOperation) ?? false;
}For LangVersion, I agree too. If you're in C# 7.3 and don't nullable annotations, then the feature flag is a no-op, so no need to report a diagnostic.
In reply to: 294017389 [](ancestors = 294017389,294014629)
| { | ||
| [Fact] | ||
| public void ExplicitAttribute_FromSource() | ||
| { |
There was a problem hiding this comment.
{ [](start = 8, length = 1)
[NullablePublicOnly] should be disallowed in source. Do we have a test for that? #Pending
| { | ||
| _needsGeneratedIsUnmanagedAttribute_Value = true; | ||
| } | ||
| internal void EnsureIsUnmanagedAttributeExists() |
There was a problem hiding this comment.
EnsureIsUnmanagedAttributeExists [](start = 22, length = 32)
nit: consider inlining this method (only one use as far as I can tell).
Comment may be applicable for other Ensure... methods as well.
| CompileAndVerify(comp, symbolValidator: AssertNoNullablePublicOnlyAttribute); | ||
|
|
||
| comp = CreateCompilation(source, options: options, parseOptions: parseOptions.WithFeature("nullablePublicOnly")); | ||
| CompileAndVerify(comp, symbolValidator: AssertNullablePublicOnlyAttribute); |
There was a problem hiding this comment.
symbolValidator: AssertNullablePublicOnlyAttribute); [](start = 35, length = 52)
consider validating that no NullablePublicOnly attribute appears on source symbols (validator: AssertNoNullablePublicOnlyAttribute)
…-types * dotnet/master: (63 commits) Fix stack overflow in requesting syntax directives (dotnet#36347) crash on ClassifyUpdate for EventFields (dotnet#35962) Disable move type when the options service isn't present (dotnet#36334) Fix crash where type inference doing method inference needs to drop nullability Fix parsing bug in invalid using statements (dotnet#36428) Do not suggest or diagnose use compound assignment when right hand of binary operator is a throw expression Add option to emit nullable metadata for public members only (dotnet#36398) Added null checks on F# external access services (dotnet#36469) Deal with discovering extra .editorconfig files Re-enable MSBuildWorkspaceTests.TestEditorConfigDiscovery Add support to VisualStudioMSBuildInstalled to support minimum versions Fix configuration of accessibilities in editorconfig Shorten a resource ID Revert "Extract the RDT implementation for Misc files and VS open file tracker" Add nullability support to use local function Add EditorFeatures.WPF dependency to F# ExternalAccess Ensure NullableWalker.AsMemberOfType locates the right new container for the member. (dotnet#36406) Replace `dynamic` with `object` when substituting constraints. (dotnet#36379) Add some string descriptions Adjust type of out var based on parameter state (dotnet#36284) ...
Relates to #35816 (work items for annotations and metadata compaction)