refactored IAssignment/ICompoundAssignment as we discussed and removed IndexPropertyReference #20431

heejaechang · 2017-06-23T19:56:26Z

refactored IAssignment/ICoumoundAssignment as we discussed. broke out IncrementalExpression from assignment expression.

merged Indexed and normal propertyreference expression and removed IndexedProperty interface.

also, made more code to use same patterns.

…to sit on top of IBaseAssignmentExpression rather than IAssignmentExpression

heejaechang · 2017-06-23T19:58:35Z

@dotnet/analyzer-ioperation @dotnet/roslyn-analysis can you take a look?

heejaechang · 2017-06-23T19:59:09Z

@jinujoseph I think we had a bug about this, but I couldn't find it. do you know what that is?

heejaechang · 2017-06-23T21:20:48Z

@AlekseyTs @cston @jinujoseph how do you think about this change?

heejaechang · 2017-06-23T21:24:55Z

@mavasani has this wok item - #18056 (comment)

which is this
...
Design Team Notes

We need to simplify the hierarchy, The intend here is we are not going to have the complex inheritance hierarchy where in the same hierarchy there is lot of base. We should try to make it more flat and narrow. IExpression might have Iincrementexpression, IcompondAssignment , IAssignementExpression and those themselves can implement side hierarchy, which can be call as infos which are not Ioperation Kind , Say IAssignmentInfos and some one can uniformally operate on this things.
...

I dont believe we can make IOperation hierarchy flat since we have cases such as ICaseClause where we want to return base type (ICaseClauses) when needed.

but we can make IIncrementExpression, ICoumpoundAssignment, IAssignmentExpression to IHasXXXX pattern if want to.

but I believe we can address that in different PR, this PR at least remove those odd cases where an interface are based interface for other interface and at the same time, itself has implementation.

…ntExpression

cston · 2017-06-26T15:44:34Z

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

+    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
+    /// change it in the future.
+    /// </remarks>
+    public interface IAssignmentExpression : IBaseAssignmentExpression


What code will be consuming or implementing IAssignmentExpression rather than IBaseAssignmentExpression?

heejaechang · 2017-06-27T21:15:53Z

@mavasani @dotnet/analyzer-ioperation can you take a look?

heejaechang · 2017-06-27T23:21:41Z

@AlekseyTs can you take a look at this PR?

heejaechang · 2017-06-27T23:32:14Z

@dotnet/roslyn-infrastructure
https://ci.dot.net/job/dotnet_roslyn/job/features_ioperation/job/windows_debug_vs-integration_prtest/126/

this seems happening in multiple PR.

heejaechang · 2017-06-29T20:23:31Z

@dotnet/roslyn-infrastructure test failed
Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicExpressionEvaluator.StateMachineTypeParameters

https://ci.dot.net/job/dotnet_roslyn/job/features_ioperation/job/windows_debug_vs-integration_prtest/127/

heejaechang · 2017-06-29T20:23:42Z

retest windows_debug_vs-integration_prtest please

heejaechang · 2017-06-29T20:27:02Z

@AlekseyTs is on vacation. can @cston @jcouv take a look? I talked about the design with @AlekseyTs and following his suggestion.

IAssignmentExpression is now purely used as base interface, implementation only exist for SimpleAssignment and CompoundAssignment.

InncrementExpression no longer inherit from CompoundAssignment. and it is its own type.

IndexedPropertyReference and PropertyReference is merged and Argument return > 0 if it is indexed property.

jinujoseph · 2017-06-29T20:37:29Z

cc @dotnet/roslyn-compiler

heejaechang · 2017-06-29T21:43:45Z

ping? can I get one more sign off from compiler team

jcouv · 2017-06-29T21:51:19Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

