Switch to primary constructors in IDE layer #68299

CyrusNajmabadi · 2023-05-22T21:56:25Z

No description provided.

CyrusNajmabadi · 2023-05-22T21:59:36Z

tmat · 2023-05-24T00:56:43Z

src/Workspaces/Core/Portable/Classification/ClassifiedSpan.cs

@@ -8,22 +8,16 @@

 namespace Microsoft.CodeAnalysis.Classification
 {
-    public readonly struct ClassifiedSpan : IEquatable<ClassifiedSpan>
+    public readonly struct ClassifiedSpan(TextSpan textSpan, string classificationType) : IEquatable<ClassifiedSpan>


ClassifiedSpan

Wouldn't a record be better?

Let me see about updating the feature to support that afterwards!

tmat · 2023-05-24T01:05:55Z

src/Workspaces/Core/Portable/Differencing/LongestCommonSubsequence.cs

-                _start = start;
-                _length = length;
-            }
+            private readonly int[] _buffer = buffer;


buffer

I don't think I'd keep the fields and also use primary ctor at the same time. Either, or. As it is now, both buffer and _buffer are in scope in all methods. If someone accidentally uses buffer instead of _buffer there is no error but the struct becomes larger.

Compiler will warn in that case. Assigning to field, but also using the parameter will warn about double storage :-)

I don't see the warning in Version 17.7.0 Preview 2.0 [33722.583.main]

Is it new?

Tracked here: https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-05-15.md#primary-constructors

LDM just approved it. Will be coming asap.

tmat

🕐

tmat · 2023-05-24T01:12:14Z

src/Workspaces/Core/Portable/Remote/RemoteServiceCallbackId.cs

@@ -8,13 +8,10 @@
 namespace Microsoft.CodeAnalysis.Remote
 {
    [DataContract]
-    internal readonly struct RemoteServiceCallbackId : IEquatable<RemoteServiceCallbackId>
+    internal readonly struct RemoteServiceCallbackId(int id) : IEquatable<RemoteServiceCallbackId>


RemoteServiceCallbackId

record?

tmat · 2023-05-24T01:14:01Z

src/Workspaces/Core/Portable/Remote/RemoteArguments.cs

-            AdditionalProperties = additionalProperties;
-            CandidateReason = candidateReason;
-        }
+        public readonly CandidateReason CandidateReason = candidateReason;


Can we remove public field declarations and instead apply DataMember attributes to the generated fields?

No. There is currently no way to get to the "Generated Fields" for primary constructors currently (we don't even guarantee they are fields on the type). There are future lang changes about being able to have a primary constructor, but be explicit if you want fields/props/etc. but they are not in 12.

You can't say [field: DataMember(...)] on the parameter?

Not currently. but LDM def aware of the desire here. I'd expect something post C#12 on this. It's too useful/valuable IMO to not have a solution for.

sharwell · 2023-05-24T16:29:08Z

src/Workspaces/Core/Portable/CodeActions/Operations/OpenDocumentOperation.cs

-            _documentId = documentId ?? throw new ArgumentNullException(nameof(documentId));
-            _activate = activateIfAlreadyOpen;
-        }
+        private readonly DocumentId _documentId = documentId ?? throw new ArgumentNullException(nameof(documentId));


❔ How does it know when to create a backing field for the parameter, and when to not create it?

➡️ Answered as part of https://github.com/dotnet/roslyn/pull/68299/files#r1203256887

So we never "create a backing field". The question is if we have to continue using the existing backing field, or if we can remove it. And the answer to that is "if the field is read/written in any way that would have differnet semantics from the parameter". A good example of that is simply:

public bool Equals(Whatever w) => _documentId == w._documentId;

Here, we cannot switch to a parameter, because then you cannot access the field anymore off of w.

sharwell · 2023-05-24T16:32:42Z

...Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Workspace/Mef/MefWorkspaceServices.cs

            public (string type, string layer)[] Metadata
-                => _services.Select(s => (s.Metadata.ServiceType, s.Metadata.Layer)).ToArray();
+                => services.Select(s => (s.Metadata.ServiceType, s.Metadata.Layer)).ToArray();


📝 I really don't like that the fields are breaking naming conventions. It's going to make IntelliSense super annoying it types because you won't be able to type _ to filter to fields.

They're not fields (seriously. the language does not think of them as fields, or treat them in any way as fields). They are captured parameters :)

They are being used as a replacement for a field of the same purpose, so they are functionally equivalent. I predict this is going to be problematic for feature adoption due to the amount of code that needs to change as part of this (all references to a field with a _ prefix need to change in source control when they start referring to the captured parameter instead). Personally, I would rather not have primary constructors than to have the captures appear with this name.

