F504: Carriage return prevents expression extraction

[addisoncrump]

In Python, a standalone carriage return character (0xd or \r) is considered whitespace. One could easily argue that this is quite problematic, given that carriage returns can potentially be used to hide parts of source code. That said, it does trigger a failure in F504 (\r replaced literally):

"" % {
  'test1': '',\r  'test2': '',
}

And a downloadable version to test against.

I would simply mark F504 as sometimes fixable, but this solution doesn't feel right. Instead, this feels indicative of incorrect handling of carriage returns as plain whitespace. At the same time, standalone carriage returns should probably be marked as invalid anyways.

I leave this to the discretion of the developers of Ruff 🙂 We should find a fix one way or another to avoid future issues with this being marked as a failure case in the fuzzer.

Discovered by #4822.

[charliermarsh]

Perhaps a bug in LibCST, which is failing to parse it?

[addisoncrump]

I'm not so clear on that. Is this because libCST is used to parse the expression for repair?

[charliermarsh]

Yeah

[charliermarsh]

The LibCST parse call throws an error, but it could be our fault, not theirs. We may need to wrap the expression in parentheses before parsing.

[dhruvmanila]

We may need to wrap the expression in parentheses before parsing.

I did a quick experiment and this doesn't help either, it still throws an error:

libcst_native::parse_expression(&format!("({})", &contents))

Failed to parse file: Internal error while parsing whitespace: Found newline at (2, 15) but it's not EOL

[MichaReiser]

This is a bug in LibCST. It seems they don't support mixed newlines:

https://github.com/Instagram/LibCST/blob/c91655fbba7c27db05423673609f10203316b9c0/native/libcst/src/tokenizer/whitespace_parser.rs#L67-L88

Changing the split to split on all new lines seems trivial enough, but I worry that there's other code that simply doesn't work if you use \r

For example:

https://github.com/Instagram/LibCST/blob/c91655fbba7c27db05423673609f10203316b9c0/native/libcst/src/tokenizer/whitespace_parser.rs#L90-L94

[MichaReiser]

Upstream fix in LibCST Instagram/LibCST#1007

[MichaReiser]

The upstream fix is merged. All that we need to do now is to update libcst 🫣