Skip to content
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.

Sign up for GitHub

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

Suggests parameter name for IntroduceLocal on an argument #10072

Closed
wants to merge 2 commits into from
Closed

Suggests parameter name for IntroduceLocal on an argument #10072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d6069a
Fix to suggest parameter name for IntroduceLocal on an argument
Mar 24, 2016
128eee3
Fixed up my failing tests.
Mar 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
269 changes: 250 additions & 19 deletions src/EditorFeatures/CSharpTest/CodeActions/IntroduceVariable/IntroduceVariableTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ public async Task TestNameVerbatimIdentifier1()
{
await TestAsync(
@"static class G<T> { public class @class { } public static void Add(object t) { } } class Program { static void Main() { G<int>.Add([|new G<int>.@class()|]); } }",
@"static class G<T> { public class @class { } public static void Add(object t) { } } class Program { static void Main() { var {|Rename:@class|} = new G<int>.@class(); G<int>.Add(@class); } }",
@"static class G<T> { public class @class { } public static void Add(object t) { } } class Program { static void Main() { var {|Rename:t|} = new G<int>.@class(); G<int>.Add(t); } }",
index: 0);
}

Expand All @@ -316,7 +316,7 @@ public async Task TestNameVerbatimIdentifier1NoVar()
{
await TestAsync(
@"static class G<T> { public class @class { } public static void Add(object t) { } } class Program { static void Main() { G<int>.Add([|new G<int>.@class()|]); } }",
@"static class G<T> { public class @class { } public static void Add(object t) { } } class Program { static void Main() { G<int>.@class {|Rename:@class|} = new G<int>.@class(); G<int>.Add(@class); } }",
@"static class G<T> { public class @class { } public static void Add(object t) { } } class Program { static void Main() { G<int>.@class {|Rename:t|} = new G<int>.@class(); G<int>.Add(t); } }",
index: 0,
options: new Dictionary<OptionKey, object> { { new OptionKey(CSharpCodeStyleOptions.UseVarWhenDeclaringLocals), false } });
}
Expand All @@ -326,15 +326,15 @@ public async Task TestNameVerbatimIdentifier2()
{
await TestAsync(
@"static class G<T> { public class @class { } public static void Add(object t) { } static void Main() { G<int>.Add([|new G<int>.@class()|]); } }",
@"static class G<T> { public class @class { } public static void Add(object t) { } static void Main() { var {|Rename:class1|} = new G<int>.@class(); G<int>.Add(class1); } }");
@"static class G<T> { public class @class { } public static void Add(object t) { } static void Main() { var {|Rename:t|} = new G<int>.@class(); G<int>.Add(t); } }");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task TestNameVerbatimIdentifier2NoVar()
{
await TestAsync(
@"static class G<T> { public class @class { } public static void Add(object t) { } static void Main() { G<int>.Add([|new G<int>.@class()|]); } }",
@"static class G<T> { public class @class { } public static void Add(object t) { } static void Main() { G<int>.@class {|Rename:class1|} = new G<int>.@class(); G<int>.Add(class1); } }",
@"static class G<T> { public class @class { } public static void Add(object t) { } static void Main() { G<int>.@class {|Rename:t|} = new G<int>.@class(); G<int>.Add(t); } }",
options: new Dictionary<OptionKey, object> { { new OptionKey(CSharpCodeStyleOptions.UseVarWhenDeclaringLocals), false } });
}

Expand Down Expand Up @@ -1176,7 +1176,7 @@ public async Task TestInSwitchSection()
{
await TestAsync(
@"class Program { int Main ( int i ) { switch ( 1 ) { case 0 : var f = Main ( [|1 + 1|] ) ; Console . WriteLine ( f ) ; } } } ",
@"class Program { int Main ( int i ) { switch ( 1 ) { case 0 : const int {|Rename:V|} = 1 + 1 ; var f = Main ( V ) ; Console . WriteLine ( f ) ; } } } ",
@"class Program { int Main ( int i ) { switch ( 1 ) { case 0 : const int {|Rename:i1|} = 1 + 1 ; var f = Main ( i1 ) ; Console . WriteLine ( f ) ; } } } ",
index: 2);
}

Expand Down Expand Up @@ -1259,7 +1259,7 @@ public async Task TestIntroduceLocalRemovesUnnecessaryCast()
{
await TestAsync(
@"using System.Collections.Generic; class C { static void Main(string[] args) { var set = new HashSet<string>(); set.Add([|set.ToString()|]); } } ",
@"using System.Collections.Generic; class C { static void Main(string[] args) { var set = new HashSet<string>(); var {|Rename:v|} = set.ToString(); set.Add(v); } } ");
@"using System.Collections.Generic; class C { static void Main(string[] args) { var set = new HashSet<string>(); var {|Rename:item|} = set.ToString(); set.Add(item); } } ");
}

[WorkItem(655498, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/655498")]
Expand Down Expand Up @@ -1353,8 +1353,8 @@ class Program
static void Main(string[] args)
{
var d = new Dictionary<string, Exception>();
var {|Rename:exception|} = new Exception();
d.Add(""a"", exception);
var {|Rename:value|} = new Exception();
d.Add(""a"", value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrmmm... when the variable is a generic type in the callee position, i'm not sure that taking the name is appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree, just by this example, exception seems to be a better name than value ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we were debating this in the team room. I'll let @Pilchie chime in...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are 3 options here:

  1. Always prefer parameter name - that's what's implemented here.
  2. Have heuristics about when to prefer parameter name, based on either the type of argument, or the type of receiving construct.
  3. Have this be a last chance, to use if we don't find a better name, and would otherwise fall back to v/v1, etc.

I'm not sure whether I prefer 1 or 3, but tentatively I think 1. 2 could work, but I'm not sure what the heuristic would be . I don't think it's as simple as just "is a generic type". I really think it's more like "is an element of a collection such that the parameter name doesn't really matter". Even in the case of something like a dictionary, extracting and getting key and value does seem useful.

}
}",
compareTokens: false);
Expand Down Expand Up @@ -1897,8 +1897,8 @@ class Complex
int real; int imaginary;
public static Complex operator +(Complex a, Complex b)
{
var {|Rename:v|} = b.real + 1;
return a.Add(v);
var {|Rename:b1|} = b.real + 1;
return a.Add(b1);
}

private Complex Add(int b)
Expand Down Expand Up @@ -1967,7 +1967,7 @@ public struct DBBool
@"using System;
public struct DBBool
{
private const int {|Rename:V|} = 1;
private const int {|Rename:value1|} = 1;
public static readonly DBBool dbFalse = new DBBool(-1);
int value;

Expand All @@ -1976,7 +1976,7 @@ public struct DBBool
this.value = value;
}

public static implicit operator DBBool(bool x) => x ? new DBBool(V) : dbFalse;
public static implicit operator DBBool(bool x) => x ? new DBBool(value1) : dbFalse;
}";

await TestAsync(code, expected, index: 0, compareTokens: false);
Expand Down Expand Up @@ -2380,8 +2380,8 @@ class TestClass
{
static void Test(string[] args)
{
var {|Rename:v|} = $""{DateTime.Now.ToString()}Text{args[0]}"";
Console.WriteLine(v);
var {|Rename:value|} = $""{DateTime.Now.ToString()}Text{args[0]}"";
Console.WriteLine(value);
}
}";

Expand Down Expand Up @@ -2409,9 +2409,9 @@ class TestClass
{
static void Test(string[] args)
{
var {|Rename:v|} = $""Text{{s}}"";
Console.WriteLine(v);
Console.WriteLine(v);
var {|Rename:value|} = $""Text{{s}}"";
Console.WriteLine(value);
Console.WriteLine(value);
}
}";

Expand Down Expand Up @@ -2571,8 +2571,8 @@ class C
{
public async Task M()
{
FormattableString {|Rename:v|} = $"""";
var f = FormattableString.Invariant(v);
FormattableString {|Rename:formattable|} = $"""";
var f = FormattableString.Invariant(formattable);
}
}
}";
Expand Down Expand Up @@ -2640,5 +2640,236 @@ class C
}";
await TestAsync(code, expected, index: 2, compareTokens: false);
}

//Kasey Tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove? :)

