Type setuptools/msvc.py dir methods and properties

[Avasam]

Summary of changes

Extracted from #4744 with additional fixes

Pull Request Checklist

• Changes have tests (type-checking tests)
• News fragment added in newsfragments/.
  (See documentation for details)

[Avasam]

This one I'm not entirely certain of, and does change the possible return types.

dir-related properties of this class are a mixed bag on whether they return None or '' as a fallthrough case. And are almost never documented as returning None (which is fixed in this PR).

However all other properties that can return None are checked for truthiness when used, this one isn't. (hence it was causing issues for #4744)

The redundant or '' makes me think this may have been intended to not return None. But the true intention is unclear to me.

---

After further research, it was auto-added here as an explicit return: https://github.com/pypa/setuptools/pull/4169/files#diff-2f10e0b183f00587feb575c7716d38f4951d4f7ded5b714396e96066ae7254bdR953

Now I'm pretty sure that the original intention was to return a '' by default. But that means maybe other methods as well ?

Anyway, if you think this should return None by default, I'll add checks to the methods that use this property instead.

[abravalheri]

Thank you very much for having a look on this.

It is a complicated decision. For the sake of backwards compatibility I think a safe approach would be to stick with the observed behaviour instead of the original intention?

Even if return None was a late automatic addition, the original function could still returning None under certain circumstances isn't it? So it may be safer for us to accept it can return None?

[Avasam]

8ff634c (#4755) Shows the diff letting this return None.

Same resolved TypeError in UniversalCRTSdkLastVersion, UCRTLibraries, UCRTIncludes and their caller when UniversalCRTSdkDir returns None.
If you want these to also keep the exact same exception raising behaviour, I can raise an explicit TypeError in them.

[abravalheri]

Thank you. I think it may make sense to keep the behaviour.

Do the type checkers allow something like the following?

try:
    ...
except TypeError as ex:
    py310.add_note(ex, "Cannot find UniversalCRTSdkDir")
    raise

(py310 => from .compat import py310)

[Avasam]

Python's type system does not propagate possible exceptions to be raised.
No type-checkers that I know of special-cases try-except either.

So no, type-checkers, won't know to silence an issue if you catch a TypeError.

It'd look more like this:

  	if (UniversalCRTSdkDir := self.UniversalCRTSdkDir) is None:
        raise TypeError("Cannot find UniversalCRTSdkDir")
    return self._use_last_dir_name(os.path.join(UniversalCRTSdkDir, 'lib'))

or

    try:
        return self._use_last_dir_name(
            os.path.join(self.UniversalCRTSdkDir, 'lib')  # type: ignore[arg-type] # Expected TypeError
        )
    except TypeError as ex:
        py310.add_note(ex, "Cannot find UniversalCRTSdkDir")
        raise

without the additional note:

    return self._use_last_dir_name(
        # Let TypeError bubble up when UniversalCRTSdkDir is None for backwards compatibility
        os.path.join(self.UniversalCRTSdkDir, 'lib')  # type: ignore[arg-type]
    )

All 3 options above preserve the behaviour of raising a TypeError because of a None UniversalCRTSdkDir

[abravalheri]

I am more inclined towards the 2nd option for being the balance between leaving things as they are and providing debugging information if necessary.

[Avasam]

Because RegistryInfo.lookup can return None, and this method returns the last falsy result if all falsy. Then the return type is potentially None. (which will happen if no kitsinstallationfolder value is found)

[abravalheri]

Thank you very much!