Black: Black inserts trailing comma after **kwargs in non-3.6 file

[dgingrich]

Operating system: macOS 10.13.6
Python version: 3.6.3
Black version: 18.6b4
Does also happen on master: yes

I'm using black --line-length=101 --safe -v to format a file that contains a snippet like below:

# No f strings or trailing commas after *, *args, or **kwarg parameters elsewhere in the file
class MyClass(BaseClass):
    def __init__(
        self,
        x,
        *args,
       # Below line is changed by black: trailing comma added
        **kwargs
    ):
        try:
            super().__init__(
                x=x
                *args,
                # ...But only if the next line also has a trialing comma
                **kwargs,
            )
        except Exception as e:
            raise

Black is always adding a trailing comma after the **kwargs in the def __init__ (below the first comment). This happens even on master and even after clearing black's cache.

A workaround is removing the trailing comma from the **kwargs in the super().__init__ call (below the second comment), even though this comma is legal in Py3.5:

$ python3.5
>>> class Base:
...     def __init__(self, *a, **ka):
...             self.a = a
...             self.ka = ka
...
>>> class Child(Base):
...     def __init__(self, *a, **ka):
...             super().__init__(
...                     *a,
...                     **ka,
...             )
...
>>> c = Child(1, 2, a=3, b=4)
>>> c
<__main__.Child object at 0x10a44e2b0>
>>> c.a
(1, 2)
>>> c.ka
{'b': 4, 'a': 3}

[zsol]

Thanks for reporting! This is clearly broken behavior on black's part. TBH I was a little surprised trailing commas are accepted after star args in python <3.6 and this complicates things a bit. Right now the simplistic verison detection used by black can only differentiate between 3.6 and non-3.6 code by (incorrectly) looking at trailing commas in declarations or function calls. Just simply looking in declarations is not enough, since black itself generates code with trailing commas in function calls, so I'll need to poke this a bit more to really fix the problem.

[asottile]

Having implemented (essentially) the same logic, trailing commas are allowed after unpackings in function calls in python 3.5+ and in function definitions in python 3.6+. The fix here is probably to add a --py35 option and detection, or to drop auto-detection based on calls altogether

[ambv]

Yeah, this is over-reaching on Black's part. We should probably stop discovering 3.6+ based on this then, which is unfortunate since in most cases by now it is a 3.6+ file which just doesn't happen to use other Python 3.6 features.

[zsol]

To demonstrate my point earlier, consider this code:

def f(
    a,
    **kwargs,
):
    return SomeLongClassName(
        the_first_parameter_is_really_long_to_really_make_sure_there_is_a_break,
        **kwargs,
     )

