Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. #22933

AlekseyTs · 2017-10-31T17:13:12Z

Closes #21311.

vso : https://devdiv.visualstudio.com/DevDiv/NET%20Developer%20Experience%20Productivity/_workitems/edit/517771

…ation, add RefKind property to ILocalSymbol. Closes dotnet#21311.

AlekseyTs · 2017-10-31T17:13:31Z

@dotnet/roslyn-compiler, @dotnet/analyzer-ioperation, @mavasani, @cston Please review 15.5 IOperation fix. #Resolved

AlekseyTs · 2017-10-31T17:14:29Z

CC @jaredpar, @gafter, @AnthonyDGreen For public API change on ILocalSymbol. #Resolved

cston · 2017-10-31T18:27:35Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.cs

+                {
+                    AddKeyword(SyntaxKind.ReadOnlyKeyword);
+                    AddSpace();
+                }


Should the SymbolDisplay change be a separate PR, perhaps post 15.5?

Should the SymbolDisplay change be a separate PR, perhaps post 15.5?

Do you have concerns about this change? It feels low risk to me.

tmat · 2017-10-31T18:44:42Z

src/Compilers/Core/Portable/Symbols/ILocalSymbol.cs

@@ -23,11 +23,17 @@ public interface ILocalSymbol : ISymbol
        bool IsConst { get; }

        /// <summary>
-        /// Returns true if this local is a ref-local.
+        /// Returns true if this local is a ref local or a ref readonly local.
+        /// Use <see cref="RefKind"/> to get more detailed information.
        /// </summary>
        bool IsRef { get; }


Why keep this property if the same information is in RefKind? #Resolved

Why keep this property if the same information is in RefKind?

We already shipped it, cannot take it back. #Resolved

I see. Ok.

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

tmat · 2017-10-31T18:46:43Z

src/Compilers/Core/Portable/Operations/ISimpleAssignmentOperation.cs

+        /// <summary>
+        /// Is this a ref assignment
+        /// </summary>
+        bool IsRef { get; }


bool IsRef { get; } [](start = 8, length = 19)

Why not expose RefKind property instead? #Resolved

Why not expose RefKind property instead?

Because the kind of the reference is irrelevant here. Even compiler doesn't capture the kind. #Resolved

OK. Thanks

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

tmat · 2017-10-31T18:47:06Z

src/Compilers/Core/Portable/Operations/IConditionalOperation.cs

+        /// <summary>
+        /// Is result a managed reference
+        /// </summary>
+        bool IsRef { get; }


bool IsRef { get; } [](start = 8, length = 19)

Expose RefKind property instead? #Resolved

Expose RefKind property instead?

Because the kind of the reference is irrelevant here. #Resolved

agocke

LGTM

jcouv · 2017-10-31T19:35:15Z

src/Compilers/CSharp/Portable/Symbols/LocalSymbol.cs

@@ -321,9 +321,9 @@ public object ConstantValue

        internal abstract ImmutableArray<Diagnostic> GetConstantValueDiagnostics(BoundExpression boundInitValue);

-        public bool IsRef => RefKind == RefKind.Ref;
+        public bool IsRef => RefKind != RefKind.None;


Consider updating some compiler tests for this. #Resolved

I added new tests along with symbol display. #Closed

jcouv · 2017-10-31T19:37:03Z

            Return False

Should this be proactively updated?

Refers to: src/Compilers/VisualBasic/Portable/Symbols/Source/LocalSymbol.vb:285 in b70c08e. [](commit_id = b70c08e, deletion_comment = False)

jcouv · 2017-10-31T19:38:57Z

src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs

