Split `Constant` to individual literal nodes by dhruvmanila · Pull Request #8064 · astral-sh/ruff

dhruvmanila · 2023-10-19T13:48:07Z

Summary

This PR splits the Constant enum as individual literal nodes. It introduces the following new nodes for each variant:

ExprStringLiteral
ExprBytesLiteral
ExprNumberLiteral
ExprBooleanLiteral
ExprNoneLiteral
ExprEllipsisLiteral

The main motivation behind this refactor is to introduce the new AST node for implicit string concatenation in the coming PR. The elements of that node will be either a string literal, bytes literal or a f-string which can be implemented using an enum. This means that a string or bytes literal cannot be represented by Constant::Str / Constant::Bytes which creates an inconsistency.

This PR avoids that inconsistency by splitting the constant nodes into it's own literal nodes, literal being the more appropriate naming convention from a static analysis tool perspective.

This also makes working with literals in the linter and formatter much more ergonomic like, for example, if one would want to check if this is a string literal, it can be done easily using Expr::is_string_literal_expr or matching against Expr::StringLiteral as oppose to matching against the ExprConstant and enum Constant. A few AST helper methods can be simplified as well which will be done in a follow-up PR.

This introduces a new Expr::is_literal_expr method which is the same as Expr::is_constant_expr. There are also intermediary changes related to implicit string concatenation which are quiet less. This is done so as to avoid having a huge PR which this already is.

Test Plan

Verify and update all of the existing snapshots (parser, visitor)
Verify that the ecosystem check output remains unchanged for both the linter and formatter

Formatter ecosystem check

`main`

project	similarity index	total files	changed files
cpython	0.75803	1799	1647
django	0.99983	2772	34
home-assistant	0.99953	10596	186
poetry	0.99891	317	17
transformers	0.99966	2657	330
twine	1.00000	33	0
typeshed	0.99978	3669	20
warehouse	0.99977	654	13
zulip	0.99970	1459	22

`dhruv/constant-to-literal`

project	similarity index	total files	changed files
cpython	0.75803	1799	1647
django	0.99983	2772	34
home-assistant	0.99953	10596	186
poetry	0.99891	317	17
transformers	0.99966	2657	330
twine	1.00000	33	0
typeshed	0.99978	3669	20
warehouse	0.99977	654	13
zulip	0.99970	1459	22

dhruvmanila · 2023-10-19T13:48:12Z

Current dependencies on/for this PR:

main
- PR Remove leftover constant tuple reference #8062
  - PR New Singleton enum for PatternMatchSingleton node #8063
    - PR Split Constant to individual literal nodes #8064 👈
      - PR Update string nodes for implicit concatenation #7927
        
        PR Explicit as_str (no deref), add no allocation methods #8826
        
        PR Remove #[allow(unused_variables)] from visitor methods #8828
        
        PR New AST nodes for f-string elements #8835
        
        PR Introduce StringLike enum #9016

This stack of pull requests is managed by Graphite.

github-actions · 2023-10-19T20:00:32Z

PR Check Results

Ecosystem

✅ ecosystem check detected no linter changes.

✅ ecosystem check detected no format changes.

crates/ruff_linter/src/rules/flake8_simplify/rules/if_else_block_instead_of_dict_lookup.rs

crates/ruff_linter/src/rules/pylint/rules/magic_value_comparison.rs

crates/ruff_linter/src/rules/pylint/settings.rs

crates/ruff_python_formatter/src/expression/expr_bytes_literal.rs

crates/ruff_python_ast/src/nodes.rs

crates/ruff_python_formatter/src/expression/expr_bytes_literal.rs

crates/ruff_python_formatter/src/expression/expr_ellipsis_literal.rs

konstin · 2023-10-23T08:46:09Z

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