Today black collapses the function declaration to a single line and keeps the return statement broken over mutliple lines, and this results in a stable format (i.e. running black over the formatted output doesn't reformat it again).

If we stop discovering 3.6+ based on trailing commas in function calls, running black over the formatted output will reformat it again because black doesn't put the trailing comma in function calls pre-3.6.

Now we could solve this by introducing 3.5 detection as well. This might be worth doing in the short term, but I think it highlights a serious shortcoming in black around python version detection: if we're trying to guess the version based on observed features in the input code, we can't rely on the output code having the same features. Looks like we've been lucky so far (or I'm just discounting @ambv's brilliance - in which case I apologize ;) ) because the current rules for version detection seem stable wrt black's reformatting.

I don't have any better suggestion for how to design this better apart from having the user explicitly tell us and defaulting to the highest version black supports.

edit: Maybe we could detect various features instead of python versions. The downside: an explosion of configuration options because people will (rightly so) want to turn them on even if the input doesn't use them.

[ambv]

Our problem is that we're sniffing trailing commas after kwargs in function calls to detect 3.6+ whereas 3.5 also supports this. In this case other 3.6+ formattings kick in and code breaks.

In your example 3.6+ will be discovered due to the trailing comma in the function signature so the function call is irrelevant.

If you're trying to say that if this trailing comma would be the only 3.6-mode feature available in the file then you're right, after removing sniffing based on this, it would be removed. This is a temporary problem that applies to Python 3.5 only. That's too bad but I don't want to add a "py35" mode for this alone. Today Python 3.5 in two releases behind. It will reach end-of-life in less 2 years (September 2020). At that point Python 3.8.0 will be already a year old.

As for whether we'd like 3.7+ or 3.8+ specific formattings: there are none at the moment. However, we can use new features like from __future__ import annotations or the walrus operator to discover 3.6+ mode just fine.

[jedie]

Because of this bug, black seems to be unusable, isn't it? Is there a work-a-round or must we wait for a new release?

EDIT: Hm, ok... After black has reformat the code and add the trailing comma after **kwargs then i can remove it and black will not add it again... Maybe until black will reformat the __init__() line ?!?

[ambv]

@jedie, consider how you word your feedback, you come across as unnecessarily heated and dramatic.

This problem affects people who used a Python 3.5-specific feature in their file which was erroneously considered a Python 3.6-specific feature by Black's 3.6+ auto-detection. This does affect users and we will fix it but it does affect a very small minority of people. The tool is perfectly usable for everybody else.

[penguinolog]

Also py36 = false for pyproject.toml not changes behavior: trailing comma added and compatibility with python 3.5 is broken

[asvetlov]

Hi.
Any progress on it?
I've tried to apply black to aiohttp.
Everything works fine except Python 3.5 is broken (SyntaxError) by trailing commas.

[JelleZijlstra]

618 should fix this; I need to get back to it.

[cpcloud]

I don't think this is fixed by #618:

╭─phillip@x86_64-conda_cos6-linux-gnu  ~/code/py/ibis  ‹ibis36› ‹fix-py35*›
╰─$ cat foo.py
def my_very_long_function_with_lots_of_arguments(a, b=None, c=None, d=None, e=None, f=None, g=None, h=None, i=None, j=None, **kwargs):
    pass
╭─phillip@x86_64-conda_cos6-linux-gnu  ~/code/py/ibis  ‹ibis36› ‹fix-py35*›
╰─$ black foo.py --line-length 79 -t py35
reformatted foo.py
All done! ✨ 🍰 ✨
1 file reformatted.
╭─phillip@x86_64-conda_cos6-linux-gnu  ~/code/py/ibis  ‹ibis36› ‹fix-py35*›
╰─$ cat foo.py
def my_very_long_function_with_lots_of_arguments(
    a,
    b=None,
    c=None,
    d=None,
    e=None,
    f=None,
    g=None,
    h=None,
    i=None,
    j=None,
    **kwargs,
):
    pass

[cpcloud]

I'm going to try my hand at a PR for a fix for this. It appears that a trailing comma is allowed when splat-splatting into a callable call, but it is not allowed when defining a function whose last argument is a splat-splat argument, so the Feature.TRAILING_COMMA isn't granular enough.

[cpcloud]

Hm, I see that this distinction is now in master. Is this fix just waiting on a release?

[cpcloud]

Ah, I see that it is fixed in master here cea13f4. Thanks.

[JelleZijlstra]

@cpcloud yes, the next release will fix this!

[jakirkham]

Any news on when the next release is?

[kamahen]

I tried the following with 19.3b0 and https://black.now.sh -- here's a diff where black adds the trailing comma in a call even though there is no **kwargs):

-gzork = this_is_something( what_is_this, 123456, [a, bcd, efgs], 12345, 4423413241, something_long(), something_more())
+gzork = this_is_something(
+    what_is_this,
+    123456,
+    [a, bcd, efgs],
+    12345,
+    4423413241,
+    something_long(),
+    something_more(),
+)

[asottile]

@kamahen yes of course, black tries to add trailing commas wherever it can (except for one line calls) because trailing commas are good

This issue is about an over-zealous application due to incorrect version detection

[kamahen]

@asottile If I wanted trailing commas, I'd put them there. Reformatting tools should not change anything in my source except whitespace.

Fortunately, there's a reformatting tool that doesn't touch my source: yapf.

[asottile]

lol k

[kamahen]

BTW, trailing commas are often good. But it's my decision whether to use them. If the reformatting tool can add commas, why stop there? ... how about changing if foo != [] to if foo? That's usually an improvement and usually correct, but ....

[asottile]

black doesn't make any transformations which would change the resulting ast -- I don't see what your point is?

it's also your decision to you know, not use the tool

[kamahen]

Adding a "," changes the parse tree. I don't think a reformatting tool should change the parse tree, but reasonable people might differ.

[asottile]

Actually, no -- adding commas where they are optional doesn't change the parse tree -- black asserts this

[nok]

JFYI, I use this ugly workaround to avoid a trailing comma:

# fmt: off
**kwargs
# fmt: on

[kamahen]

adding commas where they are optional doesn't change the parse tree -- black asserts this

Could you please point me to where this is done? I suspect that it checks that the AST isn't changed, which is different from the parse tree (CST).

[asottile]

adding commas where they are optional doesn't change the parse tree -- black asserts this

Could you please point me to where this is done? I suspect that it checks that the _AST_ isn't changed, which is different from the parse tree (CST).

adding and removing whitespace changes the CST too -- what's your point?

[kamahen]

adding and removing whitespace changes the CST too -- what's your point?

No it doesn't change the CST, or at most it changes whitespace-related things such as NEWLINE and INDENT/DEDENT (I don't think it does because NEWLINE and INDENT/DEDENT appear in the grammar only outside statements, and the formatter works at the level of statements).

