Introduce LiteralExpressionRef for all literals (#8339)

## Summary

This PR adds a new `LiteralExpressionRef` which wraps all of the literal
expression nodes in a single enum. This allows for a narrow type when
working exclusively with a literal node. Additionally, it also
implements a `Expr::as_literal_expr` method to return the new enum if
the expression is indeed a literal one.

A few rules have been updated to account for the new enum:
1. `redundant_literal_union`
2. `if_else_block_instead_of_dict_lookup`
3. `magic_value_comparison`

To account for the change in (2), a new `ComparableLiteral` has been
added which can be constructed from the new enum
(`ComparableLiteral::from(<LiteralExpressionRef>)`).

### Open Questions

1. The new `ComparableLiteral` can be exclusively used via the
`LiteralExpressionRef` enum. Should we remove all of the literal
variants from `ComparableExpr` and instead have a single
`ComparableExpr::Literal(ComparableLiteral)` variant instead?

## Test Plan

`cargo test`
This commit is contained in:
Dhruv Manilawala 2023-10-31 18:26:11 +05:30 committed by GitHub
parent a8d04cbd88
commit 97ae617fac
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 127 additions and 48 deletions

View file

@ -3,7 +3,7 @@ use std::fmt;
use ruff_diagnostics::{Diagnostic, Violation}; use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr}; use ruff_python_ast::{self as ast, Expr, LiteralExpressionRef};
use ruff_python_semantic::SemanticModel; use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged; use ruff_text_size::Ranged;
@ -59,7 +59,7 @@ impl Violation for RedundantLiteralUnion {
/// PYI051 /// PYI051
pub(crate) fn redundant_literal_union<'a>(checker: &mut Checker, union: &'a Expr) { pub(crate) fn redundant_literal_union<'a>(checker: &mut Checker, union: &'a Expr) {
let mut literal_exprs = Vec::new(); let mut typing_literal_exprs = Vec::new();
let mut builtin_types_in_union = FxHashSet::default(); let mut builtin_types_in_union = FxHashSet::default();
// Adds a member to `literal_exprs` for each value in a `Literal`, and any builtin types // Adds a member to `literal_exprs` for each value in a `Literal`, and any builtin types
@ -68,9 +68,9 @@ pub(crate) fn redundant_literal_union<'a>(checker: &mut Checker, union: &'a Expr
if let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr { if let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr {
if checker.semantic().match_typing_expr(value, "Literal") { if checker.semantic().match_typing_expr(value, "Literal") {
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() {
literal_exprs.extend(elts.iter()); typing_literal_exprs.extend(elts.iter());
} else { } else {
literal_exprs.push(slice); typing_literal_exprs.push(slice);
} }
} }
return; return;
@ -84,18 +84,20 @@ pub(crate) fn redundant_literal_union<'a>(checker: &mut Checker, union: &'a Expr
traverse_union(&mut func, checker.semantic(), union, None); traverse_union(&mut func, checker.semantic(), union, None);
for literal_expr in literal_exprs { for typing_literal_expr in typing_literal_exprs {
let Some(constant_type) = match_constant_type(literal_expr) else { let Some(literal_type) = match_literal_type(typing_literal_expr) else {
continue; continue;
}; };
if builtin_types_in_union.contains(&constant_type) { if builtin_types_in_union.contains(&literal_type) {
checker.diagnostics.push(Diagnostic::new( checker.diagnostics.push(Diagnostic::new(
RedundantLiteralUnion { RedundantLiteralUnion {
literal: SourceCodeSnippet::from_str(checker.locator().slice(literal_expr)), literal: SourceCodeSnippet::from_str(
builtin_type: constant_type, checker.locator().slice(typing_literal_expr),
),
builtin_type: literal_type,
}, },
literal_expr.range(), typing_literal_expr.range(),
)); ));
} }
} }
@ -143,19 +145,24 @@ fn match_builtin_type(expr: &Expr, semantic: &SemanticModel) -> Option<ExprType>
Some(result) Some(result)
} }
/// Return the [`ExprType`] of an [`Expr]` if it is a constant (e.g., an `int`, like `1`, or a /// Return the [`ExprType`] of an [`Expr`] if it is a literal (e.g., an `int`, like `1`, or a
/// `bool`, like `True`). /// `bool`, like `True`).
fn match_constant_type(expr: &Expr) -> Option<ExprType> { fn match_literal_type(expr: &Expr) -> Option<ExprType> {
let result = match expr { let Some(literal_expr) = expr.as_literal_expr() else {
Expr::BooleanLiteral(_) => ExprType::Bool, return None;
Expr::StringLiteral(_) => ExprType::Str, };
Expr::BytesLiteral(_) => ExprType::Bytes, let result = match literal_expr {
Expr::NumberLiteral(ast::ExprNumberLiteral { value, .. }) => match value { LiteralExpressionRef::BooleanLiteral(_) => ExprType::Bool,
LiteralExpressionRef::StringLiteral(_) => ExprType::Str,
LiteralExpressionRef::BytesLiteral(_) => ExprType::Bytes,
LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral { value, .. }) => match value {
ast::Number::Int(_) => ExprType::Int, ast::Number::Int(_) => ExprType::Int,
ast::Number::Float(_) => ExprType::Float, ast::Number::Float(_) => ExprType::Float,
ast::Number::Complex { .. } => ExprType::Complex, ast::Number::Complex { .. } => ExprType::Complex,
}, },
_ => return None, LiteralExpressionRef::NoneLiteral(_) | LiteralExpressionRef::EllipsisLiteral(_) => {
return None;
}
}; };
Some(result) Some(result)
} }

