Skip to content

[ruff] Add never-union rule to detect redundant typing.NoReturn and typing.Never#9217

Merged
charliermarsh merged 1 commit intomainfrom
charlie/union
Dec 21, 2023
Merged

[ruff] Add never-union rule to detect redundant typing.NoReturn and typing.Never#9217
charliermarsh merged 1 commit intomainfrom
charlie/union

Conversation

@charliermarsh
Copy link
Copy Markdown
Member

Summary

Adds a rule to detect unions that include typing.NoReturn or typing.Never. In such cases, the use of the bottom type is redundant.

Closes #9113.

Test Plan

cargo test

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 20, 2023
@charliermarsh
Copy link
Copy Markdown
Member Author

This should be fixable but isn't trivial, working on it separately.

@charliermarsh charliermarsh force-pushed the charlie/union branch 2 times, most recently from 128adf3 to 024a2c5 Compare December 20, 2023 18:50
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 20, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

ruff check --no-cache --exit-zero --preview --select ALL

+ tests/support/plugins/selenium.py:118:88: RUF020 `NoReturn | T` is equivalent to `T`
+ tests/support/plugins/selenium.py:132:47: RUF020 `NoReturn | T` is equivalent to `T`
+ tests/support/plugins/selenium.py:146:47: RUF020 `NoReturn | T` is equivalent to `T`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF020 3 3 0 0 0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum UnionLike {
/// E.g., `typing.Union[int, str]`
Union,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: I struggled with the name because I incorrectly assumed it represents int | str. Maybe TypingUnion? or Typing and BinOp?

}

/// RUF020
pub(crate) fn never_union<'a>(checker: &mut Checker, expr: &'a Expr) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unrelated to this PR: Hmm, one downside of quoting runtime only type annotations is that it makes all lint-rules useless because we don't parse string annotations...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We actually do!

Copy link
Copy Markdown
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Cool!

@zanieb
Copy link
Copy Markdown
Member

zanieb commented Dec 21, 2023

Interesting notes in the ecosystem e.g.

XXX: no return should be needed with NoReturn type (type-checker bug?) ref

@charliermarsh charliermarsh enabled auto-merge (squash) December 21, 2023 20:47
@charliermarsh charliermarsh merged commit a9ceef5 into main Dec 21, 2023
@charliermarsh charliermarsh deleted the charlie/union branch December 21, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add rule for improper use of NoReturn

3 participants