[semi-auto-props]: Rename ContainsFieldKeyword, revise cases that should handle FieldKeywordBackingField #61310

Youssef1313 · 2022-05-13T19:19:53Z

Test plan: #57012

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

Youssef1313 · 2022-05-15T08:39:56Z

Youssef1313 · 2022-05-16T09:23:28Z

@AlekseyTs In 1d040e5, I introduced a simple generalized approach for reporting diagnostics that depend on accessor binding. Can you take a look and see if things are good? Thanks!

Youssef1313 · 2022-05-16T09:34:01Z

src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs

@@ -215,6 +215,7 @@ private FieldSymbol GetActualField(Symbol member, NamedTypeSymbol type)
                    return (!eventSymbol.HasAssociatedField || ShouldIgnoreStructField(eventSymbol, eventSymbol.Type)) ? null : eventSymbol.AssociatedField.AsMember(type);
                case SymbolKind.Property:
                    // PROTOTYPE(semi-auto-props): Review other event associated field callers and see if we have to do anything special for properties.
+                    // Everything is reviewed except FlowAnalysis.


I prefer this to be carefully reviewed to make sure I'm not missing anything, as we later will rely on the prototype update that only FlowAnalysis is left.

Youssef1313 · 2022-05-16T11:11:46Z

##[error]The job running on agent Azure Pipelines 70 ran longer than the maximum time of 90 minutes. For more information, see https://go.microsoft.com/fwlink/?linkid=2077134

Can someone restart the build? Thanks!

Youssef1313 · 2022-05-16T11:23:54Z

It seems there is a real failure:

  Starting Azure Pipelines Test Run Test_Windows_CoreClr_Debug_Windows.10.Amd64.Open
  Sending Job to Windows.10.Amd64.Open...
  Sent Helix Job; see work items at https://helix.dot.net/api/jobs/46faea75-bd01-4448-b3ce-0b67f5bd14d0/workitems?api-version=2019-06-17
  Waiting for completion of job 46faea75-bd01-4448-b3ce-0b67f5bd14d0 on Windows.10.Amd64.Open
Roslyn Error: test timeout exceeded, dumping remaining processes
Dumping dotnet 1164 to D:\a\1\s\artifacts\log\Debug\dotnet-0.dmp ... succeeded (231701857 bytes)

I'll try to investigate

Youssef1313 · 2022-05-16T12:37:37Z

This is the call stack I'm seeing from the dump in CI.

Youssef1313 · 2022-05-16T13:35:29Z

There are now different test failures. I'm addressing them separately in #61344

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs

AlekseyTs · 2022-05-16T15:39:32Z

        _ = BackingField?.GetAttributes();

Are we doing the same for the field keyword somewhere?

In reply to: 1127829708

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs:1263 in 1d040e5. [](commit_id = 1d040e5, deletion_comment = False)

AlekseyTs · 2022-05-16T16:51:32Z

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

+public unsafe struct S1
+{
+    public S1* s;
+    public object P { get => field; }


Consider adding a test where this is a pointer and the other field is object. #Closed

And flipping the types in the other struct too.

The new test is resulting in extra binding. I haven't yet investigated why this is the case.

It looks like the extra binding comes from the StartPropertyType phase. For field symbols, the constraints check was moved to the AfterAccessorBindingChecks call, but in property symbols constraint checks occur as part of binding the property type (SourcePropertySymbolBase.cs:1658 in this branch).

There is no longer extra binding here. It's fixed in last few commits, but not sure the approach is good.

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

AlekseyTs · 2022-05-16T17:11:35Z

Done with review pass (commit 4)

Youssef1313 · 2022-05-16T19:18:59Z

        _ = BackingField?.GetAttributes();
Are we doing the same for the field keyword somewhere?

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs:1263 in 1d040e5. [](commit_id = 1d040e5, deletion_comment = False)

Not yet. A test for this will be related to applying [field: ] on the property. A basic test would be adding [field: My] where MyAttribute doesn't have AttributeTargets.Field

Currently [field: ] isn't supported at all and has a prototype comment in AllowFieldAttributeTarget. Do you want me to handle it in this PR? (The PR is small enough for now, but I don't know if you will feel comfortable handling multiple things in a single PR. Let me know)

AlekseyTs · 2022-05-16T21:04:02Z

Do you want me to handle it in this PR?

No.

In reply to: 1128044671

333fred

Done review pass (commit 6)

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs

Youssef1313 · 2022-05-17T18:08:57Z

@333fred @AlekseyTs I don't quite understand why the used assemblies test fail, and what's the visible user issue is. Are you able to help?

I'm not sure if it's a case where we can safely use NoUsedAssembliesValidation. I attempted to fix the test in a920eab, but I don't think it's a reasonable change.

Youssef1313 · 2022-05-18T15:43:13Z

Ok it looks like the last two commits are breaking things badly. The change wasn't reasonable anyway 😄

@AlekseyTs @333fred Any suggestions?

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs

AlekseyTs · 2022-05-20T22:14:28Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs

-            // This check prevents redundant ManagedAddr diagnostics on the underlying pointer field of a fixed-size buffer
-            if (!IsFixedSizeBuffer)
+            var type = Type;
+            if (type.Kind != SymbolKind.PointerType && !IsFixedSizeBuffer)


if (type.Kind != SymbolKind.PointerType && !IsFixedSizeBuffer)

It is not obvious why do we still want to perform the check here under this condition? #Closed

Which check? IsFixedSizeBuffer? Given the existing code comment, it shouldn't be necessary here. But my thought was to keep it for two reasons:

Simply making sure we align with the existing behavior (but removing shouldn't affect anything given the comment)

The check is very light, it's just (Modifiers & DeclarationModifiers.Fixed) != 0, so we avoid the need to do more work with this small check.

Let me know if you prefer not checking it here.

It is not obvious why do we still want to perform the check here under this condition?

Which check?

Sorry for the confusion. I was referring to the Type.CheckAllConstraints that is duplicated between the two methods and performed in this method under the referenced condition.

@AlekseyTs At some point I had only AfterAccessorBindingChecks, but for some reason I can't understand, there was a test failure in used assemblies tests. I was asking about that in #61310 (comment).

AlekseyTs · 2022-05-20T22:18:21Z

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

+                //     public S1* P { get => field; }
+                Diagnostic(ErrorCode.ERR_ManagedAddr, "P").WithArguments("S1").WithLocation(5, 16));
+
+            Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding);


Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding);

There is no longer extra binding here. It's fixed in last few commits, but not sure the approach is good.

It looks like the extra binding is still here. #Closed

I think it's because I'm delaying CheckAllConstraints call for pointer types in fields only.

I'll need to do special case too many other symbols using one of these approaches:

current approach (meaning updating the code in symbols and also special case them in MethodCompiler)

updating code in symbols but get back to the virtual AfterAccessorBindingChecks that you requested to revert. The reason it was reverted is we cared only about limited types of symbols, which will no longer be the case now. (my personal preference is using this approach)

Do you like any of these approaches? Is there a completely different approach you'd like me to follow?

updating code in symbols but get back to the virtual AfterAccessorBindingChecks that you requested to revert. The reason it was reverted is we cared only about limited types of symbols, which will no longer be the case now.

I'm now using this approach. @AlekseyTs Let me know what do you think. Thanks!

AlekseyTs · 2022-05-20T22:19:59Z

Done with review pass (commit 13)

Youssef1313 · 2022-05-22T15:01:11Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

+                    var typeParameters = member switch
+                    {
+                        MethodSymbol m => m.TypeParameters,
+                        NamedTypeSymbol nt => nt.TypeParameters,
+                        _ => ImmutableArray<TypeParameterSymbol>.Empty,
+                    };
+
+                    foreach (var typeParameter in typeParameters)
+                    {
+                        (typeParameter as SourceTypeParameterSymbolBase)?.AfterAccessorBindingChecks();
                    }


I'm not much confident about this.

It looks like PartialMethodWithArgumentConstraint is now flaky. 😕
@AlekseyTs @333fred Any suggestion on how to correctly avoid extra binding for type parameters constraints checks, and why the current approach is flaky?

Youssef1313 · 2022-05-23T19:01:28Z

@333fred Reverted some changes per our conversation. I'll take another look once required members feature is merged.

src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs

333fred

LGTM (commit 16). We should have the required members branch merged to main later this week, and when you're done with exams we can look at using the new phase in that branch to address the extra bindings from type constraint checking.

AlekseyTs

LGTM (commit 16)

AlekseyTs · 2022-05-24T13:17:39Z

@Youssef1313 Thanks for the contribution!

Youssef1313 added 2 commits May 13, 2022 18:47

Rename ContainsFieldKeyword and minor refactoring

d8294b4

Partially address a PROTOTYPE comment

fb933fa

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

dotnet-issue-labeler bot added the Area-Compilers label May 13, 2022

Youssef1313 commented May 13, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs Outdated Show resolved Hide resolved

Add back unintentionally deleted PROTOTYPE

3cbceba

Eliminate extra binding for ERR_ManagedAddr

1d040e5

Youssef1313 marked this pull request as ready for review May 16, 2022 09:26

Youssef1313 requested a review from a team as a code owner May 16, 2022 09:26

Youssef1313 commented May 16, 2022

View reviewed changes

Youssef1313 closed this May 16, 2022

Youssef1313 reopened this May 16, 2022

AlekseyTs reviewed May 16, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs Show resolved Hide resolved

AlekseyTs reviewed May 16, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs Show resolved Hide resolved

AlekseyTs reviewed May 16, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed May 16, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits May 16, 2022 21:48

Add test

a2065df

Address feedback

68ac2ac

333fred self-assigned this May 16, 2022

333fred reviewed May 17, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs Show resolved Hide resolved

Add assert

0201e82

Youssef1313 added 3 commits May 17, 2022 20:10

