Stability fixup: address stability issues relating to interaction between magic commas and invisible parentheses

[jayaddison]

This changeset addresses some stability issues encountered during multi-pass code formatting by black.

In particular, the changeset modifies handling of magic trailing commas within code lines. The most significant change is that the generate_trailers_to_omit method will delay or suppress yielding of an empty trailer (leaf set) when a magic comma is present in the line.

May resolve #1629.

NB: Other time commitments may mean that I won't be committing much additional code here (or be able to re-investigate the details) within the next few weeks. I'll do my best to respond to any feedback, suggestions and ideas; and if anyone's eager to, please feel free to take any of the code from this branch and develop / fix it further.

Edit 20210312: added note from #1958 (comment)

[jayaddison]

@ambv Apologies to ping directly, you might receive a lot of notifications. I think there's something of interest to you here though.

It's almost possible to make the maybe_make_parens_invisible_in_atom method generalizable by replacing checks for node.type == syms.atom with isinstance(node, Node). At which point it should perhaps be renamed maybe_make_parens_invisible_in_node.

This would allow simplifying the normalize_invisible_parens loop and conditionals.

There's a catch somewhere, though. It has to do with some of the can_omit_invisible_parens logic, and in particular it seems to have difficulty rewriting a line where a trailing comma has been added in the example test data provided in this pull request.

[JelleZijlstra]

I was very excited when I saw the PR title; I think this change may also fix many of the issues mentioned in #1629.

[jayaddison]

It seems similar, in that it does relate to the introduction/removal of invisible parentheses.

In the example I've been looking at locally for this branch, the calls to ensure_visible seem to be where the parentheses are re-surfaced during the second formatting pass.

One question is whether invisible parens should have been introduced at all (they do seem valid for return and assert statements though, I think?); another is whether they could have been valid for omission. Is is the latter case where the introduction of a trailing comma to the function argument list (arg1, arg2 -> arg1, arg2,) leads to an unexpected change in the last token here.

[jayaddison]

NB: Although the black-primer tests are failing for sqlalchemy, from build log inspection I think the reformatting of the code that's being applied is more concise and an improvement (it largely relates to assert statements, and in particular omission of optional parentheses for those).

[mvolfik]

You're welcome. However, while triaging, I found one file that doesn't work even after this patch: whs/runekit@67ce199:runekit/app/view/browser_window.py

Black log

Mode(target_versions=set(), line_length=88, string_normalization=True, experimental_string_processing=False, is_pyi=False)
--- source
+++ first pass
@@ -112,14 +112,14 @@
         ):
             # FIXME: This doesn't really work - PySide2 doesn't have QWebEngineNotification
             self.browser.page().setFeaturePermission(
                 origin, feature, QWebEnginePage.PermissionGrantedByUser
             )
-        elif (
-            feature in (QWebEnginePage.DesktopVideoCapture, QWebEnginePage.DesktopAudioVideoCapture)
-            and self.app.has_permission('pixel')
-        ):
+        elif feature in (
+            QWebEnginePage.DesktopVideoCapture,
+            QWebEnginePage.DesktopAudioVideoCapture,
+        ) and self.app.has_permission("pixel"):
             self.browser.page().setFeaturePermission(
                 origin, feature, QWebEnginePage.PermissionGrantedByUser
             )
         else:
             self.browser.page().setFeaturePermission(
--- first pass
+++ second pass
@@ -112,14 +112,18 @@
         ):
             # FIXME: This doesn't really work - PySide2 doesn't have QWebEngineNotification
             self.browser.page().setFeaturePermission(
                 origin, feature, QWebEnginePage.PermissionGrantedByUser
             )
