-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDE side lgtm.
@dotnet/roslyn-compiler for review. |
|
||
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Formatting.Indentation | ||
{ | ||
public class SmartTokenFormatterFormatTokenTests : CSharpFormatterTestsBase | ||
{ | ||
public SmartTokenFormatterFormatTokenTests(ITestOutputHelper output) : base(output) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, we don't place {}
on the same line ever. #Resolved
public CoreFormatterTestsBase(ITestOutputHelper output) | ||
{ | ||
this._output = output; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public CoreFormatterTestsBase(ITestOutputHelper output) | |
{ | |
this._output = output; | |
} | |
public CoreFormatterTestsBase(ITestOutputHelper output) | |
=> _output = output; |
Assert.Equal(expected, actual); | ||
if (actual != expected) | ||
{ | ||
_output.WriteLine(actual); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what't he value of this? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives easy access to the actual result in Test Explorer, in addition to the usual diff output with pluses and minuses. #Resolved
@@ -865,7 +865,7 @@ private static bool IsPropertyNameOfObjectInitializer(SimpleNameSyntax identifie | |||
|
|||
while (parent != null) | |||
{ | |||
if (parent.Kind() == SyntaxKind.ObjectInitializerExpression) | |||
if (parent.IsKind(SyntaxKind.ObjectInitializerExpression, SyntaxKind.WithInitializerExpression)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm fine with this. however, actual question about with
. does it allow for nested initializers like object initializers do? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with
is limited to one level, simple assignments only.
{ Property = 1 }
is okay{ [0] = 1 }
is illegal{ 1, 2, 3 }
is illegal{ Property = { ... } }
is illegal #Resolved
@@ -662,5 +662,7 @@ 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); | |||
} | |||
|
|||
// TODO2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If work still needs to be done here, could you please create a follow-up issue and reference it instead of including a TODO in source? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I'll remove the comment and file an issue.
I think OperationCloner
is used by CFG, we'll probably need to do some work there for records. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note to test plan #Resolved
@@ -3001,4 +3001,30 @@ | |||
</Comments> | |||
</Property> | |||
</Node> | |||
<Node Name="IWithExpressionOperation" Base="IOperation" ChildrenOrder="Value,Initializer"> |
There was a problem hiding this comment.
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
@@ -1974,6 +1974,18 @@ public override void VisitReDimClause(IReDimClauseOperation operation) | |||
VisitArray(operation.DimensionSizes, "DimensionSizes", logElementCount: true); | |||
} | |||
|
|||
public override void VisitWithExpression(IWithExpressionOperation operation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VisitWithExpression [](start = 29, length = 19)
We have another test visitor that should be adjusted as well #Closed
@@ -208,6 +208,10 @@ public static (SyntaxToken openBrace, SyntaxToken closeBrace) GetBraces(this Syn | |||
return (switchExpression.OpenBraceToken, switchExpression.CloseBraceToken); | |||
case PropertyPatternClauseSyntax property: | |||
return (property.OpenBraceToken, property.CloseBraceToken); | |||
#if !CODE_STYLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have any way of tracking when this #if
should be removed? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a general issue for removing all CODE_STYLE
conditions:
#41462 #Resolved
@@ -3001,4 +3001,30 @@ | |||
</Comments> | |||
</Property> | |||
</Node> | |||
<Node Name="IWithExpressionOperation" Base="IOperation" ChildrenOrder="Value,Initializer"> |
There was a problem hiding this comment.
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
? visitedInstance | ||
: new InvocationOperation(operation.CloneMethod, visitedInstance, | ||
isVirtual: false, arguments: ImmutableArray<IArgumentOperation>.Empty, | ||
semanticModel: null, operation.Syntax, operation.Type, operation.ConstantValue, IsImplicit(operation)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsImplicit(operation) [](start = 100, length = 21)
I think the call should always be implicit. #Closed
private readonly CSharpOperationFactory _operationFactory; | ||
private readonly BoundWithExpression _withExpression; | ||
|
||
internal CSharpLazyWithExpressionOperation(CSharpOperationFactory operationFactory, BoundWithExpression withExpression, IMethodSymbol constructor, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constructor [](start = 142, length = 11)
Is this the clone method? The name is confusing. #Closed
|
||
IOperation cloned = operation.CloneMethod is null | ||
? visitedInstance | ||
: new InvocationOperation(operation.CloneMethod, visitedInstance, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
It doesn't look like any tests were added on the compiler side for IOperation or CFG #Closed |
Done with review pass (iteration 9) #Closed |
I don't know what you mean. Look for calls to VerifyFlowGraph and VerifyOperationTree in RecordsTest #Closed |
I accidentally collapsed that folder. Sorry. In reply to: 639073864 [](ancestors = 639073864) |
Consider adding a note to Test Plan to add tests for scenarios involving control flow inside 'with' expression #Closed |
Left: | ||
IPropertyReferenceOperation: System.Int32 C.X { get; init; } (OperationKind.PropertyReference, Type: System.Int32) (Syntax: 'X') | ||
Instance Receiver: | ||
IInstanceReferenceOperation (ReferenceKind: ImplicitReceiver) (OperationKind.InstanceReference, Type: C, IsImplicit) (Syntax: 'X') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IInstanceReferenceOperation (ReferenceKind: ImplicitReceiver) (OperationKind.InstanceReference, Type: C, IsImplicit) (Syntax: 'X') [](start = 18, length = 130)
Consider adjusting the comment for the node and the ReferenceKind to include information about this usage #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlekseyTs I didn't understand this comment. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand this comment.
IInstanceReferenceOperation and OperationKind.InstanceReference are now used in a new scenario. Consider reflecting this in the doc comment for them
In reply to: 436106963 [](ancestors = 436106963)
Added " test control flow within with expression initializer" In reply to: 639150193 [](ancestors = 639150193) |
@@ -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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (iteration 12), modulo direct modification of a comment in a generated file
@jcouv FYI it looks like the InlineIntoWithExpression test is broken. |
IOperation cloned = operation.CloneMethod is null | ||
? MakeInvalidOperation(visitedInstance.Type, visitedInstance) | ||
: new InvocationOperation(operation.CloneMethod, visitedInstance, | ||
isVirtual: false, arguments: ImmutableArray<IArgumentOperation>.Empty, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
@jcouv did we discuss using something like |
@333fred I discussed re-using I'll start a thread with broader IOperation v-team to confirm shape and names. Thanks |
Relates to #40726 (test plan)