Move PEPropertySymbol to use the same UncommonFields pattern that PEMethodSymbol uses #74132

333fred · 2024-06-24T22:45:19Z

This will be useful for OverloadResolutionPriority as well, as most properties won't have this attribute.

333fred · 2024-06-24T22:47:27Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

@@ -814,24 +884,28 @@ internal override bool MustCallMethodsDirectly

        public override string GetDocumentationCommentXml(CultureInfo preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default(CancellationToken))
        {
-            return PEDocumentationCommentUtils.GetDocumentationComment(this, _containingType.ContainingPEModule, preferredCulture, cancellationToken, ref _lazyDocComment);
+            return PEDocumentationCommentUtils.GetDocumentationComment(this, _containingType.ContainingPEModule, preferredCulture, cancellationToken, ref AccessUncommonFields()._lazyDocComment);


This unconditional access is the same pattern used by PEMethodSymbol, so I kept it here.

…ethodSymbol uses This will be useful for OverloadResolutionPriority as well, as most properties won't have this attribute.

333fred · 2024-06-24T22:49:35Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

@@ -867,8 +938,31 @@ internal override ObsoleteAttributeData ObsoleteAttributeData
        {
            get
            {
-                ObsoleteAttributeHelpers.InitializeObsoleteDataFromMetadata(ref _lazyObsoleteAttributeData, _handle, (PEModuleSymbol)(this.ContainingModule), ignoreByRefLikeMarker: false, ignoreRequiredMemberMarker: false);
-                return _lazyObsoleteAttributeData;
+                if (!_flags.IsObsoleteAttributePopulated)


Implementation basically copied wholesale from PEMethodSymbol, with adjustments to GetObsoleteDataFromMetadata as appropriate.

…ppropriate values.

333fred · 2024-06-25T20:40:10Z

@dotnet/roslyn-compiler @jcouv @cston for review. This will contribute to overload resolution priority, but is an effectively unrelated refactoring so I separated it out.

333fred · 2024-06-26T01:48:47Z

src/Compilers/Core/Portable/Binding/UseSiteInfo.cs

+        /// Atomically initializes the cache with the given value if it is currently fully default.
+        /// This <i>will not</i> initialize <see cref="CachedUseSiteInfo{TAssemblySymbol}.Uninitialized"/>.
+        /// </summary>
+        public UseSiteInfo<TAssemblySymbol> InterlockedInitializeFromDefault(TAssemblySymbol? primaryDependency, UseSiteInfo<TAssemblySymbol> value)


Note: the calling pattern in PEMethodSymbol is pretty complicated, and explicitly calls out not using the Sentinel for race safety. It seemed like it could be simplified to me, but I didn't want to touch it here, so I just renamed these methods to make it clear what they actually do: they both initialize a CachedUseSiteInfo, one assuming that _info is null, and one assuming that _info is Sentinel. #Closed

Thanks for the rename and this context. It is surprising indeed that we couldn't always use a sentinel for the uninitialized value.

333fred · 2024-06-26T20:05:09Z

@jcouv @cston @dotnet/roslyn-compiler for review.

cston · 2024-06-26T22:30:38Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

@@ -146,7 +179,8 @@ public bool TryGetHasUnscopedRefAttribute(out bool hasUnscopedRefAttribute)

            if (propEx != null || isBad)
            {
-                result._lazyCachedUseSiteInfo.Initialize(new CSDiagnosticInfo(ErrorCode.ERR_BindToBogus, result));
+                result.AccessUncommonFields()._lazyCachedUseSiteInfo.Initialize(new CSDiagnosticInfo(ErrorCode.ERR_BindToBogus, result));


result.AccessUncommonFields()._lazyCachedUseSiteInfo.Initialize

PEMethodSymbol has a dedicated InitializeUseSiteDiagnostic() method that is used for all (?) cases where use-site diagnostics are calculated, and that also calls SetUseSiteDiagnosticPopulated(). Is there a reason to set the fields directly here rather than using that approach?

I'm frankly not entirely certain why PEMethodSymbol has as complicated an initialization strategy as it does, but it does also initialize the diagnostic from more properties than we do in PEPropertySymbol. Here, we only initialize from:

The creation steps, where the created symbol has not yet been exposed to anything else, so we don't have to worry about races.

GetUseSiteDiagnostic, which does have to worry about races.

The pattern used here is the pattern used by all the rest of the UncommonFields members, which correctly handles the occasional race of calculating that there is no use-site diagnostic/setting _flags.IsUseSiteDiagnostic and creating _uncommonFields.

333fred · 2024-06-26T23:08:41Z

@jcouv @dotnet/roslyn-compiler for a second review

jcouv · 2024-06-27T18:14:15Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

@@ -277,6 +312,33 @@ static bool anyUnexpectedRequiredModifiers(ParamInfo<TypeSymbol>[] propertyParam
            }
        }

+        private UncommonFields CreateUncommonFields()


nit: this could be a local function in AccessUncommonFields

jcouv · 2024-06-27T18:20:46Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

+        private UncommonFields CreateUncommonFields()
+        {
+            var retVal = new UncommonFields();
+            if (!_flags.IsObsoleteAttributePopulated)


I didn't understand this pattern of checking _flags when initializing _uncommonFields (here or in PEMethodSymbol).
What is the scenario where _flags.IsObsoleteAttributePopulated is set before _uncommonFields is initialized by AccessUncommonFields?
The pattern of initialization that we use (1. AccessUncommonFields(), 2. initialize ._lazyObsoleteAttributeData on it, then 3. mark the flag with SetObsoleteAttributePopulated) seems to preclude that. #Closed

Never mind, figured it out. We only conditionally do steps 1 and 2 (when there is non-trivial data to store). May be worth a comment

The scenario where there is no obsolete attribute. That will not call AccessUncommonData(), since it doesn't need to put anything in it.

jcouv · 2024-06-27T18:55:30Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

+            {
+                var result = uncommonFields._lazyCustomAttributes;
+                if (result.IsDefault)
+                {


Consider leaving a comment

jcouv

LGTM Thanks (iteration 3)

* Address PR feedback on #74132 * Spelling

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 24, 2024

333fred commented Jun 24, 2024

View reviewed changes

Move PEPropertySymbol to use the same UncommonFields pattern that PEM…

682e459

…ethodSymbol uses This will be useful for OverloadResolutionPriority as well, as most properties won't have this attribute.

333fred force-pushed the pe-uncomon branch from 83e6fbd to 682e459 Compare June 24, 2024 22:48

333fred commented Jun 24, 2024

View reviewed changes

333fred added 2 commits June 25, 2024 11:47

Handle patterns where uncommon fields could exist but wasn't set to a…

c49f442

…ppropriate values.

Adjust UseSiteInfo methods to convey actual intent.

a3f0dcc

333fred force-pushed the pe-uncomon branch from bc81ed3 to a3f0dcc Compare June 25, 2024 19:38

build-analysis bot mentioned this pull request Jun 25, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

333fred marked this pull request as ready for review June 25, 2024 20:39

333fred requested a review from a team as a code owner June 25, 2024 20:39

333fred commented Jun 26, 2024

View reviewed changes

cston reviewed Jun 26, 2024

View reviewed changes

cston approved these changes Jun 26, 2024

View reviewed changes

jcouv reviewed Jun 27, 2024

View reviewed changes

jcouv self-assigned this Jun 27, 2024

jcouv approved these changes Jun 27, 2024

View reviewed changes

333fred merged commit 5475105 into dotnet:main Jun 27, 2024
24 checks passed

333fred deleted the pe-uncomon branch June 27, 2024 20:33

dotnet-policy-service bot added this to the Next milestone Jun 27, 2024

333fred added a commit to 333fred/roslyn that referenced this pull request Jun 27, 2024

Address PR feedback on dotnet#74132

0c81586

333fred mentioned this pull request Jun 27, 2024

Address PR feedback on https://github.com/dotnet/roslyn/pull/74132 #74193

Merged

This was referenced Jul 4, 2024

[Automated] PRs inserted in VS build main-35104.04 #74270

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35108.50 #74296

Closed

333fred added a commit that referenced this pull request Jul 18, 2024

Address PR feedback on #74132 (#74193)

fa05fe4

* Address PR feedback on #74132 * Spelling

RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move PEPropertySymbol to use the same UncommonFields pattern that PEMethodSymbol uses #74132

Move PEPropertySymbol to use the same UncommonFields pattern that PEMethodSymbol uses #74132

333fred commented Jun 24, 2024

333fred Jun 24, 2024

333fred Jun 24, 2024

333fred commented Jun 25, 2024

333fred Jun 26, 2024 •

edited by jcouv

Loading

jcouv Jun 27, 2024

333fred commented Jun 26, 2024

cston Jun 26, 2024 •

edited

Loading

333fred Jun 26, 2024

333fred commented Jun 26, 2024

jcouv Jun 27, 2024

jcouv Jun 27, 2024 •

edited

Loading

jcouv Jun 27, 2024

333fred Jun 27, 2024

jcouv Jun 27, 2024

jcouv left a comment

Move PEPropertySymbol to use the same UncommonFields pattern that PEMethodSymbol uses #74132

Move PEPropertySymbol to use the same UncommonFields pattern that PEMethodSymbol uses #74132

Conversation

333fred commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jun 25, 2024

333fred Jun 26, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jun 26, 2024

cston Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jun 26, 2024

Choose a reason for hiding this comment

jcouv Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

333fred Jun 26, 2024 •

edited by jcouv

Loading

cston Jun 26, 2024 •

edited

Loading

jcouv Jun 27, 2024 •

edited

Loading