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

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

Merged
merged 16 commits into from
Nov 25, 2023
Merged
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
18 changes: 13 additions & 5 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,18 +369,24 @@ 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::StringLiteral(ast::ExprStringLiteral { value: string, .. }) =
value.as_ref()
if let Expr::StringLiteral(ast::ExprStringLiteral {
value: string_value,
..
}) = value.as_ref()
{
if attr == "join" {
// "...".join(...) call
if checker.enabled(Rule::StaticJoinToFString) {
flynt::rules::static_join_to_fstring(checker, expr, string);
flynt::rules::static_join_to_fstring(
checker,
expr,
string_value.as_str(),
);
}
Comment on lines 379 to 385
Copy link
Member

Choose a reason for hiding this comment

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

I think the previous code was easier to read than now where we have the explicit as_str call and using Deref seemed to work just fine?

Copy link
Member

Choose a reason for hiding this comment

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

The motivation here is that the Deref implementation includes an allocation -- the conversion here can allocate if the string is implicitly concatenated. So Deref is essentially hiding an allocation. I prefer that we do something explicit over an implicit allocation. It's not about the lifetimes or anything like that. It's intentionally more verbose.

Copy link
Member

@MichaReiser MichaReiser Nov 29, 2023

Choose a reason for hiding this comment

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

I think we should then call the method to_str rather than as_str according to https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv Because I assume that a function called as to be free (or extremely cheap), whereas to might be cheap, but it depends, and into_ consumes self

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think using to_str makes more sense. I was not aware of that. I'll create a follow-up PR to rename that.

} else if attr == "format" {
// "...".format(...) call
let location = expr.range();
match pyflakes::format::FormatSummary::try_from(string.as_ref()) {
match pyflakes::format::FormatSummary::try_from(string_value.as_str()) {
Err(e) => {
if checker.enabled(Rule::StringDotFormatInvalidFormat) {
checker.diagnostics.push(Diagnostic::new(
Expand Down Expand Up @@ -425,7 +431,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {

if checker.enabled(Rule::BadStringFormatCharacter) {
pylint::rules::bad_string_format_character::call(
checker, string, location,
checker,
string_value.as_str(),
location,
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,7 @@ where
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = expr {
self.deferred.string_type_definitions.push((
expr.range(),
value,
value.as_str(),
Copy link
Member

Choose a reason for hiding this comment

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

Same for all these changes. If we could keep the Deref implementation in addition to the explicit as_str method, then code that doesn't need the more relaxed lifetime could be left as is.

self.semantic.snapshot(),
));
} else {
Expand Down Expand Up @@ -1219,7 +1219,7 @@ where
{
self.deferred.string_type_definitions.push((
expr.range(),
value,
value.as_str(),
self.semantic.snapshot(),
));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -505,14 +505,10 @@ fn check_dynamically_typed<F>(
) where
F: FnOnce() -> String,
{
if let Expr::StringLiteral(ast::ExprStringLiteral {
range,
value: string,
}) = annotation
{
if let Expr::StringLiteral(ast::ExprStringLiteral { range, value }) = annotation {
// Quoted annotations
if let Ok((parsed_annotation, _)) =
parse_type_annotation(string, *range, checker.locator().contents())
parse_type_annotation(value.as_str(), *range, checker.locator().contents())
{
if type_hint_resolves_to_any(
&parsed_annotation,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn password_target(target: &Expr) -> Option<&str> {
Expr::Name(ast::ExprName { id, .. }) => id.as_str(),
// d["password"] = "s3cr3t"
Expr::Subscript(ast::ExprSubscript { slice, .. }) => match slice.as_ref() {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => value,
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => value.as_str(),
_ => return None,
},
// obj.password = "s3cr3t"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) {
let Some(string) = left.as_string_literal_expr() else {
return;
};
string.value.escape_default().to_string()
string.value.as_str().escape_default().to_string()
}
Expr::Call(ast::ExprCall { func, .. }) => {
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else {
Expand All @@ -106,7 +106,7 @@ pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) {
let Some(string) = value.as_string_literal_expr() else {
return;
};
string.value.escape_default().to_string()
string.value.as_str().escape_default().to_string()
}
// f"select * from table where val = {val}"
Expr::FString(f_string) => concatenated_f_string(f_string, checker.locator()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ pub(crate) fn hardcoded_tmp_directory(checker: &mut Checker, string: &ast::ExprS
.flake8_bandit
.hardcoded_tmp_directory
.iter()
.any(|prefix| string.value.starts_with(prefix))
.any(|prefix| string.value.as_str().starts_with(prefix))
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
// If the `url` argument is a string literal, allow `http` and `https` schemes.
if call.arguments.args.iter().all(|arg| !arg.is_starred_expr()) && call.arguments.keywords.iter().all(|keyword| keyword.arg.is_some()) {
if let Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) = &call.arguments.find_argument("url", 0) {
let url = value.trim_start();
let url = value.as_str().trim_start();
if url.starts_with("http://") || url.starts_with("https://") {
return None;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,10 @@ pub(crate) fn getattr_with_constant(
let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = arg else {
return;
};
if !is_identifier(value) {
if !is_identifier(value.as_str()) {
return;
}
if is_mangled_private(value) {
if is_mangled_private(value.as_str()) {
return;
}
if !checker.semantic().is_builtin("getattr") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ pub(crate) fn setattr_with_constant(
let Expr::StringLiteral(ast::ExprStringLiteral { value: name, .. }) = name else {
return;
};
if !is_identifier(name) {
if !is_identifier(name.as_str()) {
return;
}
if is_mangled_private(name) {
if is_mangled_private(name.as_str()) {
return;
}
if !checker.semantic().is_builtin("setattr") {
Expand All @@ -104,7 +104,7 @@ pub(crate) fn setattr_with_constant(
if expr == child.as_ref() {
let mut diagnostic = Diagnostic::new(SetAttrWithConstant, expr.range());
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
assignment(obj, name, value, checker.generator()),
assignment(obj, name.as_str(), value, checker.generator()),
expr.range(),
)));
checker.diagnostics.push(diagnostic);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ pub(crate) fn call_datetime_strptime_without_zone(checker: &mut Checker, call: &
if let Some(Expr::StringLiteral(ast::ExprStringLiteral { value: format, .. })) =
call.arguments.args.get(1).as_ref()
{
if format.contains("%z") {
if format.as_str().contains("%z") {
return;
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ fn check_log_record_attr_clash(checker: &mut Checker, extra: &Keyword) {
for key in keys {
if let Some(key) = &key {
if let Expr::StringLiteral(ast::ExprStringLiteral { value: attr, .. }) = key {
if is_reserved_attr(attr) {
if is_reserved_attr(attr.as_str()) {
checker.diagnostics.push(Diagnostic::new(
LoggingExtraAttrClash(attr.to_string()),
key.range(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, expr: &Expr, kwargs
/// Return `Some` if a key is a valid keyword argument name, or `None` otherwise.
fn as_kwarg(key: &Expr) -> Option<&str> {
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = key {
if is_identifier(value) {
if is_identifier(value.as_str()) {
return Some(value.as_str());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,8 @@ fn check_names(checker: &mut Checker, decorator: &Decorator, expr: &Expr) {
let names_type = checker.settings.flake8_pytest_style.parametrize_names_type;

match expr {
Expr::StringLiteral(ast::ExprStringLiteral { value: string, .. }) => {
let names = split_names(string);
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
let names = split_names(value.as_str());
if names.len() > 1 {
match names_type {
types::ParametrizeNameType::Tuple => {
Expand Down Expand Up @@ -475,12 +475,11 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) {
.flake8_pytest_style
.parametrize_values_row_type;

let is_multi_named =
if let Expr::StringLiteral(ast::ExprStringLiteral { value: string, .. }) = &names {
split_names(string).len() > 1
} else {
true
};
let is_multi_named = if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &names {
split_names(value.as_str()).len() > 1
} else {
true
};

match values {
Expr::List(ast::ExprList { elts, .. }) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,11 @@ pub(crate) fn use_capital_environment_variables(checker: &mut Checker, expr: &Ex
return;
}

if is_lowercase_allowed(env_var) {
if is_lowercase_allowed(env_var.as_str()) {
return;
}

let capital_env_var = env_var.to_ascii_uppercase();
let capital_env_var = env_var.as_str().to_ascii_uppercase();
if capital_env_var == env_var.as_str() {
return;
}
Expand Down Expand Up @@ -201,11 +201,11 @@ fn check_os_environ_subscript(checker: &mut Checker, expr: &Expr) {
return;
};

if is_lowercase_allowed(env_var) {
if is_lowercase_allowed(env_var.as_str()) {
return;
}

let capital_env_var = env_var.to_ascii_uppercase();
let capital_env_var = env_var.as_str().to_ascii_uppercase();
if capital_env_var == env_var.as_str() {
return;
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/pylint/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub(super) fn type_param_name(arguments: &Arguments) -> Option<&str> {
// Handle both `TypeVar("T")` and `TypeVar(name="T")`.
let name_param = arguments.find_argument("name", 0)?;
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &name_param {
Some(value)
Some(value.as_str())
} else {
None
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/pylint/rules/bad_open_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ impl TryFrom<char> for OpenMode {
}

/// Returns `true` if the open mode is valid.
fn is_valid_mode(mode: &str) -> bool {
fn is_valid_mode(mode: &ast::StringLiteralValue) -> bool {
// Flag duplicates and invalid characters.
let mut flags = OpenMode::empty();
for char in mode.chars() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl fmt::Display for RemovalKind {

/// Return `true` if a string contains duplicate characters, taking into account
/// escapes.
fn has_duplicates(s: &str) -> bool {
fn has_duplicates(s: &ast::StringLiteralValue) -> bool {
let mut escaped = false;
let mut seen = FxHashSet::default();
for ch in s.chars() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ pub(crate) fn repeated_keyword_argument(checker: &mut Checker, call: &ExprCall)
// Ex) `func(**{"a": 1, "a": 2})`
for key in keys.iter().flatten() {
if let Expr::StringLiteral(ExprStringLiteral { value, .. }) = key {
if !seen.insert(value) {
if !seen.insert(value.as_str()) {
checker.diagnostics.push(Diagnostic::new(
RepeatedKeywordArgument {
duplicate_keyword: value.to_string(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,12 @@ pub(crate) fn unspecified_encoding(checker: &mut Checker, call: &ast::ExprCall)

/// Returns `true` if the given expression is a string literal containing a `b` character.
fn is_binary_mode(expr: &ast::Expr) -> Option<bool> {
Some(expr.as_string_literal_expr()?.value.contains('b'))
Some(
expr.as_string_literal_expr()?
.value
.chars()
.any(|c| c == 'b'),
)
}

/// Returns `true` if the given call lacks an explicit `encoding`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,13 +185,13 @@ fn create_fields_from_fields_arg(fields: &Expr) -> Option<Vec<Stmt>> {
return None;
}
let ast::ExprStringLiteral { value: field, .. } = field.as_string_literal_expr()?;
if !is_identifier(field) {
if !is_identifier(field.as_str()) {
return None;
}
if is_dunder(field) {
if is_dunder(field.as_str()) {
return None;
}
Some(create_field_assignment_stmt(field, annotation))
Some(create_field_assignment_stmt(field.as_str(), annotation))
})
.collect()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,13 @@ fn fields_from_dict_literal(keys: &[Option<Expr>], values: &[Expr]) -> Option<Ve
.zip(values.iter())
.map(|(key, value)| match key {
Some(Expr::StringLiteral(ast::ExprStringLiteral { value: field, .. })) => {
if !is_identifier(field) {
if !is_identifier(field.as_str()) {
return None;
}
if is_dunder(field) {
if is_dunder(field.as_str()) {
return None;
}
Some(create_field_assignment_stmt(field, value))
Some(create_field_assignment_stmt(field.as_str(), value))
}
_ => None,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,14 +228,14 @@ fn clean_params_dictionary(right: &Expr, locator: &Locator, stylist: &Stylist) -
}) = key
{
// If the dictionary key is not a valid variable name, abort.
if !is_identifier(key_string) {
if !is_identifier(key_string.as_str()) {
return None;
}
// If there are multiple entries of the same key, abort.
if seen.contains(&key_string.as_str()) {
return None;
}
seen.push(key_string);
seen.push(key_string.as_str());
if is_multi_line {
if indent.is_none() {
indent = indentation(locator, key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub(crate) fn redundant_open_modes(checker: &mut Checker, call: &ast::ExprCall)
..
}) = &keyword.value
{
if let Ok(mode) = OpenMode::from_str(mode_param_value) {
if let Ok(mode) = OpenMode::from_str(mode_param_value.as_str()) {
checker.diagnostics.push(create_check(
call,
&keyword.value,
Expand All @@ -91,7 +91,7 @@ pub(crate) fn redundant_open_modes(checker: &mut Checker, call: &ast::ExprCall)
}
Some(mode_param) => {
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &mode_param {
if let Ok(mode) = OpenMode::from_str(value) {
if let Ok(mode) = OpenMode::from_str(value.as_str()) {
checker.diagnostics.push(create_check(
call,
mode_param,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ fn match_encoded_variable(func: &Expr) -> Option<&Expr> {

fn is_utf8_encoding_arg(arg: &Expr) -> bool {
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &arg {
UTF8_LITERALS.contains(&value.to_lowercase().as_str())
UTF8_LITERALS.contains(&value.as_str().to_lowercase().as_str())
} else {
false
}
Expand Down Expand Up @@ -161,7 +161,7 @@ pub(crate) fn unnecessary_encode_utf8(checker: &mut Checker, call: &ast::ExprCal
Expr::StringLiteral(ast::ExprStringLiteral { value: literal, .. }) => {
// Ex) `"str".encode()`, `"str".encode("utf-8")`
if let Some(encoding_arg) = match_encoding_arg(&call.arguments) {
if literal.is_ascii() {
if literal.as_str().is_ascii() {
// Ex) Convert `"foo".encode()` to `b"foo"`.
let mut diagnostic = Diagnostic::new(
UnnecessaryEncodeUTF8 {
Expand Down
8 changes: 2 additions & 6 deletions crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,10 @@ pub(crate) fn implicit_optional(checker: &mut Checker, parameters: &Parameters)
continue;
};

if let Expr::StringLiteral(ast::ExprStringLiteral {
range,
value: string,
}) = annotation.as_ref()
{
if let Expr::StringLiteral(ast::ExprStringLiteral { range, value }) = annotation.as_ref() {
// Quoted annotation.
if let Ok((annotation, kind)) =
parse_type_annotation(string, *range, checker.locator().contents())
parse_type_annotation(value.as_str(), *range, checker.locator().contents())
{
let Some(expr) = type_hint_explicitly_allows_none(
&annotation,
Expand Down
Loading
Loading