[semi-auto-props]: Add more tests (GetMembers, speculative semantic model) #59270

Youssef1313 · 2022-02-04T11:09:52Z

Test plan: #57012

Youssef1313 · 2022-02-04T11:12:17Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

+            Assert.Empty(comp.GetTypeByMetadataName("C").GetFieldsToEmit());
+
+            var fieldKeywordTypeInfo = speculativeModel.GetTypeInfo(identifier);
+            Assert.Equal(SpecialType.System_Int32, fieldKeywordTypeInfo.Type.SpecialType);


Can you please verify if this is expected to work? Currently it doesn't work because (as far as I understand), before speculation, we already marked the backing field as computed.

Can you please verify if this is expected to work?

Define "to work".
We are in the investigation territory here. Reviewers should be able to see the current behavior. Meaning, the tests should pass, they should reflect the current behavior. Then we can examine if the behavior matches our expectations.

Youssef1313 · 2022-02-04T11:20:51Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

+            model.TryGetSpeculativeSemanticModel(token.SpanStart, identifier, out var speculativeModel);
+            var fieldKeywordTypeInfo = speculativeModel.GetTypeInfo(identifier);
+
+            Assert.Empty(comp.GetTypeByMetadataName("C").GetFieldsToEmit());


This assertion fails. What I was trying to do in this test is to not cause binding to the original compilation so that the sentinel value remains, then I only bind the speculated node. This caused the property to have a backing field, which GetFieldsToEmit will return. @AlekseyTs Any pointers as to what I need to do, for both this test and the previous one?

Any pointers as to what I need to do, for both this test and the previous one?

I agree that the behavior doesn't feel right. But before doing anything to fix it, let's add more tests to better understand the set of scenarios we should handle.

Note, the tests should pass, asserts should reflect the current behavior. Otherwise, reviewers cannot "see" the current behavior. If some behavior feels unexpected to you, add a comment.

Here are additional tests that I suggest:

Speculate with a field identifier within a an accessor that uses the field keyword.

Speculate with a field identifier within a an accessor that doesn't use the field keyword, but there is another accessor that uses the keyword.

Speculate in a duplicate accessor (in the second getter, for example). Both when the first accessor uses and doesn't use the field keyword.

Once we have these tests, I might suggest more scenarios to try.

@AlekseyTs For each of those scenarios, do you want two tests?

Bind original tree first

Bind speculated tree first

as I demonstrated in #59270 (comment)

If not, which scenario of the two is the interesting one?

For each of those scenarios, do you want two tests?

Yes, let's have two a pair of tests per scenario.

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

AlekseyTs · 2022-02-04T16:22:52Z

Done with review pass (commit 3)

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

AlekseyTs · 2022-02-09T00:40:19Z

Done with review pass (commit 4)

Youssef1313 · 2022-02-09T08:56:24Z

@AlekseyTs I added more tests. This is ready for another look.

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

AlekseyTs · 2022-02-14T22:41:20Z

Is the next step to address the cases where we don't like the behavior of speculative semantic model? Are all of them covered by PROTOTYPE comments or do you see cases that're not (or vice versa)?

I didn't have a chance to complete review of changes made post commit 5. We might want to add tests for more scenarios. Once we'll have the tests in place, we will have to decide what exact behavior do we want and adjust implementation accordingly.

In reply to: 1039641573

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CompilationContext.cs

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CompilationContext.cs

AlekseyTs · 2022-02-15T16:49:43Z

Done with review pass (commit 10)

AlekseyTs · 2022-02-18T14:42:42Z

Done with review pass (commit 11). I didn't see any response to #59270 (comment).

Youssef1313 · 2022-02-19T21:46:01Z

Done with review pass (commit 11). I didn't see any response to #59270 (comment).

Sorry for missing this comment. It's partially addressed with a question regarding expression evaluator tests.

AlekseyTs · 2022-02-23T14:23:28Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

+                //     public int P { get { int field = field; return field; } }
+                Diagnostic(ErrorCode.ERR_UseDefViolation, "field").WithArguments("field").WithLocation(4, 38)
+            );
+            Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding);


Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding);

Consider also checking GetFieldsToEmit before this assert. #Closed