Update PROTOTYPE comment

2e3178d

Attempt to fix used assemblies build

a920eab

Update SourceMemberFieldSymbol.cs

2d89cbd

333fred reviewed May 18, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs Outdated Show resolved Hide resolved

Youssef1313 added 3 commits May 18, 2022 21:36

Update SourceMemberFieldSymbol.cs

b97c067

Update SourceMemberFieldSymbol.cs

796c1ab

Update SourceMemberFieldSymbol.cs

584fc64

Youssef1313 requested review from 333fred and AlekseyTs May 19, 2022 10:58

AlekseyTs reviewed May 20, 2022

View reviewed changes

Handle ERR_ManagedAddr for all symbols

7216776

Youssef1313 requested a review from AlekseyTs May 22, 2022 14:00

Fix failure

b8ad16b

Youssef1313 commented May 22, 2022

View reviewed changes

This was referenced May 23, 2022

roslyn.visualstudio.newintegrationtests.csharp.csharperrorlistnetcore.errorlist #61367

Open

roslyn.visualstudio.newintegrationtests.csharp.csharperrorlistnetcore.errorsafterclosingfile is flaky #61368

Open

Revert

60cc818

333fred reviewed May 23, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs Show resolved Hide resolved

333fred approved these changes May 23, 2022

View reviewed changes

AlekseyTs approved these changes May 24, 2022

View reviewed changes

AlekseyTs merged commit b04c5c3 into dotnet:features/semi-auto-props May 24, 2022

Youssef1313 deleted the refactor branch May 24, 2022 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[semi-auto-props]: Rename ContainsFieldKeyword, revise cases that should handle FieldKeywordBackingField #61310

[semi-auto-props]: Rename ContainsFieldKeyword, revise cases that should handle FieldKeywordBackingField #61310

Youssef1313 commented May 13, 2022 •

edited

Loading

Youssef1313 commented May 15, 2022

Youssef1313 commented May 16, 2022 •

edited

Loading

Youssef1313 May 16, 2022

Youssef1313 commented May 16, 2022

Youssef1313 commented May 16, 2022

Youssef1313 commented May 16, 2022 •

edited

Loading

Youssef1313 commented May 16, 2022

AlekseyTs commented May 16, 2022 •

edited

Loading

AlekseyTs May 16, 2022 •

edited

Loading

AlekseyTs May 16, 2022

Youssef1313 May 16, 2022

333fred May 18, 2022

Youssef1313 May 19, 2022 •

edited

Loading

AlekseyTs commented May 16, 2022

Youssef1313 commented May 16, 2022

AlekseyTs commented May 16, 2022

333fred left a comment

Youssef1313 commented May 17, 2022 •

edited

Loading

Youssef1313 commented May 18, 2022

AlekseyTs May 20, 2022 •

edited

Loading

Youssef1313 May 21, 2022

AlekseyTs May 23, 2022

Youssef1313 May 23, 2022

AlekseyTs May 20, 2022 •

edited

Loading

Youssef1313 May 21, 2022 •

edited

Loading

Youssef1313 May 22, 2022

AlekseyTs commented May 20, 2022

Youssef1313 May 22, 2022

Youssef1313 May 22, 2022

Youssef1313 commented May 23, 2022

333fred left a comment

AlekseyTs left a comment

AlekseyTs commented May 24, 2022

[semi-auto-props]: Rename ContainsFieldKeyword, revise cases that should handle FieldKeywordBackingField #61310

[semi-auto-props]: Rename ContainsFieldKeyword, revise cases that should handle FieldKeywordBackingField #61310

Conversation

Youssef1313 commented May 13, 2022 • edited Loading

Youssef1313 commented May 15, 2022

Youssef1313 commented May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 commented May 16, 2022

Youssef1313 commented May 16, 2022

Youssef1313 commented May 16, 2022 • edited Loading

Youssef1313 commented May 16, 2022

AlekseyTs commented May 16, 2022 • edited Loading

AlekseyTs May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 May 19, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 16, 2022

Youssef1313 commented May 16, 2022

AlekseyTs commented May 16, 2022

333fred left a comment

Choose a reason for hiding this comment

Youssef1313 commented May 17, 2022 • edited Loading

Youssef1313 commented May 18, 2022

AlekseyTs May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 May 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented May 23, 2022

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 24, 2022

Youssef1313 commented May 13, 2022 •

edited

Loading

Youssef1313 commented May 16, 2022 •

edited

Loading

Youssef1313 commented May 16, 2022 •

edited

Loading

AlekseyTs commented May 16, 2022 •

edited

Loading

AlekseyTs May 16, 2022 •

edited

Loading

Youssef1313 May 19, 2022 •

edited

Loading

Youssef1313 commented May 17, 2022 •

edited

Loading

AlekseyTs May 20, 2022 •

edited

Loading

AlekseyTs May 20, 2022 •

edited

Loading

Youssef1313 May 21, 2022 •

edited

Loading