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

zaytsev-victor · 2017-07-12T18:03:41Z

Customer scenario

Incorrect offer to interpolate string if used string.Format with named arguments.

Bugs this fixes:
Fixes #19162

Workarounds, if any
Don't use this refactoring for string.Format with named arguments.

Risk
Low

Performance impact

Is this a regression from a previous update?
No
Root cause analysis:
Doesn't implemented support for string.Format with named arguments

How was the bug found?

customer reported

CyrusNajmabadi · 2017-07-12T19:16:57Z

...nvertToInterpolatedString/CSharpConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

+                return null;
+            }
+
+            return argument.NameColon.Name.Identifier.Text;


nit: consider : => argument?.NameColon?.Name.Identifier.Text

Also, use ISyntaxFactsService. It already has GetNameForArgument

(can be used directly from base class).

CyrusNajmabadi · 2017-07-12T19:17:43Z

...ertToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

+
+        private TArgumentSyntax GetArgument(SeparatedSyntaxList<TArgumentSyntax> arguments, int index)
+        {
+            if (arguments.Count > 4)


needs explanation.

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.

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

zaytsev-victor · 2017-07-14T04:38:19Z

@CyrusNajmabadi, should we suggest refactoring in this case:

void Foo()
{
    string.Format("{0} string", "test");
}

?
It will produce a error CS0201

CyrusNajmabadi · 2017-07-14T05:54:01Z

should we suggest refactoring in this case:

I would be ok with that. I would also be ok with it not being shown :)

sharwell · 2017-07-18T20:40:17Z

@zaytsev-victor

should we suggest refactoring in this case:

We could always emit this as _ = $"..." (discard). Another option would be allowing this code fix to result in the error, providing a separate general code fix for CS0201 which assigns the value either to a discard or a new variable.

sharwell · 2017-07-18T20:45:50Z

...ertToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

+
+            public const string ArgsArgumentName = "args";
+
+            public static readonly Dictionary<int, string> ParamsArgumentNames = new Dictionary<int, string>


💡 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");

jmarolf

please add the same tests for VB: https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/VisualBasicTest/ConvertToInterpolatedString/ConvertPlaceholderToInterpolatedStringTests.vb

jmarolf · 2017-07-20T19:43:39Z

...ertToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

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


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

jmarolf · 2017-07-20T19:43:48Z

...ertToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

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


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

jmarolf · 2017-07-20T19:43:58Z

...ertToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

+                return arguments[index];
+            }
+
+            return arguments.FirstOrDefault(argument => GetArgumentName(argument, syntaxFactsService) == StringFormatArguments.ParamsArgumentNames[index]) ?? arguments[index];


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

jmarolf · 2017-07-21T21:14:49Z

...ertToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

+        => arguments.FirstOrDefault(argument => string.Equals(GetArgumentName(argument, syntaxFactsService), StringFormatArguments.FormatArgumentName, StringComparison.OrdinalIgnoreCase)) ?? arguments[1];
+
+        private TArgumentSyntax GetFormatArgument(SeparatedSyntaxList<TArgumentSyntax> arguments, ISyntaxFactsService syntaxFactsService)
+            => arguments.FirstOrDefault(argument => string.Equals(GetArgumentName(argument, syntaxFactsService), StringFormatArguments.FormatArgumentName, StringComparison.OrdinalIgnoreCase)) ?? arguments[0];


In VB we want the comparison to be case-insensitive, but I believe we want a case sensitive comparison for C# correct?

@jmarolf Since String.Format("{0}", ARg0: 1) is incorrect syntax 'convert to interpolate string' refactoring wouldn't be suggested

Just tried it with your changes. You are correct.

Fixed dotnet#19162

b0a3355

dnfclas added the cla-already-signed label Jul 12, 2017

Pilchie added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jul 12, 2017

Pilchie requested review from jmarolf and a team July 12, 2017 18:10

CyrusNajmabadi reviewed Jul 12, 2017

View reviewed changes

Resolved review notes.

886707f

sharwell self-assigned this Jul 13, 2017

CyrusNajmabadi approved these changes Jul 14, 2017

View reviewed changes

Pilchie approved these changes Jul 16, 2017

View reviewed changes

sharwell added this to the 15.5 milestone Jul 18, 2017

sharwell approved these changes Jul 18, 2017

View reviewed changes

Resolved review notes.

8528731

jmarolf suggested changes Jul 20, 2017

View reviewed changes

jmarolf reviewed Jul 20, 2017

View reviewed changes

Resolved review notes.

46462dc

jmarolf reviewed Jul 21, 2017

View reviewed changes

jmarolf approved these changes Jul 21, 2017

View reviewed changes

Pilchie merged commit 117082e into dotnet:master Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zaytsev-victor commented Jul 12, 2017 •

edited by sharwell

Loading

CyrusNajmabadi Jul 12, 2017

CyrusNajmabadi Jul 12, 2017

CyrusNajmabadi Jul 12, 2017

CyrusNajmabadi Jul 12, 2017

zaytsev-victor Jul 12, 2017 •

edited

Loading

CyrusNajmabadi Jul 14, 2017

zaytsev-victor commented Jul 14, 2017

CyrusNajmabadi commented Jul 14, 2017

sharwell commented Jul 18, 2017

sharwell Jul 18, 2017

jmarolf left a comment

jmarolf Jul 20, 2017

jmarolf Jul 20, 2017

jmarolf Jul 20, 2017

jmarolf Jul 21, 2017

zaytsev-victor Jul 21, 2017

jmarolf Jul 21, 2017


		public const string ArgsArgumentName = "args";

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

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

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

Conversation

zaytsev-victor commented Jul 12, 2017 • edited by sharwell Loading

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

zaytsev-victor Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaytsev-victor commented Jul 14, 2017

CyrusNajmabadi commented Jul 14, 2017

sharwell commented Jul 18, 2017

Choose a reason for hiding this comment

jmarolf left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaytsev-victor commented Jul 12, 2017 •

edited by sharwell

Loading

zaytsev-victor Jul 12, 2017 •

edited

Loading