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

Fixed incorrect offer to interpolate string if used string.Format with named arguments. #20832

Merged
merged 4 commits into from
Jul 25, 2017
Merged

Fixed incorrect offer to interpolate string if used string.Format with named arguments. #20832

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0a3355
Fixed #19162
zaytsev-victor Jul 12, 2017
886707f
Resolved review notes.
zaytsev-victor Jul 13, 2017
8528731
Resolved review notes.
zaytsev-victor Jul 19, 2017
46462dc
Resolved review notes.
zaytsev-victor Jul 21, 2017
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
120 changes: 120 additions & 0 deletions ...res/CSharpTest/ConvertToInterpolatedString/ConvertPlaceholderToInterpolatedStringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,126 @@ void M()
{
var a = [|string.Format(out string x, 1)|];
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertToInterpolatedString)]
public async Task TestFormatWithNamedArguments1()
{
await TestInRegularAndScriptAsync(
@"using System;

class T
{
void M()
{
var a = [|string.Format(arg0: ""test"", arg1: ""also"", format: ""This {0} {1} works"")|];
}
}",
@"using System;

class T
{
void M()
{
var a = $""This {""test""} {""also""} works"";
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertToInterpolatedString)]
public async Task TestFormatWithNamedArguments2()
{
await TestInRegularAndScriptAsync(
@"using System;

class T
{
void M()
{
var a = [|string.Format(""This {0} {1} works"", arg1: ""also"", arg0: ""test"")|];
}
}",
@"using System;

class T
{
void M()
{
var a = $""This {""test""} {""also""} works"";
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertToInterpolatedString)]
public async Task TestFormatWithNamedArguments3()
{
await TestInRegularAndScriptAsync(
@"using System;

class T
{
void M()
{
var a = [|string.Format(""{0} {1} {2}"", ""10"", arg1: ""11"", arg2: ""12"" )|];
}
}",
@"using System;

class T
{
void M()
{
var a = $""{""10""} {""11""} {""12""}"";
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertToInterpolatedString)]
public async Task TestFormatWithNamedArguments4()
{
await TestInRegularAndScriptAsync(
@"using System;

class T
{
void M()
{
var a = [|string.Format(""{0} {1} {2}"", ""10"", arg1: ""11"", arg2: ""12"")|];
}
}",
@"using System;

class T
{
void M()
{
var a = $""{""10""} {""11""} {""12""}"";
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertToInterpolatedString)]
public async Task TestFormatWithNamedArguments5()
{
await TestInRegularAndScriptAsync(
@"using System;

class T
{
void M()
{
var a = [|string.Format(""{0} {1} {2} {3}"", ""10"", arg1: ""11"", arg2: ""12"")|];
}
}",
@"using System;

class T
{
void M()
{
var a = $""{""10""} {""11""} {""12""} {3}"";
}
}");
}
}
Expand Down
53 changes: 46 additions & 7 deletions ...ToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.CodeAnalysis.Simplification;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.CodeRefactorings;
using System.Collections.Generic;

namespace Microsoft.CodeAnalysis.ConvertToInterpolatedString
{
Expand Down Expand Up @@ -80,7 +81,7 @@ private bool TryFindInvocation(
var arguments = syntaxFactsService.GetArgumentsOfInvocationExpression(invocation);
if (arguments.Count >= 2)
{
var firstArgumentExpression = syntaxFactsService.GetExpressionOfArgument(arguments[0]) as TLiteralExpressionSyntax;
var firstArgumentExpression = syntaxFactsService.GetExpressionOfArgument(GetFormatArgument(arguments, syntaxFactsService)) as TLiteralExpressionSyntax;
if (firstArgumentExpression != null && syntaxFactsService.IsStringLiteral(firstArgumentExpression.GetFirstToken()))
{
invocationSymbol = semanticModel.GetSymbolInfo(invocation, cancellationToken).Symbol;
Expand All @@ -106,7 +107,7 @@ private bool IsArgumentListCorrect(
CancellationToken cancellationToken)
{
var arguments = nullableArguments.Value;
var firstExpression = syntaxFactsService.GetExpressionOfArgument(arguments[0]) as TLiteralExpressionSyntax;
var firstExpression = syntaxFactsService.GetExpressionOfArgument(GetFormatArgument(arguments, syntaxFactsService)) as TLiteralExpressionSyntax;
if (arguments.Count >= 2 &&
firstExpression != null &&
syntaxFactsService.IsStringLiteral(firstExpression.GetFirstToken()))
Expand All @@ -116,7 +117,7 @@ private bool IsArgumentListCorrect(
// string[] args;
// String.Format("{0}{1}{2}", args);
return IsArgumentListNotPassingArrayToParams(
syntaxFactsService.GetExpressionOfArgument(arguments[1]),
syntaxFactsService.GetExpressionOfArgument(GetParamsArgument(arguments, syntaxFactsService)),
invocationSymbol,
formatMethods,
semanticModel,
Expand All @@ -126,7 +127,6 @@ private bool IsArgumentListCorrect(
return false;
}


private async Task<Document> CreateInterpolatedString(
TInvocationExpressionSyntax invocation,
Document document,
Expand All @@ -135,7 +135,7 @@ private async Task<Document> CreateInterpolatedString(
{
var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
var arguments = syntaxFactsService.GetArgumentsOfInvocationExpression(invocation);
var literalExpression = syntaxFactsService.GetExpressionOfArgument(arguments[0]) as TLiteralExpressionSyntax;
var literalExpression = syntaxFactsService.GetExpressionOfArgument(GetFormatArgument(arguments, syntaxFactsService)) as TLiteralExpressionSyntax;
var text = literalExpression.GetFirstToken().ToString();
var syntaxGenerator = document.Project.LanguageServices.GetService<SyntaxGenerator>();
var expandedArguments = GetExpandedArguments(semanticModel, arguments, syntaxGenerator, syntaxFactsService);
Expand All @@ -146,6 +146,31 @@ private async Task<Document> CreateInterpolatedString(
return document.WithSyntaxRoot(newRoot);
}

private string GetArgumentName(TArgumentSyntax argument, ISyntaxFactsService syntaxFactsService)
=> syntaxFactsService.GetNameForArgument(argument);

private SyntaxNode GetParamsArgument(SeparatedSyntaxList<TArgumentSyntax> arguments, ISyntaxFactsService syntaxFactsService)
{
var i = arguments.IndexOf(argument => GetArgumentName(argument, syntaxFactsService) == StringFormatArguments.ArgsArgumentName);
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work in a case-insensitive language like Visual Basic?

return i >= 0 ? arguments[i] : arguments[1];
}

private TArgumentSyntax GetFormatArgument(SeparatedSyntaxList<TArgumentSyntax> arguments, ISyntaxFactsService syntaxFactsService)
{
var index = arguments.IndexOf(argument => GetArgumentName(argument, syntaxFactsService) == StringFormatArguments.FormatArgumentName);
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work in a case-insensitive language like Visual Basic?

return index >= 0 ? arguments[index] : arguments[0];
}

private TArgumentSyntax GetArgument(SeparatedSyntaxList<TArgumentSyntax> arguments, int index, ISyntaxFactsService syntaxFactsService)
{
if (arguments.Count > 4)
Copy link
Member

Choose a reason for hiding this comment

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

needs explanation.

Copy link
Contributor Author

@zaytsev-victor zaytsev-victor Jul 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

if argument more than 4 it means that it is used the following overload String.Format(String format, params Object[] args) without using named arguments. So we 100% sure in that case that we have no named arguments.

Copy link
Member

Choose a reason for hiding this comment

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

i meant: needs explanation in the code (i.e. add a comment) :)

{
return arguments[index];
}

return arguments.FirstOrDefault(argument => GetArgumentName(argument, syntaxFactsService) == StringFormatArguments.ParamsArgumentNames[index]) ?? arguments[index];
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work in a case-insensitive language like Visual Basic?

}

private ImmutableArray<TExpressionSyntax> GetExpandedArguments(
SemanticModel semanticModel,
SeparatedSyntaxList<TArgumentSyntax> arguments,
Expand All @@ -155,7 +180,7 @@ private ImmutableArray<TExpressionSyntax> GetExpandedArguments(
var builder = ArrayBuilder<TExpressionSyntax>.GetInstance();
for (int i = 1; i < arguments.Count; i++)
{
var argumentExpression = syntaxFactsService.GetExpressionOfArgument(arguments[i]);
var argumentExpression = syntaxFactsService.GetExpressionOfArgument(GetArgument(arguments, i, syntaxFactsService));
var convertedType = semanticModel.GetTypeInfo(argumentExpression).ConvertedType;
if (convertedType == null)
{
Expand Down Expand Up @@ -214,7 +239,7 @@ private static bool ShouldIncludeFormatMethod(IMethodSymbol methodSymbol)
}

var firstParameter = methodSymbol.Parameters[0];
if (firstParameter?.Name != "format")
if (firstParameter?.Name != StringFormatArguments.FormatArgumentName)
{
return false;
}
Expand Down Expand Up @@ -246,5 +271,19 @@ public ConvertToInterpolatedStringCodeAction(string title, Func<CancellationToke
{
}
}

private static class StringFormatArguments
{
public const string FormatArgumentName = "format";

public const string ArgsArgumentName = "args";

public static readonly Dictionary<int, string> ParamsArgumentNames = new Dictionary<int, string>
Copy link
Member

Choose a reason for hiding this comment

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

💡 If you are just going to index into it (which the code currently is doing), we can benefit from both immutability and performance by changing this to ImmutableArray<string>:

public static readonly ImmutableArray<string> ParamsArgumentNames =
  ImmutableArray.Create("", "arg0", "arg1", "arg2");

{
[1] = "arg0",
[2] = "arg1",
[3] = "arg2"
};
}
}
}