Add support for `Union` declarations without `|` to PYI016 #5598

zanieb · 2023-07-07T17:27:07Z

Previously, PYI016 only supported reporting violations for unions defined with |. Now, union declarations with typing.Union are supported.

This implementation reuses the union traversal logic from #5570.

PYI016 will not attempt to fix cases where Union is used. Unlike |, removing members from a Union can result in an invalid union definition. We may be able to support this in the future as in PYI030 the complexity seems high.

Tested with new snapshot cases ./target/debug/ruff --select PYI016 crates/ruff/resources/test/fixtures/flake8_pyi/PYI016.pyi --show-source

zanieb · 2023-07-07T17:29:25Z

crates/ruff/resources/test/fixtures/flake8_pyi/PYI016.pyi

+# Shouldn't emit if in new parent type
+field16: int | dict[int, str]  # OK
+
+# Shouldn't emit if not in a union parent
+field17: dict[int, int]  # OK


I've added some more edge cases that are not relevant for this pull request but are good to cover

github-actions · 2023-07-07T17:38:30Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+3, -0, 0 error(s))

bokeh (+3, -0)

+ src/bokeh/core/property/factors.py:51:56: PYI016 Duplicate union member `tuple[str, str]`
+ src/bokeh/core/property/factors.py:52:85: PYI016 Duplicate union member `tp.Sequence[tuple[str, str]]`
+ src/bokeh/plotting/contour.py:43:88: PYI016 Duplicate union member `ContourColor`

Rules changed: 1

Rule	Changes	Additions	Removals
PYI016	3	3	0

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      9.5±0.03ms     4.3 MB/sec    1.00      9.4±0.03ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.02      2.1±0.01ms     7.9 MB/sec    1.00      2.1±0.02ms     8.0 MB/sec
formatter/numpy/globals.py                 1.00    235.5±1.50µs    12.5 MB/sec    1.00    235.3±1.40µs    12.5 MB/sec
formatter/pydantic/types.py                1.03      4.7±0.03ms     5.5 MB/sec    1.00      4.5±0.01ms     5.6 MB/sec
linter/all-rules/large/dataset.py          1.03     16.9±0.10ms     2.4 MB/sec    1.00     16.3±0.11ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.2±0.01ms     4.0 MB/sec    1.00      4.1±0.02ms     4.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    526.0±2.37µs     5.6 MB/sec    1.00    524.5±3.25µs     5.6 MB/sec
linter/all-rules/pydantic/types.py         1.02      7.4±0.03ms     3.5 MB/sec    1.00      7.3±0.03ms     3.5 MB/sec
linter/default-rules/large/dataset.py      1.08      8.8±0.04ms     4.6 MB/sec    1.00      8.1±0.02ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.05   1879.3±4.13µs     8.9 MB/sec    1.00  1792.9±48.92µs     9.3 MB/sec
linter/default-rules/numpy/globals.py      1.03    211.8±1.06µs    13.9 MB/sec    1.00    204.7±0.72µs    14.4 MB/sec
linter/default-rules/pydantic/types.py     1.07      3.9±0.02ms     6.5 MB/sec    1.00      3.7±0.01ms     7.0 MB/sec

Windows

group                                      main                                    pr
-----                                      ----                                    --
formatter/large/dataset.py                 1.00     11.1±0.43ms     3.7 MB/sec     1.00     11.2±0.43ms     3.6 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.4±0.11ms     7.0 MB/sec     1.02      2.4±0.19ms     6.9 MB/sec
formatter/numpy/globals.py                 1.00   278.4±21.02µs    10.6 MB/sec     1.03   286.8±19.95µs    10.3 MB/sec
formatter/pydantic/types.py                1.04      5.4±0.28ms     4.7 MB/sec     1.00      5.2±0.25ms     4.9 MB/sec
linter/all-rules/large/dataset.py          1.00     17.6±0.88ms     2.3 MB/sec     1.07     18.9±0.53ms     2.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.9±0.36ms     3.4 MB/sec     1.00      4.9±0.15ms     3.4 MB/sec
linter/all-rules/numpy/globals.py          1.00   552.8±35.29µs     5.3 MB/sec     1.09   601.9±31.64µs     4.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.8±0.31ms     3.3 MB/sec     1.09      8.5±0.40ms     3.0 MB/sec
linter/default-rules/large/dataset.py      1.00      8.9±0.54ms     4.6 MB/sec     1.10      9.7±0.34ms     4.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1993.0±115.12µs     8.4 MB/sec    1.02      2.0±0.08ms     8.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    244.7±9.98µs    12.1 MB/sec     1.01   246.6±13.32µs    12.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.3±0.18ms     6.0 MB/sec     1.03      4.4±0.24ms     5.8 MB/sec

