Clean up some misc. code in NamedTuple and TypedDict conversion (#6957)

- Use `Option` instead of `Result` everywhere.
- Use `field` instead of `property` (to match the nomenclature of
`NamedTuple` and `TypedDict`).
- Put the violation function at the top of the file, rather than the
bottom.

crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs

@@ -1,16 +1,15 @@
-use anyhow::{bail, Result};
 use log::debug;
-use ruff_python_ast::{
-    self as ast, Arguments, Constant, Expr, ExprContext, Identifier, Keyword, Stmt,
-};
-use ruff_text_size::{Ranged, TextRange};
 
 use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::helpers::is_dunder;
+use ruff_python_ast::{
+    self as ast, Arguments, Constant, Expr, ExprContext, Identifier, Keyword, Stmt,
+};
 use ruff_python_codegen::Generator;
 use ruff_python_semantic::SemanticModel;
 use ruff_python_stdlib::identifiers::is_identifier;
+use ruff_text_size::{Ranged, TextRange};
 
 use crate::checkers::ast::Checker;
 use crate::registry::AsRule;
@@ -66,6 +65,70 @@ impl Violation for ConvertNamedTupleFunctionalToClass {
     }
 }
 
+/// UP014
+pub(crate) fn convert_named_tuple_functional_to_class(
+    checker: &mut Checker,
+    stmt: &Stmt,
+    targets: &[Expr],
+    value: &Expr,
+) {
+    let Some((typename, args, keywords, base_class)) =
+        match_named_tuple_assign(targets, value, checker.semantic())
+    else {
+        return;
+    };
+
+    let fields = match (args, keywords) {
+        // Ex) `NamedTuple("MyType")`
+        ([_typename], []) => vec![Stmt::Pass(ast::StmtPass {
+            range: TextRange::default(),
+        })],
+        // Ex) `NamedTuple("MyType", [("a", int), ("b", str)])`
+        ([_typename, fields], []) => {
+            if let Some(fields) = create_fields_from_fields_arg(fields) {
+                fields
+            } else {
+                debug!("Skipping `NamedTuple` \"{typename}\": unable to parse fields");
+                return;
+            }
+        }
+        // Ex) `NamedTuple("MyType", a=int, b=str)`
+        ([_typename], keywords) => {
+            if let Some(fields) = create_fields_from_keywords(keywords) {
+                fields
+            } else {
+                debug!("Skipping `NamedTuple` \"{typename}\": unable to parse keywords");
+                return;
+            }
+        }
+        // Ex) `NamedTuple()`
+        _ => {
+            debug!("Skipping `NamedTuple` \"{typename}\": mixed fields and keywords");
+            return;
+        }
+    };
+
+    let mut diagnostic = Diagnostic::new(
+        ConvertNamedTupleFunctionalToClass {
+            name: typename.to_string(),
+        },
+        stmt.range(),
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        // TODO(charlie): Preserve indentation, to remove the first-column requirement.
+        if checker.locator().is_at_start_of_line(stmt.start()) {
+            diagnostic.set_fix(convert_to_class(
+                stmt,
+                typename,
+                fields,
+                base_class,
+                checker.generator(),
+            ));
+        }
+    }
+    checker.diagnostics.push(diagnostic);
+}
+
 /// Return the typename, args, keywords, and base class.
 fn match_named_tuple_assign<'a>(
     targets: &'a [Expr],
@@ -89,13 +152,12 @@ fn match_named_tuple_assign<'a>(
     Some((typename, args, keywords, func))
 }
 
