Treat NewTypes like normal subclasses #1301

colatkinson · 2021-12-17T07:12:02Z

Steps

For new features or bug fixes, add a ChangeLog entry describing what your PR does.
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 fix
	✨ New feature
	🔨 Refactoring
	📜 Docs

Related Issue

pylint-dev/pylint#3162
pylint-dev/pylint#2296

colatkinson · 2021-12-18T08:02:33Z

Switched from checking for bit_count() to bit_length(), as I didn't realize the former was introduced in 3.10.

DanielNoord

Thank you @colatkinson!

This seems like a good change and closes two old pylint issues. Let me know if you have any questions about my comments.

astroid/brain/brain_typing.py

tests/unittest_brain.py

astroid/brain/brain_typing.py

colatkinson · 2021-12-23T10:44:08Z

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 (pylint-dev/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.

astroid/brain/brain_typing.py

DanielNoord

Thanks @colatkinson

I'm a bit out of my depth with your last question so I hope another contributor with more astroid experience finds the time to review this soon. Just have some final questions myself.

astroid/brain/brain_typing.py

tests/unittest_brain.py

astroid/brain/brain_typing.py

colatkinson · 2022-02-07T08:20:26Z

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!

DanielNoord

I like the approach you took! I did a first review and left some comments. Let me know if you have any questions!

astroid/brain/brain_typing.py

DanielNoord · 2022-03-09T21:34:45Z

astroid/brain/brain_typing.py

+) -> typing.Iterator[nodes.ClassDef]:
+    """Infer a typing.NewType(...) call"""
+    try:
+        func = next(node.func.infer(context=context_itton))


If there is any ambiguity in the inference then we fail to notice that here. Perhaps we should use helpers.safe_infer?

Ran into a small issue with this. Since decimal.Decimal has two implementations (_pydecimal vs _decimal) it always seems to end up ambiguous.

Is this the desired behavior for cases like this? If so I'll make the change and rework the tests, since I chose Decimal as a test type arbitrarily anyway, but I figured I'd confirm.

astroid/brain/brain_typing.py

tests/unittest_brain.py

coveralls · 2022-05-28T11:32:39Z

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%

Totals
Change from base Build 3503977238:	0.08%
Covered Lines:	9947
Relevant Lines:	10769

💛 - Coveralls

DanielNoord

Some final comments.

I'd really like somebody with more experience on inference to take a look at this as I'm not sure the way we resolve the base names here is something we normally do in the brains or if we have utils for this.

@cdce8p Do you see chance to take a look at this PR perhaps?

DanielNoord · 2022-06-01T09:18:19Z

astroid/brain/brain_typing.py

+    if func.qname() != "typing.TypeVar":
        raise UseInferenceDefault


I believe this can be removed as it is non-sensical. This is tested by looks_like_typing_typevar.

DanielNoord · 2022-06-01T09:18:31Z

astroid/brain/brain_typing.py

+    except (InferenceError, StopIteration) as exc:
+        raise UseInferenceDefault from exc
+
+    if func.qname() != "typing.NewType":


I believe this can be removed as it is non-sensical. This is tested by looks_like_typing_newtype.

DanielNoord · 2022-06-01T09:19:09Z

astroid/brain/brain_typing.py

    except (InferenceError, StopIteration) as exc:
        raise UseInferenceDefault from exc

-    if func.qname() not in TYPING_TYPEVARS_QUALIFIED:
+    if func.qname() != "typing.TypeVar":
        raise UseInferenceDefault
    if not node.args:
        raise UseInferenceDefault


L127-128 has a drop in coverage. Could you re-create a test for it?

DanielNoord · 2022-06-01T09:19:23Z

astroid/brain/brain_typing.py

+
+    if func.qname() != "typing.NewType":
+        raise UseInferenceDefault
+    if len(node.args) != 2:


Could you create a test for this? It is currently uncovered.

DanielNoord · 2022-06-01T09:19:38Z

astroid/brain/brain_typing.py

+        raise UseInferenceDefault
+
+    # Cannot infer from a dynamic class name (f-string)
+    if isinstance(node.args[0], JoinedStr):


You can probably re-use the test for this for the comment above.

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. pylint-dev/pylint#3162 pylint-dev/pylint#2296

cdce8p

Thanks for your patience. I finally got around to look it your PR. First of all, I'm quite impressed with the work you did here @colatkinson! You obviously have a pretty good understanding of the astroid internals already.

However, as impressive as the PR is, I'm not quite sure the current approach is the correct one. You've modeled the inference result after the typing implementation. Astroid (and pylint) aren't type checkers though. They use inference to determine the runtime results. For a NewType call, that's simply the argument itself which is returned and not an instance of a subclass.

from typing import NewType

B = NewType("B", "A")

class A:
    def __init__(self, value: int):
        self.value = value

print(B(A(5)))

<__main__.A object at 0x100b1b160>

The inference result with the current implementation:

[<Instance of .B at 0...427248880>]

The CPython implementation: https://github.com/python/cpython/blob/v3.10.5/Lib/typing.py#L2482-L2483

--
Am I missing something / what do you think? In case we change the approach, I would like to suggest that you create a new PR for it. That way we can at least preserve the idea and implementation of resolving base classes should there ever be a need for it.

cdce8p · 2022-06-08T08:44:07Z

tests/unittest_brain.py

+        b = B(5)
+        b #@


Suggested change

b = B(5)

b #@

b = B(A(5))

b #@

