Comprehensions should not split between tuple-unpacking commas

[sobjornstad]

split_all_top_level_comma_separated_values inserts spurious breaks between commas in tuple-unpacking expressions in comprehensions.

Hand-formatted code:

results = {
    field_name: func(field_value)
    for field_name, field_value in self.fields.items()
}

YAPF changes this to:

results = {
    field_name: func(field_value)
    for field_name,
    field_value in self.fields.items()
}

The third line is short enough to fit within the line limit without breaking, and putting a break there decreases readability. I can't see any logical reason there should be a break there.

If I turn off split_all_top_level_comma_separated_values, YAPF does not change my hand-formatted code.

Full configuration:

[style]
based_on_style = pep8
column_limit = 88
dedent_closing_brackets = False
spaces_before_comment = 2
split_all_top_level_comma_separated_values = True
split_before_arithmetic_operator = True
split_before_logical_operator = True
split_complex_comprehension = True

[sobjornstad]

I just noticed the same thing happens with lambda expressions. YAPF formatted code:

self.add_field('quantity',
               'BatteryPackage',
               default=8,
               validator=lambda field,
               asset: field.value <= 16)

[sergiogiro]

I'd like to comment that this also happens with split_all_comma_separated_values (without the top_level), so it's not specific to the knob split_all_top_level_comma_separated_values.

[rgbmrc]

Still present in v0.31.0. The same goes for lambdas in dictionaries, see #662

Summarizing, with split_all_[top_level_]comma_separated_values

func_called_with_lambda_args('some long string',
                             'some very long string',
                             lambda arg1, arg2: arg1 + arg2)

dict_with_lambdas = {
    'difference': lambda val1, val2: val1 - val2,
    lambda val1, val2: val1 * val2: 'product'
}

dict_comprehension_with_unpacking = {
    field_name: func(field_value)
    for field_name, field_value in fields.items()
}

is formatted to

func_called_with_lambda_args('some long string',
                             'some very long string',
                             lambda arg1,
                             arg2: arg1 + arg2)

dict_with_lambdas = {
    'difference': lambda val1,
    val2: val1 - val2,
    lambda val1,
    val2: val1 * val2: 'product'
}

dict_comprehension_with_unpacking = {
    field_name: func(field_value)
    for field_name,
    field_value in fields.items()
}

[alexey-pelykh]

Re-surfacing this via #1159 as well. Since this issue is there for years, it seems that there's no solid opinion on how it's supposed to work?

[robertwb]

I think it's clear what the intent is: tuples that are part of a unit for unpacking purposes (and arguably lambda arguments fall in the same boat) should bind more tightly. That's not saying it's clear what the implementation should be.

[alexey-pelykh]

@robertwb I don't think that's the case since if we take a look at SPLIT_ALL_COMMA_SEPARATED_VALUES definition, it clearly says:

If a comma separated list (dict, list, tuple, or function def) is on a line that is too long

The current implementation neither checks for length, or for node type: https://github.com/google/yapf/blob/64f8b1beee15eba583235ee9a679f62e0e8530f9/yapf/yapflib/format_decision_state.py#L182-L183 so that

values = [
  lambda a, b: a + b,
]

becomes

values = [
    lambda a,
    b: a + b,
]

So if the implementation was at least somehow close to the description, it would seem that the issue were less severe if existed at all.

[bwendling]

Agreed that this is gross. The knob needs to be relaxed a bit to avoid splitting in such cases...I'll see what can be done.

[bwendling]

Could you try this patch to see if it helps and doesn't have any bad side effects?

$ git diff
diff --git a/yapf/pytree/subtype_assigner.py b/yapf/pytree/subtype_assigner.py
index 05d88b0fc603..e3b32777aee8 100644
--- a/yapf/pytree/subtype_assigner.py
+++ b/yapf/pytree/subtype_assigner.py
@@ -222,6 +222,11 @@ class _SubtypeAssigner(pytree_visitor.PyTreeVisitor):
       if isinstance(child, pytree.Leaf) and child.value == '**':
         _AppendTokenSubtype(child, subtypes.BINARY_OPERATOR)
 
+  def Visit_lambdef(self, node):  # pylint: disable=invalid-name
+    # trailer: '(' [arglist] ')' | '[' subscriptlist ']' | '.' NAME
+    _AppendSubtypeRec(node, subtypes.LAMBDEF)
+    self.DefaultNodeVisit(node)
+
   def Visit_trailer(self, node):  # pylint: disable=invalid-name
     for child in node.children:
       self.Visit(child)
diff --git a/yapf/yapflib/format_decision_state.py b/yapf/yapflib/format_decision_state.py
index bc7f977a79a0..8b38d09cdd3c 100644
--- a/yapf/yapflib/format_decision_state.py
+++ b/yapf/yapflib/format_decision_state.py
@@ -201,6 +201,10 @@ class FormatDecisionState(object):
         # the current line.
         return False
 
+      if (subtypes.COMP_FOR in current.subtypes or
+          subtypes.LAMBDEF in current.subtypes):
+        return False
+
       # Allow the fallthrough code to handle the closing bracket.
       if current != opening.matching_bracket:
         # If the container doesn't fit in the current line, must split
diff --git a/yapf/yapflib/subtypes.py b/yapf/yapflib/subtypes.py
index b4b7efe75271..3c234fbfb500 100644
--- a/yapf/yapflib/subtypes.py
+++ b/yapf/yapflib/subtypes.py
@@ -38,3 +38,4 @@ TYPED_NAME_ARG_LIST = 21
 SIMPLE_EXPRESSION = 22
 PARAMETER_START = 23
 PARAMETER_STOP = 24
+LAMBDEF = 25

[alexey-pelykh]

@bwendling thanks for the diff! My test-case remained unaffected by the change, I'll debug into that.

Update: Ah, I see - a bit of code reshuffling is needed, PR coming soon.