pylint-dev
Treat NewTypes like normal subclasses
Steps
- [X] For new features or bug fixes, add a ChangeLog entry describing what your PR does.
- [X] Write a good description on what the PR does.
Description
NewTypes are assumed not to inherit any members from their base classes. This results in incorrect inference results. Avoid this by changing the transformation for NewTypes to treat them like any other subclass.
Type of Changes
| Type | |
|---|---|
| ✓ | :bug: Bug fix |
| :sparkles: New feature | |
| :hammer: Refactoring | |
| :scroll: Docs |
Related Issue
https://github.com/PyCQA/pylint/issues/3162 https://github.com/PyCQA/pylint/issues/2296
Switched from checking for bit_count() to bit_length(), as I didn't realize the former was introduced in 3.10.
Made the requested changes -- let me know if I missed any! I also changed the transformation a bit, so that it takes in the correct number of arguments, and beefed up the tests a bit, both here and in the pylint PR (PyCQA/pylint#5542).
I also had to change the transformation to accurately resolve references to user-defined and imported types, so adding those tests was definitely important.
Hey sorry about the delay. I haven't had a whole lot of bandwidth to work on this, but I'm hoping to be able to get back to it this week. Just a heads up it hasn't totally fallen off my radar!
Pull Request Test Coverage Report for Build 3508001866
- 81 of 83 (97.59%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.08%) to 92.367%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| astroid/brain/brain_typing.py | 81 | 83 | 97.59% |
| <!-- | Total: | 81 | 83 |
| Totals | |
|---|---|
| Change from base Build 3503977238: | 0.08% |
| Covered Lines: | 9947 |
| Relevant Lines: | 10769 |
💛 - Coveralls
@cdce8p Thanks for taking a look! The feedback from you and @DanielNoord has been super helpful throughout this process.
Your point about the divergence of the inferred and runtime types make sense to me. I modeled the implementation on the one currently in the main branch, which leaves things in roughly the same state (inferring a class that doesn't exist at runtime), but that's kind of the source of all this trouble in the first place :wink:.
Do you have any thoughts or pointers on a more correct implementation? My initial thought would be a simple identity function, e.g.
def ChildType(x):
return x
Do you think that would be sufficient? It could be a bit strange that the code would infer a function, which would then be referenced by type annotations elsewhere, but I'm not sure if this presents any problems in practice.
It might also make sense to have the inferred implementation be identical to the upstream CPython one you linked -- though in that case, is any transformation even necessary? Based on a quick test, the results come back as Uninferable without any transforms, but I'm not sure if there's a lighter-touch way around that.
Finally, with regards to creating a new PR: I was planning on keeping the current implementation around in the git history. Though if a new PR is easier for you and other maintainers, or if the project generally makes use of squash commits, I can definitely do that instead.
Do you have any thoughts or pointers on a more correct implementation? My initial thought would be a simple identity function, e.g.
def ChildType(x): return xDo you think that would be sufficient? It could be a bit strange that the code would infer a function, which would then be referenced by type annotations elsewhere, but I'm not sure if this presents any problems in practice.
An identity function might work, but I would patch ChildType. Instead it might be better to replace the NewType inferred function itself. That could also solve the type annotation issue. Basically the inferred type should become x instead of ChildType. You just don't get the subclassing.
Tbh though, I haven't had time to investigate if it's at all possible.
Finally, with regards to creating a new PR: I was planning on keeping the current implementation around in the git history. Though if a new PR is easier for you and other maintainers, or if the project generally makes use of squash commits, I can definitely do that instead.
We do use squash merge usually. However, my reasoning was more from a practical approach. No unrelated comments and if everything else fails, we can still come back to this one.
I'll convert the PR to a draft. We can close it once the alternative is merged.