CyrusNajmabadi · 2023-06-07T18:45:39Z

Blocked because of dotnet/roslyn-analyzers#6573. @sharwell @mavasani can you guys look into this.

CyrusNajmabadi · 2023-06-08T21:14:40Z

@mavasani It looks like IDE0060 needs to be updated to understand primary constructors. Usages of PCs now results in tons of warnings like:

C:\github\roslyn\src\Workspaces\SharedUtilitiesAndExtensions\Compiler\Core\Options\OptionDefinition.cs(102,41): warning IDE0060: Remove unused parameter 'serializer' (https://docs.microsoft.com/dot
net/fundamentals/code-analysis/style-rules/ide0060) [C:\github\roslyn\src\CodeStyle\Core\Analyzers\Microsoft.CodeAnalysis.CodeStyle.csproj]

CyrusNajmabadi · 2023-06-14T17:25:09Z

@mavasani @akhera99 i'm still getting:

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeFixes/CustomCodeActions.cs#L39
src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeFixes/CustomCodeActions.cs(39,32): error CS9113: (NETCORE_ENGINEERING_TELEMETRY=Build) Parameter 'priority' is unread.

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 22, 2023

tmat reviewed May 24, 2023

View reviewed changes

tmat requested changes May 24, 2023

View reviewed changes

tmat reviewed May 24, 2023

View reviewed changes

tmat approved these changes May 24, 2023

View reviewed changes

sharwell reviewed May 24, 2023

View reviewed changes

CyrusNajmabadi force-pushed the primaryConstructors branch from b19d2a6 to e492694 Compare May 24, 2023 16:59

CyrusNajmabadi marked this pull request as ready for review May 24, 2023 17:04

CyrusNajmabadi requested review from a team as code owners May 24, 2023 17:04

CyrusNajmabadi enabled auto-merge May 24, 2023 17:05

Cosifne requested a review from a team as a code owner May 30, 2023 17:41

CyrusNajmabadi force-pushed the primaryConstructors branch from e522e46 to b91e3c1 Compare May 31, 2023 06:56

CyrusNajmabadi force-pushed the primaryConstructors branch from 9af8b0d to bf45d87 Compare June 8, 2023 19:12

CyrusNajmabadi mentioned this pull request Jun 8, 2023

IDE0060 (remove unused parameter) reported for primary constructor parameters. #68501

Closed

Use primary constructors

789f05b

CyrusNajmabadi force-pushed the primaryConstructors branch from 0f68404 to 789f05b Compare June 14, 2023 16:20

Update compiler

358bdb1

Fix

4b9834b

CyrusNajmabadi mentioned this pull request Jun 14, 2023

Using primary constructors reports erroneous NRT warnings in some cases. #68616

Closed

Revert

68dadc2

CyrusNajmabadi merged commit c6111fc into dotnet:main Jun 14, 2023

ghost added this to the Next milestone Jun 14, 2023

CyrusNajmabadi deleted the primaryConstructors branch June 14, 2023 20:27

allisonchou mentioned this pull request Jun 16, 2023

[Automated] PRs inserted in VS build main-33816.152 #68663

Closed

RikkiGibson modified the milestones: Next, 17.7 P3 Jun 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to primary constructors in IDE layer #68299

Switch to primary constructors in IDE layer #68299

CyrusNajmabadi commented May 22, 2023

CyrusNajmabadi commented May 22, 2023

tmat May 24, 2023

CyrusNajmabadi May 24, 2023

tmat May 24, 2023

CyrusNajmabadi May 24, 2023

tmat May 24, 2023

CyrusNajmabadi May 24, 2023

tmat left a comment

tmat May 24, 2023

tmat May 24, 2023

CyrusNajmabadi May 24, 2023

tmat May 24, 2023

CyrusNajmabadi May 24, 2023

sharwell May 24, 2023

sharwell May 24, 2023

CyrusNajmabadi May 24, 2023

sharwell May 24, 2023 •

edited

Loading

CyrusNajmabadi May 24, 2023

sharwell May 26, 2023

CyrusNajmabadi commented Jun 7, 2023

CyrusNajmabadi commented Jun 8, 2023

CyrusNajmabadi commented Jun 14, 2023

Switch to primary constructors in IDE layer #68299

Switch to primary constructors in IDE layer #68299

Conversation

CyrusNajmabadi commented May 22, 2023

CyrusNajmabadi commented May 22, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat left a comment

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

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

Choose a reason for hiding this comment

sharwell May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jun 7, 2023

CyrusNajmabadi commented Jun 8, 2023

CyrusNajmabadi commented Jun 14, 2023

sharwell May 24, 2023 •

edited

Loading