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.

Sign up for GitHub

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

Jump to bottom

Allow with on anonymous types #53248

Merged
merged 13 commits into from
May 21, 2021
Merged

Allow with on anonymous types #53248

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1365a81
Allow `with` on anonymous types
jcouv May 3, 2021
302fbcc
Fix scenario with invalid assignment
jcouv May 7, 2021
ff57fe8
Enable IOperation flag temporarily
jcouv May 7, 2021
dc0c581
IDE: fix property completion in 'with'
jcouv May 10, 2021
85b60cf
Avoid capture if unused
jcouv May 11, 2021
ff73199
Address feedback
jcouv May 11, 2021
13ceeb7
Address feedback
jcouv May 11, 2021
ef7097e
tweaks
jcouv May 11, 2021
f930d4b
Use properties
jcouv May 11, 2021
0e97cf9
Address feedback
jcouv May 12, 2021
2bad303
Use ArrayBuilder
jcouv May 19, 2021
9f3cdac
Remove cast
jcouv May 19, 2021
3a6e4e0
Formatting
jcouv May 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
339 changes: 215 additions & 124 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs
View file
Open in desktop

Large diffs are not rendered by default.

25 changes: 19 additions & 6 deletions src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7201,6 +7201,25 @@ IOperation handleAnonymousTypeWithExpression(WithOperation operation, int? captu
foreach (IOperation initializer in initializers)
{
var simpleAssignment = (ISimpleAssignmentOperation)initializer;
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(ISimpleAssignmentOperation)initializer

Same comment about the cast as in setAllProperties #Closed


int valueCaptureId = GetNextCaptureId(outerCaptureRegion);
VisitAndCapture(simpleAssignment.Value, valueCaptureId);
LeaveRegionsUpTo(innerCaptureRegion);
var valueCaptureRef = new FlowCaptureReferenceOperation(valueCaptureId, operation.Operand.Syntax,
operation.Operand.Type, constantValue: operation.Operand.GetConstantValue());

if (simpleAssignment.Target is InvalidOperation)
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if (simpleAssignment.Target is InvalidOperation)

I think that it would be better to use the following error recovery strategy. Anything that we cannot reliably translate to an Anonymous Type property assignment we are simply visting, adding as a statement and forgetting without capturing it and adding fake assignments in AnonymousObjectCreationOperation. We should also see what to do with duplicate initializers for the same property, I think those should be "skipped" in a similar manner. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

InvalidOperation

Since below we are casting to IPropertyReferenceOperation, consider performing a type check for that instead. If the only alternative that we expect is an InvalidOperation, we can assert that when the type check fails. #Closed

{
var badTarget = new InvalidOperation(ImmutableArray<IOperation>.Empty, semanticModel: null, simpleAssignment.Target.Syntax,
type: null, constantValue: null, isImplicit: true);

var badAssignment = new SimpleAssignmentOperation(isRef: false, badTarget, valueCaptureRef,
semanticModel: null, operation.Syntax, valueCaptureRef.Type, constantValue: null, isImplicit: true);

initializerBuilder.Add(badAssignment);
continue;
}

var propertyReference = (IPropertyReferenceOperation)simpleAssignment.Target;
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(IPropertyReferenceOperation)simpleAssignment.Target;

Is this cast always safe? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. Nested initializers and indexing initializers are parsing errors. Expressions fall into the InvalidOperation check above.


Debug.Assert(propertyReference != null);
Expand All @@ -7209,12 +7228,6 @@ IOperation handleAnonymousTypeWithExpression(WithOperation operation, int? captu
Debug.Assert(propertyReference.Instance.Kind == OperationKind.InstanceReference);
Debug.Assert(((IInstanceReferenceOperation)propertyReference.Instance).ReferenceKind == InstanceReferenceKind.ImplicitReceiver);

int valueCaptureId = GetNextCaptureId(outerCaptureRegion);
VisitAndCapture(simpleAssignment.Value, valueCaptureId);
LeaveRegionsUpTo(innerCaptureRegion);
var valueCaptureRef = new FlowCaptureReferenceOperation(valueCaptureId, operation.Operand.Syntax,
operation.Operand.Type, constantValue: operation.Operand.GetConstantValue());

var property = propertyReference.Property;
var assignment = makeAssignment(property, valueCaptureRef, operation);

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/Test/Core/Compilation/CompilationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

#nullable disable
// Uncomment to enable the IOperation test hook on all test runs. Do not commit this uncommented.
//#define ROSLYN_TEST_IOPERATION
#define ROSLYN_TEST_IOPERATION
// TODO2 restore comment

using System;
using System.Collections.Generic;
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/Test/Core/Compilation/OperationTreeVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1369,6 +1369,12 @@ public override void VisitAnonymousObjectCreation(IAnonymousObjectCreationOperat
foreach (var initializer in operation.Initializers)
{
var simpleAssignment = (ISimpleAssignmentOperation)initializer;
if (simpleAssignment.Target is InvalidOperation)
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if (simpleAssignment.Target is InvalidOperation)

I think we should keep the original invariant instead. #Closed

{
// We construct such nodes for `with` expressions on anonymous types
continue;
}

var propertyReference = (IPropertyReferenceOperation)simpleAssignment.Target;
Assert.Empty(propertyReference.Arguments);
Assert.Equal(OperationKind.InstanceReference, propertyReference.Instance.Kind);
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,12 @@ public override void VisitAnonymousObjectCreation(IAnonymousObjectCreationOperat
foreach (var initializer in operation.Initializers)
{
var simpleAssignment = (ISimpleAssignmentOperation)initializer;
if (simpleAssignment.Target is InvalidOperation)
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if (simpleAssignment.Target is InvalidOperation)

Similar here, I think we should keep the original invariant instead. #Closed

{
// We construct such nodes for `with` expressions on anonymous types
continue;
}

var propertyReference = (IPropertyReferenceOperation)simpleAssignment.Target;
Assert.Empty(propertyReference.Arguments);
Assert.Equal(OperationKind.InstanceReference, propertyReference.Instance.Kind);
Expand Down
29 changes: 29 additions & 0 deletions src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,35 @@ public static class Test
End Using
End Function

<WpfTheory, CombinatorialData>
<Trait(Traits.Feature, Traits.Features.Completion)>
Public Async Function CompletionOnWithExpressionInitializer_AnonymousType(showCompletionInArgumentLists As Boolean) As Task
Using state = TestStateFactory.CreateCSharpTestState(
<Document>
class C
{
void M()
{
var a = new { Property = 1 };
_ = a $$
}
}
</Document>,
showCompletionInArgumentLists:=showCompletionInArgumentLists, languageVersion:=LanguageVersion.Preview)

state.SendTypeChars("w")
Await state.AssertSelectedCompletionItem(displayText:="with", isHardSelected:=False)
state.SendTab()
state.SendTypeChars(" { ")
Await state.AssertSelectedCompletionItem(displayText:="Property", isHardSelected:=False)
state.SendTypeChars("P")
Await state.AssertSelectedCompletionItem(displayText:="Property", isHardSelected:=True)
state.SendTypeChars(" = 2")
Await state.AssertNoCompletionSession()
Assert.Contains("with { Property = 2", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
End Using
End Function

<WorkItem(44921, "https://github.com/dotnet/roslyn/issues/44921")>
<WpfTheory, CombinatorialData>
<Trait(Traits.Feature, Traits.Features.Completion)>
Expand Down
1 change: 1 addition & 0 deletions ...eatures/Core/Portable/Completion/Providers/AbstractObjectInitializerCompletionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ protected override Task<CompletionDescription> GetDescriptionWorkerAsync(Documen
private static bool IsLegalFieldOrProperty(ISymbol symbol)
{
return symbol.IsWriteableFieldOrProperty()
|| symbol.ContainingType.IsAnonymousType
|| CanSupportObjectInitializer(symbol);
Comment on lines +87 to 88
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the new check should be moved to CanSupportObjectInitializer?

Copy link
Member

Choose a reason for hiding this comment

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

i'm fine with either approach.

}

Expand Down