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

Interpolated string handler lowering isn't unescaping {{ and }} #54703

Closed
stephentoub opened this issue Jul 9, 2021 · 3 comments · Fixed by #54706
Closed

Interpolated string handler lowering isn't unescaping {{ and }} #54703

stephentoub opened this issue Jul 9, 2021 · 3 comments · Fixed by #54706
Assignees
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Jul 9, 2021

Version Used:
1119ab9

Steps to Reproduce:
https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQEsB2MIIA4D2ANrBACarIwJYDmAPgMSZxEnBEQAEEmUHEALAAoAAIAmAIwjRAZi4SuAYREBvEV00L5oyQAYuAWQAUWGF3QBKLgF4AfFwAkAIlWquq9AF8uXr84BuES8RET4AWxQ8KABjbl0AOgAlOGx0SISlAnC8dE4EAGVcADd0OOQ1DS0AbUSAQRhqdGA4HABVZChaCAamlpxjesaafogAFSgEbphkTJJkZC56BUkE3pHW8cnp2YLqOBiYABouOrYCAHdDVhh0PE5bLjAoImQIE4BJTAALXHQcMiPZ6vCCWAC6VU0ci4bxe5AU4i4Xxw+GIpDIexomFoAAkoJgyPl1s1NlwQCs1sMSThIR5aVptEjsLhCCQAZi6HiCUSqaNjNZ3CFhAyhQzaqtiaMOl0IIMJbzNhMphAZgkAAqTKCRFEnM5ES7XIi3e7cGxPF5vT4/P4AoEW0EQ4VaaGwziAxTIllo9lNbFcwm4OpTOCRbCS0nkoZ9Ta09ROhlQ+Se1Fs8gcv34gMIIO0EO8GDhga6AzbPPYAW+enOpPMlPo9O4zNE4OhgsKgbRBBaxbF6pgril1vICuihOJlZ6PunFv5xbuaYeLgwb4IS5cFhsAK+SvxndixIpNIZLI5PK4IoIUrlBLJ1n132N7mB9sQR0M6FIMAw/aHLgAEQgZ4blvb00wff18ljKtNDwGhilILgkCgMgCEwIgAE8uAIYAACsIF/AB9MgQ3CdCgl3BlYPQeCcEQ6AULQzCzC4IiSPQtUaHCf5qIgcix3HACgKNEDUwxcCm1wUxsC4Ih/lwF4ABleFoZcTmYsACAQcJYABLJUhgCtl1XC511YIgt1HMdoUEqBgNrO8fSxR8syk8xZJRRTlNUixpI0rSdPIPTsBORIPgAMU07SYA4ghSjIXAAH4uFg2L0HihBDJXNcN3Mvd+OswDbOE+zQLEpyIMk5j3PkoglOxbz1MigKyCC44KXCprotXOLEuS7q0twELVgKaJMAAHhib5JgcLBuJeAAhOAwEgDLFyM7KzIs6DGVEAAWU48DwXgyAUuSuyIOUDHgog4FBRdLITaE9oOo6CQi/zGnIOVknogB5RiRvxCapoQBxrtukdtqe/a6kO473qigFvqSP6AdG4Hpq4cH3h88wXnQWhMFbR49CGvQkr8qLHhyyGKOrBQYbht7OoBMaxjsYwxixl4IfuqGdEZ16yAR5q2Y5rnsbJinOtp/idsF+GWfIMXOe5m6ceY/HCdbWX8oFl7FY+1n2dVyXcYHWTtfzKWnhlvm6fHZ7YaFkXPrIYxsLww4krNzXLaJ/MSZtynYGpszdas/XncNxGvuLH2ebu9x1pMnKtod+WDeZo24/0BP1bU6StYD6SzVJidpY+sO2Ajx75Fi3AaHiicuDGAgG35NastTza8sjlu24bOoCSUThJk75Pu9Mzc+53LwgA==

public class C
{
    public string M(int i) => $"{{ {i} }}";
}

Expected Behavior:
This should produce the equivalent of "{ " + i + " }".

Actual Behavior:
Instead it's producing the equivalent of "{{ " + i + " }}".

This is a behavioral regression.

cc: @333fred

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 9, 2021
@333fred
Copy link
Member

333fred commented Jul 9, 2021

It is not the job of the C# compiler to do escaping here. This is the equivalent of string.Format("{{ {0} }}", i). The AppendLiteral call should be the thing handling the escape of the {{, just like string.Format would do. If we need interpolated strings to be doing the same escape rules in the compiler that string.Format does, I'm extremely concerned by this as it is not part of the specification for the feature and goes well beyond the standard handling we have in the compiler here.

@stephentoub
Copy link
Member Author

I completely disagree. A key tenet of this feature is the compiler is already parsing the strings at compile-time and emitting the literals to be appended so no parsing is required at run-time. The C# compiler needs to do the escaping.

@333fred
Copy link
Member

333fred commented Jul 9, 2021

I'll send an email to LDM to get approval for a spec change here and get a PR out to change this while we wait for approval.

333fred added a commit to 333fred/roslyn that referenced this issue Jul 9, 2021
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.
@jaredpar jaredpar added Feature - Interpolated String Improvements Interpolated string improvements Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 9, 2021
@jaredpar jaredpar added this to the 17.0 milestone Jul 9, 2021
333fred added a commit that referenced this issue Jul 9, 2021
* Unescape interpolated string literal components

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 invisble we can look at doing so at a later point.

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants