Skip to content

Support binary interpolation addition#55059

Merged
333fred merged 7 commits intodotnet:release/dev17.0from
333fred:interpolation-addition
Jul 30, 2021
Merged

Support binary interpolation addition#55059
333fred merged 7 commits intodotnet:release/dev17.0from
333fred:interpolation-addition

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Jul 22, 2021 •
edited by jcouv
Loading

This adds support for treating binary addition of interpolated strings as if they were a single string, for the purposes of conversions to custom handler types and lowering. I implemented this by delaying binding of binary additions composed entirely of interpolated strings until use, and adjusting all the points that currently expect a string to instead expect either an interpolated string or a binary addition of interpolated strings. @dotnet/roslyn-compiler for review: I'd suggest reviewing commit-by-commit. Commit 1 implements the basic changes just for interpolated strings used as strings, and the second commit adds support for conversions. Closes #54584.

Test plan: #51499

@333fred 333fred requested review from a team and chsienki July 22, 2021 21:44
@ghost ghost added the Area-Compilers label Jul 22, 2021
@333fred
Copy link
Member Author

333fred commented Jul 22, 2021

/cc @stephentoub fyi.

@stephentoub
Copy link
Member

stephentoub commented Jul 23, 2021

Yay. Thanks.

chsienki
chsienki reviewed Jul 23, 2021
View reviewed changes
@333fred
Copy link
Member Author

333fred commented Jul 26, 2021

@dotnet/roslyn-compiler for a second review.

@jcouv jcouv self-assigned this Jul 26, 2021
@jcouv
Copy link
Member

jcouv commented Jul 26, 2021

Looking

@jcouv
Copy link
Member

jcouv commented Jul 27, 2021 •
edited
Loading

CustomHandler c = " + expression + @";

Not blocking this PR, but we should also think about deconstruction declaration. It should behave like local declarations.
For example: (Handler x, var y) = ($"...." + $"...", 42); should behave like Handler x = $"..." + $"...";.

In reply to: 887677366

In reply to: 887677366

In reply to: 887677366

In reply to: 887677366 #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:11710 in 0f97ce1. [](commit_id = 0f97ce1, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Jul 27, 2021

CustomHandler c = " + expression + @";

Do we have tests for various groupings with parens: ($"" + $"") + $"" vs. $"" + ($"" + $"")?

In reply to: 887677366

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:11710 in 0f97ce1. [](commit_id = 0f97ce1, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Jul 28, 2021

CustomHandler c = " + expression + @";

I'm having some trouble getting an overall picture from this PR. If we're converting a suitable unconverted binary op, what are the desired type and converted type for the binary op and for the interpolated string operands? I don't have a clear idea what I should expect.
Similarly, what is the binary operator symbol we'll surface in the semantic model?
Do we have tests for those?

In reply to: 887718786

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:11710 in 0f97ce1. [](commit_id = 0f97ce1, deletion_comment = False)

jcouv
jcouv reviewed Jul 28, 2021
View reviewed changes
jcouv
jcouv reviewed Jul 28, 2021
View reviewed changes
jcouv
jcouv reviewed Jul 28, 2021
View reviewed changes
jcouv
jcouv reviewed Jul 28, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review (iteration 3). Haven't yet much looked at tests

@333fred
Copy link
Member Author

333fred commented Jul 28, 2021

CustomHandler c = " + expression + @";

what are the desired type and converted type for the binary op and for the interpolated string operands? I don't have a clear idea what I should expect.

The of the operator and all interpolated string components is string, with a converted type of the destination type. This is verified by VerifyInterpolatedStringExpression. I can add binary operator symbol validation to that method as well.

In reply to: 887936166

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:11710 in 0f97ce1. [](commit_id = 0f97ce1, deletion_comment = False)

@333fred
Copy link
Member Author

333fred commented Jul 28, 2021

CustomHandler c = " + expression + @";

Deconstruction and tuples appear to just work, actually, so I'll add a couple of tests to cover.

In reply to: 888491370

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:11710 in 0f97ce1. [](commit_id = 0f97ce1, deletion_comment = False)

@333fred
Copy link
Member Author

333fred commented Jul 28, 2021

@chsienki @jcouv thanks for the feedback. It should be addressed in the latest revision. I'll push another commit later tonight adding the expression tree exception to this PR as well, it's on my home machine and I forgot to push it yesterday.

@jcouv
Copy link
Member

jcouv commented Jul 29, 2021 

    }

Glad that deconstruction just worked out.
Not blocking this PR, but I was hoping for some explicit tests of GetConstantValue to capture the behavior you described (constant value of interpolated strings vs. that of binary ops in different conversion scenarios).

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:12045 in cbac96b. [](commit_id = cbac96b, deletion_comment = False)

jcouv
jcouv reviewed Jul 29, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 5). One question left unaddressed

jcouv
jcouv approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 6). Please follow-up on a test that is affected (remaining open issue)

chsienki
chsienki reviewed Jul 29, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
}
// https://github.com/dotnet/roslyn/issues/54583
// Better handle the constructor propogation
//public override BoundNode? VisitInterpolatedString(BoundInterpolatedString node)
Copy link
Member

@chsienki chsienki Jul 29, 2021

Choose a reason for hiding this comment

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

Nit: can we get rid of the commented code? either enter the function and return node, or remove.

Copy link
Member Author

@333fred 333fred Jul 29, 2021

Choose a reason for hiding this comment

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

I intentionally left this in to leave a place for the issue comment.

@jcouv
Copy link
Member

jcouv commented Jul 29, 2021

Will this target 17.0p3? Branch should change if so

@333fred 333fred force-pushed the interpolation-addition branch from d9cdf0e to 8b6880c Compare July 29, 2021 21:40
@333fred 333fred enabled auto-merge (squash) July 29, 2021 21:41
@333fred 333fred disabled auto-merge July 29, 2021 22:09
@333fred 333fred changed the base branch from main to release/dev17.0 July 29, 2021 22:09
@333fred 333fred enabled auto-merge (squash) July 29, 2021 22:09
@333fred 333fred merged commit 5e4c37a into dotnet:release/dev17.0 Jul 30, 2021
@333fred 333fred deleted the interpolation-addition branch July 30, 2021 01:41
@rolfbjarne rolfbjarne mentioned this pull request Aug 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcouv jcouv jcouv approved these changes

@chsienki chsienki chsienki approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Treat binary addition of interpolated string expressions as a single interpolated string expression

4 participants

@333fred @stephentoub @jcouv @chsienki