search

openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
27 stars 43 forks source link

Do not escape newline in FixStringFormatExpressions #261

Closed timtebeek closed 4 months ago

timtebeek commented 4 months ago

Fixes https://github.com/openrewrite/rewrite-static-analysis/issues/260

Bananeweizen commented 4 months ago

This should work for the given example. But it's not yet perfectly correct, IMO. If you look at a pattern made of 1, 2, 3, 4, 5... consecutive backslashes plus n, then in every second of them the replacement would be okay. It basically depends on whether the number of consecutive backslashes in front of the n is odd or even. Not sure if it's worth the effort of improving the parsing that much, however, since bigger numbers of backslashes correspond with nested levels of java code generation (my initial example was from a code generator generating java code, therefore having one more level of escaping than normally already). I'm fine with taking this a bug fix.

timtebeek commented 4 months ago

I've updated the test to show with 1, 2, 3 and 4 backslashes; I don't see the issue you've mentioned with the results being different between odd and even. Feel free to clarify with an updated test if I missed anything, and thanks again!

Bananeweizen commented 4 months ago

@timtebeek The new test is fine and nothing is broken. What I meant is: In your new test example it would be okay to also apply the recipe in line 124 of the test, leading to every second line of the progression being updated. As long as the literal can be split into nonbackslash + \\ + ... + \\ + \n, replacing the newline is okay. But really, we don't need to improve the code, this is quite esoteric. :)

timtebeek commented 4 months ago

ah cheers, yes that makes sense, but indeed I'd hope no one is relying on us to clean that up 😅