Backslash Escape Semantics #2740
Conversation
The resulting escaped value should not then be subsequently used as an escape in future replacement rounds. Additional friends, like triple backslash, which should escape the following, and quad backslash, which should escape to two single backslashes and not escape the following characters. The Quad backslash test case also has escaped double backslash at the beginning and end of the string for more edge coverage.
explicit test case for issue tox-dev#2732 which affects {/} os.sep replacement on windows platform.
When the replacement value is a backslash, it shouldn't escape the next part of the value. However, if the replacement contains a backslash, then it could affect the next value.
1. Split the value to be replaced on double backslash. 2. If there is more than 1 split, recursively perform replace on each sub value. 3. Rejoin replaced sub values on single backslash. This allows "\\" to be an escape for "\" while preserving recursive expansion semantics, and not having already-escaped backslashes affect processing replacements in other parts of the string. Fixes tox-dev#2732
if the replaced value is a single backslash, then process replace on the left and right parts of the value separately so that the replacement cannot act as an escape in other parts of the value. Fixes tox-dev#2732
|
Converted back to draft to add some additional unit test coverage. The CI just happened to expose a missing edge case. And I've written a test that passes before this patch and exposes the issue: splitting on the backslash and calling replace on the left and right subexpression breaks the parser because it can't find the trailing next simplest solution I can think of is replacing to truly solve this in a general way, might require parsing subexpressions with strict bounds. the code would be more complicated, but it could fix other weird edge cases around subexpressions expanding and breaking surrounding syntax, or having constructs that are impossible to escape within the current "whole expression" iterative replace algorithm. the simple is surely a hack, but maybe a nice compromise to unbreak windows |
|
It's up to you because you're the one needing to do the work. In the long term we'd obviously prefer the more complicated one 😅 |
|
@masenf you still plan to push this through? |
|
@gaborbernat yes, i still plan to work on it but I have been delayed with the holidays upcoming. I will close this PR since this solution is no good, and then when I write the updated parser tests and the recursive replacer, I will open a new PR to fix #2732. In the meantime if someone else wanted to take a stab at #2732, they are free to proceed or build upon my research here. |
Semantics
As written in this patch, the following rules now apply for escaping the literal backslash
\\will be treated as an escape sequence for a literal backslash\. In a value, two backslashes will never result in escaping some other special character to the right.\, then it is treated as a literal and will never result in escaping some other special character to the right.{env:FOO}whereFOO="fun with \\{env:BAR}"to avoid expanding{env:BAR}after the sub). However, this also opens the door to the shenanigans seen in tox 3.27.1 to 4.0.9, environment var substitution failing #2732 whereFOO="bad idea\\"and the value is something like{env:FOO}{env:BAR}... in this case, the trailing backslash from$FOOends up escaping the next substitution. I couldn't really think of a nice way to handle this while remaining general and not over complicating the code. I don't think it will come up often.Fixes #2732
Thanks for contribution
Please, make sure you address all the checklists (for details on how see
development documentation)!
tox -e fix)docs/changelogfolder