View file

@ -2,7 +2,7 @@ use rustc_hash::FxHashSet;
use ruff_diagnostics::{Diagnostic, Violation}; use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::comparable::ComparableLiteral;
use ruff_python_ast::helpers::contains_effect; use ruff_python_ast::helpers::contains_effect;
use ruff_python_ast::{self as ast, CmpOp, ElifElseClause, Expr, Stmt}; use ruff_python_ast::{self as ast, CmpOp, ElifElseClause, Expr, Stmt};
use ruff_python_semantic::analyze::typing::{is_sys_version_block, is_type_checking_block}; use ruff_python_semantic::analyze::typing::{is_sys_version_block, is_type_checking_block};
@ -70,9 +70,9 @@ pub(crate) fn if_else_block_instead_of_dict_lookup(checker: &mut Checker, stmt_i
let [expr] = comparators.as_slice() else { let [expr] = comparators.as_slice() else {
return; return;
}; };
if !expr.is_literal_expr() { let Some(literal_expr) = expr.as_literal_expr() else {
return; return;
} };
let [Stmt::Return(ast::StmtReturn { value, range: _ })] = body.as_slice() else { let [Stmt::Return(ast::StmtReturn { value, range: _ })] = body.as_slice() else {
return; return;
}; };
@ -95,8 +95,8 @@ pub(crate) fn if_else_block_instead_of_dict_lookup(checker: &mut Checker, stmt_i
} }
// The `expr` was checked to be a literal above, so this is safe. // The `expr` was checked to be a literal above, so this is safe.
let mut literals: FxHashSet<ComparableExpr> = FxHashSet::default(); let mut literals: FxHashSet<ComparableLiteral> = FxHashSet::default();
literals.insert(expr.into()); literals.insert(literal_expr.into());
for clause in elif_else_clauses { for clause in elif_else_clauses {
let ElifElseClause { test, body, .. } = clause; let ElifElseClause { test, body, .. } = clause;
@ -133,9 +133,9 @@ pub(crate) fn if_else_block_instead_of_dict_lookup(checker: &mut Checker, stmt_i
let [expr] = comparators.as_slice() else { let [expr] = comparators.as_slice() else {
return; return;
}; };
if !expr.is_literal_expr() { let Some(literal_expr) = expr.as_literal_expr() else {
return; return;
} };
if value.as_ref().is_some_and(|value| { if value.as_ref().is_some_and(|value| {
contains_effect(value, |id| checker.semantic().is_builtin(id)) contains_effect(value, |id| checker.semantic().is_builtin(id))
@ -144,7 +144,7 @@ pub(crate) fn if_else_block_instead_of_dict_lookup(checker: &mut Checker, stmt_i
}; };
// The `expr` was checked to be a literal above, so this is safe. // The `expr` was checked to be a literal above, so this is safe.
literals.insert(expr.into()); literals.insert(literal_expr.into());
} }
// Different `elif` // Different `elif`
_ => { _ => {

View file

@ -1,5 +1,5 @@
use itertools::Itertools; use itertools::Itertools;
use ruff_python_ast::{self as ast, Expr, Int, UnaryOp}; use ruff_python_ast::{self as ast, Expr, Int, LiteralExpressionRef, UnaryOp};
use ruff_diagnostics::{Diagnostic, Violation}; use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
@ -57,39 +57,39 @@ impl Violation for MagicValueComparison {
} }
} }
/// If an [`Expr`] is a literal (or unary operation on a literal), return the literal [`Expr`]. /// If an [`Expr`] is a literal (or unary operation on a literal), return the [`LiteralExpressionRef`].
fn as_literal(expr: &Expr) -> Option<&Expr> { fn as_literal(expr: &Expr) -> Option<LiteralExpressionRef<'_>> {
match expr { match expr {
Expr::UnaryOp(ast::ExprUnaryOp { Expr::UnaryOp(ast::ExprUnaryOp {
op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert, op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert,
operand, operand,
.. ..
}) if operand.is_literal_expr() => Some(operand.as_ref()), }) => operand.as_literal_expr(),
expr if expr.is_literal_expr() => Some(expr), _ => expr.as_literal_expr(),
_ => None,
} }
} }
fn is_magic_value(expr: &Expr, allowed_types: &[ConstantType]) -> bool { fn is_magic_value(literal_expr: LiteralExpressionRef, allowed_types: &[ConstantType]) -> bool {
if let Some(constant_type) = ConstantType::try_from_expr(expr) { if let Some(constant_type) = ConstantType::try_from_literal_expr(literal_expr) {
if allowed_types.contains(&constant_type) { if allowed_types.contains(&constant_type) {
return false; return false;
} }
} }
match expr { match literal_expr {
// Ignore `None`, `Bool`, and `Ellipsis` constants. // Ignore `None`, `Bool`, and `Ellipsis` constants.
Expr::NoneLiteral(_) | Expr::BooleanLiteral(_) | Expr::EllipsisLiteral(_) => false, LiteralExpressionRef::NoneLiteral(_)
| LiteralExpressionRef::BooleanLiteral(_)
| LiteralExpressionRef::EllipsisLiteral(_) => false,
// Special-case some common string and integer types. // Special-case some common string and integer types.
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => { LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
!matches!(value.as_str(), "" | "__main__") !matches!(value.as_str(), "" | "__main__")
} }
Expr::NumberLiteral(ast::ExprNumberLiteral { value, .. }) => match value { LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral { value, .. }) => match value {
ast::Number::Int(value) => !matches!(*value, Int::ZERO | Int::ONE), ast::Number::Int(value) => !matches!(*value, Int::ZERO | Int::ONE),
_ => true, _ => true,
}, },
Expr::BytesLiteral(_) => true, LiteralExpressionRef::BytesLiteral(_) => true,
_ => false,
} }
} }

