Skip to content

Address review comments from #33178 documented in comment stream in #33175#45454

Merged
gafter merged 2 commits intodotnet:masterfrom
gafter:master-33175
Jul 6, 2020
Merged

Address review comments from #33178 documented in comment stream in #33175#45454
gafter merged 2 commits intodotnet:masterfrom
gafter:master-33175

Conversation

@gafter
Copy link
Member

@gafter gafter commented Jun 25, 2020

Related to #33178 and #33175

@gafter gafter added Area-Compilers 4 - In Review A fix for the issue is submitted for review. Feature - Pattern Matching Pattern Matching labels Jun 25, 2020
@gafter gafter added this to the 16.8 milestone Jun 25, 2020
@gafter gafter requested a review from a team as a code owner June 25, 2020 16:31
@gafter gafter self-assigned this Jun 25, 2020
@gafter
Copy link
Member Author

gafter commented Jun 26, 2020

@AlekseyTs One of your comments from a previous review is addressed here.
@cston A handful of your comments from a previous review are addressed here.
Would you please have a look?
#Resolved

@gafter gafter closed this Jun 29, 2020
@gafter gafter reopened this Jun 29, 2020
@gafter
Copy link
Member Author

gafter commented Jul 1, 2020

@AlekseyTs One of your comments from a previous review is addressed here. Would you please have a look? #Resolved


var compilation = CreateCompilation(source);
compilation.VerifyDiagnostics(
// (6,32): error CS0154: The property or indexer 'A' cannot be used in this context because it lacks the get accessor
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 2, 2020

Choose a reason for hiding this comment

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

The property or indexer 'A' cannot be used in this context because it lacks the get accessor [](start = 41, length = 92)

The error feels misleading, E is not a property and is not an indexer. #Closed

Copy link
Member Author

@gafter gafter Jul 2, 2020

Choose a reason for hiding this comment

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

Recorded as #45659. It is not relevant to any compiler changes in this PR.


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


var compilation = CreateCompilation(source);
compilation.VerifyDiagnostics(
// (6,32): error CS0154: The property or indexer 'B' cannot be used in this context because it lacks the get accessor
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 2, 2020

Choose a reason for hiding this comment

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

The property or indexer 'B' cannot be used in this context because it lacks the get accessor [](start = 41, length = 92)

Same comment. #Closed

// (6,32): error CS0572: 'C': cannot reference a type through an expression; try 'D.C' instead
// _ = /*<bind>*/o is D { C: var c }/*</bind>*/;
Diagnostic(ErrorCode.ERR_BadTypeReference, "C").WithArguments("C", "D.C").WithLocation(6, 32),
// (6,32): error CS0154: The property or indexer 'C' cannot be used in this context because it lacks the get accessor
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 2, 2020

Choose a reason for hiding this comment

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

The property or indexer 'C' cannot be used in this context because it lacks the get accessor [](start = 41, length = 92)

Same comment #Closed

Member:
IFieldReferenceOperation: System.Int32 D.X (Static) (OperationKind.FieldReference, Type: System.Int32, Constant: 3, IsInvalid) (Syntax: 'X')
Instance Receiver:
IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: D, Constant: 3, IsInvalid, IsImplicit) (Syntax: 'X')
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 2, 2020

Choose a reason for hiding this comment

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

IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: D, Constant: 3, IsInvalid, IsImplicit) (Syntax: 'X') [](start = 18, length = 150)

I think we are removing all synthesized receivers for static member references in the operation factory. It looks like this one should be dropped as well because it is not explicitly specified in syntax. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

See IFieldReference_StaticFieldInObjectInitializer_NoInstance unit-test, for example.


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

Copy link
Member Author

@gafter gafter Jul 2, 2020

Choose a reason for hiding this comment

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

Recorded as #45660. That is preexisting behavior. It is not relevant to any compiler changes in this PR.


In reply to: 449146730 [](ancestors = 449146730,449143457)

Member:
IFieldReferenceOperation: System.Int32 D.X (Static) (OperationKind.FieldReference, Type: System.Int32, IsInvalid) (Syntax: 'X')
Instance Receiver:
IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: D, IsInvalid, IsImplicit) (Syntax: 'X')
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 2, 2020

Choose a reason for hiding this comment

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

IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: D, IsInvalid, IsImplicit) (Syntax: 'X') [](start = 18, length = 137)

Should be dropped, I think. #Closed

Member:
IPropertyReferenceOperation: System.Int32 D.X { get; } (Static) (OperationKind.PropertyReference, Type: System.Int32, IsInvalid) (Syntax: 'X')
Instance Receiver:
IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: D, IsInvalid, IsImplicit) (Syntax: 'X')
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 2, 2020

Choose a reason for hiding this comment

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

IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: D, IsInvalid, IsImplicit) (Syntax: 'X') [](start = 18, length = 137)

Should be dropped, I think. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (iteration 2)

@gafter
Copy link
Member Author

gafter commented Jul 2, 2020

@AlekseyTs I have responded to your comments. Do you have any comments on the compiler changes implemented here or the tests covering them? Note that this PR is not intended to have any effect on the behavior of the compiler.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 2)

@gafter gafter merged commit 3138112 into dotnet:master Jul 6, 2020
@ghost ghost modified the milestones: 16.8, Next Jul 6, 2020
@JoeRobich JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 - In Review A fix for the issue is submitted for review. Area-Compilers Feature - Pattern Matching Pattern Matching

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants