Skip to content

Commit

Permalink
Split Constant to individual literal nodes (#8064)
Browse files Browse the repository at this point in the history
## 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 |
  • Loading branch information
dhruvmanila committed Oct 30, 2023
1 parent 78bbf6d commit 230c9ce
Show file tree
Hide file tree
Showing 268 changed files with 6,020 additions and 6,098 deletions.
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ pub(crate) fn definitions(checker: &mut Checker) {
expr.start(),
));

if pydocstyle::helpers::should_ignore_docstring(expr) {
if expr.implicit_concatenated {
#[allow(deprecated)]
let location = checker.locator.compute_source_location(expr.start());
warn_user!(
Expand Down
41 changes: 13 additions & 28 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use ruff_python_ast::{self as ast, Arguments, Constant, Expr, ExprContext, Operator};
use ruff_python_ast::{self as ast, Arguments, Expr, ExprContext, Operator};
use ruff_python_literal::cformat::{CFormatError, CFormatErrorType};

use ruff_diagnostics::Diagnostic;
Expand Down Expand Up @@ -363,20 +363,18 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
]) {
if let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() {
let attr = attr.as_str();
if let Expr::Constant(ast::ExprConstant {
value: Constant::Str(val),
..
}) = value.as_ref()
if let Expr::StringLiteral(ast::ExprStringLiteral { value: string, .. }) =
value.as_ref()
{
if attr == "join" {
// "...".join(...) call
if checker.enabled(Rule::StaticJoinToFString) {
flynt::rules::static_join_to_fstring(checker, expr, val);
flynt::rules::static_join_to_fstring(checker, expr, string);
}
} else if attr == "format" {
// "...".format(...) call
let location = expr.range();
match pyflakes::format::FormatSummary::try_from(val.as_ref()) {
match pyflakes::format::FormatSummary::try_from(string.as_ref()) {
Err(e) => {
if checker.enabled(Rule::StringDotFormatInvalidFormat) {
checker.diagnostics.push(Diagnostic::new(
Expand Down Expand Up @@ -421,7 +419,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {

if checker.enabled(Rule::BadStringFormatCharacter) {
pylint::rules::bad_string_format_character::call(
checker, val, location,
checker, string, location,
);
}
}
Expand Down Expand Up @@ -993,11 +991,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
right,
range: _,
}) => {
if let Expr::Constant(ast::ExprConstant {
value: Constant::Str(ast::StringConstant { value, .. }),
..
}) = left.as_ref()
{
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = left.as_ref() {
if checker.any_enabled(&[
Rule::PercentFormatInvalidFormat,
Rule::PercentFormatExpectedMapping,
Expand Down Expand Up @@ -1234,38 +1228,29 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
refurb::rules::single_item_membership_test(checker, expr, left, ops, comparators);
}
}
Expr::Constant(ast::ExprConstant {
value: Constant::Int(_) | Constant::Float(_) | Constant::Complex { .. },
range: _,
}) => {
Expr::NumberLiteral(_) => {
if checker.source_type.is_stub() && checker.enabled(Rule::NumericLiteralTooLong) {
flake8_pyi::rules::numeric_literal_too_long(checker, expr);
}
}
Expr::Constant(ast::ExprConstant {
value: Constant::Bytes(_),
range: _,
}) => {
Expr::BytesLiteral(_) => {
if checker.source_type.is_stub() && checker.enabled(Rule::StringOrBytesTooLong) {
flake8_pyi::rules::string_or_bytes_too_long(checker, expr);
}
}
Expr::Constant(ast::ExprConstant {
value: Constant::Str(value),
range: _,
}) => {
Expr::StringLiteral(string) => {
if checker.enabled(Rule::HardcodedBindAllInterfaces) {
if let Some(diagnostic) =
flake8_bandit::rules::hardcoded_bind_all_interfaces(value, expr.range())
flake8_bandit::rules::hardcoded_bind_all_interfaces(string)
{
checker.diagnostics.push(diagnostic);
}
}
if checker.enabled(Rule::HardcodedTempFile) {
flake8_bandit::rules::hardcoded_tmp_directory(checker, expr, value);
flake8_bandit::rules::hardcoded_tmp_directory(checker, string);
}
if checker.enabled(Rule::UnicodeKindPrefix) {
pyupgrade::rules::unicode_kind_prefix(checker, expr, value.unicode);
pyupgrade::rules::unicode_kind_prefix(checker, string);
}
if checker.source_type.is_stub() {
if checker.enabled(Rule::StringOrBytesTooLong) {
Expand Down
16 changes: 4 additions & 12 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,8 @@ use std::path::Path;
use itertools::Itertools;
use log::debug;
use ruff_python_ast::{
self as ast, Arguments, Comprehension, Constant, ElifElseClause, ExceptHandler, Expr,
ExprContext, Keyword, MatchCase, Parameter, ParameterWithDefault, Parameters, Pattern, Stmt,
Suite, UnaryOp,
self as ast, Arguments, Comprehension, ElifElseClause, ExceptHandler, Expr, ExprContext,
Keyword, MatchCase, Parameter, ParameterWithDefault, Parameters, Pattern, Stmt, Suite, UnaryOp,
};
use ruff_text_size::{Ranged, TextRange, TextSize};

Expand Down Expand Up @@ -787,11 +786,7 @@ where
&& self.semantic.in_type_definition()
&& self.semantic.future_annotations()
{
if let Expr::Constant(ast::ExprConstant {
value: Constant::Str(value),
..
}) = expr
{
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = expr {
self.deferred.string_type_definitions.push((
expr.range(),
value,
Expand Down Expand Up @@ -1186,10 +1181,7 @@ where
}
}
}
Expr::Constant(ast::ExprConstant {
value: Constant::Str(value),
range: _,
}) => {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
if self.semantic.in_type_definition()
&& !self.semantic.in_literal()
&& !self.semantic.in_f_string()
Expand Down
18 changes: 9 additions & 9 deletions crates/ruff_linter/src/doc_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use std::iter::FusedIterator;

use ruff_python_ast::{self as ast, Constant, Expr, Stmt, Suite};
use ruff_python_ast::{self as ast, Stmt, Suite};
use ruff_python_parser::lexer::LexResult;
use ruff_python_parser::Tok;
use ruff_text_size::{Ranged, TextSize};
Expand Down Expand Up @@ -69,15 +69,15 @@ struct StringLinesVisitor<'a> {

impl StatementVisitor<'_> for StringLinesVisitor<'_> {
fn visit_stmt(&mut self, stmt: &Stmt) {
if let Stmt::Expr(ast::StmtExpr { value, range: _ }) = stmt {
if let Expr::Constant(ast::ExprConstant {
value: Constant::Str(..),
..
}) = value.as_ref()
{
if let Stmt::Expr(ast::StmtExpr {
value: expr,
range: _,
}) = stmt
{
if expr.is_string_literal_expr() {
for line in UniversalNewlineIterator::with_offset(
self.locator.slice(value.as_ref()),
value.start(),
self.locator.slice(expr.as_ref()),
expr.start(),
) {
self.string_lines.push(line.start());
}
Expand Down
19 changes: 6 additions & 13 deletions crates/ruff_linter/src/docstrings/extraction.rs
Original file line number Diff line number Diff line change
@@ -1,30 +1,23 @@
//! Extract docstrings from an AST.

use ruff_python_ast::{self as ast, Constant, Expr, Stmt};
use ruff_python_ast::{self as ast, Stmt};
use ruff_python_semantic::{Definition, DefinitionId, Definitions, Member, MemberKind};

/// Extract a docstring from a function or class body.
pub(crate) fn docstring_from(suite: &[Stmt]) -> Option<&Expr> {
pub(crate) fn docstring_from(suite: &[Stmt]) -> Option<&ast::ExprStringLiteral> {
let stmt = suite.first()?;
// Require the docstring to be a standalone expression.
let Stmt::Expr(ast::StmtExpr { value, range: _ }) = stmt else {
return None;
};
// Only match strings.
if !matches!(
value.as_ref(),
Expr::Constant(ast::ExprConstant {
value: Constant::Str(_),
..
})
) {
return None;
}
Some(value)
value.as_string_literal_expr()
}

/// Extract a docstring from a `Definition`.
pub(crate) fn extract_docstring<'a>(definition: &'a Definition<'a>) -> Option<&'a Expr> {
pub(crate) fn extract_docstring<'a>(
definition: &'a Definition<'a>,
) -> Option<&'a ast::ExprStringLiteral> {
match definition {
Definition::Module(module) => docstring_from(module.python_ast),
Definition::Member(member) => docstring_from(member.body()),
Expand Down
5 changes: 3 additions & 2 deletions crates/ruff_linter/src/docstrings/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::fmt::{Debug, Formatter};
use std::ops::Deref;

use ruff_python_ast::Expr;
use ruff_python_ast::ExprStringLiteral;
use ruff_python_semantic::Definition;
use ruff_text_size::{Ranged, TextRange};

Expand All @@ -14,7 +14,8 @@ pub(crate) mod styles;
#[derive(Debug)]
pub(crate) struct Docstring<'a> {
pub(crate) definition: &'a Definition<'a>,
pub(crate) expr: &'a Expr,
/// The literal AST node representing the docstring.
pub(crate) expr: &'a ExprStringLiteral,
/// The content of the docstring, including the leading and trailing quotes.
pub(crate) contents: &'a str,
/// The range of the docstring body (without the quotes). The range is relative to [`Self::contents`].
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::Constant;
use ruff_python_ast::Expr;
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -79,13 +78,7 @@ pub(crate) fn variable_name_task_id(
let keyword = arguments.find_keyword("task_id")?;

// If the keyword argument is not a string, we can't do anything.
let task_id = match &keyword.value {
Expr::Constant(constant) => match &constant.value {
Constant::Str(ast::StringConstant { value, .. }) => value,
_ => return None,
},
_ => return None,
};
let ast::ExprStringLiteral { value: task_id, .. } = keyword.value.as_string_literal_expr()?;

// If the target name is the same as the task_id, no violation.
if id == task_id {
Expand Down
25 changes: 11 additions & 14 deletions crates/ruff_linter/src/rules/flake8_2020/rules/compare.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, CmpOp, Constant, Expr};
use ruff_python_ast::{self as ast, CmpOp, Expr};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -230,16 +230,16 @@ pub(crate) fn compare(checker: &mut Checker, left: &Expr, ops: &[CmpOp], compara
Expr::Subscript(ast::ExprSubscript { value, slice, .. })
if is_sys(value, "version_info", checker.semantic()) =>
{
if let Expr::Constant(ast::ExprConstant {
value: Constant::Int(i),
if let Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(i),
..
}) = slice.as_ref()
{
if *i == 0 {
if let (
[CmpOp::Eq | CmpOp::NotEq],
[Expr::Constant(ast::ExprConstant {
value: Constant::Int(n),
[Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(n),
..
})],
) = (ops, comparators)
Expand All @@ -253,8 +253,8 @@ pub(crate) fn compare(checker: &mut Checker, left: &Expr, ops: &[CmpOp], compara
} else if *i == 1 {
if let (
[CmpOp::Lt | CmpOp::LtE | CmpOp::Gt | CmpOp::GtE],
[Expr::Constant(ast::ExprConstant {
value: Constant::Int(_),
[Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(_),
..
})],
) = (ops, comparators)
Expand All @@ -274,8 +274,8 @@ pub(crate) fn compare(checker: &mut Checker, left: &Expr, ops: &[CmpOp], compara
{
if let (
[CmpOp::Lt | CmpOp::LtE | CmpOp::Gt | CmpOp::GtE],
[Expr::Constant(ast::ExprConstant {
value: Constant::Int(_),
[Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(_),
..
})],
) = (ops, comparators)
Expand All @@ -294,13 +294,10 @@ pub(crate) fn compare(checker: &mut Checker, left: &Expr, ops: &[CmpOp], compara
if is_sys(left, "version", checker.semantic()) {
if let (
[CmpOp::Lt | CmpOp::LtE | CmpOp::Gt | CmpOp::GtE],
[Expr::Constant(ast::ExprConstant {
value: Constant::Str(s),
..
})],
[Expr::StringLiteral(ast::ExprStringLiteral { value, .. })],
) = (ops, comparators)
{
if s.len() == 1 {
if value.len() == 1 {
if checker.enabled(Rule::SysVersionCmpStr10) {
checker
.diagnostics
Expand Down
10 changes: 5 additions & 5 deletions crates/ruff_linter/src/rules/flake8_2020/rules/subscript.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Constant, Expr};
use ruff_python_ast::{self as ast, Expr};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -177,8 +177,8 @@ pub(crate) fn subscript(checker: &mut Checker, value: &Expr, slice: &Expr) {
step: None,
range: _,
}) => {
if let Expr::Constant(ast::ExprConstant {
value: Constant::Int(i),
if let Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(i),
..
}) = upper.as_ref()
{
Expand All @@ -194,8 +194,8 @@ pub(crate) fn subscript(checker: &mut Checker, value: &Expr, slice: &Expr) {
}
}

Expr::Constant(ast::ExprConstant {
value: Constant::Int(i),
Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(i),
..
}) => {
if *i == 2 && checker.enabled(Rule::SysVersion2) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::ReturnStatementVisitor;
use ruff_python_ast::identifier::Identifier;
use ruff_python_ast::statement_visitor::StatementVisitor;
use ruff_python_ast::{self as ast, Constant, Expr, ParameterWithDefault, Stmt};
use ruff_python_ast::{self as ast, Expr, ParameterWithDefault, Stmt};
use ruff_python_parser::typing::parse_type_annotation;
use ruff_python_semantic::analyze::visibility;
use ruff_python_semantic::Definition;
Expand Down Expand Up @@ -431,10 +431,7 @@ fn is_none_returning(body: &[Stmt]) -> bool {
visitor.visit_body(body);
for stmt in visitor.returns {
if let Some(value) = stmt.value.as_deref() {
if !matches!(
value,
Expr::Constant(constant) if constant.value.is_none()
) {
if !value.is_none_literal_expr() {
return false;
}
}
Expand All @@ -451,9 +448,10 @@ fn check_dynamically_typed<F>(
) where
F: FnOnce() -> String,
{
if let Expr::Constant(ast::ExprConstant {
if let Expr::StringLiteral(ast::ExprStringLiteral {
range,
value: Constant::Str(string),
value: string,
..
}) = annotation
{
// Quoted annotations
Expand Down
Loading

0 comments on commit 230c9ce

Please sign in to comment.