zanieb · 2023-07-07T17:43:34Z

Example bokeh change https://github.com/bokeh/bokeh/blob/ee3f7f27fb744860763edd1d44fcac9b797e36b4/src/bokeh/core/property/factors.py#L51

charliermarsh · 2023-07-08T19:20:51Z

crates/ruff/src/checkers/ast/mod.rs

@@ -2201,7 +2201,12 @@ where
                    }


Nit: this part above can be rewritten with some optional chaining:

let check = self .semantic .expr_grandparent() .and_then(Expr::as_subscript_expr) .map_or(true, |parent| { !self.semantic.match_typing_expr(&parent.value, "Union") });

This is nice because you can avoid the mut, and all the logic is contained in one fluid expression. You could even inline this condition in the if and avoid creating check entirely.

Nit: I would prefer keeping the variable (I find such lengthy if's hard to read) but give it a name with more semantical meaning. Like, what are we testing here? is_union maybe?

This has the advantage that if there's a bug I have an easier time fixing it because:

if is_union is false when the expression is a union -> Okay, the bug must be with the expression initializing is_union. The same if is_union is true when it should not.

I don't need to spend time understanding this expression if the bug is with the diagnostic or fix generation but not identifying what a union is.

is_union_but_grandparent_is_not :) I'm not sure what a good name would be yet. I'll think on it.

edit: maybe is_unchecked_union

crates/ruff/src/rules/flake8_pyi/helpers.rs

crates/ruff/src/rules/flake8_pyi/rules/duplicate_union_member.rs

charliermarsh · 2023-07-08T19:23:19Z

crates/ruff/src/rules/flake8_pyi/rules/duplicate_union_member.rs

-            // SAFETY: impossible to have a duplicate without a `parent` node.
-            let parent = parent.expect("Parent node must exist");
+                // If the parent node is not a `BinOp` we will not perform a fix
+                if let Expr::BinOp(ast::ExprBinOp { left, right, .. }) = parent {


I think this could be written as if let Some(Expr::BinOp(...)) = parent to avoid the .expect above. (I think this code already existed, but we generally prefer safe over unsafe even if the condition is always expected to hold true -- it's debatable whether this is the right policy, here's a blog post that argues for the opposite: https://blog.burntsushi.net/unwrap/.)

This is a little awkward actually we still need to have an .unwrap() when we call range() unless we want to write two if let statements? Perhaps I'm missing something. It is kind of nice to panic here because the traversal code is broken if this invariant does not hold but 🤷‍♀️ I can see both sides.

charliermarsh · 2023-07-08T19:24:49Z

crates/ruff/src/rules/flake8_pyi/rules/duplicate_union_member.rs

-        checker.diagnostics.push(diagnostic);
-    }
+    // Add all diagnostics to the checker
+    checker.diagnostics.append(&mut diagnostics);


(It'd be nice to append these directly as we go rather than allocate an intermediary vector, but it's not critical. I assume that lifetimes and borrowing got in the way per Discord.)

Yeah I tried to do this a couple ways and it seems like a pain

charliermarsh

This looks great! Love the additional test coverage too. Couple nits but nothing blocking.

MichaReiser · 2023-07-10T07:42:02Z

crates/ruff/src/checkers/ast/mod.rs

@@ -2201,7 +2201,12 @@ where
                    }


