Style/AndOr gets confused about autocorrect

[vrthra]

Style/AndOr gets confused when presented with a fragment such as

def z(a,b)
  return true unless a or b
end

And refuses to autocorrect, while happly autocorrects if we pass in

return true unless a or b

It appears that the parser parses a || b as

(or
  (lvar :a)
  (lvar :b))

while it parses the a or b in the above expression as

(or
  (send nil :a)
  (send nil :b))

On the other hand, if I change the definition to def z, the parsed expressions are for a or b

(or
  (send nil :a)
  (send nil :b))

and a || b

(or
  (send nil :a)
  (send nil :b))

However, I don't see why having a def of the variables should result in a different result.

[vrthra]

I think perhaps the autocorrect should look at both possibilities before deciding to fail

[jonas054]

I agree. There's something not quite right here. I think the problem is that AutocorrectUnlessChangingAST#autocorrect compares the rewritten a || b, parsed individually, to the a or b that was parsed in the context of the def z(a, b). That's why the AST is different. Maybe that's what you said @vrthra, but I'm restating it for my own understanding.

Anyway it seems that a bit of context is needed when parsing a || b.

[vrthra]

@jonas054 Yes, it seems that the parser interprets the a and b as variables when it is under def z(a,b) and as method calls otherwise. Is there a way we can get the parent nodes of an expression? If not, one option could be to replace all the (lvar _) to (send nil _) and then compare? Because the results of replacement are not dependent on whether the tokens represent variables or not.

[jonas054]

The code that evaluates whether the AST has changed is in AutocorrectUnlessChangingAST and should be generic and usable for any cop that wants this feature. I don't feel comfortable fiddling with the AST there before the comparison. Who knows what bugs that could introduce in the future?

No, it's better to compare a larger scope, as you also suggest. I think we can find a way to do that, even though there is no direct reference to the parent node in each node.

[bbatsov]

I think we can find a way to do that, even though there is no direct reference to the parent node in each node.

Here you go, sir - whitequark/parser#111.

[jonas054]

@bbatsov Thanks! I still had problems crafting an acceptable solution, even with that bit of help. Luckily I came up with a quite simple fix that we can use instead. Today, we're creating the new source code, build the AST and compare it against the existing node. This doesn't always work, as @vrthra noticed, becuase the existing node is affected by its context. So instead of trying to replicate that context for the rewritten code, we remove it for the existing node. You'll see the details in an upcoming pull request.