@@ -1,5 +1,9 @@
 *REMOVED*Microsoft.CodeAnalysis.Compilation.Emit(System.IO.Stream peStream, System.IO.Stream pdbStream = null, System.IO.Stream xmlDocumentationStream = null, System.IO.Stream win32Resources = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.ResourceDescription> manifestResources = null, Microsoft.CodeAnalysis.Emit.EmitOptions options = null, Microsoft.CodeAnalysis.IMethodSymbol debugEntryPoint = null, System.IO.Stream sourceLinkStream = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.EmbeddedText> embeddedTexts = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.Emit.EmitResult
 *REMOVED*Microsoft.CodeAnalysis.Emit.EmitOptions.EmitOptions(bool metadataOnly = false, Microsoft.CodeAnalysis.Emit.DebugInformationFormat debugInformationFormat = (Microsoft.CodeAnalysis.Emit.DebugInformationFormat)0, string pdbFilePath = null, string outputNameOverride = null, int fileAlignment = 0, ulong baseAddress = 0, bool highEntropyVirtualAddressSpace = false, Microsoft.CodeAnalysis.SubsystemVersion subsystemVersion = default(Microsoft.CodeAnalysis.SubsystemVersion), string runtimeMetadataVersion = null, bool tolerateErrors = false, bool includePrivateMembers = false, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Emit.InstrumentationKind> instrumentationKinds = default(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Emit.InstrumentationKind>)) -> void
+*REMOVED*Microsoft.CodeAnalysis.Semantics.IAssignmentExpression.Target.get -> Microsoft.CodeAnalysis.IOperation


Did IOperations APIs previously ship?

let me take a look. we did ship it under feature flag though

jcouv · 2017-06-29T21:55:22Z

src/Compilers/Core/Portable/Generated/Operations.xml.Generated.cs

@@ -385,10 +385,29 @@ internal abstract partial class BaseAssignmentExpression : Operation, IAssignmen
    /// <summary>
    /// Represents an assignment expression.


// a simple assignment

jcouv · 2017-06-29T21:58:55Z

src/Compilers/Core/Portable/Generated/Operations.xml.Generated.cs

    }

    /// <summary>
    /// Represents an assignment expression.
    /// </summary>