View file

@ -3,7 +3,7 @@
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use ruff_macros::CacheKey; use ruff_macros::CacheKey;
use ruff_python_ast::{Expr, ExprNumberLiteral, Number}; use ruff_python_ast::{ExprNumberLiteral, LiteralExpressionRef, Number};
#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, CacheKey)] #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, CacheKey)]
#[serde(deny_unknown_fields, rename_all = "kebab-case")] #[serde(deny_unknown_fields, rename_all = "kebab-case")]
@ -17,16 +17,18 @@ pub enum ConstantType {
} }
impl ConstantType { impl ConstantType {
pub fn try_from_expr(expr: &Expr) -> Option<Self> { pub fn try_from_literal_expr(literal_expr: LiteralExpressionRef<'_>) -> Option<Self> {
match expr { match literal_expr {
Expr::StringLiteral(_) => Some(Self::Str), LiteralExpressionRef::StringLiteral(_) => Some(Self::Str),
Expr::BytesLiteral(_) => Some(Self::Bytes), LiteralExpressionRef::BytesLiteral(_) => Some(Self::Bytes),
Expr::NumberLiteral(ExprNumberLiteral { value, .. }) => match value { LiteralExpressionRef::NumberLiteral(ExprNumberLiteral { value, .. }) => match value {
Number::Int(_) => Some(Self::Int), Number::Int(_) => Some(Self::Int),
Number::Float(_) => Some(Self::Float), Number::Float(_) => Some(Self::Float),
Number::Complex { .. } => Some(Self::Complex), Number::Complex { .. } => Some(Self::Complex),
}, },
_ => None, LiteralExpressionRef::BooleanLiteral(_)
| LiteralExpressionRef::NoneLiteral(_)
| LiteralExpressionRef::EllipsisLiteral(_) => None,
} }
} }
} }

View file

