Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brace completion and IOperation for with expression #44712

Merged
merged 14 commits into from
Jun 6, 2020
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ internal IOperation CreateInternal(BoundNode boundNode)
return CreateBoundDynamicIndexerAccessExpressionOperation((BoundDynamicIndexerAccess)boundNode);
case BoundKind.ObjectCreationExpression:
return CreateBoundObjectCreationExpressionOperation((BoundObjectCreationExpression)boundNode);
case BoundKind.WithExpression:
return CreateBoundWithExpressionOperation((BoundWithExpression)boundNode);
case BoundKind.DynamicObjectCreationExpression:
return CreateBoundDynamicObjectCreationExpressionOperation((BoundDynamicObjectCreationExpression)boundNode);
case BoundKind.ObjectInitializerExpression:
Expand Down Expand Up @@ -672,6 +674,16 @@ private IOperation CreateBoundObjectCreationExpressionOperation(BoundObjectCreat
return new CSharpLazyObjectCreationOperation(this, boundObjectCreationExpression, constructor.GetPublicSymbol(), _semanticModel, syntax, type, constantValue, isImplicit);
}

private IOperation CreateBoundWithExpressionOperation(BoundWithExpression boundWithExpression)
{
MethodSymbol constructor = boundWithExpression.CloneMethod;
SyntaxNode syntax = boundWithExpression.Syntax;
ITypeSymbol type = boundWithExpression.Type.GetPublicSymbol();
Optional<object> constantValue = ConvertToOptional(boundWithExpression.ConstantValue);
bool isImplicit = boundWithExpression.WasCompilerGenerated;
return new CSharpLazyWithExpressionOperation(this, boundWithExpression, constructor.GetPublicSymbol(), _semanticModel, syntax, type, constantValue, isImplicit);
}

private IDynamicObjectCreationOperation CreateBoundDynamicObjectCreationExpressionOperation(BoundDynamicObjectCreationExpression boundDynamicObjectCreationExpression)
{
ImmutableArray<string> argumentNames = boundDynamicObjectCreationExpression.ArgumentNamesOpt.NullToEmpty();
Expand Down
23 changes: 23 additions & 0 deletions src/Compilers/CSharp/Portable/Operations/CSharpOperationNodes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,29 @@ protected override ImmutableArray<IArgumentOperation> CreateArguments()
}
}

internal sealed class CSharpLazyWithExpressionOperation : LazyWithExpressionOperation
{
private readonly CSharpOperationFactory _operationFactory;
private readonly BoundWithExpression _withExpression;

internal CSharpLazyWithExpressionOperation(CSharpOperationFactory operationFactory, BoundWithExpression withExpression, IMethodSymbol cloneMethod, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit) :
base(cloneMethod, semanticModel, syntax, type, constantValue, isImplicit)
{
_operationFactory = operationFactory;
_withExpression = withExpression;
}

protected override IObjectOrCollectionInitializerOperation CreateInitializer()
{
return (IObjectOrCollectionInitializerOperation)_operationFactory.Create(_withExpression.InitializerExpression);
}

protected override IOperation CreateValue()
{
return _operationFactory.Create(_withExpression.Receiver);
}
}

internal sealed class CSharpLazyAnonymousObjectCreationOperation : LazyAnonymousObjectCreationOperation
{
private readonly CSharpOperationFactory _operationFactory;
Expand Down
399 changes: 395 additions & 4 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -252,5 +252,7 @@ public enum OperationKind
TypePattern = 0x6f,
/// <summary>Indicates an <see cref="IRelationalPatternOperation"/>.</summary>
RelationalPattern = 0x70,
/// <summary>Indicates an <see cref="IWithExpressionOperation"/>.</summary>
WithExpression = 0x71,
}
}
97 changes: 96 additions & 1 deletion src/Compilers/Core/Portable/Generated/Operations.Generated.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,7 @@ public interface IArrayCreationOperation : IOperation
/// Current usage:
/// (1) C# this or base expression.
/// (2) VB Me, MyClass, or MyBase expression.
/// (3) C# object or collection initializers.
/// (3) C# object or collection or 'with' expression initializers.
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or 'with' expression [](start = 37, length = 20)

