Introduce local variable supporting target-type new syntax

[MartyIX]

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)?

Footnotes

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

[MartyIX]

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.

[CyrusNajmabadi]

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

[MartyIX]

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.

[CyrusNajmabadi]

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

1. if the user has 'prefer var when apparent/elsewhere' we should generate var numbers = new Numbers();
2. 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();
3. 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]

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]

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]

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]

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]

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.

[CyrusNajmabadi]

needs docs explaining what is going on.

[MartyIX]

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.

[MartyIX]

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

[CyrusNajmabadi]

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

[MartyIX]

@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.