Implement type hints for Node subclasses #4793

Repiteo · 2025-11-30T22:14:25Z

Belated followup to #4650.

As discussed in the original PR, this isn't aiming to be a full implementation, as the full ramifications of typehinting everything are well beyond the scope of this PR. This instead finishes the job the above PR started: defining the core Node types in a controlled environment.

Contributor Checklist:

I have created a new test or updated the unit tests to cover the new/changed functionality.
I have updated CHANGES.txt and RELEASE.txt (and read the README.rst).
I have updated the appropriate documentation

Repiteo · 2025-11-30T22:18:41Z

SCons/Node/Python.py

-        """Because we're a Python value node and don't have a real
-        timestamp, we get to ignore the calculator and just use the
-        value contents.
-


The closest thing to a "functional" change this PR was removing this optional argument. This is the only instance of get_csig taking an extra argument, and it's completely unused. Based of the docstring, I assume every get_csig took a calc argument at one point, with Value specifically treating it as a no-op; that might be why it was never properly cleaned up, remaining as a vestigal argument

bdbaddog · 2025-12-01T02:34:45Z

SCons/Node/FS.py

            Link_Funcs.append(link_dict[func])

-def LinkFunc(target, source, env) -> int:
+def LinkFunc(target: list[Base], source: list[Base], env: Environment) -> int:


isn't Base imported above as Environment ?

It is! This instead refers to the Node subclass of the same name that acts as the common ancestor for Entry/File/Dir

Cue old grumble of mine: multiple clasess of the same name (and especially when it's a non-descriptive name like Base) just lead to confusion... I have gotten rid of a could of those...

bdbaddog · 2025-12-01T02:40:02Z

SCons/Node/FS.py

-        self._path = None
-        self._tpath = None
-        self._path_elements = None
+        # FIXME: Declared as resolved types for hinting purposes, but do we need


we've been using TODO: and not FIXME: for such

I think we need to keep setting to None as this is an abstract base class so if someone implemented a child of this without setting this (and the others) properly, it should break..

Ahh, I see. I wonder if there's a more elegant way to ensure a variable is declared in a subclass... Something for another PR in any case

It's not completely an abstract base class, is it - as the comment says, it's "generic", but is actually used in some cases (and has the rather grotty thing where it morphs into soemthing else? If it was a true ABC, you can turn the attribute into a property with @property and then decorate it with @abstractmethod. That doesn't seem to align well with this case.

If it's really important to catch this case programmatically (rather than via checkers, or just via comments), you can make a __new__ at the top level that includes a check. I doubt whether that's really worth it, but something like:

class Base(SCons.Node.Node): ... def __new__(cls, *args, **kwargs): if not hasattr(cls, '_path'): raise NotImplementedError("Node classes must implement '_path') return super().__new__(cls)

bdbaddog · 2025-12-01T02:42:08Z

Interesting the sphinx build broke. @mwichmann see anything in this PR which would reasonably impact sphinx?

mwichmann · 2025-12-01T14:00:19Z

SCons/Node/Python.py

        ###TODO: something reasonable about universal newlines
        contents = str(self.value)
-        for kid in self.children(None):
+        for kid in self.children(False):


Just so it gets a vote, I've been making this change, slowly, in places where it makes sense (which means @bdbaddog has approved some of them ;) ): (a) if it's really a bool, where we only actually care about truthiness, don't use None as the false value, actually use False. (b) for strings, if an empty string is good enough to indicate "not set", then use that, instead of None.

In this case I'd actually use the kwarg: ...in self.children(scan=False) to be more clear (yeah, it's only a test...)

Oh, that simplifies quite a few places then! I'll apply those where relevant here as well 👍

mwichmann · 2025-12-01T20:10:50Z

Interesting the sphinx build broke. @mwichmann see anything in this PR which would reasonably impact sphinx?

it's the same thing it's been complaining about since we started to use sphinx, it's just not been fatal... the funky Null class that we use to avoid actually having to check for a not-initialized scons class.

mwichmann · 2025-12-01T20:45:32Z

Interesting the sphinx build broke. @mwichmann see anything in this PR which would reasonably impact sphinx?

it's the same thing it's been complaining about since we started to use sphinx, it's just not been fatal... the funky Null class that we use to avoid actually having to check for a not-initialized scons class.

FWIW, I think it makes Sphinx happier if you combine Null and NullSeq (the latter is currently unused). Pretty sure I tested that at some point but didn't do the work of making a release.

Note that Sphinx 9.0.0 just dropped - yesterday (already a 9.0.1 from today), it's not unusual that something we used to be able to ignore becomes more serious in a new "major" release. I see in the test log it was indeed picking that one up:


Loaded Extensions
=================

* sphinx.ext.mathjax (9.0.0)
* alabaster (1.0.0)
* sphinxcontrib.applehelp (2.0.0)
* sphinxcontrib.devhelp (2.0.0)
* sphinxcontrib.htmlhelp (2.1.0)
* sphinxcontrib.serializinghtml (2.0.0)
* sphinxcontrib.qthelp (2.0.0)
* sphinx.ext.autodoc (9.0.0)
* sphinx.ext.autosummary (9.0.0)
* sphinx.ext.napoleon (9.0.0)
* sphinx.ext.todo (9.0.0)
* sphinx.ext.viewcode (9.0.0)

This is a snip from an api docs build on an earlier sphinx version - you can see the error is there, it just didn't abort:

WARNING: autodoc: failed to determine SCons.Util.sctypes::NullSeq._instance (Null(0x7FBEF152AA50)) to be documented, the following exception was raised:
Handler <function _skip_member at 0x7fbef23a6660> for event 'autodoc-skip-member' threw an exception (exception: cannot unpack non-iterable Null object) [autodoc]

Now we've apparently had that bubble through instead of being caught:

       File "/opt/hostedtoolcache/Python/3.14.0/x64/lib/python3.14/site-packages/sphinx/events.py", line 452, in emit
        raise ExtensionError(
        ...<3 lines>...
        ) from exc
    sphinx.errors.ExtensionError: Handler <function _skip_member at 0x7f07133f6980> for event 'autodoc-skip-member' threw an exception (exception: cannot unpack non-iterable Null object)

mwichmann · 2025-12-02T22:08:49Z

FWIW, I think it makes Sphinx happier if you combine Null and NullSeq (the latter is currently unused). Pretty sure I tested that at some point but didn't do the work of making a release.

I was wrong on both counts. There is one reference to NullSeq, NullNodeList inherits from it. That's a relationship that could be cleaned up. But - combining Null and NullSeq does not make Sphinx work, it takes another obscure exception.

mwichmann · 2025-12-02T22:15:27Z

    Traceback (most recent call last):
      File "/home/mats/.pyenv/versions/venv-system313/lib64/python3.13/site-packages/sphinx/events.py", line 441, in emit
        results.append(listener.handler(self._app, *args))
                       ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
      File "/home/mats/.pyenv/versions/venv-system313/lib64/python3.13/site-packages/sphinx/ext/napoleon/__init__.py", line 473, in _skip_member
        cls_path, _, _ = qualname.rpartition('.')
        ^^^^^^^^^^^^^^
    ValueError: not enough values to unpack (expected 3, got 0)

We don't know what qualname is here or where it comes from, and I don't know how to get a string's rpartition to return 0 rathe than three values - it always returns three unless it's throwing an exception, but no exception was reported there (it must be eating it, which is no help...)

bdbaddog · 2025-12-02T22:30:40Z

@mwichmann - we should probably file a bug against sphinx. I did a little digging with some LLM help on this. It's definitely in the napoleon plugin which is now part of the regular sphinx github repo.

Repiteo commented Nov 30, 2025

View reviewed changes

bdbaddog reviewed Dec 1, 2025

View reviewed changes

mwichmann reviewed Dec 1, 2025

View reviewed changes

Implement type hints for Node subclasses

73935e4

Repiteo force-pushed the type-hint-nodes branch from f581cb9 to 73935e4 Compare December 1, 2025 15:33

Uh oh!

Implement type hints for Node subclasses #4793

Are you sure you want to change the base?

Implement type hints for Node subclasses #4793

Conversation

Repiteo commented Nov 30, 2025

Contributor Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwichmann Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdbaddog commented Dec 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwichmann commented Dec 1, 2025

Uh oh!

mwichmann commented Dec 1, 2025

Uh oh!

mwichmann commented Dec 2, 2025

Uh oh!

mwichmann commented Dec 2, 2025

Uh oh!

bdbaddog commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwichmann Dec 1, 2025 •

edited

Loading