This is a generated file, should probably modify XML instead #Resolved

/// (4) VB With statements, object or collection initializers.
/// </para>
/// </summary>
Expand Down Expand Up @@ -2876,6 +2876,32 @@ public interface IRelationalPatternOperation : IPatternOperation
/// </summary>
IOperation Value { get; }
}
/// <summary>
/// Represents cloning of an object instance.
/// <para>
/// Current usage:
/// (1) C# with expression.
/// </para>
/// </summary>
/// <remarks>
/// This interface is reserved for implementation by its associated APIs. We reserve the right to
/// change it in the future.
/// </remarks>
public interface IWithExpressionOperation : IOperation
{
/// <summary>
/// Value to be cloned.
/// </summary>
IOperation Value { get; }
/// <summary>
/// Clone method to be invoked on the value.
/// </summary>
IMethodSymbol CloneMethod { get; }
/// <summary>
/// With collection initializer.
/// </summary>
IObjectOrCollectionInitializerOperation Initializer { get; }
}
#endregion

#region Implementations
Expand Down Expand Up @@ -8529,6 +8555,73 @@ public override IOperation Value
}
}
}
internal abstract partial class BaseWithExpressionOperation : Operation, IWithExpressionOperation
{
internal BaseWithExpressionOperation(IMethodSymbol cloneMethod, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit)
: base(OperationKind.WithExpression, semanticModel, syntax, type, constantValue, isImplicit)
{
CloneMethod = cloneMethod;
}
public abstract IOperation Value { get; }
public IMethodSymbol CloneMethod { get; }
public abstract IObjectOrCollectionInitializerOperation Initializer { get; }
public override IEnumerable<IOperation> Children
{
get
{
if (Value is object) yield return Value;
if (Initializer is object) yield return Initializer;
}
}
public override void Accept(OperationVisitor visitor) => visitor.VisitWithExpression(this);
public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument) => visitor.VisitWithExpression(this, argument);
}
internal sealed partial class WithExpressionOperation : BaseWithExpressionOperation, IWithExpressionOperation
{
internal WithExpressionOperation(IOperation value, IMethodSymbol cloneMethod, IObjectOrCollectionInitializerOperation initializer, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit)
: base(cloneMethod, semanticModel, syntax, type, constantValue, isImplicit)
{
Value = SetParentOperation(value, this);
Initializer = SetParentOperation(initializer, this);
}
public override IOperation Value { get; }
public override IObjectOrCollectionInitializerOperation Initializer { get; }
}
internal abstract partial class LazyWithExpressionOperation : BaseWithExpressionOperation, IWithExpressionOperation
{
private IOperation _lazyValue = s_unset;
private IObjectOrCollectionInitializerOperation _lazyInitializer = s_unsetObjectOrCollectionInitializer;
internal LazyWithExpressionOperation(IMethodSymbol cloneMethod, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit)
: base(cloneMethod, semanticModel, syntax, type, constantValue, isImplicit){ }
protected abstract IOperation CreateValue();
public override IOperation Value
{
get
{
if (_lazyValue == s_unset)
{
IOperation value = CreateValue();
SetParentOperation(value, this);
Interlocked.CompareExchange(ref _lazyValue, value, s_unset);
}
return _lazyValue;
}
}
protected abstract IObjectOrCollectionInitializerOperation CreateInitializer();
public override IObjectOrCollectionInitializerOperation Initializer
{
get
{
if (_lazyInitializer == s_unsetObjectOrCollectionInitializer)
{
IObjectOrCollectionInitializerOperation initializer = CreateInitializer();
SetParentOperation(initializer, this);
Interlocked.CompareExchange(ref _lazyInitializer, initializer, s_unsetObjectOrCollectionInitializer);
}
return _lazyInitializer;
}
}
}
#endregion
#region Visitors
public abstract partial class OperationVisitor
Expand Down Expand Up @@ -8655,6 +8748,7 @@ internal virtual void VisitNoneOperation(IOperation operation) { /* no-op */ }
public virtual void VisitBinaryPattern(IBinaryPatternOperation operation) => DefaultVisit(operation);
public virtual void VisitTypePattern(ITypePatternOperation operation) => DefaultVisit(operation);
public virtual void VisitRelationalPattern(IRelationalPatternOperation operation) => DefaultVisit(operation);
public virtual void VisitWithExpression(IWithExpressionOperation operation) => DefaultVisit(operation);
}
public abstract partial class OperationVisitor<TArgument, TResult>
{
Expand Down Expand Up @@ -8780,6 +8874,7 @@ public abstract partial class OperationVisitor<TArgument, TResult>
public virtual TResult VisitBinaryPattern(IBinaryPatternOperation operation, TArgument argument) => DefaultVisit(operation, argument);
public virtual TResult VisitTypePattern(ITypePatternOperation operation, TArgument argument) => DefaultVisit(operation, argument);
public virtual TResult VisitRelationalPattern(IRelationalPatternOperation operation, TArgument argument) => DefaultVisit(operation, argument);
public virtual TResult VisitWithExpression(IWithExpressionOperation operation, TArgument argument) => DefaultVisit(operation, argument);
}
#endregion
}
14 changes: 14 additions & 0 deletions src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6982,5 +6982,19 @@ public override IOperation VisitUsingDeclaration(IUsingDeclarationOperation oper
throw ExceptionUtilities.Unreachable;
}

