Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement somewhat pythonic type hints in sctypes #4569

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jul 6, 2024

Currently, the type-check functions provided by sctypes have no real way to pass the parsed logic to intellisense like isinstance; while I ultimately believe most/all of these functions should transition to isinstance eventually, that'd be a pretty radical overhaul to the repo. This PR is a stopgap solution, changing the return types of those functions to TypeGuard for Python 3.10 and later—while still returning bool, it'll make intellisense for the passed value similar to isinstance. 3.9 and earlier will see identical behavior to the existing implementation.

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 read the README.rst)
  • I have updated the appropriate documentation

@mwichmann
Copy link
Collaborator

mwichmann commented Jul 6, 2024

I agree with the problem... the home-grown typecheckers trip up mypy for sure, which has no idea we're effectively doing type narrowing with these. I've experimented with switching the types the type-check functions use to something like this:

DictTypes = MutableMapping  # includes UserDict
ListTypes = MutableSequence  # includes UserList, deque, etc.
SequenceTypes = (MutableSequence, tuple)
StringTypes = (str, UserString)

and my test version contained this comment:

# SCons type checks.
# Note that the use of these checks, which is extrememly common throughout,
# inhibits the ability of static checkers (mypy) to properly infer type
# narrowing, while the corresponding isinstance() would not.

I haven't gone down the path of changing large numbers of type checks, but the existing ones work equivalently with the "new" type definitions - hints in our little benchmarking tool is it got a hair slower, though; haven't pursued this further, enough irons in the fire...

@bdbaddog
Copy link
Contributor

bdbaddog commented Jul 7, 2024

@Repiteo would this change cause a mypy run to fail for python 3.10 and above?

@Repiteo
Copy link
Contributor Author

Repiteo commented Jul 9, 2024

@bdbaddog The python version for mypy is set to 3.8 in pyproject.toml, so it wouldn't be using 3.10+ in the first place. That having been said, mypy already registers a ton of errors in the repo, regardless of the python version chosen.

3.8

Found 1177 errors in 118 files (checked 296 source files)

3.12

Found 1177 errors in 128 files (checked 296 source files)

So as it turns out, it doesn't add any new errors! If we wanna be more serious about abiding mypy-friendly code, that portion of the settings should probably be extended as well.

@mwichmann
Copy link
Collaborator

Certainly getting anywhere near to "passing" pylint or mypy is a pipe dream at the moment, but I do look at them from time to time with an eye to flushing out some of the interesting situations they turn up. There's a lot of stuff that could indicate problems (e.g. method signatures in derived classes not matching those in parent classes) but probably don't due to how things get used. And some that just aren't ever going to change, e.g. PEP8 naming conventions for functions and methods, due to having been that way forever with many of those as official APIs. It just is what it is... So I'd say "not making things worse" with new changes is a worthy goal, and I'm fairly convinced on this one. We do have a number of requests to "make things better in an IDE", and though I haven't tried this out, it seems this does edge us in that direction.

@bdbaddog bdbaddog merged commit b4260a7 into SCons:master Jul 9, 2024
6 of 8 checks passed
@Repiteo Repiteo deleted the Pythonic-type-hinting branch July 9, 2024 23:47
@mwichmann mwichmann added this to the NextRelease milestone Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants