Fix IDE behavior and tests for some Primary Constructors scenarios #66896

AlekseyTs · 2023-02-15T14:33:46Z

No description provided.

AlekseyTs · 2023-02-15T17:17:10Z

@akhera99, @CyrusNajmabadi, @dotnet/roslyn-ide Please review.

CyrusNajmabadi · 2023-02-15T17:21:05Z

src/EditorFeatures/Test2/FindReferences/FindReferencesTests.ParameterSymbol.vb

 class Goo(int x, int {|Definition:$$y|})
 {
    public int y { get; } = [|y|];
-    public int z { get; } => [|y|];


curious about the removals here? was that intentional/needed? #Resolved

curious about the removals here? was that intentional/needed?

I added dedicated test for this scenario. The property y declared above shadows the parameter.

ah, gotcha. IMO, it would be better to leave the line in, but take off the [| |] around it (to make it clear that that 'y' is not considered a ref. But up to you.

Put the lines back as suggested

CyrusNajmabadi · 2023-02-15T17:22:51Z

src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs

+            {
+                var updatedParameters = UpdateDeclaration(@struct.ParameterList.Parameters, signaturePermutation, CreateNewParameterSyntax);
+                return @struct.WithParameterList(@struct.ParameterList.WithParameters(updatedParameters).WithAdditionalAnnotations(changeSignatureFormattingAnnotation));
+            }


could the above three 'if' statements be merged into one?

Syntax question. is 'ParameterList' added to TypeDeclSyntax, or only these three subclasses. If the latter, could we consider actually moving up to TypeDeclSyntax and having it always be null for interfaces. Or have interfaces allow parsing them out (for error recovery) and then report errors that it's not allowed there. feels like it would help prevent a bunch of duplication in consumption code. #Resolved

could the above three 'if' statements be merged into one?

At the moment we do not have WithParameterList API on a base class and I prefer to not add it because it wouldn't be appropriate for all types derived from that base.

At the moment we do not have WithParameterList API on a base class and I prefer to not add it because it wouldn't be appropriate for all types derived from that base.

I'm totally fine with that for the current state of affairs. Can this be noted though for when we bring this to API review? I'd like to make an alternative case. But that's def not blocking this work now. Thanks! :)

CyrusNajmabadi · 2023-02-15T17:24:59Z

...Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs


                primaryConstructor = typeSymbol.InstanceConstructors.FirstOrDefault(
-                    c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax);
+                    c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax or ClassDeclarationSyntax or StructDeclarationSyntax);


Suggested change

c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax or ClassDeclarationSyntax or StructDeclarationSyntax);

c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is TypeDeclarationSyntax);

Suggested change

c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax or ClassDeclarationSyntax or StructDeclarationSyntax);

c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is not ConstructorDeclarationSyntax);

Though, actually, this logic slightly worries me. Won't this return 'true' for the implicitly created constructor for a struct that does not have an actual primary constructor? Maybe we should say that the syntax has to both be the DeclSyntax And the decl syntax has a primary constructor? #Resolved

I prefer to follow the pattern established for records. TypeDeclarationSyntax would include interfaces and this is not my intention.

CyrusNajmabadi

LGTM. Some questions/suggestions, and a slight concern. But nothing blocking at all.

src/EditorFeatures/Test2/FindReferences/FindReferencesTests.ParameterSymbol.vb

Fix IDE behavior and tests for some Primary Constructors scenarios

9ea6889

AlekseyTs added Area-IDE Area-Compilers Feature - Primary Constructors labels Feb 15, 2023

AlekseyTs requested review from CyrusNajmabadi and akhera99 February 15, 2023 14:33

AlekseyTs requested a review from a team as a code owner February 15, 2023 14:33

CyrusNajmabadi reviewed Feb 15, 2023

View reviewed changes

CyrusNajmabadi approved these changes Feb 15, 2023

View reviewed changes