[pycodestyle] trailing-whitespace, blank-line-contains-whitespace (W291, W293)

[mknaw]

• failing checks
• follow pycodestyle precedent on what constitutes whitespace

[matthewlloyd]

Is this code Unicode-safe? Rust strings behave in unexpected ways when there are Unicode characters, and there are in fact a few whitespace characters that are non-ASCII (e.g. '\u{A0}').

For example, if Unicode characters are present, the byte count does not equal the character count, so line.chars().count() != line.len() and if whitespace_count == line.len() will break. I'm not sure whether Location.column is a byte or character offset, but that could potentially be an issue also.

    let whitespace_count = line.chars().rev().take_while(|c| c.is_whitespace()).count();
    if whitespace_count > 0 {
        let start = Location::new(lineno + 1, line.len() - whitespace_count);
        let end = Location::new(lineno + 1, line.len());

        if whitespace_count == line.len() {

May be worth adding some tests with UTF-8 encoded (as that's Python's default encoding) lines with some whitespace and non-whitespace Unicode characters to do some TDD and make sure everything works as expected.

[charliermarsh]

(Location.column is a character offset, not a byte offset.)

[charliermarsh]

Looking good, thank you!

[mknaw]

Is this code Unicode-safe?

It appears that characters such as \uA0 are not valid whitespace. So, although they can be used in literals, a line such as print('foo')\uA0 isn't valid python syntax in the first place.

This does however raise an interesting point; it appears that pycodestyle also checks this lint based on "physical lines," so perhaps questionably will flag whitespace at the end of a given line in a multiline literal. Worse still, as written, this PR will flag a line of a multline literal ending with an \uA0 whereas pycodestyle will not. Perhaps for feature parity I should restrict the check to just those characters from pycodestyle rather than the more extensive is_whitespace list?

[charliermarsh]

Yeah I did read that source and asked myself the same question. I was originally going to comment that this shouldn't be done on physical lines for that very reason RE multiline strings, but then saw that pycodestyle was using physical lines so I just ignored it. (FWIW we could omit lines that are included in multiline strings -- we'd probably want to track those lines on Indexer, similar to how we track commented-out lines, then omit lines here that are "string lines". But I don't think it's a blocker to merging this.) I do think it makes sense to mimic their exact logic for trailing spaces though, if you're up for it. I doubt it matters much in practice, but it's good to be consistent where we can.

[matthewlloyd]

It appears that characters such as \uA0 are not valid whitespace. So, although they can be used in literals, a line such as print('foo')\uA0 isn't valid python syntax in the first place.

Aha, good to know! For what it's worth, crates/ruff/src/ast/whitespace.rs also uses Rust's broader notion of whitespace i.e. char::is_whitespace, so consistency within Ruff would argue in favor of continuing to use is_whitespace here. (Perhaps we should fix that and use Python's notion of whitespace e.g. by checking the CPython source since docs are sometimes incorrect?)

Another consideration in favor of continuing to use is_whitespace in this specific check is that while Python may treat \uA0 as non-whitespace therefore making it a syntax error at the end of a line, in practice, we would probably still want to autofix and trim it away if a user accidentally inserts it (or some other invisible whitespace non-space char!).

I can't imagine \uA0 needing to appear at the ends of lines particularly often, and while there are perhaps specific circumstances where that might be useful, e.g. in i18n multiline strings, I would guess that's a rare case compared to accidentally pressing some combination of keys that inserts "<whitespace block>"...

[charliermarsh]

Yeah that's a good argument in favor of keeping it as-is. Honestly I could go either way -- I'll defer to the author in this case @mknaw, totally up to you.

[matthewlloyd]

CPython source

https://github.com/python/cpython/blob/main/Parser/tokenizer.c#L1597

    /* Skip spaces */
    do {
        c = tok_nextc(tok);
    } while (c == ' ' || c == '\t' || c == '\014');

\014 is Control-L (formfeed).

[charliermarsh]

@mknaw - I say we merge this as-is -- ok with you? Wdyt?

[mknaw]

Sounds reasonable to me 👍