Skip to content

Control flow graph: setup #17064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 1, 2025
Merged

Control flow graph: setup #17064

merged 14 commits into from
Apr 1, 2025

Conversation

dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Mar 30, 2025

This PR contains the scaffolding for a new control flow graph implementation, along with its application to the unreachable rule. At the moment, the implementation is a maximal over-approximation: no control flow is modeled and all statements are counted as reachable. With each additional statement type we support, this approximation will improve.

So this PR just contains:

  • A ControlFlowGraph struct and builder
  • Support for printing the flow graph as a Mermaid graph
  • Snapshot tests for the actual graphs
  • (a very bad!) reimplementation of unreachable using the new structs
  • Snapshot tests for unreachable

Instructions for Viewing Mermaid snapshots

Unfortunately I don't know how to convince GitHub to render the Mermaid graphs in the snapshots. However, you can view these locally in VSCode if you install an extension that supports Mermaid graphs in Markdown, and then add this to your settings.json:

  "files.associations": {
"*.md.snap": "markdown",
  }

@dylwil3 dylwil3 added the do-not-merge Do not merge this pull request label Mar 30, 2025
@dylwil3 dylwil3 requested a review from MichaReiser March 30, 2025 21:19
Copy link
Contributor

github-actions bot commented Mar 30, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -25 violations, +0 -0 fixes in 8 projects; 47 projects unchanged)

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

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

- airflow-core/src/airflow/models/mappedoperator.py:71:9: PLW0101 Unreachable code in `expand_start_from_trigger`
- airflow-core/src/airflow/triggers/base.py:103:9: PLW0101 Unreachable code in `run`
- airflow-core/src/airflow/triggers/testing.py:52:13: PLW0101 Unreachable code in `run`
- airflow-core/tests/unit/models/test_baseoperator.py:213:21: PLW0101 Unreachable code in `my_work`
- airflow-core/tests/unit/models/test_mappedoperator.py:1171:17: PLW0101 Unreachable code in `my_work`
- airflow-core/tests/unit/models/test_mappedoperator.py:1209:25: PLW0101 Unreachable code in `my_work`
- airflow-core/tests/unit/models/test_mappedoperator.py:1256:25: PLW0101 Unreachable code in `my_work`
- airflow-core/tests/unit/models/test_mappedoperator.py:565:21: PLW0101 Unreachable code in `my_setup`
- airflow-core/tests/unit/models/test_mappedoperator.py:636:21: PLW0101 Unreachable code in `other_setup`
- airflow-core/tests/unit/models/test_mappedoperator.py:652:21: PLW0101 Unreachable code in `my_setup`
- airflow-core/tests/unit/models/test_mappedoperator.py:678:21: PLW0101 Unreachable code in `other_setup`
- airflow-core/tests/unit/models/test_mappedoperator.py:851:21: PLW0101 Unreachable code in `my_work`
- airflow-core/tests/unit/models/test_mappedoperator.py:868:21: PLW0101 Unreachable code in `my_work`
- airflow-core/tests/unit/models/test_taskinstance.py:3423:13: PLW0101 Unreachable code in `on_finish_callable`

freedomofpress/securedrop (+0 -1 violations, +0 -0 fixes)

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

- admin/securedrop_admin/__init__.py:78:5: PLW0101 Unreachable code in `openssh_version`

langchain-ai/langchain (+0 -3 violations, +0 -0 fixes)

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

- libs/core/langchain_core/runnables/base.py:5093:13: PLW0101 Unreachable code in `astream_events`
- libs/core/tests/unit_tests/runnables/test_fallbacks.py:267:5: PLW0101 Unreachable code in `_generate_immediate_error`
- libs/core/tests/unit_tests/runnables/test_fallbacks.py:295:5: PLW0101 Unreachable code in `_agenerate_immediate_error`

latchbio/latch (+0 -1 violations, +0 -0 fixes)

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

- src/latch_cli/snakemake/single_task_snakemake.py:236:5: PLW0101 Unreachable code in `empty_generator`

