[arareko]

In https://github.com/python/typeshed/blob/294b03f75bd23d95cf4709ba14547cc4190aacdd/stubs/PyYAML/yaml/constructor.pyi the signature for construct_sequence() defines SequenceNode as the type for node (which is correct), however the _N TypeVar used in the signature for add_constructor() only allows for Node.

It would seem that MappingNode, ScalarNode and SequenceNode are missing from the TypeVar definition since add_constructor() does not consider any of them as compatible. For example:

error: Argument 2 to "add_constructor" has incompatible type "Callable[[Loader, SequenceNode], str]"; expected "Callable[[Union[Loader, FullLoader, UnsafeLoader], Node], Any]"

This PR addresses the above.

---

Update: retitled from "PyYAML: Add missing types to _N TypeVar". As discussed above, the original _N change didn't address the problem (the union is equivalent to Node, and _N isn't involved), so the new commit reverts it.

The error comes from the module-level add_constructor and add_multi_constructor in __init__.pyi. They require handlers that accept any Node and all of Loader | FullLoader | UnsafeLoader, and because Callable parameters are contravariant, a handler annotated with SequenceNode or yaml.Loader is rejected. They now use TypeVars bounded by Node and by the loader union.

Upstream: https://github.com/yaml/pyyaml/blob/957ae4d495cf8fcb5475c6c2f1bce801096b68a5/lib/yaml/__init__.py#L302

[JelleZijlstra]

These are all subclasses of Node, so adding them to the bound shouldn't do anything. Can you share a code sample on which you are getting an error?

[arareko]

@JelleZijlstra sure thing:

Save this as example.py:

import os

import yaml


def load_config(config_path: str) -> dict:
    # define custom tag handler
    def _join(loader: yaml.Loader, node: yaml.SequenceNode) -> str:
        seq = loader.construct_sequence(node)
        return os.path.join(*[str(i) for i in seq])

    # register the tag handler
    yaml.add_constructor("!join", _join)

    # load config
    with open(config_path, "r") as ymlfile:
        config = yaml.load(ymlfile, Loader=yaml.Loader)

    return config

Then check with mypy:

❯ mypy example.py
example.py:13: error: Argument 2 to "add_constructor" has incompatible type "Callable[[Loader, SequenceNode], str]"; expected "Callable[[Union[Loader, FullLoader, UnsafeLoader], Node], Any]"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

[JelleZijlstra]

Thanks for the code sample! Our stub for add_constructor is

@overload
def add_constructor(
    tag: str, constructor: Callable[[Loader | FullLoader | UnsafeLoader, Node], Any], Loader: None = ...
) -> None: ...
@overload
def add_constructor(tag: str, constructor: Callable[[_Constructor, Node], Any], Loader: type[_Constructor]) -> None: ...

_N isn't involved, so your PR is unlikely to help.

Maybe the Node type in the second argument to the constructor should be something else, but I'd have to look at the yaml code to verify that.

[arareko]

@JelleZijlstra I see what you mean. So basically, it won't work because what is defined in the constructor.pyi stub is being overloaded in __init__.pyi.

[arareko]

@JelleZijlstra add_constructor() in https://github.com/yaml/pyyaml/blob/957ae4d495cf8fcb5475c6c2f1bce801096b68a5/lib/yaml/__init__.py#L302 supports Loader, FullLoader and UnsafeLoader, which are in sync with the __init__.pyi stub.

Those loaders make use of Constructor or FullConstructor, which inherit from BaseConstructor and which handles MappingNode, ScalarNode and SequenceNode and not Node per se. As you mentioned, those three node types actually are subclasses of Node.

This would suggest that the allowed node types for add_constructor() in the stub should actually be:

@overload
def add_constructor(
    tag: str, constructor: Callable[[Loader | FullLoader | UnsafeLoader, MappingNode | ScalarNode | SequenceNode], Any], Loader: None = ...
) -> None: ...
@overload
def add_constructor(tag: str, constructor: Callable[[_Constructor, MappingNode | ScalarNode | SequenceNode], Any], Loader: type[_Constructor]) -> None: ...

What do you think?

[JelleZijlstra]

These proposed types imply that the constructor callable must accept all of MappingNode, ScalarNode, and SequenceNode (because Callable is contravariant in the parameter types). So if nothing else, that doesn't seem like it would make your original code typecheck.

[AlexWaygood]

I haven't looked into how to fix your problem, but I concur with @JelleZijlstra that this PR won't fix anything, unfortunately. The proposed union would be redundant, since MappingNode, ScalarNode, SequenceNode are all subclasses of Node.

If the problem is still present, please open an issue, and we can try to figure out how to fix it.

[arareko]

@AlexWaygood I've been intending to look into this issue further and potentially finding the right fix, however, life and work have gotten in the way. I'll probably re-open this whenever I manage to work on it.

[AlexWaygood]

@arareko no worries! Feel free to reopen, or open a new issue/PR at any time :)

[arareko]

Hi @AlexWaygood, @JelleZijlstra: as you suggested, I'm back on this. You were right that the _N change did nothing. The error comes from the module-level add_constructor / add_multi_constructor in __init__.pyi, which require handlers that accept any Node and all of Loader | FullLoader | UnsafeLoader. I've pushed a fix to this branch, merged with current main, with a regression test. I can't reopen the PR from my side. Could one of you reopen it, or would you prefer a new PR?

[arareko]

Dropped the regression test in 2a35889: the pyright test-case job installs types-PyYAML (via django-stubs), so it checks against the published stubs and can't see changes to this repo's. I verified the fix locally against the repo's stubs with mypy and pyright: example.py from the discussion above now type-checks, and a handler annotated for a specific loader or node class is accepted.

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

scipy-stubs (https://github.com/scipy/scipy-stubs)
+ tests/misc/test_submodules_empty.pyi:3: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

[srittau]

I'm not sure what the PR in its current state is supposed to fix. It just replaces the concrete types with type vars bound to those types, but the type vars only appear once in each signature, so that's basically a no op. Does the original problem even still apply? Could you provide a short example of the problem using the current stubs?

[arareko]

Yes, it still applies on current main (5cc604a). Minimal example:

import yaml

def _join(loader: yaml.Loader, node: yaml.SequenceNode) -> str:
    return ""

yaml.add_constructor("!join", _join)

mypy 2.4.0: Argument 2 to "add_constructor" has incompatible type "Callable[[Loader, SequenceNode], str]"; expected "Callable[[Loader | FullLoader | UnsafeLoader, Node], Any]". pyright 1.1.414 reports the same, for both the loader and the node parameter. Both are clean with this PR.

It isn't a no-op: Callable is contravariant in its parameters, so a handler that only accepts SequenceNode (or only Loader) isn't assignable to a callable typed with plain Node and the loader union. A TypeVar that appears once is solved per call, so each handler's own types are accepted. This is the same pattern BaseConstructor.add_constructor already uses in constructor.pyi.