What is the inference result of b? At runtime it should be an instance of A.

cdce8p · 2022-06-08T10:16:27Z

astroid/brain/brain_typing.py

+        new_node.postinit(
+            bases=new_bases, body=new_node.body, decorators=new_node.decorators
+        )


new_node is fully constructed already. It's enough to set bases manually.

Suggested change

new_node.postinit(

bases=new_bases, body=new_node.body, decorators=new_node.decorators

)

new_node.bases = new_bases

colatkinson · 2022-06-14T10:15:02Z

@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 😉.

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.

cdce8p · 2022-06-22T00:19:47Z

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.

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.

colatkinson force-pushed the fix-typing-newtype-derivation branch from 4cf70d3 to 9caef00 Compare December 17, 2021 07:12

colatkinson mentioned this pull request Dec 17, 2021

Add tests for NewType false positives pylint-dev/pylint#5542

Open

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Dec 17, 2021

Pierre-Sassoulas modified the milestones: 2.9.1, 2.10.0 Dec 17, 2021

Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Dec 17, 2021

colatkinson force-pushed the fix-typing-newtype-derivation branch from 9caef00 to b634003 Compare December 18, 2021 08:01

cdce8p self-requested a review December 18, 2021 13:16

DanielNoord requested changes Dec 20, 2021

View reviewed changes

colatkinson force-pushed the fix-typing-newtype-derivation branch from b634003 to a49de04 Compare December 23, 2021 10:34

colatkinson commented Dec 23, 2021

View reviewed changes

astroid/brain/brain_typing.py Outdated Show resolved Hide resolved

DanielNoord requested changes Dec 23, 2021

View reviewed changes

astroid/brain/brain_typing.py Outdated Show resolved Hide resolved

astroid/brain/brain_typing.py Outdated Show resolved Hide resolved

tests/unittest_brain.py Outdated Show resolved Hide resolved

colatkinson force-pushed the fix-typing-newtype-derivation branch from a49de04 to 0233147 Compare December 24, 2021 00:04

DanielNoord reviewed Dec 30, 2021

View reviewed changes

tests/unittest_brain.py Outdated Show resolved Hide resolved

tests/unittest_brain.py Outdated Show resolved Hide resolved

astroid/brain/brain_typing.py Outdated Show resolved Hide resolved

cdce8p modified the milestones: 2.10.0, 2.11.0 Feb 27, 2022

colatkinson force-pushed the fix-typing-newtype-derivation branch 2 times, most recently from 957d223 to a1370de Compare March 7, 2022 11:34

colatkinson requested a review from DanielNoord March 7, 2022 11:36

DanielNoord requested changes Mar 9, 2022

View reviewed changes

jacobtylerwalls mentioned this pull request May 25, 2022

Multiple false positives for numeric NewTypes pylint-dev/pylint#6696

Closed

colatkinson force-pushed the fix-typing-newtype-derivation branch from a1370de to e7fe8b3 Compare May 28, 2022 10:59

colatkinson requested a review from DanielNoord May 28, 2022 11:01

DanielNoord requested changes Jun 1, 2022

View reviewed changes

Improve NewType inference for string forward refs

6bbb840

colatkinson force-pushed the fix-typing-newtype-derivation branch from e7fe8b3 to 6bbb840 Compare June 4, 2022 09:24

cdce8p requested changes Jun 8, 2022

View reviewed changes

cdce8p removed this from the 2.12.0 milestone Jun 8, 2022

cdce8p marked this pull request as draft June 22, 2022 00:19

cdce8p added Blocked 🚧 A PR or issue blocked by another PR or issue inference labels Jun 22, 2022

Merge branch 'main' into fix-typing-newtype-derivation

1405f63

Pierre-Sassoulas mentioned this pull request Jun 9, 2023

False positive not-an-iterable for typing.NewType pylint-dev/pylint#2296

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat NewTypes like normal subclasses #1301

Treat NewTypes like normal subclasses #1301

colatkinson commented Dec 17, 2021

colatkinson commented Dec 18, 2021

DanielNoord left a comment

colatkinson commented Dec 23, 2021

DanielNoord left a comment

colatkinson commented Feb 7, 2022

DanielNoord left a comment

DanielNoord Mar 9, 2022

colatkinson May 28, 2022

coveralls commented May 28, 2022 •

edited

Loading

DanielNoord left a comment

DanielNoord Jun 1, 2022

DanielNoord Jun 1, 2022

DanielNoord Jun 1, 2022

DanielNoord Jun 1, 2022

DanielNoord Jun 1, 2022

cdce8p left a comment

cdce8p Jun 8, 2022

cdce8p Jun 8, 2022

colatkinson commented Jun 14, 2022

cdce8p commented Jun 22, 2022

		if func.qname() != "typing.TypeVar":
		raise UseInferenceDefault

Treat NewTypes like normal subclasses #1301

Are you sure you want to change the base?

Treat NewTypes like normal subclasses #1301

Conversation

colatkinson commented Dec 17, 2021

Steps

Description

Type of Changes

Related Issue

colatkinson commented Dec 18, 2021

DanielNoord left a comment

Choose a reason for hiding this comment

colatkinson commented Dec 23, 2021

DanielNoord left a comment

Choose a reason for hiding this comment

colatkinson commented Feb 7, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 28, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3508001866

💛 - Coveralls

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colatkinson commented Jun 14, 2022

cdce8p commented Jun 22, 2022

coveralls commented May 28, 2022 •

edited

Loading