[WorkItem(2423, "https://github.com/dotnet/roslyn/issues/2423")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task IntroduceLocalFromParameterWithNamedParameters()
{
var code =
@"class Program
{
void Main(string a, string b)
{
var y = new TextSpan(second:[|int.Parse(a)|], first:int.Parse(b));
}
}

internal class TextSpan
{
public TextSpan(int first, int second)
{
}
}";
var expected =
@"class Program
{
void Main(string a, string b)
{
var {|Rename:second|} = int.Parse(a);
var y = new TextSpan(second:second, first:int.Parse(b));
}
}

internal class TextSpan
{
public TextSpan(int first, int second)
{
}
}";
await TestAsync(code, expected, index: 0);
}

[WorkItem(2423, "https://github.com/dotnet/roslyn/issues/2423")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task IntroduceLocalFromParameterWithUnNamedParameters()
{
var code =
@"class Program
{
void Main(string a, string b)
{
var y = new TextSpan([|int.Parse(a)|], int.Parse(b));
}
}

internal class TextSpan
{
public TextSpan(int first, int second)
{
}
}";
var expected =
@"class Program
{
void Main(string a, string b)
{
var {|Rename:first|} = int.Parse(a);
var y = new TextSpan(first, int.Parse(b));
}
}

internal class TextSpan
{
public TextSpan(int first, int second)
{
}
}";
await TestAsync(code, expected, index: 0);
}

[WorkItem(2423, "https://github.com/dotnet/roslyn/issues/2423")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task IntroduceLocalFromParameterWithNameConflict()
{
var code =
@"class Program
{
void Main(string a, string b)
{
var y = new TextSpan([|int.Parse(a)|], int.Parse(b));
}
}

internal class TextSpan
{
public TextSpan(int a, int b)
{
}
}";
var expected =
@"class Program
{
void Main(string a, string b)
{
var {|Rename:a1|} = int.Parse(a);
var y = new TextSpan(a1, int.Parse(b));
}
}

internal class TextSpan
{
public TextSpan(int a, int b)
{
}
}";
await TestAsync(code, expected, index: 0);
}

[WorkItem(2423, "https://github.com/dotnet/roslyn/issues/2423")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task IntroduceLocalConstantFromParameterWithParamsArray()
{
var code =
@"class Program
{
static void Main(string a, string b)
{
var paramsarray = Foo([|1|], 2, 3, 4);
}

private static object Foo(params int[] parameters)
{
throw new NotImplementedException();
}
}";
var expected =
@"class Program
{
static void Main(string a, string b)
{
const int {|Rename:V|} = 1;
var paramsarray = Foo(V, 2, 3, 4);
}

private static object Foo(params int[] parameters)
{
throw new NotImplementedException();
}
}";
await TestAsync(code, expected, index: 2);
}

[WorkItem(2423, "https://github.com/dotnet/roslyn/issues/2423")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task IntroduceLocalFromParameterWithReducedExtension()
{
var code =
@"using System.Collections.Generic;
using System.Linq;
class Program
{
static void Main(string[] args)
{
IEnumerable<int> ints = new[] { 1, 2, 3 };
ints.Select([|a => a|]);
}
}";
var expected =
@"using System.Collections.Generic;
using System.Linq;
class Program
{
static void Main(string[] args)
{
IEnumerable<int> ints = new[] { 1, 2, 3 };
System.Func<int, int> {|Rename:selector|} = a => a;
ints.Select(selector);
}
}";
await TestAsync(code, expected, index: 0);
}

[WorkItem(2423, "https://github.com/dotnet/roslyn/issues/2423")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task IntroduceLocalFromParameterInvocationExpressionWithUnNamedParameters()
{
var code =
@"class Program
{
static void Main(string a, string b)
{
M(""hello"", [|""kitty""|]);
}
public static void M(string foo, string bar){}
}";
var expected =
@"class Program
{
static void Main(string a, string b)
{
const string {|Rename:bar|} = ""kitty"";
M(""hello"", bar);
}
public static void M(string foo, string bar){}
}";
await TestAsync(code, expected, index: 2);
}

[WorkItem(2423, "https://github.com/dotnet/roslyn/issues/2423")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
public async Task IntroduceLocalFromParameterInvocationExpressionWithNamedParameters()
{
var code =
@"class Program
{
static void Main(string a, string b)
{
M(bar:""hello"", foo:[|""kitty""|]);
}
public static void M(string foo, string bar){}
}";
var expected =
@"class Program
{
static void Main(string a, string b)
{
const string {|Rename:foo|} = ""kitty"";
M(bar:""hello"", foo:foo);
}
public static void M(string foo, string bar){}
}";
await TestAsync(code, expected, index: 2);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need VB tests as well. Need to do this work for VB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree about the need for VB support.

}
}
Loading