-    internal sealed partial class LazyAssignmentExpression : BaseAssignmentExpression, IAssignmentExpression
+    internal sealed partial class LazySimpleAssignmentExpression : BaseSimpleAssignmentExpression, ISimpleAssignmentExpression
    {
        private readonly Lazy<IOperation> _lazyTarget;


As a general question about the lazy pattern in IOperations, why not just implement this as:
class Lazy { private readonly Lazy<SimpleAssignmentExpression>; ... }

Do we expect that the different lazy parts with materialized separately?
[not blocking this PR]

we don't want to realize all children at some for example blocks.

by the way, this Lazy pattern is ver 1. so that we can share one implementation for both C# and VB. we might later move this to C#/VB explicit type that directly hold on to bound node that implements common IOperation interface for laziness. (more perf optimized version)

we decide this way since interface is the public API and implementation we can change later.

jcouv · 2017-06-29T22:05:22Z

src/Compilers/Core/Portable/Generated/Operations.xml.Generated.cs

        {
-            InitializedProperty = initializedProperty;
            _lazyValue = value ?? throw new System.ArgumentNullException("value");


nameof(value) instead of "value"
Same for other other instances of throw ArgumentNullException

jcouv · 2017-06-29T22:20:04Z

src/Test/Utilities/Portable/Diagnostics/OperationTestAnalyzer.cs

@@ -182,9 +182,9 @@ public sealed override void Initialize(AnalysisContext context)
                                                     IOperation advanceIncrement = null;
                                                     BinaryOperationKind advanceOperationCode = BinaryOperationKind.None;

-                                                     if (advanceExpression.Kind == OperationKind.AssignmentExpression)
+                                                     if (advanceExpression.Kind == OperationKind.SimpleAssignmentExpression)


Not related to this PR, but this method is getting quite deep. Consider refactoring.

jcouv

LGTM

heejaechang · 2017-06-30T03:57:25Z

@dotnet/roslyn-infrastructure windows_debug_vs-integration_prtest failed due to vsix installation.

looks like busted in multiple PR
https://ci.dot.net/job/dotnet_roslyn/job/features_ioperation/job/windows_debug_vs-integration_prtest/135/

heejaechang · 2017-07-05T18:57:36Z

#8755

heejaechang added 6 commits June 23, 2017 11:54

added IBaseAssignmentExpression and made IComponentAssigntExpression …

8753f9c

…to sit on top of IBaseAssignmentExpression rather than IAssignmentExpression

added IBaseCompoundAssignmentExpression

6054e7b

added IBasePropertyReferenceExpression

acb3bdf

fix some missing issues

56cc92f

new public interfaces

38dfa2e

removed public apis

e9b08af

dnfclas added the cla-already-signed label Jun 23, 2017

heejaechang changed the base branch from master to features/ioperation June 23, 2017 19:56

heejaechang changed the title ~~first step adding parent API~~ added Base interface to 3 interfaces (IAssignmentExpression, ICompoundAssignmentExpression, IPropertyReferenceExpression) which doesn't follow existing pattern Jun 23, 2017

changed IInrementExpression. no longer derived from ICompoundAssignme…

a859163

…ntExpression

cston reviewed Jun 26, 2017

View reviewed changes

heejaechang added 6 commits June 26, 2017 14:58

addressed AssignmentExpression design

7dada16

remove IndexedPropertyReference and merge it to PropertyReference

ba0c404

made CaseClause to follow same pattern

9597485

made symbol initializer to follow same pattern

ece36be

fixed wrong pattern

2e2bde4

more fix for consistancy

d3a5cff

heejaechang changed the title ~~added Base interfaces to 3 interfaces (IAssignmentExpression, ICompoundAssignmentExpression, IPropertyReferenceExpression) which don't follow existing pattern~~ refactored IAssignment/ICompoundAssignment as we discussed and removed IndexPropertyReference Jun 27, 2017

Pilchie added the Area-Analyzers label Jun 27, 2017

made all tests pass

03f3a68

jaredpar approved these changes Jun 29, 2017

View reviewed changes

cston mentioned this pull request Jun 29, 2017

[Test failure] BasicExpressionEvaluator.StateMachineTypeParameters: Could not find the local named Type variables #20552

Open

jcouv reviewed Jun 29, 2017

View reviewed changes

jcouv approved these changes Jun 29, 2017

View reviewed changes

heejaechang added 2 commits June 29, 2017 20:20

addressed PR feedback

5f680d6

missed adding simple to one comment

c2fcba9

heejaechang merged commit 7926277 into dotnet:features/ioperation Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactored IAssignment/ICompoundAssignment as we discussed and removed IndexPropertyReference #20431

refactored IAssignment/ICompoundAssignment as we discussed and removed IndexPropertyReference #20431

heejaechang commented Jun 23, 2017 •

edited

Loading

heejaechang commented Jun 23, 2017

heejaechang commented Jun 23, 2017

heejaechang commented Jun 23, 2017

heejaechang commented Jun 23, 2017

cston Jun 26, 2017

heejaechang commented Jun 27, 2017

heejaechang commented Jun 27, 2017

heejaechang commented Jun 27, 2017

heejaechang commented Jun 29, 2017

heejaechang commented Jun 29, 2017

heejaechang commented Jun 29, 2017

jinujoseph commented Jun 29, 2017

heejaechang commented Jun 29, 2017

jcouv Jun 29, 2017

heejaechang Jun 29, 2017

jcouv Jun 29, 2017

heejaechang Jun 29, 2017

jcouv Jun 29, 2017

heejaechang Jun 29, 2017 •

edited

Loading

jcouv Jun 29, 2017 •

edited

Loading

heejaechang Jun 29, 2017

jcouv Jun 29, 2017

jcouv left a comment

heejaechang commented Jun 30, 2017

heejaechang commented Jul 5, 2017

		@@ -385,10 +385,29 @@ internal abstract partial class BaseAssignmentExpression : Operation, IAssignmen
		/// <summary>
		/// Represents an assignment expression.

refactored IAssignment/ICompoundAssignment as we discussed and removed IndexPropertyReference #20431

refactored IAssignment/ICompoundAssignment as we discussed and removed IndexPropertyReference #20431

Conversation

heejaechang commented Jun 23, 2017 • edited Loading

heejaechang commented Jun 23, 2017

heejaechang commented Jun 23, 2017

heejaechang commented Jun 23, 2017

heejaechang commented Jun 23, 2017

Choose a reason for hiding this comment

heejaechang commented Jun 27, 2017

heejaechang commented Jun 27, 2017

heejaechang commented Jun 27, 2017

heejaechang commented Jun 29, 2017

heejaechang commented Jun 29, 2017

heejaechang commented Jun 29, 2017

jinujoseph commented Jun 29, 2017

heejaechang commented Jun 29, 2017

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

Choose a reason for hiding this comment

heejaechang Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Jun 29, 2017 • edited Loading

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

heejaechang commented Jun 30, 2017

heejaechang commented Jul 5, 2017

heejaechang commented Jun 23, 2017 •

edited

Loading

heejaechang Jun 29, 2017 •

edited

Loading

jcouv Jun 29, 2017 •

edited

Loading