[asottile]

adding and removing whitespace changes the CST too -- what's your point?

No it doesn't change the CST, or at most it changes whitespace-related things such as NEWLINE and INDENT/DEDENT (I don't think it does because NEWLINE and INDENT/DEDENT appear in the grammar only outside statements, and the formatter works at the level of statements).

hmmm, it absolutely changes the CST as the positions of the nodes are adjusted when inserting or removing space and NL / NEWLINE tokens are inserted -- the same is true for the other transformations that black does (parentheses insertion for tuples / line contiuations, removal of backslashes, adjustment of indentation, block spacing, comment reflowing, call reflowing, etc. etc.) -- but none of these change the resulting AST (unless there's a bug like the actual issue in this ticket that's been hijacked)

[kamahen]

I think we have confusion about what "it" is.

I'm saying that adding/removing whitespace doesn't change the CST (I'm not 100% sure of this; it might be the case that NEWLINEs get added/deleted). Adjustment of indentation, block spacing, comment reflowing, call reflowing are examples of these.

Other changes you mention (parenthesis insertion, adding commas) are things that do change the CST.

And I'm saying I want a way to tell Black that I don't want it to change my CST. (Which is the philosophy of yapf.)

[asottile]

Make an issue then, though it'll almost certainly get closed

[zsol]

Black checks the AST is equivalent before and after formatting, but makes no such promise about "the" CST, nor will it ever. No code formatter does AFAIK, including yapf.

Please keep the discussion in this thread focused on the issue.

[ambv]

@kamahen, raising related but separate problems on closed issues is bad form. Regardless, you are arguing against documented behavior.

Zsolt is right that no formatter, including YAPF, leaves the CST intact. We are in fact using the same CST under the hood so no formatting would be possible if we couldn't mutate the tree.

[kamahen]

My apologies. I found this discussion through a search and didn't notice that it was closed. I only wanted to add what I thought was a related example, but allowed myself to be side-tracked into a religious issue. Again my apologies.

[jonasrauber]

Could you please make a new release that includes this bugfix. The current version 19.3b0 is from before this issue was fixed.

[jonasrauber]

Thanks, the 19.10b0 release solves this