@ -644,6 +644,39 @@ pub struct ExprFString<'a> {
values: Vec<ComparableExpr<'a>>, values: Vec<ComparableExpr<'a>>,
} }
#[derive(Debug, PartialEq, Eq, Hash)]
pub enum ComparableLiteral<'a> {
None,
Ellipsis,
Bool(&'a bool),
Str { value: &'a str, unicode: &'a bool },
Bytes { value: &'a [u8] },
Number(ComparableNumber<'a>),
}
impl<'a> From<ast::LiteralExpressionRef<'a>> for ComparableLiteral<'a> {
fn from(literal: ast::LiteralExpressionRef<'a>) -> Self {
match literal {
ast::LiteralExpressionRef::NoneLiteral(_) => Self::None,
ast::LiteralExpressionRef::EllipsisLiteral(_) => Self::Ellipsis,
ast::LiteralExpressionRef::BooleanLiteral(ast::ExprBooleanLiteral {
value, ..
}) => Self::Bool(value),
ast::LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral {
value,
unicode,
..
}) => Self::Str { value, unicode },
ast::LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => {
Self::Bytes { value }
}
ast::LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral { value, .. }) => {
Self::Number(value.into())
}
}
}
}
#[derive(Debug, PartialEq, Eq, Hash)] #[derive(Debug, PartialEq, Eq, Hash)]
pub struct ExprStringLiteral<'a> { pub struct ExprStringLiteral<'a> {
value: &'a str, value: &'a str,

View file

@ -336,3 +336,28 @@ impl Ranged for ExpressionRef<'_> {
} }
} }
} }
/// Unowned pendant to all the literal variants of [`ast::Expr`] that stores a
/// reference instead of an owned value.
#[derive(Copy, Clone, Debug, PartialEq, is_macro::Is)]
pub enum LiteralExpressionRef<'a> {
StringLiteral(&'a ast::ExprStringLiteral),
BytesLiteral(&'a ast::ExprBytesLiteral),
NumberLiteral(&'a ast::ExprNumberLiteral),
BooleanLiteral(&'a ast::ExprBooleanLiteral),
NoneLiteral(&'a ast::ExprNoneLiteral),
EllipsisLiteral(&'a ast::ExprEllipsisLiteral),
}
impl Ranged for LiteralExpressionRef<'_> {
fn range(&self) -> TextRange {
match self {
LiteralExpressionRef::StringLiteral(expression) => expression.range(),
LiteralExpressionRef::BytesLiteral(expression) => expression.range(),
LiteralExpressionRef::NumberLiteral(expression) => expression.range(),
LiteralExpressionRef::BooleanLiteral(expression) => expression.range(),
LiteralExpressionRef::NoneLiteral(expression) => expression.range(),
LiteralExpressionRef::EllipsisLiteral(expression) => expression.range(),
}
}
}

View file

@ -6,7 +6,7 @@ use std::fmt;
use std::fmt::Debug; use std::fmt::Debug;
use std::ops::Deref; use std::ops::Deref;
use crate::int; use crate::{int, LiteralExpressionRef};
use ruff_text_size::{Ranged, TextRange, TextSize}; use ruff_text_size::{Ranged, TextRange, TextSize};
/// See also [mod](https://docs.python.org/3/library/ast.html#ast.mod) /// See also [mod](https://docs.python.org/3/library/ast.html#ast.mod)
@ -640,6 +640,18 @@ impl Expr {
) )
} }
pub fn as_literal_expr(&self) -> Option<LiteralExpressionRef<'_>> {
match self {
Expr::StringLiteral(expr) => Some(LiteralExpressionRef::StringLiteral(expr)),
Expr::BytesLiteral(expr) => Some(LiteralExpressionRef::BytesLiteral(expr)),
Expr::NumberLiteral(expr) => Some(LiteralExpressionRef::NumberLiteral(expr)),
Expr::BooleanLiteral(expr) => Some(LiteralExpressionRef::BooleanLiteral(expr)),
Expr::NoneLiteral(expr) => Some(LiteralExpressionRef::NoneLiteral(expr)),
Expr::EllipsisLiteral(expr) => Some(LiteralExpressionRef::EllipsisLiteral(expr)),
_ => None,
}
}
pub fn is_implicit_concatenated_string(&self) -> bool { pub fn is_implicit_concatenated_string(&self) -> bool {
match self { match self {
Expr::StringLiteral(ExprStringLiteral { Expr::StringLiteral(ExprStringLiteral {