-/// Generate a [`Stmt::AnnAssign`] representing the provided property
-/// definition.
-fn create_property_assignment_stmt(property: &str, annotation: &Expr) -> Stmt {
+/// Generate a [`Stmt::AnnAssign`] representing the provided field definition.
+fn create_field_assignment_stmt(field: &str, annotation: &Expr) -> Stmt {
     ast::StmtAnnAssign {
         target: Box::new(
             ast::ExprName {
-                id: property.into(),
+                id: field.into(),
                 ctx: ExprContext::Load,
                 range: TextRange::default(),
             }
@@ -109,56 +171,46 @@ fn create_property_assignment_stmt(property: &str, annotation: &Expr) -> Stmt {
     .into()
 }
 
-/// Create a list of property assignments from the `NamedTuple` fields argument.
-fn create_properties_from_fields_arg(fields: &Expr) -> Result<Vec<Stmt>> {
-    let Expr::List(ast::ExprList { elts, .. }) = &fields else {
-        bail!("Expected argument to be `Expr::List`");
-    };
+/// Create a list of field assignments from the `NamedTuple` fields argument.
+fn create_fields_from_fields_arg(fields: &Expr) -> Option<Vec<Stmt>> {
+    let ast::ExprList { elts, .. } = fields.as_list_expr()?;
     if elts.is_empty() {
         let node = Stmt::Pass(ast::StmtPass {
             range: TextRange::default(),
         });
-        return Ok(vec![node]);
+        Some(vec![node])
+    } else {
+        elts.iter()
+            .map(|field| {
+                let ast::ExprTuple { elts, .. } = field.as_tuple_expr()?;
+                let [field, annotation] = elts.as_slice() else {
+                    return None;
+                };
+                let field = field.as_constant_expr()?;
+                let Constant::Str(ast::StringConstant { value: field, .. }) = &field.value else {
+                    return None;
+                };
+                if !is_identifier(field) {
+                    return None;
+                }
+                if is_dunder(field) {
+                    return None;
+                }
+                Some(create_field_assignment_stmt(field, annotation))
+            })
+            .collect()
     }
-    elts.iter()
-        .map(|field| {
-            let Expr::Tuple(ast::ExprTuple { elts, .. }) = &field else {
-                bail!("Expected `field` to be `Expr::Tuple`")
-            };
-            let [field_name, annotation] = elts.as_slice() else {
-                bail!("Expected `elts` to have exactly two elements")
-            };
-            let Expr::Constant(ast::ExprConstant {
-                value:
-                    Constant::Str(ast::StringConstant {
-                        value: property, ..
-                    }),
-                ..
-            }) = &field_name
-            else {
-                bail!("Expected `field_name` to be `Constant::Str`")
-            };
-            if !is_identifier(property) {
-                bail!("Invalid property name: {}", property)
-            }
-            if is_dunder(property) {
-                bail!("Cannot use dunder property name: {}", property)
-            }
-            Ok(create_property_assignment_stmt(property, annotation))
-        })
-        .collect()
 }
 
-/// Create a list of property assignments from the `NamedTuple` keyword arguments.
-fn create_properties_from_keywords(keywords: &[Keyword]) -> Result<Vec<Stmt>> {
+/// Create a list of field assignments from the `NamedTuple` keyword arguments.
+fn create_fields_from_keywords(keywords: &[Keyword]) -> Option<Vec<Stmt>> {
     keywords
         .iter()
         .map(|keyword| {
-            let Keyword { arg, value, .. } = keyword;
-            let Some(arg) = arg else {
-                bail!("Expected `keyword` to have an `arg`")
-            };
-            Ok(create_property_assignment_stmt(arg.as_str(), value))
+            keyword
+                .arg
+                .as_ref()
+                .map(|field| create_field_assignment_stmt(field.as_str(), &keyword.value))
         })
         .collect()
 }
@@ -194,67 +246,3 @@ fn convert_to_class(
         stmt.range(),
     ))
 }
-
-/// UP014
-pub(crate) fn convert_named_tuple_functional_to_class(
-    checker: &mut Checker,
-    stmt: &Stmt,
-    targets: &[Expr],
-    value: &Expr,
-) {
-    let Some((typename, args, keywords, base_class)) =
-        match_named_tuple_assign(targets, value, checker.semantic())
-    else {
-        return;
-    };
-
-    let properties = match (args, keywords) {
-        // Ex) NamedTuple("MyType")
-        ([_typename], []) => vec![Stmt::Pass(ast::StmtPass {
-            range: TextRange::default(),
-        })],
-        // Ex) NamedTuple("MyType", [("a", int), ("b", str)])
-        ([_typename, fields], []) => {
-            if let Ok(properties) = create_properties_from_fields_arg(fields) {
-                properties
-            } else {
-                debug!("Skipping `NamedTuple` \"{typename}\": unable to parse fields");
-                return;
-            }
-        }
-        // Ex) NamedTuple("MyType", a=int, b=str)
-        ([_typename], keywords) => {
-            if let Ok(properties) = create_properties_from_keywords(keywords) {
-                properties
-            } else {
-                debug!("Skipping `NamedTuple` \"{typename}\": unable to parse keywords");
-                return;
-            }
-        }
-        // Unfixable
-        _ => {
-            debug!("Skipping `NamedTuple` \"{typename}\": mixed fields and keywords");
-            return;
-        }
-    };
-
-    let mut diagnostic = Diagnostic::new(
-        ConvertNamedTupleFunctionalToClass {
-            name: typename.to_string(),
-        },
-        stmt.range(),
-    );
-    if checker.patch(diagnostic.kind.rule()) {
-        // TODO(charlie): Preserve indentation, to remove the first-column requirement.
-        if checker.locator().is_at_start_of_line(stmt.start()) {
-            diagnostic.set_fix(convert_to_class(
-                stmt,
-                typename,
-                properties,
-                base_class,
-                checker.generator(),
-            ));
-        }
-    }
-    checker.diagnostics.push(diagnostic);
-}

crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs

@@ -1,6 +1,3 @@
-use anyhow::{bail, Result};
-use log::debug;
-
 use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::helpers::is_dunder;
@@ -64,6 +61,45 @@ impl Violation for ConvertTypedDictFunctionalToClass {
     }
 }
 
+/// UP013
+pub(crate) fn convert_typed_dict_functional_to_class(
+    checker: &mut Checker,
+    stmt: &Stmt,
+    targets: &[Expr],
+    value: &Expr,
+) {
+    let Some((class_name, arguments, base_class)) =
+        match_typed_dict_assign(targets, value, checker.semantic())
+    else {
+        return;
+    };
+
+    let Some((body, total_keyword)) = match_fields_and_total(arguments) else {
+        return;
+    };
+
+    let mut diagnostic = Diagnostic::new(
+        ConvertTypedDictFunctionalToClass {
+            name: class_name.to_string(),
+        },
+        stmt.range(),
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        // TODO(charlie): Preserve indentation, to remove the first-column requirement.
+        if checker.locator().is_at_start_of_line(stmt.start()) {
+            diagnostic.set_fix(convert_to_class(
+                stmt,
+                class_name,
+                body,
+                total_keyword,
+                base_class,
+                checker.generator(),
+            ));
+        }
+    }
+    checker.diagnostics.push(diagnostic);
+}
+
 /// Return the class name, arguments, keywords and base class for a `TypedDict`
 /// assignment.
 fn match_typed_dict_assign<'a>(
@@ -88,13 +124,12 @@ fn match_typed_dict_assign<'a>(
     Some((class_name, arguments, func))
 }
 
-/// Generate a [`Stmt::AnnAssign`] representing the provided property
-/// definition.
-fn create_property_assignment_stmt(property: &str, annotation: &Expr) -> Stmt {
+/// Generate a [`Stmt::AnnAssign`] representing the provided field definition.
+fn create_field_assignment_stmt(field: &str, annotation: &Expr) -> Stmt {
     ast::StmtAnnAssign {
         target: Box::new(
             ast::ExprName {
-                id: property.into(),
+                id: field.into(),
                 ctx: ExprContext::Load,
                 range: TextRange::default(),
             }
@@ -108,8 +143,7 @@ fn create_property_assignment_stmt(property: &str, annotation: &Expr) -> Stmt {
     .into()
 }
 
-/// Generate a `StmtKind:ClassDef` statement based on the provided body,
-/// keywords and base class.
+/// Generate a `StmtKind:ClassDef` statement based on the provided body, keywords, and base class.
 fn create_class_def_stmt(
     class_name: &str,
     body: Vec<Stmt>,
@@ -134,81 +168,72 @@ fn create_class_def_stmt(
     .into()
 }
 
-fn properties_from_dict_literal(keys: &[Option<Expr>], values: &[Expr]) -> Result<Vec<Stmt>> {
+fn fields_from_dict_literal(keys: &[Option<Expr>], values: &[Expr]) -> Option<Vec<Stmt>> {
     if keys.is_empty() {
         let node = Stmt::Pass(ast::StmtPass {
             range: TextRange::default(),
         });
-        return Ok(vec![node]);
+        Some(vec![node])
+    } else {
+        keys.iter()
+            .zip(values.iter())
+            .map(|(key, value)| match key {
+                Some(Expr::Constant(ast::ExprConstant {
+                    value: Constant::Str(ast::StringConstant { value: field, .. }),
+                    ..
+                })) => {
+                    if !is_identifier(field) {
+                        return None;
+                    }
+                    if is_dunder(field) {
+                        return None;
+                    }
+                    Some(create_field_assignment_stmt(field, value))
+                }
+                _ => None,
+            })
+            .collect()
     }
-
-    keys.iter()
-        .zip(values.iter())
-        .map(|(key, value)| match key {
-            Some(Expr::Constant(ast::ExprConstant {
-                value:
-                    Constant::Str(ast::StringConstant {
-                        value: property, ..
-                    }),
-                ..
-            })) => {
-                if !is_identifier(property) {
-                    bail!("Invalid property name: {}", property)
-                }
-                if is_dunder(property) {
-                    bail!("Cannot use dunder property name: {}", property)
-                }
-                Ok(create_property_assignment_stmt(property, value))
-            }
-            _ => bail!("Expected `key` to be `Constant::Str`"),
-        })
-        .collect()
 }
 
-fn properties_from_dict_call(func: &Expr, keywords: &[Keyword]) -> Result<Vec<Stmt>> {
-    let Expr::Name(ast::ExprName { id, .. }) = func else {
-        bail!("Expected `func` to be `Expr::Name`")
-    };
+fn fields_from_dict_call(func: &Expr, keywords: &[Keyword]) -> Option<Vec<Stmt>> {
+    let ast::ExprName { id, .. } = func.as_name_expr()?;
     if id != "dict" {
-        bail!("Expected `id` to be `\"dict\"`")
+        return None;
     }
+
     if keywords.is_empty() {
         let node = Stmt::Pass(ast::StmtPass {
             range: TextRange::default(),
         });
-        return Ok(vec![node]);
+        Some(vec![node])
+    } else {
+        fields_from_keywords(keywords)
     }
-
-    properties_from_keywords(keywords)
 }
 
 // Deprecated in Python 3.11, removed in Python 3.13.
-fn properties_from_keywords(keywords: &[Keyword]) -> Result<Vec<Stmt>> {
+fn fields_from_keywords(keywords: &[Keyword]) -> Option<Vec<Stmt>> {
     if keywords.is_empty() {
         let node = Stmt::Pass(ast::StmtPass {
             range: TextRange::default(),
         });
-        return Ok(vec![node]);
+        return Some(vec![node]);
     }
 
     keywords
         .iter()
         .map(|keyword| {
-            if let Some(property) = &keyword.arg {
-                Ok(create_property_assignment_stmt(property, &keyword.value))
-            } else {
-                bail!("Expected `arg` to be `Some`")
-            }
+            keyword
+                .arg
+                .as_ref()
+                .map(|field| create_field_assignment_stmt(field, &keyword.value))
         })
         .collect()
 }
 
-fn match_properties_and_total(arguments: &Arguments) -> Result<(Vec<Stmt>, Option<&Keyword>)> {
-    // We don't have to manage the hybrid case because it's not possible to have a
-    // dict and keywords. For example, the following is illegal:
-    // ```
-    // MyType = TypedDict('MyType', {'a': int, 'b': str}, a=int, b=str)
-    // ```
+/// Match the fields and `total` keyword from a `TypedDict` call.
+fn match_fields_and_total(arguments: &Arguments) -> Option<(Vec<Stmt>, Option<&Keyword>)> {
     match (arguments.args.as_slice(), arguments.keywords.as_slice()) {
         // Ex) `TypedDict("MyType", {"a": int, "b": str})`
         ([_typename, fields], [..]) => {
@@ -218,13 +243,13 @@ fn match_properties_and_total(arguments: &Arguments) -> Result<(Vec<Stmt>, Optio
                     keys,
                     values,
                     range: _,
-                }) => Ok((properties_from_dict_literal(keys, values)?, total)),
+                }) => Some((fields_from_dict_literal(keys, values)?, total)),
                 Expr::Call(ast::ExprCall {
                     func,
                     arguments: Arguments { keywords, .. },
                     range: _,
-                }) => Ok((properties_from_dict_call(func, keywords)?, total)),
-                _ => bail!("Expected `arg` to be `Expr::Dict` or `Expr::Call`"),
+                }) => Some((fields_from_dict_call(func, keywords)?, total)),
+                _ => None,
             }
         }
         // Ex) `TypedDict("MyType")`
@@ -232,11 +257,12 @@ fn match_properties_and_total(arguments: &Arguments) -> Result<(Vec<Stmt>, Optio
             let node = Stmt::Pass(ast::StmtPass {
                 range: TextRange::default(),
             });
-            Ok((vec![node], None))
+            Some((vec![node], None))
         }
         // Ex) `TypedDict("MyType", a=int, b=str)`
-        ([_typename], fields) => Ok((properties_from_keywords(fields)?, None)),
-        _ => bail!("Expected `args` to have exactly one or two elements"),
+        ([_typename], fields) => Some((fields_from_keywords(fields)?, None)),
+        // Ex) `TypedDict()`
+        _ => None,
     }
 }
 
@@ -259,46 +285,3 @@ fn convert_to_class(
         stmt.range(),
     ))
 }
-
-/// UP013
-pub(crate) fn convert_typed_dict_functional_to_class(
-    checker: &mut Checker,
-    stmt: &Stmt,
-    targets: &[Expr],
-    value: &Expr,
-) {
-    let Some((class_name, arguments, base_class)) =
-        match_typed_dict_assign(targets, value, checker.semantic())
-    else {
-        return;
-    };
-
-    let (body, total_keyword) = match match_properties_and_total(arguments) {
-        Ok((body, total_keyword)) => (body, total_keyword),
-        Err(err) => {
-            debug!("Skipping ineligible `TypedDict` \"{class_name}\": {err}");
-            return;
-        }
-    };
-
-    let mut diagnostic = Diagnostic::new(
-        ConvertTypedDictFunctionalToClass {
-            name: class_name.to_string(),
-        },
-        stmt.range(),
-    );
-    if checker.patch(diagnostic.kind.rule()) {
-        // TODO(charlie): Preserve indentation, to remove the first-column requirement.
-        if checker.locator().is_at_start_of_line(stmt.start()) {
-            diagnostic.set_fix(convert_to_class(
-                stmt,
-                class_name,
-                body,
-                total_keyword,
-                base_class,
-                checker.generator(),
-            ));
-        }
-    }
-    checker.diagnostics.push(diagnostic);
-}