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

Replace String with SmolStr for nodes Identifier and ExprName #9202

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ serde_json = { version = "1.0.108" }
shellexpand = { version = "3.0.0" }
similar = { version = "2.3.0", features = ["inline"] }
smallvec = { version = "1.11.2" }
smol_str = "0.2.0"
static_assertions = "1.1.0"
strum = { version = "0.25.0", features = ["strum_macros"] }
strum_macros = { version = "0.25.3" }
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ fn extract_import_map(path: &Path, package: Option<&Path>, blocks: &[&Block]) ->
}) => {
let level = level.unwrap_or_default() as usize;
let module = if let Some(module) = module {
let module: &String = module.as_ref();
let module = module.as_str();
if level == 0 {
Cow::Borrowed(module)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ pub(crate) fn variable_name_task_id(
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 task_id == id {
if task_id == id.as_str() {
return None;
}

Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/flake8_annotations/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ impl AutoPythonType {
)
.ok()?;
let expr = Expr::Name(ast::ExprName {
id: binding,
id: binding.into(),
range: TextRange::default(),
ctx: ExprContext::Load,
});
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_linter/src/rules/flake8_gettext/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ pub mod settings;
/// Returns true if the [`Expr`] is an internationalization function call.
pub(crate) fn is_gettext_func_call(func: &Expr, functions_names: &[String]) -> bool {
if let Expr::Name(ast::ExprName { id, .. }) = func {
functions_names.contains(id)
functions_names
.iter()
.any(|function_name| function_name == id)
} else {
false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ fn make_literal_expr(subscript: Option<Expr>, exprs: Vec<&Expr>) -> Expr {
subscript.unwrap().clone()
} else {
Expr::Name(ast::ExprName {
id: "Literal".to_string(),
id: "Literal".into(),
range: TextRange::default(),
ctx: ast::ExprContext::Load,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr)
.into_iter()
.map(|type_member| {
Expr::Name(ast::ExprName {
id: type_member,
id: type_member.into(),
ctx: ExprContext::Load,
range: TextRange::default(),
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ pub(crate) fn private_member_access(checker: &mut Checker, expr: &Expr) {
.settings
.flake8_self
.ignore_names
.contains(attr.as_ref())
.iter()
.any(|name| name == attr.as_str())
{
return;
}
Expand Down
6 changes: 5 additions & 1 deletion crates/ruff_linter/src/rules/pyflakes/rules/strings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,11 @@ pub(crate) fn string_dot_format_extra_named_arguments(
let missing: Vec<(usize, &str)> = keywords
.enumerate()
.filter_map(|(index, keyword)| {
if summary.keywords.contains(keyword.as_ref()) {
if summary
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
.keywords
.iter()
.any(|summary_keyword| summary_keyword == keyword.as_str())
{
None
} else {
Some((index, keyword.as_str()))
Expand Down
20 changes: 11 additions & 9 deletions crates/ruff_linter/src/rules/pylint/rules/no_method_decorator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,22 +116,24 @@ fn get_undecorated_methods(
{
if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() {
if id == method_name && checker.semantic().is_builtin(method_name) {
if arguments.args.len() != 1 {
if targets.len() != 1 {
continue;
}

if targets.len() != 1 {
let [arg] = arguments.args.as_slice() else {
continue;
}
};

let target_name = match targets.first() {
Some(Expr::Name(ast::ExprName { id, .. })) => id.to_string(),
_ => continue,
let Some(Expr::Name(ast::ExprName {
id: target_name, ..
})) = targets.first()
else {
continue;
};

if let Expr::Name(ast::ExprName { id, .. }) = &arguments.args[0] {
if target_name == *id {
explicit_decorator_calls.insert(id.clone(), stmt.range());
Copy link
Member

Choose a reason for hiding this comment

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

GitHub doesn't allow me to make a suggestion on the entire code. We can avoid calling to_string here by changing explicit_decorator_calls to HashMap<&str, TextRange>.

The id.to_string call on linne 128 also seems unnecessary (we can use the SmolStr directly or use as_str)

if let Expr::Name(ast::ExprName { id, .. }) = &arg {
if target_name == id {
explicit_decorator_calls.insert(id.to_string(), stmt.range());
}
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,12 @@ pub(crate) fn non_pep695_type_alias(checker: &mut Checker, stmt: &StmtAnnAssign)

// TODO(zanie): We should check for generic type variables used in the value and define them
// as type params instead
let mut diagnostic = Diagnostic::new(NonPEP695TypeAlias { name: name.clone() }, stmt.range());
let mut diagnostic = Diagnostic::new(
NonPEP695TypeAlias {
name: name.to_string(),
},
stmt.range(),
);
let mut visitor = TypeVarReferenceVisitor {
vars: vec![],
semantic: checker.semantic(),
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/refurb/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use ruff_text_size::TextRange;
pub(super) fn generate_method_call(name: &str, method: &str, generator: Generator) -> String {
// Construct `name`.
let var = ast::ExprName {
id: name.to_string(),
id: name.into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
Expand Down Expand Up @@ -43,7 +43,7 @@ pub(super) fn generate_none_identity_comparison(
) -> String {
// Construct `name`.
let var = ast::ExprName {
id: name.to_string(),
id: name.into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ fn make_suggestion(open: &FileOpen<'_>, generator: Generator) -> SourceCodeSnipp
ReadMode::Bytes => "read_bytes",
};
let name = ast::ExprName {
id: method_name.to_string(),
id: method_name.into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ fn try_construct_call(
/// Construct the call to `itertools.starmap` for suggestion.
fn construct_starmap_call(starmap_binding: String, iter: &Expr, func: &Expr) -> ast::ExprCall {
let starmap = ast::ExprName {
id: starmap_binding,
id: starmap_binding.into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
Expand All @@ -315,7 +315,7 @@ fn construct_starmap_call(starmap_binding: String, iter: &Expr, func: &Expr) ->
/// Wrap given function call with yet another call.
fn wrap_with_call_to(call: ast::ExprCall, func_name: &str) -> ast::ExprCall {
let name = ast::ExprName {
id: func_name.to_string(),
id: func_name.into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,15 +236,15 @@ impl fmt::Display for EnumerateSubset {
fn generate_range_len_call(name: &str, generator: Generator) -> String {
// Construct `name`.
let var = ast::ExprName {
id: name.to_string(),
id: name.into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
// Construct `len(name)`.
let len = ast::ExprCall {
func: Box::new(
ast::ExprName {
id: "len".to_string(),
id: "len".into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
}
Expand All @@ -261,7 +261,7 @@ fn generate_range_len_call(name: &str, generator: Generator) -> String {
let range = ast::ExprCall {
func: Box::new(
ast::ExprName {
id: "range".to_string(),
id: "range".into(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ fn generate_fix(checker: &Checker, conversion_type: ConversionType, expr: &Expr)
let new_expr = Expr::Subscript(ast::ExprSubscript {
range: TextRange::default(),
value: Box::new(Expr::Name(ast::ExprName {
id: binding,
id: binding.into(),
ctx: ast::ExprContext::Store,
range: TextRange::default(),
})),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_ast/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ rustc-hash = { workspace = true }
serde = { workspace = true, optional = true }
smallvec = { workspace = true }
static_assertions = "1.1.0"
smol_str = { workspace = true }

[dev-dependencies]
insta = { workspace = true }
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_python_ast/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1508,7 +1508,7 @@ pub fn pep_604_union(elts: &[Expr]) -> Expr {
pub fn typing_optional(elt: Expr, binding: String) -> Expr {
Expr::Subscript(ast::ExprSubscript {
value: Box::new(Expr::Name(ast::ExprName {
id: binding,
id: binding.into(),
range: TextRange::default(),
ctx: ExprContext::Load,
})),
Expand Down Expand Up @@ -1539,7 +1539,7 @@ pub fn typing_union(elts: &[Expr], binding: String) -> Expr {

Expr::Subscript(ast::ExprSubscript {
value: Box::new(Expr::Name(ast::ExprName {
id: binding,
id: binding.into(),
range: TextRange::default(),
ctx: ExprContext::Load,
})),
Expand Down Expand Up @@ -1605,7 +1605,7 @@ mod tests {
fn any_over_stmt_type_alias() {
let seen = RefCell::new(Vec::new());
let name = Expr::Name(ExprName {
id: "x".to_string(),
id: "x".into(),
range: TextRange::default(),
ctx: ExprContext::Load,
});
Expand Down
28 changes: 18 additions & 10 deletions crates/ruff_python_ast/src/nodes.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
#![allow(clippy::derive_partial_eq_without_eq)]

use itertools::Itertools;
use smol_str::SmolStr;
use std::cell::OnceCell;

use std::fmt;
use std::fmt::Debug;
use std::ops::Deref;
use std::slice::{Iter, IterMut};

use itertools::Itertools;

use ruff_text_size::{Ranged, TextRange, TextSize};

use crate::{int, LiteralExpressionRef};
Expand Down Expand Up @@ -1739,7 +1740,7 @@ impl From<ExprStarred> for Expr {
#[derive(Clone, Debug, PartialEq)]
pub struct ExprName {
pub range: TextRange,
pub id: String,
pub id: SmolStr,
Copy link
Member

Choose a reason for hiding this comment

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

How did you decide where to use SmolStr? Should ExprIpyEscapeCommand, StringLiteral, and FStringElement use SmolStr too or is it intentional that they do not because they're less likely to be short?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already answered most of this question in another comment, but I didn't mention ExprIpyEscapeCommand. I haven't seen many examples of real world use of ExprIpyEscapeCommand so I don't know if they are usually short, like Names and Identifiers. Can't really tell if they are a suitable candidate to use SmolStr.

pub ctx: ExprContext,
}

Expand Down Expand Up @@ -3225,13 +3226,13 @@ impl IpyEscapeKind {

#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct Identifier {
id: String,
range: TextRange,
pub id: SmolStr,
pub range: TextRange,
}

impl Identifier {
#[inline]
pub fn new(id: impl Into<String>, range: TextRange) -> Self {
pub fn new(id: impl Into<SmolStr>, range: TextRange) -> Self {
Self {
id: id.into(),
range,
Expand All @@ -3256,7 +3257,7 @@ impl PartialEq<str> for Identifier {
impl PartialEq<String> for Identifier {
#[inline]
fn eq(&self, other: &String) -> bool {
&self.id == other
self.id == other
}
}

Expand All @@ -3275,9 +3276,9 @@ impl AsRef<str> for Identifier {
}
}

impl AsRef<String> for Identifier {
impl AsRef<SmolStr> for Identifier {
#[inline]
fn as_ref(&self) -> &String {
fn as_ref(&self) -> &SmolStr {
&self.id
}
}
Expand All @@ -3291,6 +3292,13 @@ impl std::fmt::Display for Identifier {
impl From<Identifier> for String {
#[inline]
fn from(identifier: Identifier) -> String {
identifier.id.to_string()
}
}

impl From<Identifier> for SmolStr {
#[inline]
fn from(identifier: Identifier) -> Self {
identifier.id
}
}
Expand Down Expand Up @@ -3836,6 +3844,6 @@ mod size_assertions {
assert_eq_size!(StmtClassDef, [u8; 104]);
assert_eq_size!(StmtTry, [u8; 112]);
assert_eq_size!(Expr, [u8; 80]);
assert_eq_size!(Pattern, [u8; 96]);
assert_eq_size!(Pattern, [u8; 88]);
assert_eq_size!(Mod, [u8; 32]);
}
1 change: 1 addition & 0 deletions crates/ruff_python_parser/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ is-macro = { workspace = true }
itertools = { workspace = true }
lalrpop-util = { version = "0.20.0", default-features = false }
memchr = { workspace = true }
smol_str = { workspace = true }
unicode-ident = { workspace = true }
unicode_names2 = { workspace = true }
rustc-hash = { workspace = true }
Expand Down
3 changes: 2 additions & 1 deletion crates/ruff_python_parser/src/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use std::iter::FusedIterator;
use std::{char, cmp::Ordering, str::FromStr};

use smol_str::SmolStr;
use unicode_ident::{is_xid_continue, is_xid_start};

use ruff_python_ast::{Int, IpyEscapeKind};
Expand Down Expand Up @@ -241,7 +242,7 @@ impl<'source> Lexer<'source> {
"yield" => Tok::Yield,
_ => {
return Ok(Tok::Name {
name: text.to_string(),
name: SmolStr::new(text),
})
}
};
Expand Down
Loading
Loading