Skip to content
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

Split Constant to individual literal nodes #8064

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Oct 19, 2023

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

  1. Verify and update all of the existing snapshots (parser, visitor)
  2. 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 dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from 2b94722 to 07dce4c Compare October 19, 2023 17:52
@dhruvmanila dhruvmanila force-pushed the dhruv/constant-to-literal branch from 5b20992 to 66ae3fc Compare October 19, 2023 17:52
@dhruvmanila dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from 07dce4c to db9d767 Compare October 19, 2023 19:27
@dhruvmanila dhruvmanila force-pushed the dhruv/constant-to-literal branch 2 times, most recently from fd9f090 to 7111576 Compare October 19, 2023 19:33
@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no linter changes.

✅ ecosystem check detected no format changes.

@dhruvmanila dhruvmanila added the internal An internal refactor or improvement label Oct 20, 2023
@dhruvmanila dhruvmanila marked this pull request as ready for review October 20, 2023 10:35
@dhruvmanila dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from db9d767 to 666eb09 Compare October 20, 2023 10:46
@dhruvmanila dhruvmanila force-pushed the dhruv/constant-to-literal branch from 7111576 to f544b3f Compare October 20, 2023 10:47
crates/ruff_python_ast/src/nodes.rs Show resolved Hide resolved
Comment on lines -1189 to +1184
Expr::Constant(ast::ExprConstant {
value: Constant::Str(value),
range: _,
}) => {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Those changes are a nice simplification!

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.

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 Outdated Show resolved Hide resolved
crates/ruff_linter/src/doc_lines.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/docstrings/extraction.rs Outdated Show resolved Hide resolved
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.

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

@@ -149,7 +149,14 @@ pub(crate) fn repeated_keys(checker: &mut Checker, dict: &ast::ExprDict) {
};

match key {
Expr::Constant(_) | Expr::Tuple(_) | Expr::FString(_) => {
Expr::StringLiteral(_)
Copy link
Member

Choose a reason for hiding this comment

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

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/settings.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/statement/suite.rs Outdated Show resolved Hide resolved
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.

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
Copy link
Member Author

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 dhruvmanila force-pushed the dhruv/match-pattern-singleton branch from 666eb09 to 9321710 Compare October 30, 2023 05:25
@dhruvmanila 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
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.

3 participants