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

## Summary

This PR is a follow-up to the AST refactor which does the following:
- Remove `Deref` implementation on `StringLiteralValue` and use explicit
`as_str` calls instead. The `Deref` implementation would implicitly
perform allocations in case of implicitly concatenated strings. This is
to make sure the allocation is explicit.
- Now, certain methods can be implemented to do zero allocations which
have been implemented in this PR. They are:
    - `is_empty`
    - `len`
    - `chars`
    - Custom `PartialEq` implementation to compare each character

## Test Plan

Run the linter test suite and make sure all tests pass.
This commit is contained in:
Dhruv Manilawala 2023-11-24 18:03:59 -06:00 committed by GitHub
parent 017e829115
commit 626b0577cd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
28 changed files with 95 additions and 77 deletions

View File

@ -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(),
);
}
} 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(
@ -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,
);
}
}

View File

@ -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(),
self.semantic.snapshot(),
));
} else {
@ -1219,7 +1219,7 @@ where
{
self.deferred.string_type_definitions.push((
expr.range(),
value,
value.as_str(),
self.semantic.snapshot(),
));
}

View File

@ -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,

View File

@ -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"

View File

@ -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 {
@ -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()),

View File

@ -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;
}

View File

@ -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;
}

View File

@ -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") {

View File

@ -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") {
@ -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);

View File

@ -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;
}
};

View File

@ -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(),

View File

@ -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());
}
}

View File

@ -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 => {
@ -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, .. }) => {

View File

@ -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;
}
@ -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;
}

View File

@ -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
}

View File

@ -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() {

View File

@ -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() {

View File

@ -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(),

View File

@ -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`.

View File

@ -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()
}

View File

@ -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,
})

View File

@ -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);

View File

@ -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,
@ -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,

View File

@ -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
}
@ -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 {

View File

@ -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,

View File

@ -108,11 +108,10 @@ impl<'a> TypingTarget<'a> {
..
}) => Some(TypingTarget::PEP604Union(left, right)),
Expr::NoneLiteral(_) => Some(TypingTarget::None),
Expr::StringLiteral(ast::ExprStringLiteral {
value: string,
range,
}) => parse_type_annotation(string, *range, locator.contents())
.map_or(None, |(expr, _)| Some(TypingTarget::ForwardReference(expr))),
Expr::StringLiteral(ast::ExprStringLiteral { value, range }) => {
parse_type_annotation(value.as_str(), *range, locator.contents())
.map_or(None, |(expr, _)| Some(TypingTarget::ForwardReference(expr)))
}
_ => semantic.resolve_call_path(expr).map_or(
// If we can't resolve the call path, it must be defined in the
// same file, so we assume it's `Any` as it could be a type alias.

View File

@ -24,7 +24,7 @@ where
fn add_to_names<'a>(elts: &'a [Expr], names: &mut Vec<&'a str>, flags: &mut DunderAllFlags) {
for elt in elts {
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = elt {
names.push(value);
names.push(value.as_str());
} else {
*flags |= DunderAllFlags::INVALID_OBJECT;
}

View File

@ -1213,7 +1213,26 @@ impl StringLiteralValue {
}
}
/// Returns `true` if the string literal value is empty.
pub fn is_empty(&self) -> bool {
self.len() == 0
}
/// Returns the total length of the string literal value, in bytes, not
/// [`char`]s or graphemes.
pub fn len(&self) -> usize {
self.parts().fold(0, |acc, part| acc + part.value.len())
}
/// Returns an iterator over the [`char`]s of each string literal part.
pub fn chars(&self) -> impl Iterator<Item = char> + '_ {
self.parts().flat_map(|part| part.value.chars())
}
/// Returns the concatenated string value as a [`str`].
///
/// Note that this will perform an allocation on the first invocation if the
/// string value is implicitly concatenated.
pub fn as_str(&self) -> &str {
match &self.inner {
StringLiteralValueInner::Single(value) => value.as_str(),
@ -1224,21 +1243,17 @@ impl StringLiteralValue {
impl PartialEq<str> for StringLiteralValue {
fn eq(&self, other: &str) -> bool {
self.as_str() == other
if self.len() != other.len() {
return false;
}
// The `zip` here is safe because we have checked the length of both parts.
self.chars().zip(other.chars()).all(|(c1, c2)| c1 == c2)
}
}
impl PartialEq<String> for StringLiteralValue {
fn eq(&self, other: &String) -> bool {
self.as_str() == other
}
}
impl Deref for StringLiteralValue {
type Target = str;
fn deref(&self) -> &Self::Target {
self.as_str()
self == other.as_str()
}
}