Avoid generating or emitting NullablePublicOnlyAttribute when no other nullable attributes are emitted #37019
Conversation
|
@dotnet/roslyn-compiler please review. |
| @@ -456,6 +469,12 @@ private void EnsureEmbeddableAttributeExists(EmbeddableAttributes attribute, Dia | |||
| { | |||
| SetNeedsGeneratedAttributes(attribute); | |||
| } | |||
|
|
|||
| if ((attribute & (EmbeddableAttributes.NullableAttribute | EmbeddableAttributes.NullableContextAttribute)) != 0 && | |||
| modifyCompilation) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I guess I'm really wondering when/why modifyCompilation would be false. #ByDesign
There was a problem hiding this comment.
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)
| var comp = CreateCompilation(sources, options: options, parseOptions: parseOptions); | ||
| CompileAndVerify(comp, symbolValidator: AssertNoNullablePublicOnlyAttribute); | ||
|
|
||
| comp = CreateCompilation(sources, options: options, parseOptions: parseOptions.WithFeature("nullablePublicOnly")); |
There was a problem hiding this comment.
parseOptions.WithFeature("nullablePublicOnly") [](start = 78, length = 46)
Consider making a TestOptions option for these, so we don't have to keep typing the flag out. #Pending
There was a problem hiding this comment.
| @@ -28,6 +28,7 @@ public partial class CSharpCompilation | |||
| /// </summary> | |||
| private Symbol[] _lazyWellKnownTypeMembers; | |||
|
|
|||
| private bool _usesNullableAttributes; | |||
| private int _needsGeneratedAttributes; | |||
| private bool _needsGeneratedAttributes_IsFrozen; | |||
There was a problem hiding this comment.
nit: should this field be renamed, since now used for freezing both "needsGenerated" and "usesNullable"?
There was a problem hiding this comment.
Yes, I considered renaming the field but didn't find a better name.
In reply to: 301354830 [](ancestors = 301354830)
| private void SetUsesNullableAttributes() | ||
| { | ||
| Debug.Assert(!_needsGeneratedAttributes_IsFrozen); | ||
| _usesNullableAttributes = true; |
There was a problem hiding this comment.
_usesNullableAttributes = true; [](start = 12, length = 31)
just to confirm, we don't need any special operation to ensure this set is atomic, correct?
There was a problem hiding this comment.
| @@ -255,18 +255,6 @@ internal override void ForceComplete(SourceLocation locationOpt, CancellationTok | |||
| _state.SpinWaitComplete(CompletionPart.FinishValidatingReferencedAssemblies, cancellationToken); | |||
| break; | |||
|
|
|||
| case CompletionPart.StartMemberChecks: | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
| { | ||
| var compilation = DeclaringCompilation; | ||
| return compilation.EmitNullablePublicOnly && | ||
| compilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes); |
There was a problem hiding this comment.
We used to check the feature flag, but I didn't see whether that is still handled somehow. Was that unnecessary?
There was a problem hiding this comment.
We only emit NullablePublicOnlyAttribute if we're also emitting NullableAttribute or NullableContextAttribute.
In reply to: 301355808 [](ancestors = 301355808)
…ncat-order * upstream/master: (1532 commits) Ensure we have stack spilling support for the recently-added expression node `BoundReadOnlySpanFromArray` (dotnet#37057) Review feedback Avoid generating or emitting NullablePublicOnlyAttribute when no other nullable attributes are emitted (dotnet#37019) Respond to more feedback Fixes from feedback Call `.WithoutNullability()` less Fix ngen for assets from nuget Make NullableWalker aware of calls to Interlocked.CompareExchange fix error Update AnonymousObjectCreation implementation to be more robust to errors by using MemberIndexOpt. Address minor PR comments. Add support to ngen assets included from nuget package Modified node removal to keep original leading trivia Fix Solution.WithDocumentFilePath not updating the file path of the tree Improve docs. PR Feedback cleanup. Use pattern-matching in MetadataWriter for readability and possibly performance. (dotnet#36219) Renamed helpers in SyntaxFactsService. More RefactoringHelpers tests (mostly for extraction). Add general tests for RefactoringHelpersService. Optimize flow analysis assembly ...
Fixes #36977