fix helpful string check #4010

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,

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<bool> {
         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<HelpfulStringReason> {
+        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<HelpfulStringReason> {
+        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,

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'}"
+"#,
+);

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;

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

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`