+
+            if (operation.IsRef)
+            {
+                LogString(" (IsRef)");


I didn't see which test exercises this. #Resolved

I didn't see which test exercises this.

None at the moment. Only lowered tree can have nodes like this. Once ref reassignment feature is supported, then we can test this API.

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

jcouv

LGTM with one test suggestion.

AlekseyTs · 2017-10-31T19:42:38Z

            Return False

VB doesn't support ref locals.

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

Refers to: src/Compilers/VisualBasic/Portable/Symbols/Source/LocalSymbol.vb:285 in b70c08e. [](commit_id = b70c08e, deletion_comment = False)

VSadov · 2017-10-31T20:18:34Z

src/Compilers/Core/Portable/Symbols/ILocalSymbol.cs

@@ -23,11 +23,17 @@ public interface ILocalSymbol : ISymbol
        bool IsConst { get; }

        /// <summary>
-        /// Returns true if this local is a ref-local.
+        /// Returns true if this local is a ref local or a ref readonly local.
+        /// Use <see cref="RefKind"/> to get more detailed information.
        /// </summary>


So, "IsRef" is true for readonly references as well.
I do not have a strong opinion on this, but we have similar API for methods/properties, should we make sure it is also "inclusive"?

So, "IsRef" is true for readonly references as well.

Yes.

I do not have a strong opinion on this, but we have similar API for methods/properties, should we make sure it is also "inclusive"?

IMethodSymbol and IPropertySymbol also have ReturnsByRefReadonly property, I'll let you decide what correlation should be between it and ReturnsByRef property. If any adjustments are needed, I think they are out of scope of this PR.

jinujoseph · 2017-11-03T00:26:41Z

pre-approved to merge via link

* dotnet/master: (36 commits) Remove usage of IOperation feature flag in newly added unit tests More fixes for IOperation tree. (dotnet#23028) Address PR feedback and remove unused resource and comment out unused error code Remove IOperation feature flag and internal registration APIs for operations Add unit test for VB Address review feedback and add more unit tests Fix unit tests Fix build errors from merge resolution Revert unintentional newline deletion Address recent feedback and do not generate an IParenthesizedOperation for C#. This change is now just a test-only change that verifies the current behavior. Address PR feedback and use singleton for null instance Address PR feedback Address PR feedback Do not invoke GetStandaloneNode helper in GetOperationWorker Fix InvalidCastException in CSharpOperationFactory for invalid nested member initializer (dotnet#22983) Use ICollection<string>.Contains to avoid Enumerator allocation Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. (dotnet#22933) Enable peverify-compat mode for langver < 7.2 (dotnet#22772) Properly detect root of a tree in Operation.SearchParentOperation (dotnet#22974) Fix IArgument and IArrayInitializer to have null types ...

* dotnet/master: (28 commits) Fix the operation verifier code now that getoperationinternal no longer exists. Remove usage of IOperation feature flag in newly added unit tests More fixes for IOperation tree. (dotnet#23028) Address PR feedback and remove unused resource and comment out unused error code Remove IOperation feature flag and internal registration APIs for operations Add unit test for VB Address review feedback and add more unit tests Fix unit tests Fix build errors from merge resolution Revert unintentional newline deletion Address recent feedback and do not generate an IParenthesizedOperation for C#. This change is now just a test-only change that verifies the current behavior. Address PR feedback and use singleton for null instance Address PR feedback Address PR feedback Do not invoke GetStandaloneNode helper in GetOperationWorker Fix InvalidCastException in CSharpOperationFactory for invalid nested member initializer (dotnet#22983) Use ICollection<string>.Contains to avoid Enumerator allocation Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. (dotnet#22933) Enable peverify-compat mode for langver < 7.2 (dotnet#22772) Fix IArgument and IArrayInitializer to have null types ...

Add IsRef property to IConditionalOperation and ISimpleAssignmentOper…

b70c08e

…ation, add RefKind property to ILocalSymbol. Closes dotnet#21311.

AlekseyTs added Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation labels Oct 31, 2017

AlekseyTs added this to the 15.5 milestone Oct 31, 2017

AlekseyTs requested review from cston and a team October 31, 2017 17:13

dnfclas added the cla-already-signed label Oct 31, 2017

jinujoseph added the Needs Shiproom Approval label Oct 31, 2017

cston reviewed Oct 31, 2017

View reviewed changes

tmat reviewed Oct 31, 2017

View reviewed changes

cston approved these changes Oct 31, 2017

View reviewed changes

mavasani approved these changes Oct 31, 2017

View reviewed changes

agocke approved these changes Oct 31, 2017

View reviewed changes

jcouv reviewed Oct 31, 2017

View reviewed changes

jcouv approved these changes Oct 31, 2017

View reviewed changes

VSadov reviewed Oct 31, 2017

View reviewed changes

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Nov 3, 2017

Merge branch 'master' into Issue21311

e3408ef

AlekseyTs merged commit 79fc0a8 into dotnet:master Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. #22933

Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. #22933

AlekseyTs commented Oct 31, 2017 •

edited by jinujoseph

Loading

AlekseyTs commented Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs commented Oct 31, 2017 •

edited by jcouv

Loading

cston Oct 31, 2017

AlekseyTs Oct 31, 2017

tmat Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

tmat Oct 31, 2017

tmat Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

tmat Oct 31, 2017

tmat Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

agocke left a comment

jcouv Oct 31, 2017 •

edited by AlekseyTs

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

jcouv commented Oct 31, 2017

jcouv Oct 31, 2017 •

edited by AlekseyTs

Loading

AlekseyTs Oct 31, 2017

jcouv left a comment

AlekseyTs commented Oct 31, 2017

VSadov Oct 31, 2017

AlekseyTs Oct 31, 2017

jinujoseph commented Nov 3, 2017

Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. #22933

Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. #22933

Conversation

AlekseyTs commented Oct 31, 2017 • edited by jinujoseph Loading

AlekseyTs commented Oct 31, 2017 • edited by jcouv Loading

AlekseyTs commented Oct 31, 2017 • edited by jcouv Loading

cston Oct 31, 2017

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2017

Choose a reason for hiding this comment

tmat Oct 31, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

tmat Oct 31, 2017

Choose a reason for hiding this comment

tmat Oct 31, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

tmat Oct 31, 2017

Choose a reason for hiding this comment

tmat Oct 31, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

jcouv Oct 31, 2017 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Oct 31, 2017

jcouv Oct 31, 2017 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2017

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Oct 31, 2017

VSadov Oct 31, 2017

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2017

Choose a reason for hiding this comment

jinujoseph commented Nov 3, 2017

AlekseyTs commented Oct 31, 2017 •

edited by jinujoseph

Loading

AlekseyTs commented Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs commented Oct 31, 2017 •

edited by jcouv

Loading

tmat Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

tmat Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

tmat Oct 31, 2017 •

edited by jcouv

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

jcouv Oct 31, 2017 •

edited by AlekseyTs

Loading

AlekseyTs Oct 31, 2017 •

edited by jcouv

Loading

jcouv Oct 31, 2017 •

edited by AlekseyTs

Loading