From 3a651d82479616e28209b2aa0d675d3bd69ab114 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Thu, 2 Jul 2026 13:29:38 +0900 Subject: [PATCH] fix --- crates/pyrefly_config/src/error_kind.rs | 3 + pyrefly/lib/alt/expr.rs | 173 +++++++++++++++++++++++- pyrefly/lib/test/helpful_string.rs | 55 ++++++++ pyrefly/lib/test/mod.rs | 1 + pyrefly/lib/test/util.rs | 10 ++ website/docs/error-kinds.mdx | 13 ++ 6 files changed, 249 insertions(+), 6 deletions(-) create mode 100644 pyrefly/lib/test/helpful_string.rs diff --git a/crates/pyrefly_config/src/error_kind.rs b/crates/pyrefly_config/src/error_kind.rs index b4759c1e5c..b94a473306 100644 --- a/crates/pyrefly_config/src/error_kind.rs +++ b/crates/pyrefly_config/src/error_kind.rs @@ -156,6 +156,8 @@ pub enum ErrorKind { DivisionByZero, /// Explicit usage of `typing.Any` in an annotation. ExplicitAny, + /// Formatting a value whose default string representation is unlikely to be helpful. + HelpfulString, /// Raised when a class implicitly becomes abstract by defining abstract members without /// inheriting from `abc.ABC` or using `abc.ABCMeta`. ImplicitAbstractClass, @@ -479,6 +481,7 @@ impl ErrorKind { ErrorKind::Deprecated => Severity::Warn, ErrorKind::DivisionByZero => Severity::Warn, ErrorKind::ExplicitAny => Severity::Ignore, + ErrorKind::HelpfulString => Severity::Ignore, ErrorKind::ImplicitAbstractClass => Severity::Ignore, ErrorKind::ImplicitAny => Severity::Ignore, ErrorKind::ImplicitAnyAttribute => Severity::Ignore, diff --git a/pyrefly/lib/alt/expr.rs b/pyrefly/lib/alt/expr.rs index c3386e44fc..bc5327c2bf 100644 --- a/pyrefly/lib/alt/expr.rs +++ b/pyrefly/lib/alt/expr.rs @@ -46,6 +46,7 @@ use pyrefly_util::visit::Visit; use ruff_python_ast::Arguments; use ruff_python_ast::BoolOp; use ruff_python_ast::Comprehension; +use ruff_python_ast::ConversionFlag; use ruff_python_ast::DictItem; use ruff_python_ast::Expr; use ruff_python_ast::ExprBinOp; @@ -56,7 +57,11 @@ use ruff_python_ast::ExprSlice; use ruff_python_ast::ExprStarred; use ruff_python_ast::ExprStringLiteral; use ruff_python_ast::ExprTuple; +use ruff_python_ast::FString; +use ruff_python_ast::FStringPart; use ruff_python_ast::Identifier; +use ruff_python_ast::InterpolatedElement; +use ruff_python_ast::InterpolatedStringElement; use ruff_python_ast::Keyword; use ruff_python_ast::Number; use ruff_python_ast::Operator; @@ -216,6 +221,12 @@ enum ConditionRedundantReason { InstanceAlwaysTruthy(Name), } +#[derive(Debug, Clone, Copy)] +enum HelpfulStringReason { + BadBuiltin, + MissingStringMethod, +} + impl ConditionRedundantReason { fn equivalent_boolean(&self) -> Option { match self { @@ -735,12 +746,9 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { } Expr::FString(x) => { let mut all_literal_strings = true; - x.visit(&mut |x| { - let fstring_expr_ty = self.expr_infer(x, errors); - if !fstring_expr_ty.is_literal_string() { - all_literal_strings = false; - } - }); + for part in &x.value { + self.fstring_part_infer(part, &mut all_literal_strings, errors); + } match Lit::from_fstring(x) { Some(lit) => lit.to_implicit_type(), _ if all_literal_strings => self.heap.mk_literal_string(LitStyle::Implicit), @@ -3568,6 +3576,159 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { .mk_type_of(self.specialize(cls, vec![target_ty], range, errors)) } + fn fstring_part_infer( + &self, + part: &FStringPart, + all_literal_strings: &mut bool, + errors: &ErrorCollector, + ) { + match part { + FStringPart::Literal(_) => {} + FStringPart::FString(fstring) => { + self.fstring_infer(fstring, all_literal_strings, errors) + } + } + } + + fn fstring_infer( + &self, + fstring: &FString, + all_literal_strings: &mut bool, + errors: &ErrorCollector, + ) { + for element in &fstring.elements { + let InterpolatedStringElement::Interpolation(interpolation) = element else { + continue; + }; + self.fstring_interpolation_infer(interpolation, all_literal_strings, errors); + } + } + + fn fstring_interpolation_infer( + &self, + interpolation: &InterpolatedElement, + all_literal_strings: &mut bool, + errors: &ErrorCollector, + ) { + let ty = self.expr_infer(&interpolation.expression, errors); + if !ty.is_literal_string() { + *all_literal_strings = false; + } + if interpolation.conversion == ConversionFlag::None { + self.check_helpful_string(&ty, interpolation.expression.range(), errors); + } + if let Some(format_spec) = &interpolation.format_spec { + for element in &format_spec.elements { + let InterpolatedStringElement::Interpolation(interpolation) = element else { + continue; + }; + self.fstring_interpolation_infer(interpolation, all_literal_strings, errors); + } + } + } + + fn check_helpful_string(&self, ty: &Type, range: TextRange, errors: &ErrorCollector) { + if let Some(reason) = self.get_helpful_string_reason(ty) { + let type_display = self.for_display(ty.clone()); + let msg = match reason { + HelpfulStringReason::BadBuiltin => { + format!("The string for `{type_display}` isn't helpful in a user-facing string") + } + HelpfulStringReason::MissingStringMethod => { + format!( + "The type `{type_display}` doesn't define a custom `__format__`, `__str__`, or `__repr__` method" + ) + } + }; + self.error(errors, range, ErrorKind::HelpfulString, msg); + } + } + + fn get_helpful_string_reason(&self, ty: &Type) -> Option { + match ty { + Type::Any(_) | Type::Never(_) => None, + Type::Literal(_) | Type::LiteralString(_) | Type::Tuple(_) | Type::TypedDict(_) => None, + Type::None => Some(HelpfulStringReason::BadBuiltin), + Type::Union(union) => union + .members + .iter() + .find_map(|member| self.get_helpful_string_reason(member)), + Type::ClassType(class_type) => { + self.get_class_helpful_string_reason(class_type.class_object()) + } + Type::ClassDef(cls) => { + if cls.module_name().as_str() == "builtins" { + None + } else { + Some(HelpfulStringReason::MissingStringMethod) + } + } + Type::Function(_) | Type::BoundMethod(_) | Type::Callable(_) | Type::Overload(_) => { + Some(HelpfulStringReason::MissingStringMethod) + } + _ => None, + } + } + + fn get_class_helpful_string_reason(&self, cls: &Class) -> Option { + let metadata = self.get_metadata_for_class(cls); + if metadata.dataclass_metadata().is_some() { + return None; + } + if cls.is_builtin("object") || self.class_has_bad_builtin_string(cls) { + return Some(HelpfulStringReason::BadBuiltin); + } + if self.class_has_custom_string_method(cls) { + return None; + } + if cls.module_name().as_str() == "builtins" || self.class_has_builtin_base(cls) { + None + } else { + Some(HelpfulStringReason::MissingStringMethod) + } + } + + fn class_has_custom_string_method(&self, cls: &Class) -> bool { + [cls] + .into_iter() + .chain( + self.get_mro_for_class(cls) + .ancestors_no_object() + .iter() + .map(|ancestor| ancestor.class_object()), + ) + .any(|cls| { + [dunder::FORMAT, dunder::STR, dunder::REPR] + .iter() + .any(|name| self.get_field_from_current_class_only(cls, name).is_some()) + }) + } + + fn class_has_bad_builtin_string(&self, cls: &Class) -> bool { + [cls] + .into_iter() + .chain( + self.get_mro_for_class(cls) + .ancestors_no_object() + .iter() + .map(|ancestor| ancestor.class_object()), + ) + .any(|cls| { + cls.module_name().as_str() == "builtins" + && matches!( + cls.name().as_str(), + "enumerate" | "filter" | "map" | "object" | "reversed" | "super" | "zip" + ) + }) + } + + fn class_has_builtin_base(&self, cls: &Class) -> bool { + self.get_mro_for_class(cls) + .ancestors_no_object() + .iter() + .any(|ancestor| ancestor.class_object().module_name().as_str() == "builtins") + } + fn class_subscript_infer( &self, cls: &Class, diff --git a/pyrefly/lib/test/helpful_string.rs b/pyrefly/lib/test/helpful_string.rs new file mode 100644 index 0000000000..af3b31a66e --- /dev/null +++ b/pyrefly/lib/test/helpful_string.rs @@ -0,0 +1,55 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +use crate::test::util::TestEnv; +use crate::testcase; + +testcase!( + test_helpful_string, + TestEnv::new().enable_helpful_string_error(), + r#" +from dataclasses import dataclass + +class A: ... +f"{A()}" # E: doesn't define a custom `__format__`, `__str__`, or `__repr__` method + +def returns_none() -> None: + pass +f"{returns_none()}" # E: The string for `None` isn't helpful in a user-facing string + +o = object() +f"{o}" # E: The string for `object` isn't helpful in a user-facing string + +def func(o: object) -> None: ... +f"{func}" # E: doesn't define a custom `__format__`, `__str__`, or `__repr__` method + +class WithStr: + def __str__(self) -> str: + return "ok" +f"{WithStr()}" + +class WithRepr: + def __repr__(self) -> str: + return "ok" +f"{WithRepr()}" + +class WithFormat: + def __format__(self, format_spec: str) -> str: + return "ok" +f"{WithFormat()}" + +@dataclass +class Data: + x: int +f"{Data(1)}" + +f"{A()!s}" +f"{A()!r}" +f"{1}" +f"{'x'}" +"#, +); diff --git a/pyrefly/lib/test/mod.rs b/pyrefly/lib/test/mod.rs index 050e44312e..ceecec976b 100644 --- a/pyrefly/lib/test/mod.rs +++ b/pyrefly/lib/test/mod.rs @@ -41,6 +41,7 @@ mod generic_callable_degeneracy; mod generic_legacy; mod generic_restrictions; mod generic_sub; +mod helpful_string; mod imports; mod incremental; mod inference; diff --git a/pyrefly/lib/test/util.rs b/pyrefly/lib/test/util.rs index c6a447f053..b9b974c07b 100644 --- a/pyrefly/lib/test/util.rs +++ b/pyrefly/lib/test/util.rs @@ -113,6 +113,7 @@ pub struct TestEnv { implicit_any_parameter_error: bool, implicit_any_attribute_error: bool, implicit_abstract_class_error: bool, + helpful_string_error: bool, open_unpacking_error: bool, missing_override_decorator_error: bool, not_required_key_access_error: bool, @@ -150,6 +151,7 @@ impl TestEnv { implicit_any_parameter_error: false, implicit_any_attribute_error: false, implicit_abstract_class_error: false, + helpful_string_error: false, open_unpacking_error: false, missing_override_decorator_error: false, not_required_key_access_error: false, @@ -288,6 +290,11 @@ impl TestEnv { self } + pub fn enable_helpful_string_error(mut self) -> Self { + self.helpful_string_error = true; + self + } + pub fn enable_open_unpacking_error(mut self) -> Self { self.open_unpacking_error = true; self @@ -459,6 +466,9 @@ impl TestEnv { if self.implicit_abstract_class_error { errors.set_error_severity(ErrorKind::ImplicitAbstractClass, Severity::Error); } + if self.helpful_string_error { + errors.set_error_severity(ErrorKind::HelpfulString, Severity::Error); + } if self.open_unpacking_error { errors.set_error_severity(ErrorKind::OpenUnpacking, Severity::Error); } diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index b6edbaf73e..d901e58488 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -489,6 +489,19 @@ def f(x: object) -> object: return x ``` +## helpful-string + +Default severity: `ignore` + +This diagnostic catches values in f-strings whose default string representation is unlikely to be useful in a user-facing message, such as `None` or an object whose class does not define a custom `__str__`, `__repr__`, or `__format__`. + +```python +class User: ... + +f"{User()}" # helpful-string +f"{None}" # helpful-string +``` + ## implicit-abstract-class Default severity: `ignore`