Nit: I would prefer keeping the variable (I find such lengthy if's hard to read) but give it a name with more semantical meaning. Like, what are we testing here? is_union maybe?

This has the advantage that if there's a bug I have an easier time fixing it because:

if is_union is false when the expression is a union -> Okay, the bug must be with the expression initializing is_union. The same if is_union is true when it should not.

I don't need to spend time understanding this expression if the bug is with the diagnostic or fix generation but not identifying what a union is.

MichaReiser · 2023-07-10T07:42:59Z

crates/ruff/src/rules/flake8_pyi/helpers.rs

+        range: _,
+    }) = expr
+    {
+        // The union data structure usually looks like this:


These comments are awesome. They make reviewing the code much easier and will certainly be useful when reading this code in the future.

These actually came from the previous contribution so I can't take credit :)

crates/ruff/src/rules/flake8_pyi/mod.rs

zanieb added 4 commits July 7, 2023 12:20

Add support for Union declarations without | to PYI016

f6ff7e2

Resolve borrow complaint by stashing diagnostics in temporary vector

db4a200

Add new tests; call rule in ast checker; do not emit fixes for Union

5c41384

Fixup expected comment

3eeba27

zanieb commented Jul 7, 2023

View reviewed changes

charliermarsh reviewed Jul 8, 2023

View reviewed changes

crates/ruff/src/rules/flake8_pyi/helpers.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Jul 8, 2023

View reviewed changes

crates/ruff/src/rules/flake8_pyi/rules/duplicate_union_member.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Jul 8, 2023

View reviewed changes

charliermarsh approved these changes Jul 8, 2023

View reviewed changes

dhruvmanila approved these changes Jul 9, 2023

View reviewed changes

MichaReiser approved these changes Jul 10, 2023

View reviewed changes

zanieb added 5 commits July 10, 2023 10:12

Simplify check for parent union in checker

4c13b2d

Update snapshot

1c75b56

Reduce public exposure of traverse_union

7e92fdd

Import HashSet

22c9ac8

Avoid calling parent.expect

cffdd34

zanieb requested a review from charliermarsh July 10, 2023 16:52

charliermarsh approved these changes Jul 10, 2023

View reviewed changes

crates/ruff/src/rules/flake8_pyi/mod.rs Outdated Show resolved Hide resolved

Reduce public scope of flake8_pyi::helpers

4f71aa5

zanieb enabled auto-merge (squash) July 10, 2023 17:02

zanieb merged commit d19839f into astral-sh:main Jul 10, 2023

p-linnane mentioned this pull request Jul 12, 2023

ruff 0.0.278 Homebrew/homebrew-core#136446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `Union` declarations without `|` to PYI016 #5598

Add support for `Union` declarations without `|` to PYI016 #5598

zanieb commented Jul 7, 2023

zanieb Jul 7, 2023

github-actions bot commented Jul 7, 2023 •

edited

Loading

zanieb commented Jul 7, 2023

charliermarsh Jul 8, 2023 •

edited

Loading

MichaReiser Jul 10, 2023

zanieb Jul 10, 2023 •

edited

Loading

charliermarsh Jul 8, 2023 •

edited

Loading

zanieb Jul 10, 2023

zanieb Jul 10, 2023

charliermarsh Jul 8, 2023

zanieb Jul 10, 2023

charliermarsh left a comment

MichaReiser Jul 10, 2023

MichaReiser Jul 10, 2023

zanieb Jul 10, 2023

Add support for Union declarations without | to PYI016 #5598

Add support for Union declarations without | to PYI016 #5598

Conversation

zanieb commented Jul 7, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 7, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

zanieb commented Jul 7, 2023

charliermarsh Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support for `Union` declarations without `|` to PYI016 #5598

Add support for `Union` declarations without `|` to PYI016 #5598

github-actions bot commented Jul 7, 2023 •

edited

Loading

charliermarsh Jul 8, 2023 •

edited

Loading

zanieb Jul 10, 2023 •

edited

Loading

charliermarsh Jul 8, 2023 •

edited

Loading