Latest `black` changes ASTs

[godlygeek]

Describe the bug

The README says:

Also, as a temporary safety measure, Black will check that the reformatted code still produces a valid AST that is equivalent to the original. This slows it down. If you're feeling confident, use --fast.

But after #1740, docstrings beginning with a double quote have an extra space prepended to them. This produces a different AST than the file had before, but --safe doesn't complain about this.

Is Black no longer promising to produce identical ASTs?

To Reproduce

$ cat test.py
def foo():
    '''"something" is what this does.'''


print(foo.__doc__)
$ python test.py
"something" is what this does.
$ black --safe test.py
reformatted test.py
All done! ✨ 🍰 ✨
1 file reformatted.
$ cat test.py
def foo():
    """ "something" is what this does."""


print(foo.__doc__)
$ python test.py
 "something" is what this does.
$

Expected behavior:

I believe that black is never supposed to change the AST, and therefore rewriting the docstring to contain this extra space is not a valid transformation for black to perform.

Environment:

• Version: black, version 21.4b0
• OS and Python version: Linux, Python 3.8.8

Does this bug also happen on master?

Yes

Additional context

$ python -m ast test.py >ast1.out
$ black test.py
reformatted test.py
All done! ✨ 🍰 ✨
1 file reformatted.
$ python -m ast test.py >ast2.out
$ diff -U0 ast1.out ast2.out
--- ast1.out    2021-04-26 19:05:58.174110800 -0400
+++ ast2.out    2021-04-26 19:08:57.324110800 -0400
@@ -13 +13 @@
-               value=Constant(value='"something" is what this does.'))],
+               value=Constant(value=' "something" is what this does.'))],

[JelleZijlstra]

Black does change the AST in a few places. This was so even before the most recent release; the previous release already made some changes to docstrings. We should update that sentence in the documentation.

[godlygeek]

That's disappointing. The fact that I could trust Black to never change the behavior of a program was one of its greatest selling points, and made me feel comfortable applying Black indiscriminately without needing to check its work.

Now that I know it may change my program's behavior, I'll be much more cautious applying it to new projects.

[JelleZijlstra]

We do still have the safety check (in the _stringify_ast function), but it allows some changes in the AST. The most important one is that it allows changes in whitespace (only) within strings: before comparing strings it strips all whitespace from the beginning and end of each line in the string. There is another allowed change in del statements: del a, b and del (a, b) are equivalent but have different ASTs, and Black sometimes changes one to the other.

[godlygeek]

Well, I'm not sure what to say beyond that I'm very dismayed to learn this. The fact that Black would prove that it did not change my program's behavior was an important - probably the most important - feature to me, and I'm disappointed to hear that it has been lost.

Tools like Sphinx read docstrings, and now I can't know whether running Black on my library has changed what Sphinx will generate without diffing and checking its work. That's a really serious loss. Better formatting is not a win if I can't trust that it didn't break anything in the process.

[JelleZijlstra]

That's a fair point! We at least should have updated the documentation, and I would consider adding an option to disable docstring formatting.

[ambv]

Do you have a concrete problem with the changes to docstrings or are you here only to voice your disappointment and dismay?

The changes made to docstrings are pretty conservative and have to do with indentation and leading/trailing whitespace.
Re-aligning docstrings when re-indenting code was one of the most requested features of Black. How does this break Sphinx?

[godlygeek]

Do you have a concrete problem with the changes to docstrings or are you here only to voice your disappointment and dismay?

I came here because I noticed that the new version of Black had made a change that resulted in a different AST, which as far as I could tell from the docs and from a PyCon lightning talk I remembered, it's never supposed to do.

I originally thought this was just a bug, but after finding the 21.4b0 changelog I saw that this was intentional, and @JelleZijlstra has clarified that this wasn't the first time - other AST changes have been allowed before, and --safe has been changed from checking whether the AST is the same to checking if it's similar.

When I learned that, I switched to expressing my disappointment and dismay at a feature I consider so important being quietly lost. I have been confident using and advocating for Black because I could trust that it would never change a module's behavior. This lets me apply it indiscriminately to any project without checking its work, since it would check for me that the AST was the same and therefore the behavior almost certainly will be as well. But above, I've got a 3 line program that exhibits different behavior before and after running Black on it. I find this scary.

How does this break Sphinx?

I didn't say that this would break Sphinx, but I said that it could change what Sphinx will generate. After some experimentation, it doesn't seem to - it seems as though Sphinx is stripping the leading whitespace before rendering the docstring to HTML or to LaTeX. I didn't know that going into this. I'm still not entirely convinced that there isn't some Sphinx plugin whose behavior could change because it sees the docstring in an intermediate state before the leading whitespace has been stripped, though.

[jaklan]

@godlygeek But above, I've got a 3 line program that exhibits different behavior before and after running Black on it. I find this scary. - as for now you haven't proved any different --> behaviour <--. If there's no case when additional leading whitespace changes anything - it means the behaviour is the same (even if AST not).

[godlygeek]

as for now you haven't proved any different --> behaviour <--.

The user-observable behavior of that program is that something is printed to stdout. The thing that's printed to stdout is different after running Black.

[godlygeek]

Another example of somewhere where this might cause issues is PLY, which has a parsing DSL that uses docstrings as regular expressions to match tokens - the addition of an extra leading space to a PLY token regex like:

def t_DOUBLE_QUOTED_STRING(t):
    '"[^"]*"'
    t.value = t.value[1:-2]
    return t

would absolutely change the program's behavior.

[jaklan]

@godlygeek I could argue about the first point, but definitely not about the latter. But actually... it's not the case, because this docstring is not reformatted. And playing a bit more - it seems to be even more strange:

For:

def t_DOUBLE_QUOTED_STRING(t):
    '"[^"]*"'
    t.value = t.value[1:-2]
    return t
    
def foo():
    '''"something" is what this does.'''
    pass
    
    
def foo2():
    ''' "something" '''
    pass
    
def foo3():
    """ 'something' is what this does."""
    pass

we get:

def t_DOUBLE_QUOTED_STRING(t):
    '"[^"]*"'
    t.value = t.value[1:-2]
    return t


def foo():
    """ "something" is what this does."""
    pass


def foo2():
    '''"something"'''
    pass


def foo3():
    """'something' is what this does."""
    pass

For foo2 and foo3 the implicit trailing whitespace is removed, and for foo2 quotes are also not normalised...
I can understand why it's removed - readability of """" vs """' (however it's arguable if the difference is worth of such a inconsistency), however the lack of quotes normalisation is surprising for me.

Playground with the code is here.

[jaklan]

@godlygeek to be precise - your example should look this way (in yours we don't have docstrings at all):

def t_LEFT_QUOTED_STRING(t):
    """"[^"]*"""
    t.value = t.value[1:-2]
    return t

and output:

def t_LEFT_QUOTED_STRING(t):
    """ "[^"]*"""
    t.value = t.value[1:-2]
    return t

[godlygeek]

in yours we don't have docstrings at all

Well, we do have a docstring, just not a triple-quoted string - but fair enough; I didn't realize that single quoted strings wouldn't be affected by this, but should have. Thanks for the correction!

[ichard26]

For foo2 and foo3 the implicit trailing whitespace is removed, and for foo2 quotes are also not normalised...

There's a lot of bugs / edge cases with docstring handling so I'm guessing this is one of them :( Consistency is definitely lacking right now.

edit: most of them have probably been reported, but if you do find a new one, feel free to report it! (I was just clarifying that the behaviour noted above isn't expected or anything)

edit 2: Łukasz has corrected me that none of the examples above are in fact bugs, I clearly didn't know what I was thinking so I'm hiding this to avoid spreading incorrect information.