Skip to content

Commit

Permalink
[pylint] Re-implement unreachable (PLW0101) (#10891)
Browse files Browse the repository at this point in the history
## Summary

This PR re-introduces the control-flow graph implementation which was
first introduced in #5384, and then removed in #9463 due to not being
feature complete. Mainly, it lacked the ability to process
`try`-`except` blocks, along with some more minor bugs.

Closes #8958 and #8959 and #14881.

## Overview of Changes

I will now highlight the major changes implemented in this PR, in order
of implementation.

1. Introduced a post-processing step in loop handling to find any
`continue` or `break` statements within the loop body and redirect them
appropriately.
2. Introduced a loop-continue block which is always placed at the end of
loop blocks, and ensures proper looping regardless of the internal logic
of the block. This resolves #8958.
3. Implemented `try` processing with the following logic (resolves
#8959):
1. In the example below the cfg first encounters a conditional
`ExceptionRaised` forking if an exception was (or will be) raised in the
try block. This is not possible to know (except for trivial cases) so we
assume both paths can be taken unconditionally.
2. Going down the `try` path the cfg goes `try`->`else`->`finally`
unconditionally.
3. Going down the `except` path the cfg will meet several conditional
`ExceptionCaught` which fork depending on the nature of the exception
caught. Again there's no way to know which exceptions may be raised so
both paths are assumed to be taken unconditionally.
4. If none of the exception blocks catch the exception then the cfg
terminates by raising a new exception.
5. A post-processing step is also implemented to redirect any `raises`
or `returns` within the blocks appropriately.
```python
def func():
    try:
        print("try")
    except Exception:
        print("Exception")
    except OtherException as e:
        print("OtherException")
    else:
        print("else")
    finally:
        print("finally")
```
```mermaid
flowchart TD
  start(("Start"))
  return(("End"))
  block0[["`*(empty)*`"]]
  block1["print(#quot;finally#quot;)\n"]
  block2["print(#quot;else#quot;)\n"]
  block3["print(#quot;try#quot;)\n"]
  block4[["Exception raised"]]
  block5["print(#quot;OtherException#quot;)\n"]
  block6["try:
        print(#quot;try#quot;)
    except Exception:
        print(#quot;Exception#quot;)
    except OtherException as e:
        print(#quot;OtherException#quot;)
    else:
        print(#quot;else#quot;)
    finally:
        print(#quot;finally#quot;)\n"]
  block7["print(#quot;Exception#quot;)\n"]
  block8["try:
        print(#quot;try#quot;)
    except Exception:
        print(#quot;Exception#quot;)
    except OtherException as e:
        print(#quot;OtherException#quot;)
    else:
        print(#quot;else#quot;)
    finally:
        print(#quot;finally#quot;)\n"]
  block9["try:
        print(#quot;try#quot;)
    except Exception:
        print(#quot;Exception#quot;)
    except OtherException as e:
        print(#quot;OtherException#quot;)
    else:
        print(#quot;else#quot;)
    finally:
        print(#quot;finally#quot;)\n"]

  start --> block9
  block9 -- "Exception raised" --> block8
  block9 -- "else" --> block3
  block8 -- "Exception" --> block7
  block8 -- "else" --> block6
  block7 --> block1
  block6 -- "OtherException" --> block5
  block6 -- "else" --> block4
  block5 --> block1
  block4 --> return
  block3 --> block2
  block2 --> block1
  block1 --> block0
  block0 --> return
``` 
6. Implemented `with` processing with the following logic:
1. `with` statements have no conditional execution (apart from the
hidden logic handling the enter and exit), so the block is assumed to
execute unconditionally.
2. The one exception is that exceptions raised within the block may
result in control flow resuming at the end of the block. Since it is not
possible know if an exception will be raised, or if it will be handled
by the context manager, we assume that execution always continues after
`with` blocks even if the blocks contain `raise` or `return` statements.
This is handled in a post-processing step.

## Test Plan

Additional test fixtures and control-flow fixtures were added.

---------

Co-authored-by: Micha Reiser <[email protected]>
Co-authored-by: dylwil3 <[email protected]>
  • Loading branch information
3 people authored Jan 3, 2025
1 parent d464ef6 commit a3d873e
Show file tree
Hide file tree
Showing 25 changed files with 6,357 additions and 25 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/ruff_linter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ license = { workspace = true }
[dependencies]
ruff_cache = { workspace = true }
ruff_diagnostics = { workspace = true, features = ["serde"] }
ruff_index = { workspace = true }
ruff_notebook = { workspace = true }
ruff_macros = { workspace = true }
ruff_python_ast = { workspace = true, features = ["serde", "cache"] }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,29 @@ def func():

def func():
assert False, "oops"

def func():
y = 2
assert y == 2
assert y > 1
assert y < 3

def func():
for i in range(3):
assert i < x

def func():
for j in range(3):
x = 2
else:
assert False
return 1

def func():
for j in range(3):
if j == 2:
print('yay')
break
else:
assert False
return 1
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ def func():
if True:
break

# TODO(charlie): The `pass` here does not get properly redirected to the top of the
# loop, unlike below.
def func():
for i in range(5):
pass
Expand All @@ -54,3 +52,59 @@ def func():
else:
return 1
x = 1

def func():
for i in range(5):
pass
else:
pass

def func():
for i in range(3):
if i == 2:
assert i is not None
break
else:
raise Exception()
x = 0

def func():
for i in range(13):
for i in range(12):
x = 2
if True:
break

x = 3
if True:
break

print('hello')


def func():
for i in range(13):
for i in range(12):
x = 2
if True:
continue

x = 3
if True:
break

print('hello')


def func():
for i in range(13):
for i in range(12):
x = 2
if True:
break

x = 3
if True:
continue

print('hello')
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,25 @@ def func(self, obj: BytesRep) -> bytes:
self.error(f"can't resolve buffer '{id}'")

return buffer.data

def func(x):
if x == 1:
return 1
elif False:
return 2
elif x == 3:
return 3
elif True:
return 4
elif x == 5:
return 5
elif x == 6:
return 6

def func():
if x:
return
else:
assert x

print('pop')
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,14 @@ def func():
i = 0
i += 2
return i

def func():
with x:
i = 0
i = 1

def func():
with x:
i = 0
return 1
i = 1
111 changes: 94 additions & 17 deletions crates/ruff_linter/resources/test/fixtures/control-flow-graph/try.py
Original file line number Diff line number Diff line change
@@ -1,41 +1,118 @@
def func():
try:
...
print("try")
except Exception:
...
print("Exception")
except OtherException as e:
...
print("OtherException")
else:
...
print("else")
finally:
...
print("finally")

def func():
try:
...
except Exception:
...
print("try")
except:
print("Exception")

def func():
try:
print("try")
except:
print("Exception")
except OtherException as e:
print("OtherException")

def func():
try:
...
print("try")
except Exception:
...
print("Exception")
except OtherException as e:
...
print("OtherException")

def func():
try:
...
print("try")
except Exception:
...
print("Exception")
except OtherException as e:
...
print("OtherException")
else:
...
print("else")

def func():
try:
print("try")
finally:
print("finally")

def func():
try:
...
return 0
except:
return 1
finally:
return 2

def func():
try:
raise Exception()
except:
print("reached")

def func():
try:
assert False
print("unreachable")
except:
print("reached")

def func():
try:
raise Exception()
finally:
print('reached')
return 2

def func():
try:
assert False
print("unreachable")
finally:
print("reached")

# Test case from ibis caused overflow
def func():
try:
if catalog is not None:
try:
x = 0
except PySparkParseException:
x = 1
try:
x = 2
except PySparkParseException:
x = 3
x = 8
finally:
if catalog is not None:
try:
x = 4
except PySparkParseException:
x = 5
try:
x = 6
except PySparkParseException:
x = 7


def func():
try:
assert False
except ex:
raise ex

finally:
...
raise Exception("other")
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,39 @@ def func():
if True:
break

'''
TODO: because `try` statements aren't handled this triggers a false positive as
the last statement is reached, but the rules thinks it isn't (it doesn't
see/process the break statement).
def func():
while True:
x = 0
x = 1
break
x = 2
x = 3

def func():
while True:
x = 0
x = 1
continue
x = 2
x = 3

def func():
while True:
x = 0
x = 1
return
x = 2
x = 3

def func():
while True:
x = 0
x = 1
raise Exception
x = 2
x = 3

# Test case found in the Bokeh repository that trigger a false positive.
# Test case found in the Bokeh repository that triggered a false positive.
def bokeh2(self, host: str = DEFAULT_HOST, port: int = DEFAULT_PORT) -> None:
self.stop_serving = False
while True:
Expand All @@ -118,4 +145,3 @@ def bokeh2(self, host: str = DEFAULT_HOST, port: int = DEFAULT_PORT) -> None:
port += 1

self.thread = threading.Thread(target=self._run_web_server)
'''
Loading

0 comments on commit a3d873e

Please sign in to comment.