Simplify and reduce amount of work performed by SourcePropertySymbolBase constructor. by AlekseyTs · Pull Request #49360 · dotnet/roslyn

AlekseyTs · 2020-11-13T18:32:06Z

Closes #45489.

…ase constructor. Closes dotnet#45489.

AlekseyTs · 2020-11-16T16:45:29Z

@cston, @jcouv, @dotnet/roslyn-compiler Please review

AlekseyTs · 2020-11-17T15:50:44Z

@cston, @jcouv, @dotnet/roslyn-compiler Please review

jaredpar · 2020-11-17T16:53:43Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+
+                        // Due to the fact that this method is potentially reentrant, we must use a 
+                        // reentrant lock to avoid deadlock and cannot assert that at this point the work
+                        // has completed (_state.HasComplete(CompletionPart.FinishPropertyEnsureSignature)).


It seems that a number of the callers of EnsureSignature assume that the call will exit with the various fields being properly set. Most notable _lazyType. The re-entrant nature of the method though indicates that this is not the case. For example if EnsureSignature calls TypeWithAnnotations then that call will possibly because the second call to EnsureSignature will be re-entrant and won't guarantee all of the values are set.

How do we guard against this type of problem?

I think it's the type of error that would pretty much always play out in single threaded scenarios (i.e. it's not a race). So exhaustive testing should be enough. Wanted to make sure you agreed with that assertion. #Resolved

How do we guard against this type of problem?

All the fields that are expected to be used in case of reentrancy are set by EnsureSignature to some preliminary values before it does anything that could cause a reentrancy. We are already using similar mechanism for method symbols.

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

jaredpar · 2020-11-17T16:57:47Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+            {
+                Debug.Assert(GetMethod is object);
+
+                if (!IsStatic && SetMethod is object && !SetMethod.IsInitOnly)


Consider a pattern here to avoid the double read of SetMethod

Suggested change

if (!IsStatic && SetMethod is object && !SetMethod.IsInitOnly)

if (!IsStatic && SetMethod is { IsInitOnly: false }

``` #Resolved

jaredpar · 2020-11-17T17:00:26Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs


+                    case CompletionPart.StartPropertyEnsureSignature:
+                    case CompletionPart.FinishPropertyEnsureSignature:
+                        EnsureSignature();


Believe we need the following here

_state.SpinWaitComplete(CompletionPart.FinishPropertyEnsureSignature, cancellationToken);

This is necessary if the call to EnsureSignature could call out to ForceComplete. In that case the call to EnsureSignature would be re-entrant here and hence wouldn't satisfy the contract of ForceComplete.

Unsure if this is or isn't possible given the call graph of EnsureSignature. At the least I think we should have an assert that validates the part is marked as complete after EnsureSignature is called. #Resolved

We have SpinWaitComplete after this switch.

if the call to EnsureSignature could call out to ForceComplete.

I do not think this can happen. ForceComplete is there to make sure we collected all declaration diagnostics, it is not meant to be used for any other purpose.

I will add the suggested assert.

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

AlekseyTs · 2020-11-18T16:00:56Z

@jaredpar Addressed feedback

AlekseyTs · 2020-11-18T16:01:20Z

@cston, @jcouv, @dotnet/roslyn-compiler Please review

jaredpar · 2020-11-18T16:15:22Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+            Location location)
        {
+            Debug.Assert(!isExpressionBodied || !isAutoProperty);
+            Debug.Assert(!isExpressionBodied || !hasInitializer);


Does it make sense to add this assert? Or am I misunderstanding the relationship between the properties here?

Debug.Assert(!isExplicitInterfaceImplementation || explicitInterfaceType is not null); ``` #Resolved

Does it make sense to add this assert? Or am I misunderstanding the relationship between the properties here?

In error scenarios we may fail to find the type, but the property is still an explicit implementation. It is purely a syntax check

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

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

jaredpar · 2020-11-18T16:19:53Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

            }
+        }

+        private void EnsureSignature(DiagnosticBag diagnostics)


Believe this can only legally called from EnsureSignature() correct? If so did you consider making this a local function there to prevent bad calls?

Not something I feel strongly about, just curious. #Resolved

Believe this can only legally called from EnsureSignature() correct? If so did you consider making this a local function there to prevent bad calls?

To simplify review process I tried to keep original code as close to the original place as possible and tried to make as little as possible changes around it. Since the method is private, I do not see significant advantage in using a local function at this point.

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

jaredpar · 2020-11-18T16:45:19Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPropertySymbol.cs

+        protected override SourcePropertyAccessorSymbol CreateGetAccessorSymbol(bool isAutoPropertyAccessor, bool isExplicitInterfaceImplementation, PropertySymbol? explicitlyImplementedPropertyOpt, DiagnosticBag diagnostics)
        {
-            // Nothing to do here
+            Debug.Assert(isAutoPropertyAccessor);


Does this assert make sense

Debug.Assert(!isExplicitInterfaceImplementation); ``` #Resolved

Does this assert make sense

That would be a valid assumption, but I don't think it is worth verifying given the nature of this sealed class.

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

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

AlekseyTs · 2020-11-19T15:50:29Z

@cston, @jcouv, @dotnet/roslyn-compiler Please review, need a second sign-off

jcouv · 2020-11-19T19:27:33Z

Looking #Resolved

jcouv · 2020-11-19T21:25:53Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+                        // requires that we return here even though the work is not complete. That's because
+                        // signature is processed by first populating the return type and parameters by binding
+                        // the syntax from source.  Those values are visible to the same thread for the purpose
+                        // of computing which methods are implemented and overridden.  But then those values


Might we be able to use signature-only symbols, rather than partially constructed symbols, to find which methods are implemented and overridden? #Resolved

Might we be able to use signature-only symbols, rather than partially constructed symbols, to find which methods are implemented and overridden?

I think technically it should be possible to locate overridden/implemented property with an alternative signature representation. A signature-only symbol is one way to do that. Perhaps, if we were implementing this from scratch, we should have taken that route. At this point, however, I prefer sticking to the existing way of doing things, which has been verified to work (in the old constructor and with method symbols). Trying to use signature-only symbols, will likely require changes elsewhere, the symbol itself is getting updated in the process, the symbol itself is getting captured somewhere. For example, in diagnostics, etc.

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

jcouv

LGTM Thanks (iteration 8)

* upstream/master: (47 commits) Make compiler server logging explicit (dotnet#48557) [master] Update dependencies from dotnet/arcade (dotnet#48274) Handle removed project in ExternalErrorDiagnosticUpdateSource Report an error for ref-returning auto-properties declared in an interface. (dotnet#49510) Add usings on paste (dotnet#48501) Determinism test needs to use bootstrap compiler (dotnet#49483) Simplify and reduce amount of work performed by SourcePropertySymbolBase constructor. (dotnet#49360) Updates test Simplify Split Document/Workspace handler into only handling open/closed documents respectively. only report watson once. Additional usage of a PooledHashset. (dotnet#49459) Loc checkin Update src/Features/CSharp/Portable/CodeRefactorings/ConvertLocalFunctionToMethod/CSharpConvertLocalFunctionToMethodCodeRefactoringProvider.cs Preserve annotation on comment trivia when performing formatting. Validate arguments to IAsyncCompletionSource methods Determinism fixes for AnonymousTypes in VB (dotnet#49467) Collect nfw information for a crash we're seeing. Make sure to not discard text changes when no reference changes are present Create an unsafe method from a local function when necessary (dotnet#49389) ...

Simplify and reduce amaunt of work performed by SourcePropertySymbolB…

7d3fe09

…ase constructor. Closes dotnet#45489.

AlekseyTs added Area-Compilers Concept-Continuous Improvement labels Nov 13, 2020

AlekseyTs requested review from a team, cston and jcouv November 13, 2020 18:32

AlekseyTs added 2 commits November 13, 2020 11:21

Nullable fixup

8748dbf

Nullable fixup

c0557dc

jcouv self-assigned this Nov 13, 2020

Formatting fixup

f3c7456

jaredpar reviewed Nov 17, 2020

View reviewed changes

AlekseyTs added 2 commits November 18, 2020 05:52

Merge remote-tracking branch 'dotnet/master' into Issue45489

c715605

PR feedback

167e2a1

jaredpar approved these changes Nov 18, 2020

View reviewed changes

AlekseyTs and others added 2 commits November 18, 2020 09:57

Adgjust comment as suggested

5f41eed

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

Rename private function

24391b7

jcouv reviewed Nov 19, 2020

View reviewed changes

jcouv approved these changes Nov 19, 2020

View reviewed changes

AlekseyTs merged commit bd92aeb into dotnet:master Nov 19, 2020

ghost added this to the Next milestone Nov 19, 2020

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

	if (!IsStatic && SetMethod is object && !SetMethod.IsInitOnly)
	if (!IsStatic && SetMethod is { IsInitOnly: false }
	``` #Resolved

Conversation

AlekseyTs commented Nov 13, 2020

Uh oh!

AlekseyTs commented Nov 16, 2020

Uh oh!

AlekseyTs commented Nov 17, 2020

Uh oh!

jaredpar Nov 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Nov 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Nov 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 18, 2020

Uh oh!

AlekseyTs commented Nov 18, 2020

Uh oh!

jaredpar Nov 18, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaredpar Nov 18, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Nov 18, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 19, 2020

Uh oh!

jcouv commented Nov 19, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 19, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jaredpar Nov 17, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Nov 17, 2020 •

edited

Loading

jaredpar Nov 17, 2020 •

edited by AlekseyTs

Loading

jaredpar Nov 17, 2020 •

edited by AlekseyTs

Loading

jaredpar Nov 18, 2020 •

edited by AlekseyTs

Loading

jaredpar Nov 18, 2020 •

edited by AlekseyTs

Loading

jaredpar Nov 18, 2020 •

edited by AlekseyTs

Loading

jcouv commented Nov 19, 2020 •

edited by AlekseyTs

Loading

jcouv Nov 19, 2020 •

edited by AlekseyTs

Loading