Skip to content

Conversation

@abonie
Copy link
Member

@abonie abonie commented Nov 28, 2022

Fixes #11782

The problem was, that in interpolated strings we turn {{ into { (and }} into }) during lexing.

This fixes it by converting it back to {{ (and }}) before calling FormattableStringFactory.Create (after interpolation expressions ({expr}) have already been processed).

@abonie abonie force-pushed the fix-formattablestring-with-braces branch from a082089 to 79b114c Compare November 28, 2022 18:22
@abonie abonie marked this pull request as ready for review November 29, 2022 10:50
@abonie abonie requested a review from a team as a code owner November 29, 2022 10:50
@abonie abonie force-pushed the fix-formattablestring-with-braces branch from 79b114c to 5fc1054 Compare November 29, 2022 11:40
0101
0101 previously approved these changes Nov 29, 2022
psfinaki
psfinaki previously approved these changes Nov 29, 2022
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the bug, nevertheless I'd appreciate some meaningful examples in tests, like more or less real usecases for all this quite specific topic.

Also, since you've become a guru in the F# interpolated strings, consider checking correctness and completeness of our docs. Also, I guess it's worth adding a note about F# to the docs here and here.

@abonie
Copy link
Member Author

abonie commented Nov 29, 2022

Thanks for fixing the bug, nevertheless I'd appreciate some meaningful examples in tests, like more or less real usecases for all this quite specific topic.

Also, since you've become a guru in the F# interpolated strings, consider checking correctness and completeness of our docs. Also, I guess it's worth adding a note about F# to the docs here and here.

Thanks for the comment, somehow it made me take another look at this change and realize it will be broken again if we implement this extended interpolated strings RFC. I'll turn this PR back to draft and give this some more thought, because it doesn't make sense to introduce a hacky fix if we already see how it will break again.

Regarding request for more tests, I actually don't think we should extensively test FormattableStrings in F# outside of some cases that may interact weirdly with F# idiosyncrasies (like this bug).

@abonie abonie marked this pull request as draft November 29, 2022 15:17
@abonie
Copy link
Member Author

abonie commented Nov 29, 2022

Actually, I might have too hastily proclaimed that this will be broken by extended interpolation syntax - I think it happens to work fine, because whatever { and } are there after turning interpolated expressions into %P()s should be escaped before being passed to FormattableStringFactory.Create.

But it is now more apparent to me, that this is a hacky fix, because it implicitly couples this part of checking to some behavior of the lexer. On the other hand, it seems to me that they are already somewhat coupled together...

edit: Did some thinking and some testing - it will be fine after all, even with extended string interpolation syntax. Marking as "ready for review" :)

@abonie abonie dismissed stale reviews from psfinaki and 0101 via 3a70148 November 30, 2022 14:24
@abonie abonie marked this pull request as ready for review November 30, 2022 14:41
@abonie abonie force-pushed the fix-formattablestring-with-braces branch from 3a70148 to 0562991 Compare November 30, 2022 15:54
Interpolation expressions have already been replaced by %P() at this
point, so it should be safe and should only undo the opposite
transformation done by lexer.
@abonie abonie force-pushed the fix-formattablestring-with-braces branch from 0562991 to 551aeac Compare November 30, 2022 16:59
@abonie abonie requested review from 0101 and psfinaki December 1, 2022 10:29
@psfinaki
Copy link
Contributor

psfinaki commented Dec 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

psfinaki
psfinaki previously approved these changes Dec 1, 2022
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Same feedback - thanks for the fix, some clearer examples in tests and maybe some docs would be also helpful :)

@vzarytovskii vzarytovskii requested a review from dsyme December 2, 2022 13:23
T-Gro
T-Gro previously approved these changes Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

FormattableString interpolated string mishandles escaped brackets

5 participants