6.0.0: isort is not stable on a file which imports multiple members from the same module, one aliased and another imported directly, with --profile black

[sarahboyce]

On Django, our isort config is:

[tool.isort]
profile = "black"
default_section = "THIRDPARTY"
known_first_party = "django"

With isort==5.13.2, the following imports are sorted as:

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import SQLUpdateCompiler

But on isort==6.0.0, they are now sorted as:

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import (
    SQLUpdateCompiler,
)

We don't believe this is a desirable change and so are pinned at 5.13.2

(Please let me know if you need more details. Our pr for reference: django/django#19109)

[LepkoQQ]

We also noticed this issue, but with an extra step. This doesn't always happen the first time depending on the input.

For example if you start with code like this:

from another_file_aaaaaaaaaaaaaaaaaa import \
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED
from another_file_aaaaaaaaaaaaaaaaaa import some_func

then you run isort . --profile black and the output is:

from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import some_func

then you run isort . --profile black AGAIN and then the output is:

from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import (
    some_func,
)

[aholmes]

This seems related to this breaking change (not documented as such!) in default behavior. #2236

In my case it breaks compatibility with ruff, but the result is exactly the same. Here is the relevant Dependabot PR (with the fix) in my affected repository. https://github.com/uclahs-cds/Ligare/pull/175/files

diff --git a/pyproject.toml b/pyproject.toml
index c3dd914..2a98b33 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -284,8 +284,11 @@ skip-magic-trailing-comma = false
 line-ending = "auto"
 docstring-code-format = false
 docstring-code-line-length = "dynamic"
+[tool.ruff.lint.isort]
+split-on-trailing-comma = false

 [tool.isort]
 profile = "black"
 src_paths = ["src"]
 skip_glob = ["src/*/typings", "src/*/build"]
+split_on_trailing_comma = false

[staticdev]

@aholmes technically it is not a breaking anything. It is just a regular improvement on sorting. Django is a projecto se have integration tests for QA, but we had to comment because of this single case.

@sarahboyce I would advice to upgrade to 6.0.0 and keep the new sorting. It will work perfectly and everything is sorted as it should.

[aholmes]

@staticdev correct me if I'm mistaken, but this is an observable change in isort's default behavior, right?

[LepkoQQ]

Even if this is an intended change, (which I think is still not correct since the second import did not have a trailing comma that would make it split lines) - I can live with that if you think it is correct.

However the problem I described in my comment is still an issue in my opinion since it requires running isort twice to stabilize the sort. It already happened to us that a developer ran isort locally only once and pushed the changes which then failed on CI because the output changes when you run isort the second time.

Initially I thought this was related, but if the change is intended I can split the "you have to run isort twice" in to to a separate issue if you like.

[staticdev]

@LepkoQQ in this case I would definitely agree. Isort must be stable. I would like to confirm it is really happening.

[DanielNoord]

I would agree that this is indeed a regression, even if the sorting is stable.

I hope to devote some time tonight to look into this.

I must confess when I looked at the original django diff I wasn't 99% sure it was correct but since the PR fixed a clear bug I leaned towards "It is likely correct". Considering users are now raising this as a bug that was likely the incorrect opinion. Sorry about that!

[staticdev]

@LepkoQQ I just cloned the Django repository and actually the sorting is stable, it works on the first time (no additional changes on second run).
If @LepkoQQ and @DanielNoord agree I will close this issue as not a bug, this is a feature to me.

[sarahboyce]

The Django repo already had isort applied at least once. I can replicate with the instructions of this comment 👍

[aholmes]

@staticdev Thanks for confirming. I think I should clarify my usage of "breaking change." I don't mean to say that this change is broken or wrong; I mean that the change alters previous behavior and, as a result, potentially creates downstream breakages. In my case, this change causes my CI pipeline to fail because isort's expectation does not match ruff's formatting. This is important to communicate in the release notes. Because it was not indicated there, I (and maybe others) had to do a little digging into PRs to discover what had changed.

Either way, the point is somewhat moot because this is in a major-version change, anyway. I appreciate the work that went into this release and to improve isort; I just wish this observable change in behavior was reported in the release notes and wanted to highlight this point for future consideration.

[aholmes]

By the way I can also replicate #2352 (comment)

#!/bin/bash
read -r -d '' UNFORMATTED <<EOF
from another_file_aaaaaaaaaaaaaaaaaa import \\
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED
from another_file_aaaaaaaaaaaaaaaaaa import some_func
EOF

echo Unformatted:
echo "$UNFORMATTED"
echo '-------------------------------------------------'

FORMATTED1="`isort --profile black - <<<"$UNFORMATTED"`"
echo Formatted 1:
echo "$FORMATTED1"
echo '-------------------------------------------------'

FORMATTED2="`isort --profile black - <<<"$FORMATTED1"`"
echo Formatted 2:
echo "$FORMATTED2"

Unformatted:
from another_file_aaaaaaaaaaaaaaaaaa import \
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED
from another_file_aaaaaaaaaaaaaaaaaa import some_func
-------------------------------------------------
Formatted 1:
from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import some_func
-------------------------------------------------
Formatted 2:
from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import (
    some_func,
)

[DanielNoord]

@sarahboyce What would the desired output be of the original code block?

Wouldn't this be more correct?

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
    SQLUpdateCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler

What about the black profile makes is so that SQLUpdateCompiler should remain on its own line? Perhaps @staticdev knows more about this.

Anyway, this is indeed a regression. What (I think) is happening is that we "correctly" identify that django.db.models.sql.compiler is using a trailing comma, but then we also apply that to the line with SQLUpdateCompiler and add a comma there. Even though it is below the max line length of the profile.

If @staticdev gives me some pointers I can likely try and come up with a fix that either keeps the code as is, or puts the import of SQLUpdateCompiler with the other imports that don't use as.

[DanielNoord]

A fix with test would be:

diff --git a/isort/output.py b/isort/output.py
index ade4ad29..e098d858 100644
--- a/isort/output.py
+++ b/isort/output.py
@@ -513,7 +513,7 @@ def _with_from_imports(
                     do_multiline_reformat = True
 
                 if (
-                    import_statement
+                    len(import_statement) > config.line_length
                     and config.split_on_trailing_comma
                     and module in parsed.trailing_commas
                 ):
diff --git a/tests/integration/test_projects_using_isort.py b/tests/integration/test_projects_using_isort.py
index 61f66e7e..01ce9835 100644
--- a/tests/integration/test_projects_using_isort.py
+++ b/tests/integration/test_projects_using_isort.py
@@ -28,10 +28,6 @@ def run_isort(arguments: Generator[str, None, None] | Sequence[str]):
     main(["--check-only", "--diff", *arguments])
 
 
-@pytest.mark.xfail(
-    reason="Project is incorrectly formatted after PR #2236, should be fixed "
-    "after a release and the project formatting again."
-)
 def test_django(tmpdir):
     git_clone("https://github.com/django/django.git", tmpdir)
     run_isort(
diff --git a/tests/unit/profiles/test_black.py b/tests/unit/profiles/test_black.py
index 209d83d8..19885d77 100644
--- a/tests/unit/profiles/test_black.py
+++ b/tests/unit/profiles/test_black.py
@@ -456,3 +456,24 @@ from x import (
 )
 """,
     )
+
+
+def test_black_trailing_comma_below_line_length():
+    black_test(
+"""from django.db.models.sql.compiler import (
+    SQLAggregateCompiler,
+    SQLCompiler,
+    SQLDeleteCompiler,
+)
+from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
+from django.db.models.sql.compiler import SQLUpdateCompiler
+""",
+"""from django.db.models.sql.compiler import (
+    SQLAggregateCompiler,
+    SQLCompiler,
+    SQLDeleteCompiler,
+)
+from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
+from django.db.models.sql.compiler import SQLUpdateCompiler
+""",
+    )

However, this doesn't actually work as we would no longer respect actual trailing commas if the line length is below the max line length. The issue is that at the place where I make the change to len(import_statement) > config.line_length we don't seem to have access to the original line to check whether it has a trailing comma.

This was originally implemented with module in parsed.trailing_commas is True, but in this case it actually shouldn't be True. django.db.models.sql.compiler does indeed have a trailing comma, but not all the time. The current implementation can't account for this and I don't see an easy way to allow for this.

[staticdev]

@sarahboyce I need to know if after this discussion you still think we have an issue and what exactly is this issue. We need to be on the same page on this to see if a change is needed on isort or not.

As mentioned before, we had a change that affect how black profile deals split_on_trailing_comma (#2340). When such changes happen, it is normal that isort will resort some file differently like it happened on Django repo.

If you still think after upgrade, the output is not what is desired, I need to know exactly why and what would you expect as the output so we can further analyse. Otherwise, you can close this issue if you think this was clarified and we can continue with this output.

[vonschultz]

Reading the discussion, personally I think it's clear that we have an issue, and it's illustrated beautifully by @aholmes comment #2352 (comment)

The issue is not that isort has made a change going from "Unformatted" to "Formatted 1" in @aholmes's example. The issue is that "Formatted 1" and "Formatted 2" are different. This issue can be viewed in two ways: (A) something that has already been isorted shouldn't change just because we isort it again, and (B) the correct formatting of "Unformatted" is actually "Formatted 1", not "Formatted 2" — going straight from "Unformatted" to "Formatted 2" would solve (A) but not (B). We want "Formatted 1" to be the solution when formatting "Unformatted".

Please take close look at #2352 (comment) and see if you agree. I think a change is needed on isort.

(Thanks also to @LepkoQQ for #2352 (comment).)

[sarahboyce]

When I first opened the issue, I wanted to have this confirmed by isort whether this is a wanted changed. I don't have an opinion on whether the output on 5.13.2 is more correct than the new output on 6.0.0. If you believe this is how it should be @staticdev - that's fine.
I agree that the main issue is that the result takes 2 runs to be stable (as outlined in the a comment). I can update the issue title to better reflect this

[DanielNoord]

I still believe this is all caused by the issue I wrote about above. The handling of trailing commas for modules that appear multiple times in the import statements is inconsistent and causes unstable behaviour.

Input:

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import SQLUpdateCompiler

Output:

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import (
    SQLUpdateCompiler,
)

Input:

from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import SQLUpdateCompiler

Output:

from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import SQLUpdateCompiler

As you can see, the existence of one import statement where django.db.models.sql.compiler has a trailing comma changes the behaviour for an import statement that does not have a trailing comma. If the first import statement is left out we see that there is no diff, even though the code is the same.

This is exactly the same behaviour that we see in the report for unstable behaviour reported by @LepkoQQ.

Input:

from another_file_aaaaaaaaaaaaaaaaaa import \
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED
from another_file_aaaaaaaaaaaaaaaaaa import some_func

First output:

from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import some_func

See how this adds a trailing comma to another_file_aaaaaaaaaaaaaaaaaa as it is beyond the max line length and now gets a trailing comma.

Second output:

from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import (
    some_func,
)

The first import statement now has a trailing comma. Because of the behaviour change and influence this now has on other import statements a second run of isort will now also add a trailing comma to the second import statement. Even though it shouldn't be added as it is well below the maximum line length.

Fixing the issue where trailing commas in one import statement influence trailing commas for different import statements (but from the same module) will fix both the originally reported issue as well as remove the instability.

See my post above for more context on why it is difficult to fix this with the current infrastructure/code.