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.

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

Support binary interpolation addition #55059

Merged
merged 7 commits into from
Jul 30, 2021

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jul 22, 2021

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 chsienki and a team July 22, 2021 21:44
@333fred
Copy link
Member Author

333fred commented Jul 22, 2021

/cc @stephentoub fyi.

@stephentoub
Copy link
Member

Yay. Thanks.

@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

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)

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)

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

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)

}
// https://github.com/dotnet/roslyn/issues/54583
// Better handle the constructor propogation
//public override BoundNode? VisitInterpolatedString(BoundInterpolatedString node)
Copy link
Contributor

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

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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
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