Black moves comments to wrong place

[leos]

Describe the bug A clear and concise description of what the bug is.
Black moves comments from their lines to a spot where they don't make sense.
To Reproduce Steps to reproduce the behavior:

Start with this code:

INFO = [
    {
        'a': 'advertisers',
        'b': [
            {'active': [0, 1]}, # All non-deleted rows
            {'active': -1}      # All deleted rows
        ],
        'c': 'd',
    },
]

2. Run Black on it: black -t py37 -S
3. Results in:

INFO = [
    {
        'a': 'advertisers',
        'b': [
            {'active': [0, 1]},
            {'active': -1},
        ],  # All non-deleted rows  # All deleted rows
        'c': 'd',
    },
]

Expected behavior A clear and concise description of what you expected to happen.
Trailing comma added, comment location preserved

Environment (please complete the following information):

• Version: 19.10b0
• OS and Python version: Ubuntu 18.04 & Python 3.7.5

Does this bug also happen on master? To answer this, you have two options:
Strangely, this does not repro on either master or stable on https://black.now.sh/ :-/

Additional context Add any other context about the problem here.

[hugovk]

Duplicate of #379?

[leos]

@hugovk I'm not sure that it's the same issue as Black didn't reformat the lines at all and just moved the comments. The other strange thing is that in our code this block is in a list of very similar blocks and the comments get moved differently in different blocks - which makes me suspect there's something non-deterministic or some sort of state bug happening.

[ichard26]

I cannot reproduce the bug when running Black as you commented... but

Interestingly, I can reproduce this weird bug when running Black on the same file twice with different options supplied. The first one as black -t py37 -S -l 94 and the second as black -t py37 -S.

A few things I noted of this specific chain of Black calls:

1. For the first run, --line-length needs to be >93.
2. Also for the first run, --diff can't be used
3. The value for --line-length must be really specific for the bug to show up.

Here's a few more command chains that reproduce the bug...

black -t  py37  -S -l 100
black -t py37 -S

black -t  py37  -S -l 100
black -t py37 -S -l 90

black -t py37 -S
black -t  py37  -S -l 100
black -t py37 -S 90

IMO, I think its something relating to how Black behaves when ran on the same file multiple of times over... maybe something relating to caching? I could be very wrong though since I am not knowledgeable on how Black works. Feel free to ask for more info though.

Environment:

• Version: 19.10b0
• OS and Python version: Windows 10 Home Build 18363 & Python 3.8.1

[leos]

Oh, wow, thank you @ichard26 ! Amazing detective work - that's actually what I had done, I was playing around with different line lengths and rerunning Black to see what it would output and ran across this issue as I was spot checking. Didn't even realize Black maintained a cache.

[ichard26]

Actually, on further reflection and testing, I think that the cache has nothing to do with the bug. Even when I do:

black -t py37 -S -l 94
black -t py37 -S

and deleting the cache files myself in between the first and second run, the bug still appears. I think that the real bug is when running Black again on the example which you get after black -t py37 -S -l 94:

INFO = [
    {
        'a': 'advertisers',
        'b': [{'active': [0, 1]}, {'active': -1},],  # All non-deleted rows  # All deleted rows
        'c': 'd',
    },
]

Also though, I am pretty sure that the example above is related to #1353. So a bug on an edge case (I am pretty sure Black doesn't merge comments)...

[JelleZijlstra]

The current behavior here is entirely reasonable:

 % black --diff ~/py/tmp/info.py -S -t py37 --preview
--- /Users/jelle/py/tmp/info.py	2023-04-29 02:14:39.326010 +0000
+++ /Users/jelle/py/tmp/info.py	2023-04-29 02:15:05.270084 +0000
@@ -1,10 +1,10 @@
 INFO = [
     {
         'a': 'advertisers',
         'b': [
-            {'active': [0, 1]}, # All non-deleted rows
-            {'active': -1}      # All deleted rows
+            {'active': [0, 1]},  # All non-deleted rows
+            {'active': -1},  # All deleted rows
         ],
         'c': 'd',
     },
 ]

@ichard26 mentioned above that the comments get squashed together if you first run Black with -l 100 and then don't get moved back, but I think that's reasonable behavior: if you set -l 100, Black tries to move the comments together, and then if you run it with a shorter line length again, it doesn't know how to move the comments where they came from.

[leos]

Thanks for cleaning up this ticket @JelleZijlstra. What you're saying makes sense and I'm sure that there's a lot of other cases that led to the decision to merge comments in Black like that, but here's a way that I think this behavior may not be as reasonable:

[
            {'active': [0, 1]},  # DO NOT CHANGE
            {'active': -1},  # This is ok to modify
]

Then reformatting that to:

            [{'active': [0, 1]}, {'active': -1}] # DO NOT CHANGE This is ok to modify

could lead to confusion.

The two lines of comments could be parts of a multiline comment (which is what I'm guessing led to the behavior of merging them) but if they're two separate lines/sentences/sentence fragments merging them carries that risk.

[JelleZijlstra]

I'd actually be OK with never merging comments, as it can definitely lead to confusing output. That should be a new issue, though, and I'm not sure how easy it would be to implement.