[red-knot] add if-statement support to FlowGraph #11673

carljm · 2024-06-01T04:01:16Z

Summary

Add if-statement support to FlowGraph. This introduces branches and joins in the graph for the first time.

Test Plan

Added tests.

plredmond · 2024-06-01T06:56:46Z

crates/red_knot/src/symbols.rs

+                    self.pending.push(branch_node.predecessor);
+                }
+                FlowNode::Phi(phi_node) => {
+                    self.pending.extend_from_slice(&phi_node.predecessors);


Does the order that we explore the basic blocks prior to a phi node matter? I guess it's going to just be whatever order they are within .predecessors

In principle it shouldn't matter; in practice it may be more intuitive if, when we synthesize a union type from multiple reachable definitions, we try to display that union with elements in source order. I can look at this.

I wonder if the text-locations on the AST nodes can be included in these predecessors, and then instead of a vec we could store them as a heap. That would allow you to add them in any order but obtain source order.

I think we could do that, but we can also achieve the same result just by being careful about the order we list predecessors in a phi node and push them to the pending list in the flow graph.

Got this working and pushed.

I was wrong here (inadequate test cases.) It's not generally possible to do this the way I was trying to do it (statically in the structure of the flow graph), since some names may be defined in branches where others aren't. If we really want to guarantee source order of inferred unions, we would have to explicitly include source-order information in definitions. Not going to prioritize that right now, though.

MichaReiser

Nice! The functionality looks good to me. You may want to consider to move the implementation to symbols/cfg (or rename symbols to semantic or binding).

I think we should spend some more time to see if we can come up with a design that avoids the nested Vec inside of the Phi node. The Vec is problematic because it means that we create a ton of allocations because Phi nodes are very common.

crates/red_knot/src/symbols.rs

carljm · 2024-06-01T14:26:03Z

I was holding off on renaming / restructuring for now so as to minimize conflicts with your salsa work and Patrick's symbol table work. But I definitely want to do that.

MichaReiser · 2024-06-03T06:46:41Z

@carljm I'm still very far off with my salsa work and I probably need to redo my stack once we figured out the entire data model (or at least parts of it) because @AlexWaygood already made many changes that I need to incorporate somehow. So, don't feel blocked by my salsa work but I'm sure Patrick appreciates it.

* main: (25 commits) Isolate non-breaking whitespace indentation test case (#11721) Generator should add a newline before type statement (#11720) Remove less used parser dependencies (#11718) Use string expression for parsing type annotation (#11717) Re-order lexer methods (#11716) Maintain synchronicity between the lexer and the parser (#11457) Update NPM Development dependencies (#11713) Update pre-commit dependencies (#11712) Update cloudflare/wrangler-action action to v3.6.1 (#11709) Update dependency monaco-editor to ^0.49.0 (#11710) Update Rust crate tracing-tree to v0.3.1 (#11703) Update Rust crate libcst to v1.4.0 (#11707) Update Rust crate itertools to 0.13.0 (#11706) Update Rust crate insta to v1.39.0 (#11705) Update Rust crate proc-macro2 to v1.0.85 (#11700) Update Rust crate toml to v0.8.13 (#11702) Update Rust crate strum_macros to v0.26.3 (#11701) Update UP035 for Python 3.13 and the latest version of typing_extensions (#11693) Add RDJson support. (#11682) [`pyupgrade`] Write empty string in lieu of panic (#11696) ...

* cjm/cfg1: (26 commits) review comments Isolate non-breaking whitespace indentation test case (#11721) Generator should add a newline before type statement (#11720) Remove less used parser dependencies (#11718) Use string expression for parsing type annotation (#11717) Re-order lexer methods (#11716) Maintain synchronicity between the lexer and the parser (#11457) Update NPM Development dependencies (#11713) Update pre-commit dependencies (#11712) Update cloudflare/wrangler-action action to v3.6.1 (#11709) Update dependency monaco-editor to ^0.49.0 (#11710) Update Rust crate tracing-tree to v0.3.1 (#11703) Update Rust crate libcst to v1.4.0 (#11707) Update Rust crate itertools to 0.13.0 (#11706) Update Rust crate insta to v1.39.0 (#11705) Update Rust crate proc-macro2 to v1.0.85 (#11700) Update Rust crate toml to v0.8.13 (#11702) Update Rust crate strum_macros to v0.26.3 (#11701) Update UP035 for Python 3.13 and the latest version of typing_extensions (#11693) Add RDJson support. (#11682) ...

* cjm/cfg2: (27 commits) review comments review comments Isolate non-breaking whitespace indentation test case (#11721) Generator should add a newline before type statement (#11720) Remove less used parser dependencies (#11718) Use string expression for parsing type annotation (#11717) Re-order lexer methods (#11716) Maintain synchronicity between the lexer and the parser (#11457) Update NPM Development dependencies (#11713) Update pre-commit dependencies (#11712) Update cloudflare/wrangler-action action to v3.6.1 (#11709) Update dependency monaco-editor to ^0.49.0 (#11710) Update Rust crate tracing-tree to v0.3.1 (#11703) Update Rust crate libcst to v1.4.0 (#11707) Update Rust crate itertools to 0.13.0 (#11706) Update Rust crate insta to v1.39.0 (#11705) Update Rust crate proc-macro2 to v1.0.85 (#11700) Update Rust crate toml to v0.8.13 (#11702) Update Rust crate strum_macros to v0.26.3 (#11701) Update UP035 for Python 3.13 and the latest version of typing_extensions (#11693) ...

* main: [red-knot] infer_symbol_public_type infers union of all definitions (#11669)

* cjm/cfg2: [red-knot] infer_symbol_public_type infers union of all definitions (#11669)

* cjm/cfg3: (29 commits) [red-knot] infer_symbol_public_type infers union of all definitions (#11669) review comments review comments review comments Isolate non-breaking whitespace indentation test case (#11721) Generator should add a newline before type statement (#11720) Remove less used parser dependencies (#11718) Use string expression for parsing type annotation (#11717) Re-order lexer methods (#11716) Maintain synchronicity between the lexer and the parser (#11457) Update NPM Development dependencies (#11713) Update pre-commit dependencies (#11712) Update cloudflare/wrangler-action action to v3.6.1 (#11709) Update dependency monaco-editor to ^0.49.0 (#11710) Update Rust crate tracing-tree to v0.3.1 (#11703) Update Rust crate libcst to v1.4.0 (#11707) Update Rust crate itertools to 0.13.0 (#11706) Update Rust crate insta to v1.39.0 (#11705) Update Rust crate proc-macro2 to v1.0.85 (#11700) Update Rust crate toml to v0.8.13 (#11702) ...

* main: [red-knot] extract helper functions in inference tests (#11671) [red-knot] use reachable definitions in infer_expression_type (#11670)

github-actions · 2024-06-04T00:12:05Z

`ruff-ecosystem` results

Linter (stable)

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

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

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

- examples/server/app/selection_histogram.py:88:42: NPY001 [*] Type alias `np.bool` is deprecated, replace with builtin type

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
NPY001	1	0	1	0	0

Linter (preview)

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

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

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

- examples/server/app/selection_histogram.py:88:42: NPY001 [*] Type alias `np.bool` is deprecated, replace with builtin type

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
NPY001	1	0	1	0	0

* main: [`flake8-pyi`] Implement `PYI063` (#11699) Fix `red-knot` compilation (#11727)

* main: CI: add job to run tests under minimum supported rust version (msrv) (#11737) Lexer should consider BOM for the start offset (#11732) Use cursor offset for lexer checkpoint (#11734) red-knot: Change `resolve_global_symbol` to take `Module` as an argument (#11723) red-knot: Use `parse_unchecked` to get all parse errors (#11725) Respect per-file ignores for blanket and redirected noqa rules (#11728) [`pygrep_hooks`] Check blanket ignores via file-level pragmas (`PGH004`) (#11540)

carljm · 2024-06-04T19:36:40Z

Refactoring CFG out of symbol table is tracked in #11657, not going to do it in this PR.

carljm added 10 commits May 31, 2024 17:24

rename infer_symbol_type to infer_symbol_public_type

1b3ff5e

infer public type from all definitions

ffd589f

add TODO comment

b01cbe3

add TODO comment about union-of-all-defs

18a64d8

[red-knot] use reachable definitions in infer_expression_type

a72293e

[red-knot] extract helper functions in inference tests

55d7428

remove need for db = case.db in each test

ad223c7

also extract write_to_path

76c566f

use textwrap::dedent

89452b9

[red-knot] add if-statement support to FlowGraph

9f494ee

carljm added the red-knot Multi-file analysis & type inference label Jun 1, 2024

carljm requested a review from AlexWaygood June 1, 2024 04:01

carljm requested a review from MichaReiser as a code owner June 1, 2024 04:01

carljm added 3 commits May 31, 2024 22:06

improve tests

07ab4cd

remove redundant Branch node

ebb67bf

FlowGraph doesn't need to be pub(crate)

ac934bb

plredmond reviewed Jun 1, 2024

View reviewed changes

MichaReiser approved these changes Jun 1, 2024

View reviewed changes

crates/red_knot/src/symbols.rs Outdated Show resolved Hide resolved

crates/red_knot/src/symbols.rs Outdated Show resolved Hide resolved

crates/red_knot/src/symbols.rs Outdated Show resolved Hide resolved

crates/red_knot/src/symbols.rs Show resolved Hide resolved

carljm added 9 commits June 3, 2024 15:58

review comments

b8535b7

review comments

d73db36

review comments

ec113a1

Merge branch 'main' into cjm/cfg2

3d52dfa

* main: [red-knot] infer_symbol_public_type infers union of all definitions (#11669)

Merge branch 'cjm/cfg2' into cjm/cfg3

fda62af

* cjm/cfg2: [red-knot] infer_symbol_public_type infers union of all definitions (#11669)

Base automatically changed from cjm/cfg3 to main June 3, 2024 23:46

Merge branch 'main' into cjm/cfg4

f9312db

* main: [red-knot] extract helper functions in inference tests (#11671) [red-knot] use reachable definitions in infer_expression_type (#11670)

carljm added 4 commits June 3, 2024 21:51

Merge branch 'main' into cjm/cfg4

1d3048d

* main: [`flake8-pyi`] Implement `PYI063` (#11699) Fix `red-knot` compilation (#11727)

only two preds per phi, source order visit

21c430d

code review comments

d42fff9

carljm merged commit d056d09 into main Jun 4, 2024
20 checks passed

carljm deleted the cjm/cfg4 branch June 4, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] add if-statement support to FlowGraph #11673

[red-knot] add if-statement support to FlowGraph #11673

carljm commented Jun 1, 2024

plredmond Jun 1, 2024

carljm Jun 1, 2024

plredmond Jun 3, 2024

carljm Jun 3, 2024

carljm Jun 4, 2024

carljm Jun 5, 2024

MichaReiser left a comment

carljm commented Jun 1, 2024

MichaReiser commented Jun 3, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

carljm commented Jun 4, 2024

[red-knot] add if-statement support to FlowGraph #11673

[red-knot] add if-statement support to FlowGraph #11673

Conversation

carljm commented Jun 1, 2024

Summary

Test Plan

plredmond Jun 1, 2024

Choose a reason for hiding this comment

carljm Jun 1, 2024

Choose a reason for hiding this comment

plredmond Jun 3, 2024

Choose a reason for hiding this comment

carljm Jun 3, 2024

Choose a reason for hiding this comment

carljm Jun 4, 2024

Choose a reason for hiding this comment

carljm Jun 5, 2024

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

carljm commented Jun 1, 2024

MichaReiser commented Jun 3, 2024 • edited Loading

github-actions bot commented Jun 4, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

carljm commented Jun 4, 2024

MichaReiser commented Jun 3, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

`ruff-ecosystem` results