Skip to content
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.

Sign up for GitHub

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

Avoid generating or emitting NullablePublicOnlyAttribute when no other nullable attributes are emitted #37019

Merged
merged 3 commits into from
Jul 9, 2019
Merged

Avoid generating or emitting NullablePublicOnlyAttribute when no other nullable attributes are emitted #37019

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f2b3ee
Avoid generating or emitting NullablePublicOnlyAttribute when no null…
cston Jul 4, 2019
34f1a74
Remove PEModuleBuilder._usesNullableAttributes
cston Jul 5, 2019
1e5cbf5
More tests
cston Jul 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,12 @@ private void CreateEmbeddedAttributesIfNeeded(DiagnosticBag diagnostics)
{
EmbeddableAttributes needsAttributes = GetNeedsGeneratedAttributes();

if (needsAttributes == 0)
if (ShouldEmitNullablePublicOnlyAttribute() &&
Compilation.CheckIfAttributeShouldBeEmbedded(EmbeddableAttributes.NullablePublicOnlyAttribute, diagnostics, Location.None))
{
needsAttributes |= EmbeddableAttributes.NullablePublicOnlyAttribute;
}
else if (needsAttributes == 0)
{
return;
}
Expand Down
9 changes: 8 additions & 1 deletion src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ private EmbeddableAttributes GetNeedsGeneratedAttributesInternal()
return (EmbeddableAttributes)_needsGeneratedAttributes | Compilation.GetNeedsGeneratedAttributes();
}

internal void SetNeedsGeneratedAttributes(EmbeddableAttributes attributes)
private void SetNeedsGeneratedAttributes(EmbeddableAttributes attributes)
{
Debug.Assert(!_needsGeneratedAttributes_IsFrozen);
ThreadSafeFlagOperations.Set(ref _needsGeneratedAttributes, (int)attributes);
Expand Down Expand Up @@ -1542,6 +1542,13 @@ internal virtual SynthesizedAttributeData SynthesizeNullableContextAttribute(Imm
return Compilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_NullableContextAttribute__ctor, arguments, isOptionalUse: true);
}

internal bool ShouldEmitNullablePublicOnlyAttribute()
{
// No need to look at this.GetNeedsGeneratedAttributes() since those bits are
// only set for members generated by the rewriter which are not public.
return Compilation.GetUsesNullableAttributes() && Compilation.EmitNullablePublicOnly;
}

internal virtual SynthesizedAttributeData SynthesizeNullablePublicOnlyAttribute(ImmutableArray<TypedConstant> arguments)
{
// For modules, this attribute should be present. Only assemblies generate and embed this type.
Expand Down
26 changes: 20 additions & 6 deletions src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public partial class CSharpCompilation
/// </summary>
private Symbol[] _lazyWellKnownTypeMembers;

private bool _usesNullableAttributes;
private int _needsGeneratedAttributes;
private bool _needsGeneratedAttributes_IsFrozen;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should this field be renamed, since now used for freezing both "needsGenerated" and "usesNullable"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I considered renaming the field but didn't find a better name.

In reply to: 301354830 [](ancestors = 301354830)


Expand All @@ -42,12 +43,24 @@ internal EmbeddableAttributes GetNeedsGeneratedAttributes()
return (EmbeddableAttributes)_needsGeneratedAttributes;
}

internal void SetNeedsGeneratedAttributes(EmbeddableAttributes attributes)
private void SetNeedsGeneratedAttributes(EmbeddableAttributes attributes)
{
Debug.Assert(!_needsGeneratedAttributes_IsFrozen);
ThreadSafeFlagOperations.Set(ref _needsGeneratedAttributes, (int)attributes);
}

internal bool GetUsesNullableAttributes()
{
_needsGeneratedAttributes_IsFrozen = true;
return _usesNullableAttributes;
}

private void SetUsesNullableAttributes()
{
Debug.Assert(!_needsGeneratedAttributes_IsFrozen);
_usesNullableAttributes = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_usesNullableAttributes = true; [](start = 12, length = 31)

just to confirm, we don't need any special operation to ensure this set is atomic, correct?

Copy link
Member Author

@cston cston Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need - reading and writing a bool is atomic.

In reply to: 301355085 [](ancestors = 301355085)

}

/// <summary>
/// Lookup member declaration in well known type used by this Compilation.
/// </summary>
Expand Down Expand Up @@ -456,6 +469,12 @@ private void EnsureEmbeddableAttributeExists(EmbeddableAttributes attribute, Dia
{
SetNeedsGeneratedAttributes(attribute);
}

if ((attribute & (EmbeddableAttributes.NullableAttribute | EmbeddableAttributes.NullableContextAttribute)) != 0 &&
modifyCompilation)
Copy link
Contributor

@RikkiGibson RikkiGibson Jul 5, 2019 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so do we expect that in batch compilation, something passes modifyCompilation: true here, and in SemanticModel or whatnot we don't get here or we pass modifyCompilation: false? #ByDesign

Copy link
Contributor

@RikkiGibson RikkiGibson Jul 5, 2019 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm really wondering when/why modifyCompilation would be false. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we pass modifyCompilation: false in cases where we want to report diagnostics but don't expect to emit any attributes - in particular binding lambdas and local functions.

In reply to: 300809615 [](ancestors = 300809615)

{
SetUsesNullableAttributes();
}
}

internal void EnsureIsReadOnlyAttributeExists(DiagnosticBag diagnostics, Location location, bool modifyCompilation)
Expand Down Expand Up @@ -483,11 +502,6 @@ internal void EnsureNullableContextAttributeExists(DiagnosticBag diagnostics, Lo
EnsureEmbeddableAttributeExists(EmbeddableAttributes.NullableContextAttribute, diagnostics, location, modifyCompilation);
}

internal void EnsureNullablePublicOnlyAttributeExists(DiagnosticBag diagnostics, Location location, bool modifyCompilation)
{
EnsureEmbeddableAttributeExists(EmbeddableAttributes.NullablePublicOnlyAttribute, diagnostics, location, modifyCompilation);
}

internal bool CheckIfAttributeShouldBeEmbedded(EmbeddableAttributes attribute, DiagnosticBag diagnosticsOpt, Location locationOpt)
{
switch (attribute)
Expand Down
32 changes: 1 addition & 31 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceModuleSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,18 +255,6 @@ internal override void ForceComplete(SourceLocation locationOpt, CancellationTok
_state.SpinWaitComplete(CompletionPart.FinishValidatingReferencedAssemblies, cancellationToken);
break;

case CompletionPart.StartMemberChecks:
Copy link
Member

@jcouv jcouv Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case [](start = 20, length = 4)

I didn't understand why this could be removed. Aren't we losing some diagnostics? Never mind, we were dicarding those diagnostics anyways. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were reporting those diagnostics before, but now emitting of this attribute is tied to the other attributes so any diagnostics will be reported from those attributes.

In reply to: 301355338 [](ancestors = 301355338)

case CompletionPart.FinishMemberChecks:
if (_state.NotePartComplete(CompletionPart.StartMemberChecks))
{
var diagnostics = DiagnosticBag.GetInstance();
AfterMembersChecks(diagnostics);
AddDeclarationDiagnostics(diagnostics);
diagnostics.Free();
_state.NotePartComplete(CompletionPart.FinishMemberChecks);
}
break;

case CompletionPart.MembersCompleted:
this.GlobalNamespace.ForceComplete(locationOpt, cancellationToken);

Expand Down Expand Up @@ -533,24 +521,6 @@ internal override void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArgu
}
}

private bool EmitNullablePublicOnlyAttribute
{
get
{
var compilation = DeclaringCompilation;
return compilation.EmitNullablePublicOnly &&
compilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to check the feature flag, but I didn't see whether that is still handled somehow. Was that unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only emit NullablePublicOnlyAttribute if we're also emitting NullableAttribute or NullableContextAttribute.

In reply to: 301355808 [](ancestors = 301355808)

}
}

private void AfterMembersChecks(DiagnosticBag diagnostics)
{
if (EmitNullablePublicOnlyAttribute)
{
DeclaringCompilation.EnsureNullablePublicOnlyAttributeExists(diagnostics, location: NoLocation.Singleton, modifyCompilation: true);
}
}

internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)
{
base.AddSynthesizedAttributes(moduleBuilder, ref attributes);
Expand All @@ -566,7 +536,7 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r
}
}

if (EmitNullablePublicOnlyAttribute)
if (moduleBuilder.ShouldEmitNullablePublicOnlyAttribute())
{
var includesInternals = ImmutableArray.Create(
new TypedConstant(compilation.GetSpecialType(SpecialType.System_Boolean), TypedConstantKind.Primitive, _assemblySymbol.InternalsAreVisible));
Expand Down
Loading