[`ruff`] Re-implement `unreachable` #10891

augustelalande · 2024-04-11T22:36:41Z

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 some more minor bugs.

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

Introduced a post-processing step in loop handling to find any continue or break statements within the loop body and redirect them appropriately.
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 Control-flow graph fails to connect body-to-loop when loop is the last statement #8958.
Implemented try processing with the following logic (resolves Complete try-except handling in control-flow graph #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.

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

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

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

Additionaly test fixtures, and control-flow fixtures were added.

github-actions · 2024-04-11T22:56:00Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+22 -0 violations, +0 -0 fixes in 4 projects; 40 projects unchanged)

RasaHQ/rasa (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ tests/core/test_exporter.py:312:13: RUF014 Unreachable code in _mocked_fetch

apache/airflow (+19 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/providers/jenkins/operators/jenkins_job_trigger.py:74:5: RUF014 Unreachable code in jenkins_request_with_headers
+ airflow/triggers/base.py:76:9: RUF014 Unreachable code in run
+ airflow/triggers/testing.py:52:13: RUF014 Unreachable code in run
+ dev/example_dags/update_example_dags_paths.py:50:5: RUF014 Unreachable code in check_if_url_exists
+ dev/example_dags/update_example_dags_paths.py:52:9: RUF014 Unreachable code in check_if_url_exists
+ dev/example_dags/update_example_dags_paths.py:53:5: RUF014 Unreachable code in check_if_url_exists
+ dev/example_dags/update_example_dags_paths.py:54:9: RUF014 Unreachable code in check_if_url_exists
+ dev/example_dags/update_example_dags_paths.py:55:5: RUF014 Unreachable code in check_if_url_exists
+ tests/models/test_baseoperator.py:1016:21: RUF014 Unreachable code in my_work
+ tests/models/test_mappedoperator.py:1026:21: RUF014 Unreachable code in other_setup
+ tests/models/test_mappedoperator.py:1042:21: RUF014 Unreachable code in my_setup
+ tests/models/test_mappedoperator.py:1068:21: RUF014 Unreachable code in other_setup
+ tests/models/test_mappedoperator.py:1241:21: RUF014 Unreachable code in my_work
+ tests/models/test_mappedoperator.py:1258:21: RUF014 Unreachable code in my_work
+ tests/models/test_mappedoperator.py:1560:17: RUF014 Unreachable code in my_work
+ tests/models/test_mappedoperator.py:1598:25: RUF014 Unreachable code in my_work
+ tests/models/test_mappedoperator.py:1645:25: RUF014 Unreachable code in my_work
+ tests/models/test_mappedoperator.py:955:21: RUF014 Unreachable code in my_setup
+ tests/models/test_taskinstance.py:2857:13: RUF014 Unreachable code in on_finish_callable

pandas-dev/pandas (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pandas/io/parsers/python_parser.py:1145:25: RUF014 Unreachable code in _get_lines

python-poetry/poetry (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ src/poetry/utils/authenticator.py:256:9: RUF014 Unreachable code in request

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF014	22	22	0	0	0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

charliermarsh · 2024-04-11T23:05:16Z

Nice, I'm a fan of this.

augustelalande · 2024-04-11T23:25:44Z

No promises, just started taking a look at it.

augustelalande · 2024-04-11T23:26:17Z

Can you give me some background on why this was never activated? Was there just too many false positives?

charliermarsh · 2024-04-12T00:34:11Z

No, I think it's fairly reliable, but I believe try-except handling wasn't quite finished: #8959. There's also at least one bug to fix: #8958.

codspeed-hq · 2024-04-16T00:06:02Z

CodSpeed Performance Report

Merging #10891 will not alter performance

_{Comparing augustelalande:unreachable (87a3e96) with main (bd6ca3d)}

Summary

✅ 30 untouched benchmarks

This reverts commit f9dd7bb.

…ith multiple blocks

…that needs it.

augustelalande · 2024-04-27T21:30:53Z

This is probably not completely ready for merging, but considering the size it's probably worth getting some feedback now. All the ecosystem checks seem like true positives, there might still be some false negatives, but those will be harder to find.

augustelalande · 2024-05-09T04:52:22Z

@charliermarsh when you get the time please have a look at this

augustelalande force-pushed the unreachable branch 9 times, most recently from c993823 to 5897adb Compare April 16, 2024 00:00

augustelalande force-pushed the unreachable branch 2 times, most recently from 4493aaf to 9dc225a Compare April 16, 2024 16:54

augustelalande added 10 commits April 17, 2024 02:04

Revert "Remove unreachable-code feature (astral-sh#9463)"

dcfd247

This reverts commit f9dd7bb.

remove feature gate

d4b3f15

fix up

487b731

mark preview

45ff3ab

Implement post_processing to handle break and continue in loops w…

85b6c36

…ith multiple blocks

Always get an after_block to avoid having to get one for each branch …

cbac4ea

…that needs it.

Fix astral-sh#8958

eb9c180

fix infinite recursion

a196433

Always close the loop

7698614

fix handling of asserts in postprocessing

99fb689

augustelalande force-pushed the unreachable branch from eda9b6b to 557974b Compare April 17, 2024 06:04

initial implementation of try statement handling

552cb09

augustelalande force-pushed the unreachable branch from 557974b to 552cb09 Compare April 17, 2024 06:12

augustelalande added 11 commits April 19, 2024 21:45

clean up docs

59bd963

always process finally

fb37979

these were fixed

0212cd8

add cfg tests for try

f3d6e49

clippy

edb24de

execute blank except unconditionally

670a75c

process with statements

23260d4

this was fixed

72413c0

maybe fall through with statement on error

22a4c6a

remove force_next_block which would change the flow incorrectly

af989a7

improve processing of finallys

87a3e96

augustelalande force-pushed the unreachable branch from 4d724c3 to 87a3e96 Compare April 22, 2024 03:45

augustelalande marked this pull request as ready for review April 27, 2024 21:29

Batalex mentioned this pull request May 2, 2024

[DPE-3929] - feat: secrets integration canonical/kafka-k8s-operator#105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`ruff`] Re-implement `unreachable` #10891

[`ruff`] Re-implement `unreachable` #10891

augustelalande commented Apr 11, 2024 •

edited

github-actions bot commented Apr 11, 2024 •

edited

charliermarsh commented Apr 11, 2024

augustelalande commented Apr 11, 2024

augustelalande commented Apr 11, 2024

charliermarsh commented Apr 12, 2024

codspeed-hq bot commented Apr 16, 2024 •

edited

augustelalande commented Apr 27, 2024 •

edited

augustelalande commented May 9, 2024

[ruff] Re-implement unreachable #10891

Are you sure you want to change the base?

[ruff] Re-implement unreachable #10891

Conversation

augustelalande commented Apr 11, 2024 • edited

Summary

Test Plan

github-actions bot commented Apr 11, 2024 • edited

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

charliermarsh commented Apr 11, 2024

augustelalande commented Apr 11, 2024

augustelalande commented Apr 11, 2024

charliermarsh commented Apr 12, 2024

codspeed-hq bot commented Apr 16, 2024 • edited

CodSpeed Performance Report

Merging #10891 will not alter performance

Summary

augustelalande commented Apr 27, 2024 • edited

augustelalande commented May 9, 2024

[`ruff`] Re-implement `unreachable` #10891

[`ruff`] Re-implement `unreachable` #10891

augustelalande commented Apr 11, 2024 •

edited

github-actions bot commented Apr 11, 2024 •

edited

`ruff-ecosystem` results

codspeed-hq bot commented Apr 16, 2024 •

edited

augustelalande commented Apr 27, 2024 •

edited