From b07a53ab6829f0426c109ebc3013a3daae284b9b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 14 Jan 2026 13:42:35 -0500 Subject: [PATCH] [ty] Emit diagnostics for invalid dynamic namedtuple fields (#22575) ## Summary Removes some TODOs from the dynamic namedtuple implementation. --- .../resources/mdtest/named_tuple.md | 25 +++++- .../src/types/infer/builder.rs | 85 ++++++++++++++++--- 2 files changed, 95 insertions(+), 15 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/named_tuple.md b/crates/ty_python_semantic/resources/mdtest/named_tuple.md index 6d40981339..9a38edfce1 100644 --- a/crates/ty_python_semantic/resources/mdtest/named_tuple.md +++ b/crates/ty_python_semantic/resources/mdtest/named_tuple.md @@ -466,6 +466,27 @@ Point2 = collections.namedtuple("Point2", ["_x", "class"], rename=1) reveal_type(Point2) # revealed: reveal_type(Point2.__new__) # revealed: (cls: type, _0: Any, _1: Any) -> Point2 +# Without `rename=True`, invalid field names emit diagnostics: +# - Field names starting with underscore +# error: [invalid-named-tuple] "Field name `_x` in `namedtuple()` cannot start with an underscore" +Underscore = collections.namedtuple("Underscore", ["_x", "y"]) +reveal_type(Underscore) # revealed: + +# - Python keywords +# error: [invalid-named-tuple] "Field name `class` in `namedtuple()` cannot be a Python keyword" +Keyword = collections.namedtuple("Keyword", ["x", "class"]) +reveal_type(Keyword) # revealed: + +# - Duplicate field names +# error: [invalid-named-tuple] "Duplicate field name `x` in `namedtuple()`" +Duplicate = collections.namedtuple("Duplicate", ["x", "y", "x"]) +reveal_type(Duplicate) # revealed: + +# - Invalid identifiers (e.g., containing spaces) +# error: [invalid-named-tuple] "Field name `not valid` in `namedtuple()` is not a valid identifier" +Invalid = collections.namedtuple("Invalid", ["not valid", "ok"]) +reveal_type(Invalid) # revealed: + # `defaults` provides default values for the rightmost fields Person = collections.namedtuple("Person", ["name", "age", "city"], defaults=["Unknown"]) reveal_type(Person) # revealed: @@ -483,8 +504,8 @@ reveal_type(person2.city) # revealed: Any Config = collections.namedtuple("Config", ["host", "port"], module="myapp") reveal_type(Config) # revealed: -# When more defaults are provided than fields, we treat all fields as having defaults. -# TODO: This should emit a diagnostic since it would fail at runtime. +# When more defaults are provided than fields, an error is emitted. +# error: [invalid-named-tuple] "Too many defaults for `namedtuple()`" TooManyDefaults = collections.namedtuple("TooManyDefaults", ["x", "y"], defaults=("a", "b", "c")) reveal_type(TooManyDefaults) # revealed: reveal_type(TooManyDefaults.__new__) # revealed: (cls: type, x: Any = "a", y: Any = "b") -> TooManyDefaults diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index 591aa43322..496468daaa 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -6565,6 +6565,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { // Infer keyword arguments. let mut default_types: Vec> = vec![]; + let mut defaults_kw: Option<&ast::Keyword> = None; let mut rename_type = None; for kw in keywords { @@ -6575,6 +6576,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { }; match arg.id.as_str() { "defaults" if kind.is_collections() => { + defaults_kw = Some(kw); // Extract element types from AST literals (using already-inferred types) // or fall back to the inferred tuple spec. match &kw.value { @@ -6779,16 +6781,60 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } if let Some(mut field_names) = maybe_field_names { - // TODO: When `rename` is false (or not specified), emit diagnostics for: - // - Duplicate field names (e.g., `namedtuple("Foo", "x x")`) - // - Field names starting with underscore (e.g., `namedtuple("Bar", "_x")`) - // - Field names that are Python keywords (e.g., `namedtuple("Baz", "class")`) - // - Field names that are not valid identifiers - // These all raise ValueError at runtime. When `rename=True`, invalid names - // are automatically replaced with `_0`, `_1`, etc., so no diagnostic is needed. + // When `rename` is false (or not specified), emit diagnostics for invalid + // field names. These all raise ValueError at runtime. When `rename=True`, + // invalid names are automatically replaced with `_0`, `_1`, etc., so no + // diagnostic is needed. + if !rename { + for (i, field_name) in field_names.iter().enumerate() { + let name_str = field_name.as_str(); - // Apply rename logic. - if rename { + if field_names[..i].iter().any(|f| f.as_str() == name_str) + && let Some(builder) = + self.context.report_lint(&INVALID_NAMED_TUPLE, fields_arg) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Duplicate field name `{name_str}` in `namedtuple()`" + )); + diagnostic.set_primary_message(format_args!( + "Field `{name_str}` already defined; will raise `ValueError` at runtime" + )); + } + + if name_str.starts_with('_') + && let Some(builder) = + self.context.report_lint(&INVALID_NAMED_TUPLE, fields_arg) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Field name `{name_str}` in `namedtuple()` cannot start with an underscore" + )); + diagnostic.set_primary_message(format_args!( + "Will raise `ValueError` at runtime" + )); + } else if is_keyword(name_str) + && let Some(builder) = + self.context.report_lint(&INVALID_NAMED_TUPLE, fields_arg) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Field name `{name_str}` in `namedtuple()` cannot be a Python keyword" + )); + diagnostic.set_primary_message(format_args!( + "Will raise `ValueError` at runtime" + )); + } else if !is_identifier(name_str) + && let Some(builder) = + self.context.report_lint(&INVALID_NAMED_TUPLE, fields_arg) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Field name `{name_str}` in `namedtuple()` is not a valid identifier" + )); + diagnostic.set_primary_message(format_args!( + "Will raise `ValueError` at runtime" + )); + } + } + } else { + // Apply rename logic. let mut seen_names = FxHashSet::<&str>::default(); for (i, field_name) in field_names.iter_mut().enumerate() { let name_str = field_name.as_str(); @@ -6803,11 +6849,24 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } } - // Build fields with `Any` type and optional defaults. - // TODO: emit a diagnostic when `defaults_count > num_fields` (which would - // fail at runtime with `TypeError: Got more default values than field names`). let num_fields = field_names.len(); - let defaults_count = default_types.len().min(num_fields); + let defaults_count = default_types.len(); + + if defaults_count > num_fields + && let Some(defaults_kw) = defaults_kw + && let Some(builder) = + self.context.report_lint(&INVALID_NAMED_TUPLE, defaults_kw) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Too many defaults for `namedtuple()`" + )); + diagnostic.set_primary_message(format_args!( + "Got {defaults_count} default values but only {num_fields} field names" + )); + diagnostic.info("This will raise `TypeError` at runtime"); + } + + let defaults_count = defaults_count.min(num_fields); let fields = field_names .iter() .enumerate()