Prepare partial properties feature for merge #73773

RikkiGibson · 2024-05-29T19:07:56Z

#73090
Introduces several links to tracking issue #73772
VS test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/554460
Baseline insertion for comparison: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/554589

Related spec change: dotnet/csharplang#8169

…nto partial-properties-10

RikkiGibson · 2024-05-30T21:19:58Z

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

+            // The property is responsible for completion of the backing field
+            // NB: when the **field keyword feature** is implemented, it's possible that synthesized field symbols will also be merged or shared between partial property parts.
+            // If we do that then this check should possibly be moved, and asserts adjusted accordingly.
+            _ = BackingField?.GetAttributes();


@cston This comment and the below assert are likely relevant for the field keyword feature.

RikkiGibson · 2024-05-30T21:21:20Z

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

+            Debug.Assert(PartialDefinitionPart is null
+                // We might still get here when asking for the attributes on a backing field.
+                // This is an error scenario (requires using a property initializer and field-targeted attributes on partial property implementation part).
+                || this.BackingField is not null);


@cston I was anticipating that perhaps the synthesized BackingField symbol will be shared between partial property parts, or otherwise the two field symbols will reference each other similarly as the associated partial property symbols. But, this work won't happen until field keyword feature is implemented, and how specifically it will be done is TBD.

Currently, a different backing field symbol can be created for each partial property part, which is used only for error recovery (e.g. when an initializer is used on the property.)

RikkiGibson · 2024-05-30T21:22:35Z

src/Compilers/Test/Core/Assert/AssertEx.cs

-                        string s => $@"""{s}""",
-                        _ => obj.ToString()
-                    };
+                    itemInspector = new Func<T, string>(obj => (obj != null) ? obj.ToString() : "<null>");


Reverted to main version

RikkiGibson · 2024-05-30T21:24:05Z

IDE layer changes are comment-only @Cosifne. These comments are also enumerated in #73772.

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs

jcouv

Compiler changes LGTM Thanks (iteration 9)

RikkiGibson · 2024-05-31T15:05:36Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/CustomModifierCopyTests.cs

+        /// Test copying custom modifiers in/on parameters/return types.
+        /// </summary>
+        [Fact]
+        public void TestPartialMethodOverrideCombinations()


I didn't see any similar test to this already so figured we might as well have it.

RikkiGibson · 2024-05-31T15:06:15Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/CustomModifierCopyTests.cs

+partial class Derived : PropertyCustomModifierCombinations
+{
+    public override partial int[] Property11 { get; set; }
+    public override partial int[] Property11 { get => new int[0]; set { } }


Had to change the test away from using [] due to #73804.

jcouv

LGTM Thanks (iteration 10)

…nto partial-properties-10

src/Features/Core/Portable/Completion/Providers/AbstractPartialMethodCompletionProvider.cs

src/Workspaces/CSharp/Portable/Rename/CSharpRenameRewriterLanguageService.cs

cston · 2024-05-31T18:12:55Z

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs

+        [Fact]
+        public void NullableDifference_Analysis()
+        {
+            // Nullable annotations from the *definition* part parameters are used when analyzing implementations.


definition part parameters are used when analyzing implementations.

Is this statement correct? If we used the definition part parameters for analyzing the implementation, why are reporting warnings // 3 and // 4?

You're right, I'll adjust the comment and the spec PR. Don't know how I got mixed up on this.

cston · 2024-05-31T18:18:54Z

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs

+                // (10,27): error CS8050: Only auto-implemented properties can have initializers.
+                //     public partial string P3 { get => ""; set { } } = "d";
+                Diagnostic(ErrorCode.ERR_InitializerOnNonAutoProperty, "P3").WithLocation(10, 27));
+        }


}

Are we binding the initializers in these error cases? Consider adding a test such as:

partial string P4 { get; set; } = nameof(Unknown); partial string P4 { get => ""; set { } } ``` #Closed

cston · 2024-05-31T18:29:38Z

                public partial int this[int p2] { get => p2; set { } }

Consider using p2 in the set accessor as well: set { _ = p2; }. #Closed

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs:4589 in 63e1536. [](commit_id = 63e1536, deletion_comment = False)

cston · 2024-05-31T18:36:07Z

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs

+            {
+                var indexer = module.GlobalNamespace.GetMember<NamedTypeSymbol>("C").Indexers.Single();
+                Assert.Equal("p1", indexer.Parameters.Single().Name);
+            }
        }


}

Would it make sense to test which parameter names are in scope? For instance:

class A : System.Attribute { public A(object x, object y) { } } partial class C { public partial int this[int p1] { [param: A(nameof(p1), nameof(p2))] set; } public partial int this[int p2] { [param: A(nameof(p1), nameof(p2))] set { } } }

…lMethodCompletionProvider.cs Co-authored-by: Rikki Gibson <rigibson@microsoft.com>

…uageService.cs Co-authored-by: Rikki Gibson <rigibson@microsoft.com>

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs

…s.cs

RikkiGibson · 2024-05-31T19:31:09Z

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs

+                Diagnostic(ErrorCode.ERR_NameNotInContext, "p2").WithArguments("p2").WithLocation(9, 18),
+                // (14,29): error CS0103: The name 'p1' does not exist in the current context
+                //         [param: Attr(nameof(p1))] // 3
+                Diagnostic(ErrorCode.ERR_NameNotInContext, "p1").WithArguments("p1").WithLocation(14, 29),


I know why this is happening. The implementation part owns binding attributes on parameters.

https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/extended-nameof-scope.md doesn't appear to spec what should happen here.

I am hesitant to even spec the behavior here (which is already shipped for methods).

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 29, 2024

RikkiGibson added 2 commits May 29, 2024 14:13

Bookkeeping of prototype comments

6b5a681

pack enum values

c61014b

RikkiGibson changed the base branch from main to features/partial-properties May 30, 2024 01:17

Merge remote-tracking branch 'upstream/features/partial-properties' i…

f546237

…nto partial-properties-10

RikkiGibson force-pushed the partial-properties-10 branch from f242061 to f546237 Compare May 30, 2024 01:18

RikkiGibson changed the title ~~Partial properties 10~~ Prepare partial properties feature for merge May 30, 2024

RikkiGibson added 3 commits May 29, 2024 22:59

regenerate errorfacts

b1f178f

insertion verified

b1418ad

Address test plan review feedback

9872ff5

RikkiGibson marked this pull request as ready for review May 30, 2024 21:19

RikkiGibson requested review from a team as code owners May 30, 2024 21:19

RikkiGibson commented May 30, 2024

View reviewed changes

RikkiGibson requested review from jcouv and Cosifne May 30, 2024 21:23

jcouv reviewed May 30, 2024

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs Outdated Show resolved Hide resolved

fix indent

1578737

jcouv reviewed May 30, 2024

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs Show resolved Hide resolved

RikkiGibson added 2 commits May 30, 2024 15:16

Add more suggested tests

2f064c3

fix display string

0e84cb2

This comment was marked as resolved.

Sign in to view

jcouv self-assigned this May 31, 2024

jcouv approved these changes May 31, 2024

View reviewed changes

Fix tests in UsedAssemblies leg

93fb17a

RikkiGibson commented May 31, 2024

View reviewed changes

jcouv approved these changes May 31, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/features/partial-properties' i…

63e1536

…nto partial-properties-10

RikkiGibson requested a review from cston May 31, 2024 17:33

Cosifne reviewed May 31, 2024

View reviewed changes

src/Features/Core/Portable/Completion/Providers/AbstractPartialMethodCompletionProvider.cs Outdated Show resolved Hide resolved

Cosifne reviewed May 31, 2024

View reviewed changes

src/Workspaces/CSharp/Portable/Rename/CSharpRenameRewriterLanguageService.cs Outdated Show resolved Hide resolved

cston mentioned this pull request May 31, 2024

Test plan for "field keyword in properties" #57012

Open

Cosifne approved these changes May 31, 2024

View reviewed changes

cston reviewed May 31, 2024

View reviewed changes

Cosifne and others added 2 commits May 31, 2024 11:44

Update src/Features/Core/Portable/Completion/Providers/AbstractPartia…

74eeafc

…lMethodCompletionProvider.cs Co-authored-by: Rikki Gibson <rigibson@microsoft.com>

Update src/Workspaces/CSharp/Portable/Rename/CSharpRenameRewriterLang…

787e342

…uageService.cs Co-authored-by: Rikki Gibson <rigibson@microsoft.com>

RikkiGibson commented May 31, 2024

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs Outdated Show resolved Hide resolved

cston approved these changes May 31, 2024

View reviewed changes

Update src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTest…

a10c81f

…s.cs

cston approved these changes May 31, 2024

View reviewed changes

RikkiGibson added 2 commits May 31, 2024 12:21

add suggested tests

58385ce

method parameter attribute

c743ca9

RikkiGibson commented May 31, 2024

View reviewed changes

RikkiGibson enabled auto-merge (squash) May 31, 2024 19:32

RikkiGibson merged commit b61f0e4 into dotnet:features/partial-properties May 31, 2024
25 of 28 checks passed

RikkiGibson deleted the partial-properties-10 branch May 31, 2024 20:48

build-analysis bot mentioned this pull request May 31, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

This was referenced Jun 4, 2024

[Automated] PRs inserted in VS build main-35004.16 #73835

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35004.137 #73853

Closed

[Automated] PRs inserted in VS build feature.debugger.inlineValues-35005.76 #73866

Closed

dotnet-bot mentioned this pull request Jun 27, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-35026.97 #74176

Closed

dibarbet mentioned this pull request Jul 25, 2024

Find-All-References results incomplete and unstable dotnet/vscode-csharp#7391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare partial properties feature for merge #73773

Prepare partial properties feature for merge #73773

RikkiGibson commented May 29, 2024 •

edited

Loading

RikkiGibson May 30, 2024

RikkiGibson May 30, 2024 •

edited

Loading

RikkiGibson May 30, 2024 •

edited

Loading

RikkiGibson commented May 30, 2024

This comment was marked as resolved.

jcouv left a comment

RikkiGibson May 31, 2024

RikkiGibson May 31, 2024

jcouv left a comment

cston May 31, 2024

RikkiGibson May 31, 2024

cston May 31, 2024 •

edited

Loading

cston commented May 31, 2024 •

edited

Loading

cston May 31, 2024

RikkiGibson May 31, 2024

Prepare partial properties feature for merge #73773

Prepare partial properties feature for merge #73773

Conversation

RikkiGibson commented May 29, 2024 • edited Loading

RikkiGibson May 30, 2024

Choose a reason for hiding this comment

RikkiGibson May 30, 2024 • edited Loading

Choose a reason for hiding this comment

RikkiGibson May 30, 2024 • edited Loading

Choose a reason for hiding this comment

RikkiGibson commented May 30, 2024

This comment was marked as resolved.

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson May 31, 2024

Choose a reason for hiding this comment

RikkiGibson May 31, 2024

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

cston May 31, 2024

Choose a reason for hiding this comment

RikkiGibson May 31, 2024

Choose a reason for hiding this comment

cston May 31, 2024 • edited Loading

Choose a reason for hiding this comment

cston commented May 31, 2024 • edited Loading

cston May 31, 2024

Choose a reason for hiding this comment

RikkiGibson May 31, 2024

Choose a reason for hiding this comment

RikkiGibson commented May 29, 2024 •

edited

Loading

RikkiGibson May 30, 2024 •

edited

Loading

RikkiGibson May 30, 2024 •

edited

Loading

cston May 31, 2024 •

edited

Loading

cston commented May 31, 2024 •

edited

Loading