-            Expr::Constant(ast::ExprConstant {
-                value: Constant::Str(value),
-                range: _,
-            }) => {
+            Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {


Those changes are a nice simplification!

MichaReiser

I haven't managed to get through all of it yet, but here a first few comments.

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

crates/ruff_linter/src/doc_lines.rs

crates/ruff_linter/src/docstrings/extraction.rs

crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_literal_union.rs

MichaReiser

Looks good to me. Thanks for working on this large refactor.

I think adding a LiteralExpression union would be useful in a few places and might be something you want to consider adding as part of this PR or as a follow up PR

MichaReiser · 2023-10-27T00:14:02Z

crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs


        match key {
-            Expr::Constant(_) | Expr::Tuple(_) | Expr::FString(_) => {
+            Expr::StringLiteral(_)


Not having a single union will make it more challenging to identify all places where a new literal type would need to be added. But that's probably fine, new literal types don't happen everyday.

crates/ruff_linter/src/rules/pylint/rules/magic_value_comparison.rs

crates/ruff_linter/src/rules/pylint/settings.rs

crates/ruff_linter/src/rules/pyupgrade/rules/native_literals.rs

crates/ruff_python_formatter/src/expression/expr_boolean_literal.rs

crates/ruff_python_formatter/src/expression/expr_bytes_literal.rs

crates/ruff_python_formatter/src/expression/expr_number_literal.rs

crates/ruff_python_formatter/src/expression/number.rs

crates/ruff_python_formatter/src/statement/suite.rs

MichaReiser

Looks good to me. Thanks for working on this large refactor.

I think adding a LiteralExpression union would be useful in a few places and might be something you want to consider adding as part of this PR or as a follow up PR

dhruvmanila · 2023-10-27T13:16:28Z

Thanks for the review! I'll make most of the smaller changes in this PR and move a few larger ones in the follow-up PRs:

Introduce LiteralExpressionRef which can be constructed using Expr::as_literal_expr
Unify multiple implementation of having constant type (LiteralKind, LiteralType, ConstantType) on the AST node as expr::literal_type() -> Option<LiteralType>

dhruvmanila mentioned this pull request Oct 19, 2023

Remove leftover constant tuple reference #8062

Merged

This was referenced Oct 19, 2023

New Singleton enum for PatternMatchSingleton node #8063

Merged

Update string nodes for implicit concatenation #7927

Merged

dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from 2b94722 to 07dce4c Compare October 19, 2023 17:52

dhruvmanila force-pushed the dhruv/constant-to-literal branch from 5b20992 to 66ae3fc Compare October 19, 2023 17:52

dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from 07dce4c to db9d767 Compare October 19, 2023 19:27

dhruvmanila force-pushed the dhruv/constant-to-literal branch 2 times, most recently from fd9f090 to 7111576 Compare October 19, 2023 19:33

dhruvmanila commented Oct 20, 2023

View reviewed changes

dhruvmanila added the internal An internal refactor or improvement label Oct 20, 2023

dhruvmanila marked this pull request as ready for review October 20, 2023 10:35

dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from db9d767 to 666eb09 Compare October 20, 2023 10:46

dhruvmanila force-pushed the dhruv/constant-to-literal branch from 7111576 to f544b3f Compare October 20, 2023 10:47

dhruvmanila requested review from MichaReiser, charliermarsh and konstin October 20, 2023 10:49

dhruvmanila commented Oct 23, 2023

View reviewed changes

crates/ruff_python_ast/src/nodes.rs Show resolved Hide resolved

dhruvmanila commented Oct 23, 2023

View reviewed changes

crates/ruff_python_ast/src/nodes.rs Show resolved Hide resolved

konstin approved these changes Oct 23, 2023

View reviewed changes

MichaReiser reviewed Oct 26, 2023

View reviewed changes

MichaReiser approved these changes Oct 27, 2023

View reviewed changes

dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from 666eb09 to 9321710 Compare October 30, 2023 05:25

dhruvmanila force-pushed the dhruv/constant-to-literal branch from f544b3f to d623cf9 Compare October 30, 2023 05:25

Base automatically changed from dhruv/match-pattern-singleton to main October 30, 2023 05:48

dhruvmanila added 2 commits October 30, 2023 11:19

Split Constant to individual literal nodes

7bf5e2c

Update snapshots

ad8939c

dhruvmanila added 2 commits October 30, 2023 11:19

Code review changes

5e65638

New changes due to rebase

aaf65a1

dhruvmanila force-pushed the dhruv/constant-to-literal branch from cf8ee21 to aaf65a1 Compare October 30, 2023 05:50

dhruvmanila closed this Oct 30, 2023

dhruvmanila reopened this Oct 30, 2023

dhruvmanila merged commit 230c9ce into main Oct 30, 2023

dhruvmanila deleted the dhruv/constant-to-literal branch October 30, 2023 06:43

dhruvmanila mentioned this pull request Oct 30, 2023

Fix FBT001 false negative with unions and optional #7501

Merged

This was referenced Nov 23, 2023

Explicit as_str (no deref), add no allocation methods #8826

Merged

Remove #[allow(unused_variables)] from visitor methods #8828

Merged

New AST nodes for f-string elements #8835

Merged

dhruvmanila mentioned this pull request Dec 5, 2023

Introduce StringLike enum #9016

Merged

dhruvmanila mentioned this pull request Sep 4, 2024

[red-knot] Handle multiple comprehension targets #13213

Merged

Comments

Conversation

dhruvmanila commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Formatter ecosystem check

main

dhruv/constant-to-literal

Uh oh!

dhruvmanila commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

konstin Oct 23, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

dhruvmanila commented Oct 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dhruvmanila commented Oct 19, 2023 •

edited

Loading

`main`

`dhruv/constant-to-literal`

dhruvmanila commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 19, 2023 •

edited

Loading