Introduce local variable supporting target-type new syntax #76342

MartyIX · 2024-12-09T21:11:06Z

Our project uses target-type new syntax. Whenever, I have a line like this one:

int sum = Sum([|new Numbers()|]);

and I want to extract a new variable from the marked expression, I get

Numbers numbers = new Numbers();
int sum = Sum(numbers);

but I would like to get

Numbers numbers = new(); // i.e. `new()` here and not `new Numbers()`.
int sum = Sum(numbers);

That's why I created this PR. Are you interested in such feature?¹ Could you give me some pointers how to finish this PR (as it does not work at the moment)?

I sure am as I hit this daily. :) ↩

MartyIX · 2024-12-09T21:11:38Z

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs

@@ -54,7 +55,7 @@ protected override Document IntroduceLocal(
                [VariableDeclarator(
                    newLocalNameToken.WithAdditionalAnnotations(RenameAnnotation.Create()),
                    argumentList: null,
-                    EqualsValueClause(expression.WithoutTrivia()))]));
+                    EqualsValueClause(expression.WithAdditionalAnnotations(Simplifier.Annotation)).WithoutTrivia())]));


Is there any chance that this might work? It does not but I'm not exactly sure why it doesn't. I thought it might.

i would not use the simplifier annotation. I would just generate the right expected results based ont he user's options specified.

I took the time to debug TestIntroduceLocalInExpressionBodiedIndexer. It is my understanding that var in that test (instead of explicit type name) is added in a postprocessing step. However, that postprocessing is not able to convert it to "target-type form".

So then your suggestion is a reasonably easy solution here.

src/Features/CSharpTest/IntroduceVariable/IntroduceVariableTests.cs

CyrusNajmabadi · 2024-12-09T21:27:00Z

Happy to help with this. And we would accept a PR for this as logn as the the following holds:

if the user has 'prefer var when apparent/elsewhere' we should generate var numbers = new Numbers();
if the user does not have those options on, but does have csharp_style_implicit_object_creation_when_type_is_apparent, then we should do Numbers numbers = new();
otherwise, we should generate Numbers numbers = new Numbers();

If you can confirm this is the logic the PR maintains,a nd you have tests for this, i can review asap. I can also review any questions blocking you if you call them out. Thanks!

MartyIX · 2024-12-09T21:33:48Z

Yes, that's I would like to achieve.

I added a few comments to specific lines of code where some help would be great

CyrusNajmabadi · 2024-12-22T20:37:03Z

FYI, it's vacation time. So things may be slow to respond. Feel free to ping me on jan2 if you haven't heard from anyone else!

MartyIX · 2024-12-22T20:40:18Z

FYI, it's vacation time. So things may be slow to respond. Feel free to ping me on jan2 if you haven't heard from anyone else!

Thank you for the heads up. It would be great if at least tests could be run for the PR, I would get some feedback. If not, no problem, I can wait.

Merry Christmas!

CyrusNajmabadi · 2024-12-22T22:01:33Z

It would be great if at least tests could be run for the PR

Tears will automatically run for you pr as long as it builds successfully :-)

MartyIX · 2024-12-23T08:07:02Z

Hm, I'm not too sure what that CI error

Caught exception querying ADO for test runs: System.InvalidOperationException: Sequence contains more than one matching element
   at System.Linq.ThrowHelper.ThrowMoreThanOneMatchException()
   at System.Linq.Enumerable.TryGetSingle[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
   at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
   at RunTests.TestHistoryManager.GetRunForStageAsync(Build build, String phaseName, TestResultsHttpClient testClient, CancellationToken cancellationToken) in /_/src/Tools/Source/RunTests/TestHistoryManager.cs:line 177
Unable to get a run with name Test_Windows_CoreClr_Debug from build https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_apis/build/Builds/900968.

is about.

src/Features/CSharpTest/IntroduceVariable/IntroduceVariableTests.cs

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs

CyrusNajmabadi · 2024-12-23T13:59:04Z

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs

+        var updatedExpression = expression.WithoutTrivia();
+
+        if (options.SimplifierOptions is CSharpSimplifierOptions csOptions && csOptions.ImplicitObjectCreationWhenTypeIsApparent.Value
+            && csOptions.GetUseVarPreference() == UseVarPreference.None)


needs docs explaining what is going on.

I documented my approach. It is a really conservative one (either target-type new syntax is used exclusively, or codebase opts into using "var" in any case). My reasoning is as follows:

It's a very conservative change, so it should not introduce some unwanted behavior.

It's hard for me to tell what codebases in the wild actually use and want (i.e., how complex the implementation should be), so I basically wanted to cover my use-case because I understand it well (and hopefully, it's what other customers will like).