public override IOperation VisitWithExpression(IWithExpressionOperation operation, int? argument)
{
EvalStackFrame frame = PushStackFrame();
// Initializer is removed from the tree and turned into a series of statements that assign to the cloned instance
IOperation visitedInstance = Visit(operation.Value);

IOperation cloned = operation.CloneMethod is null
? MakeInvalidOperation(visitedInstance.Type, visitedInstance)
: new InvocationOperation(operation.CloneMethod, visitedInstance,
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

visitedInstance [](start = 65, length = 15)

Is Clone method an instance method? #Closed

Copy link
Member Author

@jcouv jcouv Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is an instance method #Resolved

isVirtual: false, arguments: ImmutableArray<IArgumentOperation>.Empty,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false [](start = 31, length = 5)

Based on #44852, the call probably should be virtual

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will fix in a follow-up PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlekseyTs It looks like Andy fixed this already (we now have isVirtual: true in master.

semanticModel: null, operation.Syntax, operation.Type, operation.ConstantValue, isImplicit: true);

return PopStackFrame(frame, HandleObjectOrCollectionInitializer(operation.Initializer, cloned));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ public enum InstanceReferenceKind
ContainingTypeInstance,
/// <summary>
/// Reference to the object being initialized in C# or VB object or collection initializer,
/// anonymous type creation initializer, or to the object being referred to in a VB With statement.
/// anonymous type creation initializer, or to the object being referred to in a VB With statement,
/// or the C# 'with' expression initializer.
/// </summary>
ImplicitReceiver,
/// <summary>
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/Core/Portable/Operations/OperationCloner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -662,5 +662,10 @@ public override IOperation VisitUsingDeclaration(IUsingDeclarationOperation oper
{
return new UsingDeclarationOperation(Visit(operation.DeclarationGroup), operation.IsAsynchronous, ((Operation)operation).OwningSemanticModel, operation.Syntax, operation.Type, operation.ConstantValue, operation.IsImplicit);
}

public override IOperation VisitWithExpression(IWithExpressionOperation operation, object argument)
{
return new WithExpressionOperation(Visit(operation.Value), operation.CloneMethod, Visit(operation.Initializer), ((Operation)operation).OwningSemanticModel, operation.Syntax, operation.Type, operation.ConstantValue, operation.IsImplicit);
}
}
}
30 changes: 30 additions & 0 deletions src/Compilers/Core/Portable/Operations/OperationInterfaces.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
UnusedOperationKinds indicates kinds that are currently skipped by the OperationKind
enum. They can be used by future nodes by inserting those nodes at the correct point
in the list.

When implementing new operations, run tests with additional IOperation validation enabled.
You can test with `Build.cmd -testIOperation`.
Or to repro in VS, you can do `set ROSLYN_TEST_IOPERATION=true` then `devenv`.
-->

<UnusedOperationKinds>
Expand Down Expand Up @@ -3001,4 +3005,30 @@
</Comments>
</Property>
</Node>
<Node Name="IWithExpressionOperation" Base="IOperation" ChildrenOrder="Value,Initializer">
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IWithExpressionOperation [](start = 14, length = 24)

I think we would want a special rewrite for this node in CFG, something very similar to a an object initializer operation #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IWithExpressionOperation [](start = 14, length = 24)

Please run all tests with -testIOperation switch locally and confirm that none of the failures are related to the feature. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IWithExpressionOperation [](start = 14, length = 24)

I think we need to adjust OperationCloner for this node #Closed

<Comments>
<summary>
Represents cloning of an object instance.
<para>
Current usage:
(1) C# with expression.
</para>
</summary>
</Comments>
<Property Name="Value" Type="IOperation">
<Comments>
<summary>Value to be cloned.</summary>
</Comments>
</Property>
<Property Name="CloneMethod" Type="IMethodSymbol">
<Comments>
<summary>Clone method to be invoked on the value.</summary>
</Comments>
</Property>
<Property Name="Initializer" Type="IObjectOrCollectionInitializerOperation">
<Comments>
<summary>With collection initializer.</summary>
</Comments>
</Property>
</Node>
</Tree>
7 changes: 7 additions & 0 deletions src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Microsoft.CodeAnalysis.OperationKind.BinaryPattern = 110 -> Microsoft.CodeAnalys
Microsoft.CodeAnalysis.OperationKind.NegatedPattern = 109 -> Microsoft.CodeAnalysis.OperationKind
Microsoft.CodeAnalysis.OperationKind.RelationalPattern = 112 -> Microsoft.CodeAnalysis.OperationKind
Microsoft.CodeAnalysis.OperationKind.TypePattern = 111 -> Microsoft.CodeAnalysis.OperationKind
Microsoft.CodeAnalysis.OperationKind.WithExpression = 113 -> Microsoft.CodeAnalysis.OperationKind
Microsoft.CodeAnalysis.Operations.CommonConversion.IsNullable.get -> bool
Microsoft.CodeAnalysis.Operations.IBinaryPatternOperation
Microsoft.CodeAnalysis.Operations.IBinaryPatternOperation.LeftPattern.get -> Microsoft.CodeAnalysis.Operations.IPatternOperation
Expand All @@ -25,16 +26,22 @@ Microsoft.CodeAnalysis.Operations.IRelationalPatternOperation.OperatorKind.get -
Microsoft.CodeAnalysis.Operations.IRelationalPatternOperation.Value.get -> Microsoft.CodeAnalysis.IOperation
Microsoft.CodeAnalysis.Operations.ITypePatternOperation
Microsoft.CodeAnalysis.Operations.ITypePatternOperation.MatchedType.get -> Microsoft.CodeAnalysis.ITypeSymbol
Microsoft.CodeAnalysis.Operations.IWithExpressionOperation
Microsoft.CodeAnalysis.Operations.IWithExpressionOperation.CloneMethod.get -> Microsoft.CodeAnalysis.IMethodSymbol
Microsoft.CodeAnalysis.Operations.IWithExpressionOperation.Initializer.get -> Microsoft.CodeAnalysis.Operations.IObjectOrCollectionInitializerOperation
Microsoft.CodeAnalysis.Operations.IWithExpressionOperation.Value.get -> Microsoft.CodeAnalysis.IOperation
Microsoft.CodeAnalysis.SymbolKind.FunctionPointer = 20 -> Microsoft.CodeAnalysis.SymbolKind
Microsoft.CodeAnalysis.TypeKind.FunctionPointer = 13 -> Microsoft.CodeAnalysis.TypeKind
static Microsoft.CodeAnalysis.AnalyzerConfigSet.Create<TList>(TList analyzerConfigs, out System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Diagnostic> diagnostics) -> Microsoft.CodeAnalysis.AnalyzerConfigSet
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor.VisitBinaryPattern(Microsoft.CodeAnalysis.Operations.IBinaryPatternOperation operation) -> void
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor.VisitNegatedPattern(Microsoft.CodeAnalysis.Operations.INegatedPatternOperation operation) -> void
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor.VisitRelationalPattern(Microsoft.CodeAnalysis.Operations.IRelationalPatternOperation operation) -> void
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor.VisitTypePattern(Microsoft.CodeAnalysis.Operations.ITypePatternOperation operation) -> void
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor.VisitWithExpression(Microsoft.CodeAnalysis.Operations.IWithExpressionOperation operation) -> void
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor<TArgument, TResult>.VisitBinaryPattern(Microsoft.CodeAnalysis.Operations.IBinaryPatternOperation operation, TArgument argument) -> TResult
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor<TArgument, TResult>.VisitNegatedPattern(Microsoft.CodeAnalysis.Operations.INegatedPatternOperation operation, TArgument argument) -> TResult
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor<TArgument, TResult>.VisitRelationalPattern(Microsoft.CodeAnalysis.Operations.IRelationalPatternOperation operation, TArgument argument) -> TResult
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor<TArgument, TResult>.VisitTypePattern(Microsoft.CodeAnalysis.Operations.ITypePatternOperation operation, TArgument argument) -> TResult
virtual Microsoft.CodeAnalysis.Operations.OperationVisitor<TArgument, TResult>.VisitWithExpression(Microsoft.CodeAnalysis.Operations.IWithExpressionOperation operation, TArgument argument) -> TResult
virtual Microsoft.CodeAnalysis.SymbolVisitor.VisitFunctionPointerType(Microsoft.CodeAnalysis.IFunctionPointerTypeSymbol symbol) -> void
virtual Microsoft.CodeAnalysis.SymbolVisitor<TResult>.VisitFunctionPointerType(Microsoft.CodeAnalysis.IFunctionPointerTypeSymbol symbol) -> TResult
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ private static (SyntaxToken openingDelimeter, SyntaxToken closingDelimiter) GetD
return (bracketedArgumentList.OpenBracketToken, bracketedArgumentList.CloseBracketToken);

case SyntaxKind.ObjectInitializerExpression:
case SyntaxKind.WithInitializerExpression:
var initializerExpressionSyntax = (InitializerExpressionSyntax)currentNode;
return (initializerExpressionSyntax.OpenBraceToken, initializerExpressionSyntax.CloseBraceToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,60 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.AutomaticCompletion
{
public class AutomaticBraceCompletionTests : AbstractAutomaticBraceCompletionTests
{
[WpfFact, Trait(Traits.Feature, Traits.Features.AutomaticCompletion)]
public void WithExpressionBracesSameLine()
{
var code = @"
class C
{
void M(C c)
{
c = c with $$
}
}";

var expected = @"
class C
{
void M(C c)
{
c = c with { }
}
}";
using var session = CreateSession(code);
Assert.NotNull(session);

CheckStart(session.Session);
CheckText(session.Session, expected);
}

[WpfFact, Trait(Traits.Feature, Traits.Features.AutomaticCompletion)]
public void WithExpressionBracesSameLine_Enter()
{
var code = @"
class C
{
void M(C c)
{
c = c with $$
}
}";
var expected = @"
class C
{
void M(C c)
{
c = c with
{

}
}
}";
using var session = CreateSession(code);
CheckStart(session.Session);
CheckReturn(session.Session, 12, expected);
}

[WpfFact, Trait(Traits.Feature, Traits.Features.AutomaticCompletion)]
public void Creation()
{
Expand Down
Loading