E712: Invalid parentheses removal

[addisoncrump]

E712 fix may remove parentheses in a way which causes invalid syntax:

def a(x):
  return x == 5

if(a(2)) == True:
  print("it's five")

Becomes:

def a(x):
  return x == 5

ifa(2) is True:
  print("it's five")

Discovered by #4822.

[charliermarsh]

\cc @MichaReiser - A parenthesized node would solve this (just tabulating).

In the meantime, we'd need to rewrite with LibCST I think.

[MichaReiser]

\cc @MichaReiser - A parenthesized node would solve this (just tabulating).

In the meantime, we'd need to rewrite with LibCST I think.

Thanks for the ping. Mutating the tree instead of manually creating text edits would solve that too

[charliermarsh]

Ideally the edit would be "just" replacing == with is, rather than rewriting the entire expression 😂

[addisoncrump]

Sadly would not work for the opposite reason:

if a==True:
  ...

would become

if aisTrue:
  ...

[jasikpark]

Just repro'ed another version of this w/ #4822 in libafl mode 😁

if(True) == TrueElement or x == TrueElement:
    pass

assert (not Soo) in bar
assert {"x": not foo} in bar

cargo run --bin ruff -- --fix ./minimized-from-crash-2c30da3ded030419
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
     Running `target/debug/ruff --fix ./minimized-from-crash-2c30da3ded030419`
warning: Detected debug build without --no-cache.
error: Autofix introduced a syntax error in `minimized-from-crash-2c30da3ded030419` with rule codes E712: invalid syntax. Got unexpected token Newline at byte offset 42
---
ifTrue is TrueElement or x == TrueElement:
    pass

assert (not Soo) in bar
assert {"x": not foo} in bar

---
minimized-from-crash-2c30da3ded030419:1:4: E712 Comparison to `True` should be `cond is True` or `if cond:`
minimized-from-crash-2c30da3ded030419:1:13: F821 Undefined name `TrueElement`
minimized-from-crash-2c30da3ded030419:1:28: F821 Undefined name `x`
minimized-from-crash-2c30da3ded030419:1:33: F821 Undefined name `TrueElement`
minimized-from-crash-2c30da3ded030419:3:14: F821 Undefined name `Soo`
minimized-from-crash-2c30da3ded030419:3:22: F821 Undefined name `bar`
minimized-from-crash-2c30da3ded030419:4:18: F821 Undefined name `foo`
minimized-from-crash-2c30da3ded030419:4:26: F821 Undefined name `bar`
Found 8 errors.

[addisoncrump]

I found another, more exciting example of this; consider the following snippet:

x = range(0, 10)

def y():
  global x

  for i in x:
    if (yield i) == True:
      print("even")
    else:
      print("odd")

v = y()
next(v)

for i in range(1, 10):
  print(v.send(i % 2 == 0))

This is a really dumb program that uses generators wrong, but it gets the point across. Ruff attempts to fix this to:

x = range(0, 10)

def y():
  global x

  for i in x:
    if yield i is True:
      print("even")
    else:
      print("odd")

v = y()
next(v)

for i in range(1, 10):
  print(v.send(i % 2 == 0))

Which is invalid because the yield must be surrounded by parentheses. I think the answer to this is to simply not remove the parentheses, as it seems to be not a totally sane fix.

[dhruvmanila]

Which is invalid because the yield must be surrounded by parentheses. I think the answer to this is to simply not remove the parentheses, as it seems to be not a totally sane fix.

This seems like a regression as I can only reproduce this on v0.0.279 onwards and not on any previous versions.

[dhruvmanila]

A bisection search gives me 875e04e.

Right, so the patch generation was moved from Generator-based to Locator-based which is not aware of the parenthesis while the Generator-based is.