[ruff] improve handling of intermixed comments inside from-imports

[amyreese]

Resolves the crash when attempting to format code like:

from x import (a as # whatever
b)

And chooses to format it as:

from x import (
    a as b,  # whatever
)

Fixes issue #19138

[github-actions]

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[MichaReiser]

Thanks for looking into this. We'll need to add tests (see resources/fixtures). I also suggest playing with different comment combinations. E.g. the following now flips the order of comments which is always undesired:

from x import (a # more 
 as # whatever
b)

Gets formatted to

from x import (
    a as b,  # more
    # whatever
)

This one doesn't flip the comment order but it places the other comment that used to be before b after b

from x import (a
 as # whatever
 # other
b)

Not suggesting that you should or that it makes your life easier in any way. But an alternative fix is to update place_comment so that it makes comments between as and the first alias leading comments of the first alias (as was done in #20589)

[MichaReiser]

Passing 0 for the width is incorrect here. The width is used to determine how much space the Printer needs to reserve for the comments. Zero means that they take zero width (or the width is ignored) and they always fit if the content before fits on the line.

The reason line_suffix has a width is that Ruff excludes pragma comments from the width calculation, but all other comments need to fit into the 88-character limit, or the line is split over multiple lines.

You can use trailing_comments if you want to format the comments as end-of-line comments

[amyreese]

Working on adding some test fixtures to validate results, and considering alternative approaches.

[amyreese]

In this given example putting comments in every possible position, Black will maintain placement of all the comments, rather than re-flowing any elements together and merging comments:

# alpha
from x import (
    # bravo
    a  # charlie
    # delta
    as  # echo
    # foxtrot
    b  # golf
    # hotel
    ,  # india
    # juliet
)  # kilo

Do we want to ensure that we follow Black's style and preserve all of these comments in their original positions? Or do we want to have an opinion and re-flow/merge various comments to ensure that a as b, is always on one line when possible?

[MichaReiser]

Do we want to ensure that we follow Black's style and preserve all of these comments in their original positions? Or do we want to have an opinion and re-flow/merge various comments to ensure that a as b, is always on one line when possible?

We've used Black's comment formatting as inspiration and we try to match it for "common" comment placements but it's not a strict requirement that we have to match black precisely (I also found Black inconsistent about when it tries to be smart/opinionated and when it's strict about preserving comment placements).

We do try to preserve own-line comments as own-line comments and end-of line comments should remain end-of-line comments. It's also important to preserve the ordering of comments.

Trying your example with a few "similar" syntax elements (this is the formatted output):

# alpha
(
    # bravo
    a  # charlie
    # delta
    +  # echo
    # foxtrot
    b,  # golf
    # hotel
    # india
    # juliet
)  # kilo

# scrambles own line and end of line comments

# alpha
with (
    # bravo
    a as (  # charlie
    # delta  # echo
        # foxtrot
        b
    ),  # golf
    # hotel
    # india
    # juliet
):  # kilo
    ...

match x:
    # alpha
    case (
        # bravo
        a  # charlie
        # delta
        as  # echo
        # foxtrot
        b,  # golf
        # hotel
        # india
        # juliet
    ):  # kilo
        ...

[amyreese]

Updated to use trailing_comments. It did not seem feasible to make it fully preserve all comment positions like black does, but I ensured that comment order is at least preserved, and this provides the "opinion" of keeping a as b on the same line. Updated fixtures/snapshots with the example from yesterday.

[MichaReiser]

Does this change reflow comments of already formatted code?

Could we do something similar to the MatchAs formatting

ruff/crates/ruff_python_formatter/src/pattern/pattern_match_as.rs

Lines 27 to 44 in 63b3f7a

	if comments.has_trailing(pattern.as_ref()) {
	write!(f, [hard_line_break()])?;
	} else {
	write!(f, [space()])?;
	}
	write!(f, [token("as")])?;
	let trailing_as_comments = comments.dangling(item);
	if trailing_as_comments.is_empty() {
	write!(f, [space()])?;
	} else if trailing_as_comments
	.iter()
	.all(|comment| comment.line_position().is_own_line())
	{
	write!(f, [hard_line_break()])?;
	}
	write!(f, [dangling_comments(trailing_as_comments)])?;

I'm asking because the formatting of

from x import (a # more 
 as # whatever
b)


from x import (a
 as # whatever
 # other
b)

to

from x import (
    a as b,  # more
    # whatever
)


from x import (
    a as b,  # whatever
    # other
)

or

# alpha
from x import (
    # bravo
    a
    
    as  # echo
    # foxtrot
    b  # golf
    # hotel
)

where the foxdrot comment now comes after b (it was a leading comment but it now becomes a trailing comment and the indent for golf is off)

# alpha
from x import (
    # bravo
    a as b,  # echo
    # foxtrot
      # golf
    # hotel
)

The indent of # golf is problematic because this is now a case where the formatter needs two passes to reach convergence. We need to fix this at least and add them as test cases.

[amyreese]

Hmm, I'm seeing different results from you when I test this locally:

$ cargo run --bin ruff_python_formatter -- --emit stdout scratch.py
...

from x import (
    a as b,  # more  # whatever
)


from x import (
    a as b,  # whatever
    # other
)

# alpha
from x import (
    # bravo
    a as b,  # echo
    # foxtrot  # golf
    # hotel
)

[MichaReiser]

Ah, sorry. I think pulling the changes this morning failed and I then tested against the previous revision. Adding those tests might still make sense because they demonstrate other issues that your PR fixes.

[amyreese]

I'm revisiting the comment placement mechanisms, and attempting to better associate comments within an alias node to the identifiers they belong with.

[amyreese]

Reworked the ways comments are associated, specifically for the Alias and StmtImportFrom nodes, preserving comment placement at every position within a from-import and matching black's formatting:

$ cargo run --bin ruff_python_formatter -- --emit stdout crates/ruff_python_formatter/resources/test/fixtures/black/cases/import_comments.py
   Compiling ruff_db v0.0.0 (/Users/amethyst/workspace/ruff/crates/ruff_db)
   Compiling ruff_python_formatter v0.0.0 (/Users/amethyst/workspace/ruff/crates/ruff_python_formatter)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.18s
     Running `target/debug/ruff_python_formatter --emit stdout crates/ruff_python_formatter/resources/test/fixtures/black/cases/import_comments.py`
# ensure trailing comments are preserved
import x  # comment
from x import a  # comment
from x import a, b  # comment
from x import a as b  # comment
from x import a as b, b as c  # comment

# ensure intermixed end- and own-line comments are all preserved
from x import (  # one
    # two
    a  # three
    # four
    ,  # five
    # six
)  # seven

from x import (  # alpha
    # bravo
    a  # charlie
    # delta
    as  # echo
    # foxtrot
    b  # golf
    # hotel
    ,  # india
    # juliet
)  # kilo

Passing ruff's output black confirms no changes:

$ cargo run -p ruff -- format - < crates/ruff_python_formatter/resources/test/fixtures/black/cases/import_comments.py | uvx black -
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s
     Running `target/debug/ruff format -`
# ensure trailing comments are preserved
import x  # comment
from x import a  # comment
from x import a, b  # comment
from x import a as b  # comment
from x import a as b, b as c  # comment

# ensure intermixed end- and own-line comments are all preserved
from x import (  # one
    # two
    a  # three
    # four
    ,  # five
    # six
)  # seven

from x import (  # alpha
    # bravo
    a  # charlie
    # delta
    as  # echo
    # foxtrot
    b  # golf
    # hotel
    ,  # india
    # juliet
)  # kilo
All done! ✨ 🍰 ✨
1 file left unchanged.

[MichaReiser]

Nice!

Given that we now preserve the original placement in more cases, it should also mean that we don't reformat any existing code (and, thus, doesn't require any preview gating)

[MichaReiser]

Nit: The place_comments logic tends to be complicated and its often non obvious what cases each branch is handling. That's why we tend to add inline comments that show examples of the patterns they match on (see the comments above).

Could you add such comments for this branch and to handle_alias_comment too?

// this should be a dangling comment but only if it comes before the `,`
// ```python
// from foo import (
//      baz # comment     
//      , 
//      bar 
// )
// ```

[amyreese]

Documented placement functions and changes, as well as in the alias formatter. Also added more test cases and updated the logic to better match black style and merge lines/comments when no own-line comments are between the alias and the trailing comma:

Eg,

from foo import (
    bar  # one
    ,  # two

Becomes:

from foo import (
    bar,  # one  # two
)

Edit: to be clear, this change is also closer to the existing behavior of Ruff, so even though it changes formatting, it's no different than what Ruff already does today:

main $ cargo run -p ruff --  format --diff -
...

from foo import (
    bar  # comment
    ,  # another
    baz,
)
^D
@@ -1,5 +1,4 @@
 from foo import (
-    bar  # comment
-    ,  # another
+    bar,  # comment  # another
     baz,
 )

> [1]