-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce local variable supporting target-type new syntax #76342
Changes from 3 commits
42634bd
934aff0
7f2db9d
99d5122
364e938
523eab5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,10 +9,12 @@ | |||||
using System.Diagnostics; | ||||||
using System.Linq; | ||||||
using System.Threading; | ||||||
using System.Threading.Tasks; | ||||||
using Microsoft.CodeAnalysis; | ||||||
using Microsoft.CodeAnalysis.CodeActions; | ||||||
using Microsoft.CodeAnalysis.CodeCleanup; | ||||||
using Microsoft.CodeAnalysis.CSharp.CodeStyle.TypeStyle; | ||||||
using Microsoft.CodeAnalysis.CSharp.Extensions; | ||||||
using Microsoft.CodeAnalysis.CSharp.Simplification; | ||||||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||||||
using Microsoft.CodeAnalysis.Editing; | ||||||
using Microsoft.CodeAnalysis.Formatting; | ||||||
|
@@ -28,6 +30,7 @@ internal sealed partial class CSharpIntroduceVariableService | |||||
{ | ||||||
protected override Document IntroduceLocal( | ||||||
SemanticDocument document, | ||||||
CodeCleanupOptions options, | ||||||
ExpressionSyntax expression, | ||||||
bool allOccurrences, | ||||||
bool isConstant, | ||||||
|
@@ -47,14 +50,25 @@ protected override Document IntroduceLocal( | |||||
? TokenList(ConstKeyword) | ||||||
: default; | ||||||
|
||||||
var updatedExpression = expression.WithoutTrivia(); | ||||||
var csOptions = (CSharpSimplifierOptions)options.SimplifierOptions; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
// If the target-type new syntax is preferred and "var" is not preferred under any circumstance, then we use the target-type new syntax. | ||||||
// The approach is not exhaustive. We aim to support codebases that rely strictly on the target-type new syntax (i.e., no "var"). | ||||||
if (csOptions.ImplicitObjectCreationWhenTypeIsApparent.Value && csOptions.GetUseVarPreference() == UseVarPreference.None | ||||||
&& expression is ObjectCreationExpressionSyntax objectCreationExpression) | ||||||
{ | ||||||
updatedExpression = ImplicitObjectCreationExpression(objectCreationExpression.ArgumentList, objectCreationExpression.Initializer); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
} | ||||||
|
||||||
var declarationStatement = LocalDeclarationStatement( | ||||||
modifiers, | ||||||
VariableDeclaration( | ||||||
GetTypeSyntax(document, expression, cancellationToken), | ||||||
[VariableDeclarator( | ||||||
newLocalNameToken.WithAdditionalAnnotations(RenameAnnotation.Create()), | ||||||
argumentList: null, | ||||||
EqualsValueClause(expression.WithoutTrivia()))])); | ||||||
EqualsValueClause(updatedExpression))])); | ||||||
|
||||||
switch (containerToGenerateInto) | ||||||
{ | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -935,7 +935,7 @@ class Program | |
{ | ||
static void Main() | ||
{ | ||
G<int>.@class {|Rename:@class|} = new G<int>.@class(); | ||
G<int>.@class {|Rename:@class|} = new(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
G<int>.Add(@class); | ||
} | ||
} | ||
|
@@ -1018,7 +1018,7 @@ public static void Add(object @class) | |
|
||
static void Main() | ||
{ | ||
G<int>.@class {|Rename:@class|} = new G<int>.@class(); | ||
G<int>.@class {|Rename:@class|} = new(); | ||
G<int>.Add(@class); | ||
} | ||
} | ||
|
@@ -3001,7 +3001,7 @@ class Program | |
{ | ||
static void Main() | ||
{ | ||
Nullable<int*> {|Rename:v|} = new Nullable<int*>(); | ||
Nullable<int*> {|Rename:v|} = new(); | ||
v.GetValueOrDefault(); | ||
} | ||
} | ||
|
@@ -4283,6 +4283,56 @@ public T this[int i] | |
await TestInRegularAndScriptAsync(code, expected, options: ImplicitTypingEverywhere()); | ||
} | ||
|
||
[Fact] | ||
public async Task TestIntroduceLocalWithTargetTypedNew() | ||
{ | ||
var code = | ||
""" | ||
using System; | ||
class SampleType | ||
{ | ||
public SampleType() | ||
{ | ||
int sum = Sum([|new Numbers()|]); | ||
} | ||
|
||
private int Sum(Numbers numbers) | ||
{ | ||
return 42; | ||
} | ||
|
||
private class Numbers {} | ||
} | ||
"""; | ||
|
||
var expected = | ||
""" | ||
using System; | ||
class SampleType | ||
{ | ||
public SampleType() | ||
{ | ||
Numbers {|Rename:numbers|} = new(); | ||
int sum = Sum(numbers); | ||
} | ||
|
||
private int Sum(Numbers numbers) | ||
{ | ||
return 42; | ||
} | ||
|
||
private class Numbers {} | ||
} | ||
"""; | ||
|
||
OptionsCollection optionsCollection = new(GetLanguage()) | ||
{ | ||
{ CSharpCodeStyleOptions.ImplicitObjectCreationWhenTypeIsApparent, new CodeStyleOption2<bool>(true, NotificationOption2.Warning) }, | ||
MartyIX marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
await TestInRegularAndScriptAsync(code, expected, options: optionsCollection); | ||
} | ||
|
||
[Fact] | ||
public async Task TestIntroduceFieldInExpressionBodiedPropertyGetter() | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor, but we do not tend to use things like
cs
in our code for abbreviations. We'd either usecsharpOptions
here, or justsimplifierOptions
(no need for the 'csharp' name to distringuish it.