AlekseyTs · 2022-02-23T14:53:09Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

+            {
+                Assert.Equal("System.Int32 field", fieldKeywordSymbolInfo.Symbol.GetSymbol().ToTestDisplayString());
+                Assert.Equal(SymbolKind.Local, fieldKeywordSymbolInfo.Symbol.Kind);
+                Assert.Equal(2, accessorBindingData.NumberOfPerformedAccessorBinding);


Assert.Equal(2, accessorBindingData.NumberOfPerformedAccessorBinding);

I think it would be good to determine what triggers the binding (by asserting NumberOfPerformedAccessorBinding in right places), the GetFieldsToEmit above or speculative binding. #Closed

AlekseyTs · 2022-02-23T14:58:07Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

+        [InlineData("get { return field; } get => 0;")]
+        [InlineData("get { return 0; } get { return field; }")]
+        [InlineData("get { return field; } get { return 0; }")]
+        [InlineData("get { return field; } get { int field = 0; return field; }")]


[InlineData("get { return field; } get { int field = 0; return field; }")]

Is testing different order not interesting? #Closed

AlekseyTs · 2022-02-23T15:09:09Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

+        [InlineData("get { return 0; } get { return 1; }", 27, 0, false)]
+        [InlineData("get { return 0; } get { return 1; }", 27, 1, false)]
+        [InlineData("get { double field = 0; return field; } get { return 1; }", 49, 0, true)]
+        [InlineData("get { double field = 0; return field; } get { return 1; }", 49, 1, false)]


false

We had an internal discussion about expected behavior for speculative binding. We settled on: "field in the speculative model does not bind to a backing field if the original location was not a semi-auto property." This is captured as a check box in #57012. It might be worth adding a PROTOTYPE comment here because the behavior doesn't match the expectation. #Closed

"field in the speculative model does not bind to a backing field if the original location was not a semi-auto property."

@AlekseyTs In case the speculated tree was bound first, we don't yet know if the original is semi-auto property or not. What should we do in that case?

It might be worth adding a PROTOTYPE comment here because the behavior doesn't match the expectation.

@AlekseyTs Taking a look at this specific test again, it seems that we already don't bind to a backing field (Assert.Null(fieldKeywordSymbolInfo.Symbol);). Was your comment intended for another test? Am I missing something?

Taking a look at this specific test again, it seems that we already don't bind to a backing field (Assert.Null(fieldKeywordSymbolInfo.Symbol);). Was your comment intended for another test? Am I missing something?

I probably misinterpreted the meaning of the speculatingProducesLocal parameter. Assumed that false means field rather than "not found".

In case the speculated tree was bound first, we don't yet know if the original is semi-auto property or not. What should we do in that case?

One way or the other we probably need to figure out if the original property is a semi-auto property. Just need to find a "right" way to do that. Always force binding the accessors in the process of speculation obviously is going to work, but we should consider if there are "better" alternatives.

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

AlekseyTs · 2022-02-23T15:24:31Z

Done with review pass (commit 13)

AlekseyTs

LGTM (commit 14)

AlekseyTs · 2022-02-24T19:05:24Z

@Youssef1313 Thanks for the contribution.

Youssef1313 · 2022-02-24T19:21:50Z

Thanks for reviewing @AlekseyTs! What do you think the next step should be? A new scenario? Fixing speculative semantic model tests? something else?

333fred · 2022-02-24T20:21:25Z

@Youssef1313 I'll take look over the next day or so and let you know.

Youssef1313 added 2 commits February 4, 2022 02:14

[semi-auto-props]: GetMembers tests

1c7a9b4

Add a semantic model test

123db98

Youssef1313 requested a review from a team as a code owner February 4, 2022 11:09

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 4, 2022

dotnet-issue-labeler bot added the Area-Compilers label Feb 4, 2022

Youssef1313 commented Feb 4, 2022

View reviewed changes

Add another test

f403873

Youssef1313 commented Feb 4, 2022

View reviewed changes

Youssef1313 changed the title ~~[semi-auto-props]: Add more tests~~ [semi-auto-props]: Add more tests (GetMembers, speculative semantic model) Feb 4, 2022

AlekseyTs reviewed Feb 4, 2022

View reviewed changes