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

Unescape interpolated string literal components #54706

Merged
merged 4 commits into from
Jul 9, 2021
Merged

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jul 9, 2021

Fixes #54703. Spec change is at dotnet/csharplang#4910.

Note that there is an interesting wrinkle here in that, for an interpolated string used as a string, the change in constant value makes the lowering mechanism an observable side effect. While doing this step during lowering instead of initial binding is technically feasible, it would be quite complex as there are several different ways an AppendLiteral call could get to lowering (dynamic, conversion to object, regular literal, passed by in, etc). For now, I'm opting for simple strategy of accepting this observable side-effect, and if we want to try and make that side effect invisible we can look at doing so at a later point.

Test plan #51499

Fixes dotnet#54703. Spec change is at dotnet/csharplang#4910.

Note that there is an interesting wrinkle here in that, for an interpolated string used as a string, the change in constant value makes the lowering mechanism an observable side effect. While doing this step during lowering instead of initial binding is technically feasible, it would be quite complex as there are several different ways an AppendLiteral call could get to lowering (dynamic, conversion to object, regular literal, passed by in, etc). For now, I'm opting for simple strategy of accepting this observable side-effect, and if we want to try and make that side effect invisble we can look at doing so at a later point.
@333fred 333fred requested a review from a team as a code owner July 9, 2021 06:00
@jcouv jcouv self-assigned this Jul 9, 2021
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 1)

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Minor feedback on the length being passed to handler ctor. Don't think that should be blocking though.

Thanks for getting this done!

@333fred 333fred requested review from jaredpar and jcouv July 9, 2021 06:33
@333fred
Copy link
Member Author

333fred commented Jul 9, 2021

@jaredpar @jcouv re-requesting review because the refactor for the correct length is slightly longer than I'm comfortable with just assuming you still sign off on.

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thank you!

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 4)

@333fred 333fred merged commit 29af59d into dotnet:main Jul 9, 2021
@ghost ghost added this to the Next milestone Jul 9, 2021
@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
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.

Interpolated string handler lowering isn't unescaping {{ and }}
5 participants