-        elif feature in (
-            QWebEnginePage.DesktopVideoCapture,
-            QWebEnginePage.DesktopAudioVideoCapture,
-        ) and self.app.has_permission("pixel"):
+        elif (
+            feature
+            in (
+                QWebEnginePage.DesktopVideoCapture,
+                QWebEnginePage.DesktopAudioVideoCapture,
+            )
+            and self.app.has_permission("pixel")
+        ):
             self.browser.page().setFeaturePermission(
                 origin, feature, QWebEnginePage.PermissionGrantedByUser
             )
         else:
             self.browser.page().setFeaturePermission(

[mvolfik]

Oh sorry, nevermind, I got lost in the black installation virtualenvs. That file works too

[jayaddison]

No worries, thank you again - both successful confirmations, and remaining failure cases are good to know about.

I do have a remaining concern that although the changes appear to work in most cases, they also allow exceeding the line-length in some situations, and that might or might not be acceptable to the project team. We'll see.

[mvolfik]

Ok, this time I really got something:

def f():
    def f():
        def f():
            optr = self._hptr + {ps.icmp6.ROUTER_SOLICITATION: 12, ps.icmp6.ROUTER_ADVERTISEMENT: 16, ps.icmp6.NEIGHBOR_SOLICITATION: 24, ps.icmp6.NEIGHBOR_ADVERTISEMENT: 24}[self.type]

Black log, identical on master and this patch

Mode(target_versions=set(), line_length=88, string_normalization=True, magic_trailing_comma=True, experimental_string_processing=False, is_pyi=False)
--- source
+++ first pass
@@ -1,4 +1,12 @@
 def f():
     def f():
         def f():
-            optr = self._hptr + {ps.icmp6.ROUTER_SOLICITATION: 12, ps.icmp6.ROUTER_ADVERTISEMENT: 16, ps.icmp6.NEIGHBOR_SOLICITATION: 24, ps.icmp6.NEIGHBOR_ADVERTISEMENT: 24}[self.type]
+            optr = (
+                self._hptr
+                + {
+                    ps.icmp6.ROUTER_SOLICITATION: 12,
+                    ps.icmp6.ROUTER_ADVERTISEMENT: 16,
+                    ps.icmp6.NEIGHBOR_SOLICITATION: 24,
+                    ps.icmp6.NEIGHBOR_ADVERTISEMENT: 24,
+                }[self.type]
+            )
--- first pass
+++ second pass
@@ -1,12 +1,9 @@
 def f():
     def f():
         def f():
-            optr = (
-                self._hptr
-                + {
-                    ps.icmp6.ROUTER_SOLICITATION: 12,
-                    ps.icmp6.ROUTER_ADVERTISEMENT: 16,
-                    ps.icmp6.NEIGHBOR_SOLICITATION: 24,
-                    ps.icmp6.NEIGHBOR_ADVERTISEMENT: 24,
-                }[self.type]
-            )
+            optr = self._hptr + {
+                ps.icmp6.ROUTER_SOLICITATION: 12,
+                ps.icmp6.ROUTER_ADVERTISEMENT: 16,
+                ps.icmp6.NEIGHBOR_SOLICITATION: 24,
+                ps.icmp6.NEIGHBOR_ADVERTISEMENT: 24,
+            }[self.type]

From #1900

[jayaddison]

Good find, thanks @mvolfik. I'll look into that case soon.

[jayaddison]

@mvolfik Just a note to let you know that I've taken a brief look at the test case you found - it's definitely valid, and I think it's possible that it exposes some kind of serious/core problem with this branch as it stands. I've experimented with a few adjustments to try to handle it, but overall I still don't yet fully understand the root problem in the parentheses/magic comma interaction, and I think that will be required for a complete fix.

[jayaddison]

Other time commitments may mean that I won't be committing much additional code here (or be able to re-investigate the details) within the next few weeks. I'll do my best to respond to any feedback, suggestions and ideas; and if anyone's eager to, please feel free to take any of the code from this branch and develop / fix it further.

[ambv]

@jayaddison, thanks for the tremendous amount of work you did on this PR. It only goes to show that the optional parentheses + optional trailing commas + magic trailing commas are a killer combination and it's impractical to solve the instability entirely, at least without introducing disruptive formatting changes in the process.

I decided to go with #2126 a simpler, if less elegant, approach to this problem.

[jayaddison]

Glad to have helped, @ambv; it was a good learning exercise - I think my favourite finding as a result was discovering the term trivia in relation to ASTs.

I would definitely concur that something that becomes this puzzling is a difficult combination; if there's a way to update the codebase to simplify some of that in future -- potentially by reverting and backtracking on a few features like the ones you mention -- perhaps that'd help improve long-term maintainability.