[internal] Return statements in finally block point to end block for unreachable (PLW0101)#15276
[internal] Return statements in finally block point to end block for unreachable (PLW0101)#15276dylwil3 merged 3 commits intoastral-sh:mainfrom
internal] Return statements in finally block point to end block for unreachable (PLW0101)#15276Conversation
|
|
Is this overlapping with #15278 or do they fix different issues? |
|
Yes they are attempting to fix the same issue. Technically, this PR is more of a band-aid because the CFG should not fall through to the finally block in the |
|
I agree we should apply both corrections to the CFG construction, but I think we should keep the rule as "test" for now. I'm not so worried about discovering more edge cases where the CFG needs improvements, that's fine. However, infinite loops or panics are more disruptive since they cause the whole |
|
Thanks for the explanation. What's the CFG if we wrap the |
Like this, which looks correct to me: def l():
while T:
try:
try:
while ():
if 3:
break
finally:
return
finally:
returnflowchart TD
start(("Start"))
return(("End"))
block0[["`*(empty)*`"]]
block1[["Loop continue"]]
block2["return\n"]
block3["return\n"]
block4[["Loop continue"]]
block5["break\n"]
block6["if 3:
break\n"]
block7["while ():
if 3:
break\n"]
block8[["Exception raised"]]
block9["try:
while ():
if 3:
break
finally:
return\n"]
block10[["Exception raised"]]
block11["try:
try:
while ():
if 3:
break
finally:
return
finally:
return\n"]
block12["while T:
try:
try:
while ():
if 3:
break
finally:
return
finally:
return\n"]
start --> block12
block12 -- "T" --> block11
block12 -- "else" --> block0
block11 -- "Exception raised" --> block10
block11 -- "else" --> block9
block10 --> block2
block9 -- "Exception raised" --> block8
block9 -- "else" --> block7
block8 --> block3
block7 -- "()" --> block6
block7 -- "else" --> block3
block6 -- "3" --> block5
block6 -- "else" --> block4
block5 --> block3
block4 --> block7
block3 --> block2
block2 --> return
block1 --> block12
block0 --> return
|
| } | ||
| // re-route to finally if present and not already re-routed | ||
| if let Some(finally_index) = finally_index { | ||
| blocks.blocks[idx].next = NextBlock::Always(finally_index); |
There was a problem hiding this comment.
This would probably be a small refactor and is something better left for its own PR but we could have better runtime assertions around preventing loops by introducing a blocks.set_next(idx, next) method that contains an assertion that idx and the next's index aren't equal.
* main: [`pylint`] Fix `unreachable` infinite loop (`PLW0101`) (#15278) fix invalid syntax in workflow file (#15357) [`pycodestyle`] Avoid false positives related to type aliases (`E252`) (#15356) [`flake8-builtins`] Disapply `A005` to stub files (#15350) Improve logging system using `logLevel`, avoid trace value (#15232) [`flake8-builtins`] Rename `A005` and improve its error message (#15348) Spruce up docs for pydoclint rules (#15325) [`flake8-type-checking`] Apply `TC008` more eagerly in `TYPE_CHECKING` blocks and disapply it in stubs (#15180) [red-knot] `knot_extensions` Python API (#15103) Display Union of Literals as a Literal (#14993) [red-knot] all types are assignable to object (#15332) [`ruff`] Parenthesize arguments to `int` when removing `int` would change semantics in `unnecessary-cast-to-int` (`RUF046`) (#15277) [`eradicate`] Correctly handle metadata blocks directly followed by normal blocks (`ERA001`) (#15330) Narrowing for class patterns in match statements (#15223) [red-knot] add call checking (#15200) Spruce up docs for `slice-to-remove-prefix-or-suffix` (`FURB188`) (#15328) [`internal`] Return statements in finally block point to end block for `unreachable` (`PLW0101`) (#15276) [`ruff`] Treat `)` as a regex metacharacter (`RUF043`, `RUF055`) (#15318) Use uv consistently throughout the documentation (#15302)
4 participants
Note:
PLW0101remains in testing rather than preview, so this PR does not modify any public behavior (hence the title beginning withinternalrather thanpylint, for the sake of the changelog.)Fixes an error in the processing of
trystatements in the control flow graph builder.When processing a try statement, the block following a
returnwas forced to point to thefinallyblock. However, if the return was in thefinallyblock, this caused the block to point to itself. In the case where the wholetry-finallystatement was also included inside of a loop, this caused an infinite loop in the builder for the control flow graph as it attempted to resolve edges.Closes #15248
Test function
Source
Control Flow Graph
flowchart TD start(("Start")) return(("End")) block0[["`*(empty)*`"]] block1[["Loop continue"]] block2["return\n"] block3[["Loop continue"]] block4["break\n"] block5["if 3: break\n"] block6["while (): if 3: break\n"] block7[["Exception raised"]] block8["try: while (): if 3: break finally: return\n"] block9["while T: try: while (): if 3: break finally: return\n"] start --> block9 block9 -- "T" --> block8 block9 -- "else" --> block0 block8 -- "Exception raised" --> block7 block8 -- "else" --> block6 block7 --> block2 block6 -- "()" --> block5 block6 -- "else" --> block2 block5 -- "3" --> block4 block5 -- "else" --> block3 block4 --> block2 block3 --> block6 block2 --> return block1 --> block9 block0 --> return