milvus-io/pymilvus (+0 -1 violations, +0 -0 fixes)

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

- examples/iterator/iterator.py:64:9: PLW0101 Unreachable code in `external_filter_func`

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

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

- pandas/io/parsers/python_parser.py:1314:25: PLW0101 Unreachable code in `_get_lines`

pytest-dev/pytest (+0 -3 violations, +0 -0 fixes)

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

- src/_pytest/config/__init__.py:1870:9: PLW0101 Unreachable code in `_assertion_supported`
- testing/code/test_code.py:151:17: PLW0101 Unreachable code in `test_bad_getsource`
- testing/code/test_code.py:167:17: PLW0101 Unreachable code in `test_getsource`

astropy/astropy (+0 -1 violations, +0 -0 fixes)

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

- astropy/table/tests/test_bst.py:18:5: PLW0101 Unreachable code in `tree`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PLW0101 25 0 25 0 0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@dylwil3 dylwil3 mentioned this pull request Mar 30, 2025
9 tasks
@MichaReiser
Copy link
Member

MichaReiser commented Mar 31, 2025

I don't think I follow your PR approach. Is the idea that you want to reuse this PR as the target of stacked PRs later on? If so, what's the reason for merging the stacked PRs into this PR over directly merging to main (the unreachable rule has never been shipped)?

To phrase it differently. What prevents us from merging this to main?

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks good to me. I left a few in detail comments.

The biggest one is whether we want to consider an alternative representation for Block and Edges that avoids the nested vectors, which is also something we can consider doing as a separate PR (and maybe even better to get an understanding for the performance characteristics)

@dylwil3
Copy link
Collaborator Author

dylwil3 commented Mar 31, 2025

I don't think I follow your PR approach. Is the idea that you want to reuse this PR as the target of stacked PRs later on? If so, what's the reason for merging the stacked PRs into this PR over directly merging to main (the unreachable rule has never been shipped)?

To phrase it differently. What prevents us from merging this to main?

Nothing prevents us from merging into main except that the code would just be sitting there not used for anything for a bit. If it makes things less complicated I'm happy to just do sequential PRs directly into main!

@MichaReiser
Copy link
Member

Nothing prevents us from merging into main except that the code would just be sitting there not used for anything for a bit. If it makes things less complicated I'm happy to just do sequential PRs directly into main!

That seems not much different from the unreachable code that's already on main. I'm all for merging to main because it makes our live easier.

@dylwil3 dylwil3 added internal An internal refactor or improvement and removed do-not-merge Do not merge this pull request labels Mar 31, 2025
@dylwil3 dylwil3 changed the title Reimplement the control flow graph Control flow graph: setup Apr 1, 2025
@dylwil3 dylwil3 merged commit aa93005 into astral-sh:main Apr 1, 2025
40 checks passed
dcreager added a commit that referenced this pull request Apr 1, 2025
* main:
  [red-knot] Add property tests for callable types (#17006)
  [red-knot] Disjointness for callable types (#17094)
  [red-knot] Flatten `Type::Callable` into four `Type` variants (#17126)
  mdtest.py: do a full mdtest run immediately when the script is executed (#17128)
  [red-knot] Fix callable subtyping for standard parameters (#17125)
  [red-knot] Fix more `redundant-cast` false positives (#17119)
  Sync vendored typeshed stubs (#17106)
  [red-knot] support Any as a class in typeshed (#17107)
  Visit `Identifier` node as part of the `SourceOrderVisitor` (#17110)
  [red-knot] Don't infer Todo for quite so many tuple type expressions (#17116)
  CI: Run pre-commit on depot machine (#17120)
  Error instead of `panic!` when running Ruff from a deleted directory (#16903) (#17054)
  Control flow graph: setup (#17064)
  [red-knot] Playground improvements (#17109)
  [red-knot] IDE crate (#17045)
  Update dependency vite to v6.2.4 (#17104)
  [red-knot] Add redundant-cast error (#17100)
  [red-knot] Narrowing on `in tuple[...]` and `in str` (#17059)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants