Skip to content

Commit

Permalink
WIP: Update string nodes for implicit string concatenation
Browse files Browse the repository at this point in the history
  • Loading branch information
dhruvmanila committed Nov 14, 2023
1 parent 96b265c commit 07e49e4
Show file tree
Hide file tree
Showing 57 changed files with 11,004 additions and 10,054 deletions.
16 changes: 9 additions & 7 deletions crates/ruff_linter/src/checkers/ast/analyze/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,21 +158,23 @@ pub(crate) fn definitions(checker: &mut Checker) {
}

// Extract a `Docstring` from a `Definition`.
let Some(expr) = docstring else {
let Some(string_literal) = docstring else {
pydocstyle::rules::not_missing(checker, definition, *visibility);
continue;
};

let contents = checker.locator().slice(expr);
let contents = checker.locator().slice(string_literal);

let indentation = checker.locator().slice(TextRange::new(
checker.locator.line_start(expr.start()),
expr.start(),
checker.locator.line_start(string_literal.start()),
string_literal.start(),
));

if expr.implicit_concatenated {
if string_literal.value.is_implicit_concatenated() {
#[allow(deprecated)]
let location = checker.locator.compute_source_location(expr.start());
let location = checker
.locator
.compute_source_location(string_literal.start());
warn_user!(
"Docstring at {}:{}:{} contains implicit string concatenation; ignoring...",
relativize_path(checker.path),
Expand All @@ -186,7 +188,7 @@ pub(crate) fn definitions(checker: &mut Checker) {
let body_range = raw_contents_range(contents).unwrap();
let docstring = Docstring {
definition,
expr,
expr: string_literal,
contents,
body_range,
indentation,
Expand Down
26 changes: 15 additions & 11 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,10 +372,11 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if let Expr::StringLiteral(ast::ExprStringLiteral { value: string, .. }) =
value.as_ref()
{
let string = string.as_str();
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);
}
} else if attr == "format" {
// "...".format(...) call
Expand Down Expand Up @@ -425,7 +426,7 @@ 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, location,
);
}
}
Expand Down Expand Up @@ -985,16 +986,17 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
pylint::rules::await_outside_async(checker, expr);
}
}
Expr::FString(fstring @ ast::ExprFString { values, .. }) => {
Expr::FString(fstring) => {
if checker.enabled(Rule::FStringMissingPlaceholders) {
pyflakes::rules::f_string_missing_placeholders(fstring, checker);
}
if checker.enabled(Rule::HardcodedSQLExpression) {
flake8_bandit::rules::hardcoded_sql_expression(checker, expr);
}
if checker.enabled(Rule::ExplicitFStringTypeConversion) {
ruff::rules::explicit_f_string_type_conversion(checker, expr, values);
}
// TODO
// if checker.enabled(Rule::HardcodedSQLExpression) {
// flake8_bandit::rules::hardcoded_sql_expression(checker, expr);
// }
// if checker.enabled(Rule::ExplicitFStringTypeConversion) {
// ruff::rules::explicit_f_string_type_conversion(checker, expr, values);
// }
}
Expr::BinOp(ast::ExprBinOp {
left,
Expand Down Expand Up @@ -1024,7 +1026,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
Rule::PercentFormatUnsupportedFormatCharacter,
]) {
let location = expr.range();
match pyflakes::cformat::CFormatSummary::try_from(value.as_str()) {
match pyflakes::cformat::CFormatSummary::try_from(value.as_str().as_ref()) {
Err(CFormatError {
typ: CFormatErrorType::UnsupportedFormatChar(c),
..
Expand Down Expand Up @@ -1273,7 +1275,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_bandit::rules::hardcoded_tmp_directory(checker, string);
}
if checker.enabled(Rule::UnicodeKindPrefix) {
pyupgrade::rules::unicode_kind_prefix(checker, string);
for string_part in string.value.parts() {
pyupgrade::rules::unicode_kind_prefix(checker, string_part);
}
}
if checker.source_type.is_stub() {
if checker.enabled(Rule::StringOrBytesTooLong) {
Expand Down
10 changes: 5 additions & 5 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,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 {
Expand Down Expand Up @@ -1188,7 +1188,7 @@ where
{
self.deferred.string_type_definitions.push((
expr.range(),
value,
value.as_str().as_ref(),
self.semantic.snapshot(),
));
}
Expand Down Expand Up @@ -1272,9 +1272,9 @@ where

fn visit_format_spec(&mut self, format_spec: &'b Expr) {
match format_spec {
Expr::FString(ast::ExprFString { values, .. }) => {
for value in values {
self.visit_expr(value);
Expr::FString(ast::ExprFString { value, .. }) => {
for expr in value.elements() {
self.visit_expr(expr);
}
}
_ => unreachable!("Unexpected expression for format_spec"),
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 id == task_id {
if task_id == id {
return None;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -455,9 +455,11 @@ fn check_dynamically_typed<F>(
}) = annotation
{
// Quoted annotations
if let Ok((parsed_annotation, _)) =
parse_type_annotation(string, *range, checker.locator().contents())
{
if let Ok((parsed_annotation, _)) = parse_type_annotation(
string.as_str().as_ref(),
*range,
checker.locator().contents(),
) {
if type_hint_resolves_to_any(
&parsed_annotation,
checker.semantic(),
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/flake8_bandit/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ static PASSWORD_CANDIDATE_REGEX: Lazy<Regex> = Lazy::new(|| {

pub(super) fn string_literal(expr: &Expr) -> Option<&str> {
match expr {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => Some(value),
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => Some(value.as_str().as_ref()),
_ => None,
}
}
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().as_ref(),
_ => return None,
},
// obj.password = "s3cr3t"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,17 @@ impl Violation for HardcodedTempFile {
}

/// S108
pub(crate) fn hardcoded_tmp_directory(checker: &mut Checker, string: &ast::ExprStringLiteral) {
pub(crate) fn hardcoded_tmp_directory(
checker: &mut Checker,
string_literal: &ast::ExprStringLiteral,
) {
let string = string_literal.value.as_str();
if !checker
.settings
.flake8_bandit
.hardcoded_tmp_directory
.iter()
.any(|prefix| string.value.starts_with(prefix))
.any(|prefix| string.starts_with(prefix))
{
return;
}
Expand All @@ -76,8 +80,8 @@ pub(crate) fn hardcoded_tmp_directory(checker: &mut Checker, string: &ast::ExprS

checker.diagnostics.push(Diagnostic::new(
HardcodedTempFile {
string: string.value.clone(),
string: string.into_owned(),
},
string.range,
string_literal.range,
));
}
Original file line number Diff line number Diff line change
Expand Up @@ -854,11 +854,11 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
["six", "moves", "urllib", "request", "urlopen" | "urlretrieve" | "Request"] => {
// 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: url, .. })) = &call.arguments.find_argument("url", 0) {
let url = url.trim_start();
if url.starts_with("http://") || url.starts_with("https://") {
return None;
}
if let Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) = &call.arguments.find_argument("url", 0) {
let url = value.as_str().trim_start();
if url.starts_with("http://") || url.starts_with("https://") {
return None;
}
}
}
Some(SuspiciousURLOpenUsage.into())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,11 @@ pub(crate) fn getattr_with_constant(
let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = arg else {
return;
};
if !is_identifier(value) {
let string = value.as_str();
if !is_identifier(&string) {
return;
}
if is_mangled_private(value) {
if is_mangled_private(&string) {
return;
}
if !checker.semantic().is_builtin("getattr") {
Expand All @@ -87,12 +88,12 @@ pub(crate) fn getattr_with_constant(
Expr::Name(_) | Expr::Attribute(_) | Expr::Subscript(_) | Expr::Call(_)
) && !checker.locator().contains_line_break(obj.range())
{
format!("{}.{}", checker.locator().slice(obj), value)
format!("{}.{}", checker.locator().slice(obj), string)
} else {
// Defensively parenthesize any other expressions. For example, attribute accesses
// on `int` literals must be parenthesized, e.g., `getattr(1, "real")` becomes
// `(1).real`. The same is true for named expressions and others.
format!("({}).{}", checker.locator().slice(obj), value)
format!("({}).{}", checker.locator().slice(obj), string)
},
expr.range(),
checker.locator(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,11 @@ pub(crate) fn setattr_with_constant(
let Expr::StringLiteral(ast::ExprStringLiteral { value: name, .. }) = name else {
return;
};
if !is_identifier(name) {
let name = name.as_str();
if !is_identifier(&name) {
return;
}
if is_mangled_private(name) {
if is_mangled_private(&name) {
return;
}
if !checker.semantic().is_builtin("setattr") {
Expand All @@ -104,7 +105,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, 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 @@ -92,10 +92,11 @@ fn check_log_record_attr_clash(checker: &mut Checker, extra: &Keyword) {
Expr::Dict(ast::ExprDict { keys, .. }) => {
for key in keys {
if let Some(key) = &key {
if let Expr::StringLiteral(ast::ExprStringLiteral { value: attr, .. }) = key {
if is_reserved_attr(attr) {
if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = key {
let attr = value.as_str();
if is_reserved_attr(&attr) {
checker.diagnostics.push(Diagnostic::new(
LoggingExtraAttrClash(attr.to_string()),
LoggingExtraAttrClash(attr.into_owned()),
key.range(),
));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,9 @@ 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) {
return Some(value);
let kwarg = value.as_str().as_ref();
if is_identifier(kwarg) {
return Some(kwarg);
}
}
None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,11 @@ pub(crate) fn unrecognized_platform(checker: &mut Checker, test: &Expr) {
// Other values are possible but we don't need them right now.
// This protects against typos.
if checker.enabled(Rule::UnrecognizedPlatformName) {
if !matches!(value.as_str(), "linux" | "win32" | "cygwin" | "darwin") {
let string = value.as_str();
if !matches!(string.as_ref(), "linux" | "win32" | "cygwin" | "darwin") {
checker.diagnostics.push(Diagnostic::new(
UnrecognizedPlatformName {
platform: value.clone(),
platform: string.into_owned(),
},
right.range(),
));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,13 @@ pub(super) fn is_empty_or_null_string(expr: &Expr) -> bool {
match expr {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => value.is_empty(),
Expr::NoneLiteral(_) => true,
Expr::FString(ast::ExprFString { values, .. }) => {
values.iter().all(is_empty_or_null_string)
Expr::FString(ast::ExprFString { value, .. }) => {
value.parts().all(|f_string_part| match f_string_part {
ast::FStringPartRef::Literal(literal) => literal.is_empty(),
ast::FStringPartRef::FString(f_string) => {
f_string.values.iter().all(is_empty_or_null_string)
}
})
}
_ => false,
}
Expand Down
Loading

0 comments on commit 07e49e4

Please sign in to comment.