Fix edge cases triggered by newlines in arrow functions

[vjeux]

This one is pretty crazy. In #927, I changed

concat(["(", join(concat([", "]), printed), ")"]),

into

concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),

which makes the example in #1203 look ugly. The crazy thing is that JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded). So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for traverseInDoc() is findInDoc(), right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for findInDoc() but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix #1203

[jlongster]

Yeah, this is a good solution. I had deferred thinking about this and never got around to it. It was a known issue, and it's hard to know if we can traverse all the conditions or not. But here this should work, because as you said it doesn't propagate them across the conditional group anyway, so it doesn't affect anything outside the group.

findInDoc is different. willBreak should tell you statically if something is going to break (which uses findInDoc). How do you know which condition is going to be chosen at compile time? You don't, so you can't just traverse in all of the conditions looking for a breakParent, there'd be way too many false positives. It was intentional to only search the first condition because the idea was that it was the most collapsed form, and if that breaks, than all of them probably are. But I think you're right; the gist is that breakParent that inside a conditional group doesn't really make much sense anyway, since we don't propagate it.

So willBreak doesn't need to descend into conditional groups, but getFirstString probably should read the first group. Yes, it's a little arbitrary, but it should always return the next real string (and the actual strings are probably the same in all conditions of a group).

I changed it to not search on any and none of the tests are failing

I don't see that change in the PR? getFirstString should still traverse...

I tried to go through all the conditions for findInDoc() but it triggers a few failures

What do you mean by conditions?

[vjeux]

I don't call traverseDocRec(doc.contents); if we're not in shouldTraverseConditionalGroups

[jlongster]

But it traverses into contents in a later branch which is the first group of a conditional group

[jlongster]

Just to clarify, as my above comment is wrong, I see that it will enter only this branch if it's a conditional group. But expandedStates are all of the conditions, including the first one. contents happens to point to the first one for convenience.

[vjeux]

getFirstString should still traverse...

Here are the only three callsites of getFirstString:

function isCurlyBracket(doc) {
  const str = getFirstString(doc);
  return str === "{" || str === "{}";
}

function isEmptyBlock(doc) {
  const str = getFirstString(doc);
  return str === "{}";
}

case "BlockStatement": {
  var naked = path.call(function(bodyPath) {
    return printStatementSequence(bodyPath, options, print);
  }, "body");
  const hasContent = getFirstString(naked);

Do you know why those are reading the docs instead of checking the AST?

[jlongster]

Do you know why those are reading the docs instead of checking the AST?

I think it's just more straight-forward. But if there aren't enough uses of it we can try that.

[vjeux]

I removed isCurlyBracket and isEmptyBlock in #1221. I'm looking into hasContent now.

[vjeux]

Turns out, removing the last callsite of getFirstString was trivial: #1222

So, we can kill getFirstString (after those two PRs are merged) and not worry about this edge case in findInDoc :)

[vjeux]

I tried to go through all the conditions for findInDoc() but it triggers a few failures
What do you mean by conditions?

All the expandedStates. Basically, not adding a flag and always iterating through all the expandedStates.

[jlongster]

With your other PRs merged, are you going to get rid of findInDoc and make traverseDoc always recurse into conditional groups?

[vjeux]

I just got rid of getFirstString. The other two calls of findInDoc are isLineNext which is used by JSX and willBreak which is used everywhere. Trying to change them to recurse through all the conditional groups end up breaking a few tests, so I can't do that.

https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

[vjeux]

One thing I can do is to leave the behavior of going through the first one. This way it's a less risky change.