With my PR, it should be easy to add missing cases, if people actually want it.

That being said, I'm certainly open to modify it.

I think it is what #76342 (comment) mentions.

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs

CyrusNajmabadi · 2024-12-23T14:00:34Z

@dotnet/roslyn-infrastructure can someone help this community member with their PR? something has gone wonky with the test machines. Thanks!

MartyIX · 2024-12-23T20:42:41Z

@dotnet/roslyn-infrastructure can someone help this community member with their PR? something has gone wonky with the test machines. Thanks!

Perhaps it was my fault. I fixed a few failing tests.

MartyIX · 2025-01-02T16:25:05Z

FYI, it's vacation time. So things may be slow to respond. Feel free to ping me on jan2 if you haven't heard from anyone else!

@CyrusNajmabadi Pinging :)

CyrusNajmabadi · 2025-01-02T16:46:31Z

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs

@@ -47,14 +50,25 @@ protected override Document IntroduceLocal(
            ? TokenList(ConstKeyword)
            : default;

+        var updatedExpression = expression.WithoutTrivia();
+        var csOptions = (CSharpSimplifierOptions)options.SimplifierOptions;


minor, but we do not tend to use things like cs in our code for abbreviations. We'd either use csharpOptions here, or just simplifierOptions (no need for the 'csharp' name to distringuish it.

CyrusNajmabadi · 2025-01-02T16:46:40Z

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs

@@ -47,14 +50,25 @@ protected override Document IntroduceLocal(
            ? TokenList(ConstKeyword)
            : default;

+        var updatedExpression = expression.WithoutTrivia();
+        var csOptions = (CSharpSimplifierOptions)options.SimplifierOptions;


Suggested change

var csOptions = (CSharpSimplifierOptions)options.SimplifierOptions;

var simplifierOptions = (CSharpSimplifierOptions)options.SimplifierOptions;

CyrusNajmabadi · 2025-01-02T16:47:59Z

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs

+        if (csOptions.ImplicitObjectCreationWhenTypeIsApparent.Value && csOptions.GetUseVarPreference() == UseVarPreference.None
+            && expression is ObjectCreationExpressionSyntax objectCreationExpression)
+        {
+            updatedExpression = ImplicitObjectCreationExpression(objectCreationExpression.ArgumentList, objectCreationExpression.Initializer);


you removed the trivia from expression when making updatedExpression at the top of the method. but not here. consider either doign the same here, or doign updatedExpression is ObjectCreationExpressionSyntax ... to ensure that original op wasn't lost.

CyrusNajmabadi · 2025-01-02T16:48:50Z

src/Features/CSharpTest/IntroduceVariable/IntroduceVariableTests.cs

@@ -935,7 +935,7 @@ class Program
            {
                static void Main()
                {
-                    G<int>.@class {|Rename:@class|} = new G<int>.@class();
+                    G<int>.@class {|Rename:@class|} = new();


can you add sibling tests that have the same input, but which have the ImplicitObjectCreationWhenTypeIsApparent set to false, to show we get the old output in that case?

CyrusNajmabadi

everything about the code change looks good. there are some small tweaks i'd like you to make, and some test additions. Preemptive approving assuming that happens. when that change goes in and everything is green, ping me and i can merge. thanks!

CyrusNajmabadi · 2025-01-02T16:50:13Z

Note: if you're low on time, and you'd prefer it, i'm happy to make the above change requests for you. Up to you.

MartyIX · 2025-01-02T17:19:58Z

Note: if you're low on time, and you'd prefer it, i'm happy to make the above change requests for you. Up to you.

If you have some time to do it, it would be great. First days after holidays are busy for me, so I cannot do the work immediately.

…TargetTypeNew-for-IntroduceLocalVariable

CyrusNajmabadi · 2025-01-02T18:43:34Z

Thanks for the contribution :)

CyrusNajmabadi · 2025-01-02T19:53:19Z

Merged. Thanks very much!

MartyIX · 2025-01-02T20:05:03Z

Merged. Thanks very much!

Thank you for finishing the work! I'm very excited to try this feature in some new Visual Studio version! :)

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 9, 2024

dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels Dec 9, 2024

MartyIX commented Dec 9, 2024

View reviewed changes

src/Features/CSharpTest/IntroduceVariable/IntroduceVariableTests.cs Show resolved Hide resolved

CyrusNajmabadi self-assigned this Dec 9, 2024

MartyIX force-pushed the feature/2024-12-09-TargetTypeNew-for-IntroduceLocalVariable branch from b01eca3 to 12a96b8 Compare December 22, 2024 20:13

MartyIX changed the title ~~[wip] Introduce local variable supporting target-type new syntax~~ Introduce local variable supporting target-type new syntax Dec 22, 2024

MartyIX marked this pull request as ready for review December 22, 2024 20:16

MartyIX requested a review from a team as a code owner December 22, 2024 20:16

First attempt

42634bd

MartyIX force-pushed the feature/2024-12-09-TargetTypeNew-for-IntroduceLocalVariable branch from 12a96b8 to 42634bd Compare December 22, 2024 20:38

CyrusNajmabadi reviewed Dec 23, 2024

View reviewed changes

src/Features/CSharpTest/IntroduceVariable/IntroduceVariableTests.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 23, 2024

View reviewed changes

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 23, 2024

View reviewed changes

src/Features/CSharp/Portable/IntroduceVariable/CSharpIntroduceVariableService_IntroduceLocal.cs Outdated Show resolved Hide resolved

Address review comments + fix failing tests

934aff0

MartyIX requested a review from CyrusNajmabadi December 25, 2024 19:25

CyrusNajmabadi reviewed Jan 2, 2025

View reviewed changes

CyrusNajmabadi approved these changes Jan 2, 2025

View reviewed changes

CyrusNajmabadi added 3 commits January 2, 2025 09:23

Merge remote-tracking branch 'upstream/main' into feature/2024-12-09-…

7f2db9d

…TargetTypeNew-for-IntroduceLocalVariable

PR feedback

99d5122

Add test demonstreating old behavior

364e938

CyrusNajmabadi approved these changes Jan 2, 2025

View reviewed changes

CyrusNajmabadi enabled auto-merge January 2, 2025 17:33

fix test

523eab5

CyrusNajmabadi merged commit a7ff9bf into dotnet:main Jan 2, 2025
25 checks passed

dotnet-policy-service bot added this to the Next milestone Jan 2, 2025

MartyIX deleted the feature/2024-12-09-TargetTypeNew-for-IntroduceLocalVariable branch January 2, 2025 20:05

dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025

This was referenced Jan 8, 2025

[Automated] PRs inserted in VS build feature.debugger.main-35707.166 #76680

Closed

[Automated] PRs inserted in VS build main-35707.127 #76681

Closed

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35708.113 #76688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce local variable supporting target-type new syntax #76342

Introduce local variable supporting target-type new syntax #76342

MartyIX commented Dec 9, 2024

MartyIX Dec 9, 2024

CyrusNajmabadi Dec 16, 2024

MartyIX Dec 16, 2024

CyrusNajmabadi commented Dec 9, 2024

MartyIX commented Dec 9, 2024

CyrusNajmabadi commented Dec 22, 2024

MartyIX commented Dec 22, 2024 •

edited

Loading

CyrusNajmabadi commented Dec 22, 2024

MartyIX commented Dec 23, 2024

CyrusNajmabadi Dec 23, 2024

MartyIX Dec 23, 2024

MartyIX Dec 23, 2024

CyrusNajmabadi commented Dec 23, 2024

MartyIX commented Dec 23, 2024

MartyIX commented Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi left a comment

CyrusNajmabadi commented Jan 2, 2025

MartyIX commented Jan 2, 2025

CyrusNajmabadi commented Jan 2, 2025

CyrusNajmabadi commented Jan 2, 2025

MartyIX commented Jan 2, 2025

	var csOptions = (CSharpSimplifierOptions)options.SimplifierOptions;
	var simplifierOptions = (CSharpSimplifierOptions)options.SimplifierOptions;

Introduce local variable supporting target-type new syntax #76342

Introduce local variable supporting target-type new syntax #76342

Conversation

MartyIX commented Dec 9, 2024

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 9, 2024

MartyIX commented Dec 9, 2024

CyrusNajmabadi commented Dec 22, 2024

MartyIX commented Dec 22, 2024 • edited Loading

CyrusNajmabadi commented Dec 22, 2024

MartyIX commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 23, 2024

MartyIX commented Dec 23, 2024

MartyIX commented Jan 2, 2025

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

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 2, 2025

MartyIX commented Jan 2, 2025

CyrusNajmabadi commented Jan 2, 2025

CyrusNajmabadi commented Jan 2, 2025

MartyIX commented Jan 2, 2025

MartyIX commented Dec 22, 2024 •

edited

Loading