Does crates/ruff/resources/test/fixtures/pyupgrade/UP014.py contain incorrect NamedTuple calls?

[harupy]

ruff/crates/ruff/resources/test/fixtures/pyupgrade/UP014.py

Lines 7 to 12 in f012ed2

	# with default values as list
	MyType = NamedTuple(
	"MyType",
	[("a", int), ("b", str), ("c", list[bool])],
	defaults=["foo", [True]],
	)

Is this a valid NamedTuple call?

> python3 --version
Python 3.11.4

> cat a.py
from typing import NamedTuple

MyType = NamedTuple(
    "MyType",
    [("a", int), ("b", str), ("c", list[bool])],
    defaults=["foo", [True]],
)

> python a.py
Traceback (most recent call last):
  File "/home/haru/Desktop/repositories/ruff/a.py", line 3, in <module>
    MyType = NamedTuple(
             ^^^^^^^^^^^
  File "/home/haru/miniconda3/envs/py311/lib/python3.11/typing.py", line 2916, in NamedTuple
    raise TypeError("Either list of fields or keywords"
TypeError: Either list of fields or keywords can be provided to NamedTuple, not both

[harupy]

or is this intended?

[cnpryer]

Is this a valid NamedTuple call?

If I understand this function signature correctly, no, right?

https://github.com/python/cpython/blob/8c177294899b621fe04ae755abd41b4d319dd4b5/Lib/typing.py#L2770

def NamedTuple(typename, fields=_sentinel, /, **kwargs):

edit: it's recently changed

[cnpryer]

or is this intended?

Is this what you saw?

from typing import NamedTuple
 
MyType = NamedTuple(
    "MyType",
    [("a", int), ("b", str), ("c", list[bool])],
    defaults=["foo", [True]],
)

crates/ruff_python_formatter/scratch.py:6:16: F821 Undefined name `foo`
Found 1 error.

[harupy]

I saw that, and then found this issue. What I meant by is this intended was:

MyType = NamedTuple(
    "MyType",
    [("a", int), ("b", str), ("c", list[bool])],
    defaults=[1, "foo", [True]],
)

is an invalid NamedTuple call, but syntactically correct. It could be useful if ruff automatically fixes this to:

class MyType(NamedTuple):
    a: int = 1
    b: str = "foo"
    c: list[bool] = [True]

Ruff currently does.

[charliermarsh]

I think it's intended, just to see how we would handle that case, even if it's not valid at runtime.

[harupy]

Makes sense. Is F821 on foo unintended (#5794 (comment))?

[charliermarsh]

Is passing defaults as a list supported at all? I wonder why we support that at all? (Maybe that's why you opened the issue and I just misunderstood.)

[charliermarsh]

(I assumed there was some case in which passing a list of defaults was supported, just that you couldn't mix it with specifying the fields that way.)

[harupy]

from typing import NamedTuple

# valid
MyType = NamedTuple(
    "MyType",
    defaults=str,
)

# valid
MyType = NamedTuple(
    "MyType",
    defaults=list[str],
)

# invalid, doesn't throw but doesn't make sense because `[str]` is not a valid type annotation
MyType = NamedTuple(
    "MyType",
    defaults=[str],
)

# invalid, throws `TypeError: Either list of fields or keywords can be provided to NamedTuple, not both`
MyType = NamedTuple(
    "MyType",
    [("a", int), ("b", str), ("c", list[bool])],
    defaults=[1, "foo", [True]],
)

You can pass a list to defaults as the third example does, but that makes no sense (it would make sense if [str] was an alias for list[str]).

[charliermarsh]

In each case, defaults is just treated as a standard field though, right? I feel like UP014 is under the mistaken impression that you can provide a list of default values via defaults... If so, we should remove that.

[charliermarsh]

Yeah, we should remove all the special-casing around defaults.

[charliermarsh]

E.g., this and related code seems unnecessary:

/// Match the `defaults` keyword in a `NamedTuple(...)` call.
fn match_defaults(keywords: &[Keyword]) -> Result<&[Expr]> {
    let defaults = keywords.iter().find(|keyword| {
        if let Some(arg) = &keyword.arg {
            arg == "defaults"
        } else {
            false
        }
    });
    match defaults {
        Some(defaults) => match &defaults.value {
            Expr::List(ast::ExprList { elts, .. }) => Ok(elts),
            Expr::Tuple(ast::ExprTuple { elts, .. }) => Ok(elts),
            _ => bail!("Expected defaults to be `Expr::List` | `Expr::Tuple`"),
        },
        None => Ok(&[]),
    }
}

[harupy]

I agree.

In each case, defaults is just treated as a standard field though, right?

defaults is visited as a type annotation:

ruff/crates/ruff/src/checkers/ast/mod.rs

Lines 3690 to 3695 in d692ed0

	// Ex) NamedTuple("a", a=int)
	for keyword in keywords {
	let Keyword { value, .. } = keyword;
	self.visit_type_definition(value);
	}
	}

Ruff thinks "foo" is a forward ref, but no foo exists, F821 is thrown on it.

[harupy]

Happy to file a PR.