close
Skip to content

[Refactoring] ConvertConcatenationToInterpolatedString: Support inlineing of other interpolated strings#49229

Merged
CyrusNajmabadi merged 27 commits into
dotnet:masterfrom
MaStr11:SupportInliningOfInterpolatedStringInConcatinationRefactoring
Nov 30, 2020
Merged

[Refactoring] ConvertConcatenationToInterpolatedString: Support inlineing of other interpolated strings#49229
CyrusNajmabadi merged 27 commits into
dotnet:masterfrom
MaStr11:SupportInliningOfInterpolatedStringInConcatinationRefactoring

Conversation

@MaStr11
Copy link
Copy Markdown
Contributor

@MaStr11 MaStr11 commented Nov 8, 2020

Fixes #48900
Supersedes #49207

@MaStr11
Copy link
Copy Markdown
Contributor Author

MaStr11 commented Nov 9, 2020

@CyrusNajmabadi Thanks for the review.

In my previous PR I added support for the removal of superfluous parentheses in Interpolation.Expression. I used the existing CanRemoveParentheses(SemanticModel semanticModel) extension method. So in case there are potentially removable expressions in the new InterploatedString, I requested a new sematicModel and removed the parentheses if possible:

https://github.com/MaStr11/roslyn/blob/b7928515a2f32da7b73803d9ffbe7f452a5eec1d/src/Features/CSharp/Portable/CodeRefactorings/ConvertStringConcatToInterpolated/CSharpConvertStringConcatToInterpolatedRefactoringProvider.cs#L104-L144

        private static async Task<Document> UpdateDocumentAsync(Document document, BinaryExpressionSyntax binaryExpression, ImmutableArray<ExpressionSyntax> concatParts, CancellationToken cancellationToken)
        {
            var interpolatedStringContent = ConvertConcatPartsToInterpolatedStringContent(concatParts);
            var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
            var editor = new SyntaxEditor(root, CSharpSyntaxGenerator.Instance);
            var interpolated = InterpolatedStringExpression(
                Token(SyntaxKind.InterpolatedStringStartToken),
                List(interpolatedStringContent));
            // expressions may have superfluous parenthesis after the conversion to an interpolated string:
            // "a" + (1 + 1) -> $"a{(1 + 1)}"
            // if there is any (InterpolationSyntax.Expression is ParenthesizedExpressionSyntax) we need to
            // recompile and check via parenthesized.CanRemoveParentheses(semanticModel) if it can be removed.
            var anyPotentiallyRemovableParenthesis = interpolated.Contents.Any(IsParenthesizedInterpolation);
            var annotation = new SyntaxAnnotation("InterpolatedStringExpressionWithPotentiallyRemovableParanthesisKind");
            if (anyPotentiallyRemovableParenthesis)
            {
                // annotate "interpolated" because we need to find it after the changes are applied
                interpolated = interpolated.WithAdditionalAnnotations(annotation);
            }
            editor.ReplaceNode(binaryExpression, interpolated);

            if (anyPotentiallyRemovableParenthesis)
            {
                // Take the changed document and go through interpolatedNode.Contents to check if any parenthesis can be removed.
                var newDocument = document.WithSyntaxRoot(editor.GetChangedRoot());
                var newRoot = await newDocument.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
                var interpolatedNode = (InterpolatedStringExpressionSyntax)newRoot.GetAnnotatedNodes(annotation).Single();
                var semanticModel = await newDocument.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
                editor = new SyntaxEditor(newRoot, CSharpSyntaxGenerator.Instance);
                foreach (var content in interpolatedNode.Contents.Where(IsParenthesizedInterpolation).Cast<InterpolationSyntax>())
                {
                    var parenthesized = (ParenthesizedExpressionSyntax)content.Expression;
                    if (parenthesized.CanRemoveParentheses(semanticModel))
                    {
                        editor.ReplaceNode(parenthesized, parenthesized.Expression);
                    }
                }
            }

            return document.WithSyntaxRoot(editor.GetChangedRoot());
        }

Test cases are here:
https://github.com/MaStr11/roslyn/blob/b7928515a2f32da7b73803d9ffbe7f452a5eec1d/src/EditorFeatures/CSharpTest/CodeActions/ConvertStringConcatToInterpolated/ConvertStringConcatToInterpolatedTests.cs#L197-L228

Do we want to reuse that approach here or not?

@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 10, 2020
…tConvertConcatenationToInterpolatedStringRefactoringProvider.cs

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Thanks!

@MaStr11
Copy link
Copy Markdown
Contributor Author

MaStr11 commented Nov 11, 2020

@CyrusNajmabadi Sorry, but I haven't seen your LGTM before I pushed more commits. The commits are only minor clean-ups. I have one more question though: Should I add some logic to remove superfluous parenthesis? There are some approaches to do that. I implemented one with tests as described here: #49229 (comment). I could do this as part of this PR or a new one.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

I have one more question though: Should I add some logic to remove superfluous parenthesis?

In general, an easy way to do that is to just add the SimplifierAnnotation to the topmost node you're creating. We should then go through and clean up those parens. See if that works for you. Otherwise, i wouldn't bother doing more here (unless it was super easy).

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi Sorry, but I haven't seen your LGTM before I pushed more commits.

that's fine. i still review the later commits :)

@MaStr11
Copy link
Copy Markdown
Contributor Author

MaStr11 commented Nov 11, 2020

SimplifierAnnotation to the topmost node

The Simplifier worked like a charm, but I needed to place it on each Expression of a newly generated InterploationSyntax (I also tried to place it on the final InterpolatedStringExpression but that didn't work).

@MaStr11
Copy link
Copy Markdown
Contributor Author

MaStr11 commented Nov 12, 2020

@CyrusNajmabadi Tests are green now. I added the simplifier annotation to the expressions that gets capsulated in an Interpolation, but only if the expression is a ParenthesisExpression.

@aolszowka
Copy link
Copy Markdown

I am sorry I am new to this process, where does this go? How does this get mainlined?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit d140d08 into dotnet:master Nov 30, 2020
@ghost ghost added this to the Next milestone Nov 30, 2020
@MaStr11 MaStr11 deleted the SupportInliningOfInterpolatedStringInConcatinationRefactoring branch December 1, 2020 06:47
@dibarbet dibarbet modified the milestones: Next, 16.9.P3 Dec 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Interpolating Ternary Operations

6 participants