Add a class attribute hook to the plugin system #9881

FuegoFro · 2021-01-05T21:54:55Z

Description

This adds a get_class_attribute_hook to plugins to modify attributes on classes (as opposed to the existing get_attribute_hook, which is for attributes on instances). The modifications and added tests were modeled off of 3acbf3f.

Fixes #9645.

Test Plan

A new test is added which exercises the new functionality.

FuegoFro

Hey there! Let me know if you think this is something that is even worth having. It does solve a need for me that I don't think is otherwise possible currently (see the issue linked in the description), but I don't know how much extra surface area/ongoing maintenance it'll cause for you. Also let me know if there's something else I should be updating (documentation, maybe?). As mentioned in the description, I looked to the PR that added get_function_signature_hook to determine which files needed changing, and I didn't see anything around documentation changed there. Thanks in advance, and happy new year!

FuegoFro · 2021-01-05T21:55:11Z

mypy/checkmember.py

+        # Call the class attribute hook before returning.
+        fullname = '{}.{}'.format(info.fullname, name)
+        hook = mx.chk.plugin.get_class_attribute_hook(fullname)
+        if hook:
+            result = hook(AttributeContext(get_proper_type(mx.original_type),
+                                           result, mx.context, mx.chk))


I wasn't sure if the hook should be invoked for every exit point of this function. If so, I can pull the hook invocation into a wrapper function that calls into this one (or similar). I'm not familiar enough with this code to be confident where it would and would not make sense to use the hook, so advice is appreciated!

We should call the hook in all/most exit points, for consistency. It would be good to have tests for other exit points as well. I can help with them if you are unsure when they will be triggered, or you can try adding assert False to them and running tests to find examples.

I'd prefer to avoid defining a nested function, since currently they are kind of unoptimized in mypyc.

Brainstorming: Maybe something along these lines would work?

# Towards the top of the function fullname = ... hook = ... ... # At each exit point where we don't return None; apply_plugin_hook would be a new function return apply_plugin_hook(mx, hook, result)

Just so I understand, calling through to another function like this is fine, but doing the "opposite" is unoptimized? Though, I realize I wasn't super clear in my previous comment haha. To be a bit more explicit, what I was suggesting was to rename analyze_class_attribute_access to something like analyze_class_attribute_access_inner and then create a new

def analyze_class_attribute_access(...): result = analyze_class_attribute_access_inner(...) fullname = '{}.{}'.format(info.fullname, name) hook = mx.chk.plugin.get_class_attribute_hook(fullname) if hook: result = hook(AttributeContext(get_proper_type(mx.original_type), result, mx.context, mx.chk))

Would that pattern be unoptimized, or is it functions/closures declared within a function that's unoptimized?

In any case, when looking through this a bit more it does seem there's maybe a few exit points we don't want to call the hook for (which I wanted to double check with you) in addition to the None return, namely these two cases, which might make the whole _inner idea above moot anyway:

mypy/mypy/checkmember.py

Lines 782 to 789 in 6e8c0cd

elif isinstance(node.node, Var):

mx.not_ready_callback(name, mx.context)

return AnyType(TypeOfAny.special_form)

if isinstance(node.node, TypeVarExpr):

mx.msg.fail(message_registry.CANNOT_USE_TYPEVAR_AS_EXPRESSION.format(

info.name, name), mx.context)

return AnyType(TypeOfAny.from_error)

Do we want to call the hook for these two exit points?

JukkaL

Thanks for the PR! A plugin hook for class attributes seems like a reasonable thing to have.

JukkaL · 2021-01-06T14:58:52Z

mypy/checkmember.py

+        # Call the class attribute hook before returning.
+        fullname = '{}.{}'.format(info.fullname, name)
+        hook = mx.chk.plugin.get_class_attribute_hook(fullname)
+        if hook:
+            result = hook(AttributeContext(get_proper_type(mx.original_type),
+                                           result, mx.context, mx.chk))


We should call the hook in all/most exit points, for consistency. It would be good to have tests for other exit points as well. I can help with them if you are unsure when they will be triggered, or you can try adding assert False to them and running tests to find examples.

I'd prefer to avoid defining a nested function, since currently they are kind of unoptimized in mypyc.

Brainstorming: Maybe something along these lines would work?

# Towards the top of the function fullname = ... hook = ... ... # At each exit point where we don't return None; apply_plugin_hook would be a new function return apply_plugin_hook(mx, hook, result)

test-data/unit/check-custom-plugin.test

FuegoFro

Thank you for the review! 😀 I'll get the extra exit points and extra tests up soon 🙂

FuegoFro · 2021-01-06T18:19:24Z

mypy/checkmember.py

+        # Call the class attribute hook before returning.
+        fullname = '{}.{}'.format(info.fullname, name)
+        hook = mx.chk.plugin.get_class_attribute_hook(fullname)
+        if hook:
+            result = hook(AttributeContext(get_proper_type(mx.original_type),
+                                           result, mx.context, mx.chk))


Just so I understand, calling through to another function like this is fine, but doing the "opposite" is unoptimized? Though, I realize I wasn't super clear in my previous comment haha. To be a bit more explicit, what I was suggesting was to rename analyze_class_attribute_access to something like analyze_class_attribute_access_inner and then create a new

def analyze_class_attribute_access(...): result = analyze_class_attribute_access_inner(...) fullname = '{}.{}'.format(info.fullname, name) hook = mx.chk.plugin.get_class_attribute_hook(fullname) if hook: result = hook(AttributeContext(get_proper_type(mx.original_type), result, mx.context, mx.chk))

Would that pattern be unoptimized, or is it functions/closures declared within a function that's unoptimized?

In any case, when looking through this a bit more it does seem there's maybe a few exit points we don't want to call the hook for (which I wanted to double check with you) in addition to the None return, namely these two cases, which might make the whole _inner idea above moot anyway:

mypy/mypy/checkmember.py

Lines 782 to 789 in 6e8c0cd

elif isinstance(node.node, Var):

mx.not_ready_callback(name, mx.context)

return AnyType(TypeOfAny.special_form)

if isinstance(node.node, TypeVarExpr):

mx.msg.fail(message_registry.CANNOT_USE_TYPEVAR_AS_EXPRESSION.format(

info.name, name), mx.context)

return AnyType(TypeOfAny.from_error)

Do we want to call the hook for these two exit points?

test-data/unit/check-custom-plugin.test

mypy/checkmember.py

FuegoFro · 2021-01-11T19:47:43Z

@JukkaL I've got a few questions that I'd love some help/feedback on when you have a bit to look things over 🙂

JukkaL

Thanks for the updates! Overall looks good. Answered various questions.

mypy/checkmember.py

test-data/unit/check-custom-plugin.test

mypy/checkmember.py

FuegoFro · 2021-01-15T18:33:47Z

@JukkaL Thanks so much for the response! I think this should be ready to go now 🙂

FuegoFro · 2021-01-22T20:24:27Z

@JukkaL is there anything else you think needs to be changed here before merging? 🙂

FuegoFro · 2021-02-03T21:42:40Z

Heya @JukkaL just checking in to see if we can get this merged if there's nothing else needed here 🙂

FuegoFro · 2021-02-18T01:13:48Z

@JukkaL friendly ping, would love to get this merged in 🙂

charmoniumQ · 2021-04-15T18:56:54Z

Any update on this?

mezz · 2021-07-30T23:14:21Z

Hi @JukkaL, I think this PR is ready. Can you give it a final review?

This adds a hook to modify attributes on *classes* (as opposed to the existing attribute hook, which is for attributes on *instances*). This also adds a test to demonstrate the new behavior. The modifications and added tests were modeled off of python@3acbf3f. This is intended to solve python#9645

…in, marked with 'TODO RIGHT NOW NOMERGE'

…hem.

sobolevn

Thank you! This mostly looks good to me, but I think that we should add a bit more tests. And don't forget about docs: https://mypy.readthedocs.io/en/stable/extending_mypy.html

sobolevn · 2022-02-20T10:23:01Z

mypy/plugin.py

+
+            Cls.x
+
+        get_class_attribute_hook is called with '__main__.Cls.x'.


Suggested change

get_class_attribute_hook is called with '__main__.Cls.x'.

get_class_attribute_hook is called with '__main__.Cls.x' as fullname.

Changed 👍

sobolevn · 2022-02-20T10:26:37Z

test-data/unit/check-custom-plugin.test

+    attr = 'test'
+
+B: Any
+class Cls(B, metaclass=M):


I suggest to use one more test example with real B, because subtyping Any is not the same as subtyping a regular class.

sobolevn · 2022-02-20T10:27:38Z

test-data/unit/check-custom-plugin.test

+class Cls:
+    attr = None
+    def f(self) -> int:
+        return Cls.attr + 1


What about self.__class__.attr? Should we support that?

I think we can also check that c: Type[Cls]; c.attr works correctly.

Hm, so it looks like adding a test for __class__ is actually somewhat difficult, but also unclear that it actually adds much new value here. In particular it looks like the __class__ being specified as generic is done in typeshed here: https://github.com/python/typeshed/blob/master/stdlib/builtins.pyi#L87-L88

The way this is done in other tests is via [builtins fixtures/__new__.pyi] but that just defines __class__ = object and isn't generic. But even if we were able to use the real stubs, then really all that __class__ would be testing is that the generics system+stubs work correctly, and less about the class attribute plugin itself. But if you think testing the generic __class__ is important I can add the stub manually within this test!

The other one is already tested above:

mypy/test-data/unit/check-custom-plugin.test

Lines 918 to 919 in af49c12

x: Type[Cls]

reveal_type(x.attr) # N: Revealed type is "builtins.int"

FuegoFro

Thanks for the review! I added docs, here's a screenshot for convenience:

Let me know if there's anything else I can do to help get this PR merged! After ~a year, it'd be great to stop maintaining our own fork of mypy 🙏

FuegoFro · 2022-02-22T17:49:11Z

test-data/unit/check-custom-plugin.test

+    attr = 'test'
+
+B: Any
+class Cls(B, metaclass=M):


FuegoFro · 2022-02-22T17:51:13Z

test-data/unit/check-custom-plugin.test

+class Cls:
+    attr = None
+    def f(self) -> int:
+        return Cls.attr + 1


Hm, so it looks like adding a test for __class__ is actually somewhat difficult, but also unclear that it actually adds much new value here. In particular it looks like the __class__ being specified as generic is done in typeshed here: https://github.com/python/typeshed/blob/master/stdlib/builtins.pyi#L87-L88

The way this is done in other tests is via [builtins fixtures/__new__.pyi] but that just defines __class__ = object and isn't generic. But even if we were able to use the real stubs, then really all that __class__ would be testing is that the generics system+stubs work correctly, and less about the class attribute plugin itself. But if you think testing the generic __class__ is important I can add the stub manually within this test!

The other one is already tested above:

mypy/test-data/unit/check-custom-plugin.test

Lines 918 to 919 in af49c12

x: Type[Cls]

reveal_type(x.attr) # N: Revealed type is "builtins.int"

FuegoFro · 2022-02-22T17:51:29Z

mypy/plugin.py

+
+            Cls.x
+
+        get_class_attribute_hook is called with '__main__.Cls.x'.


Changed 👍

github-actions · 2022-02-22T19:55:07Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

hauntsaninja

Thanks for your patience, let's make this happen.

It wasn't clear to me why e.g. we don't apply the hook to things like type aliases, but then I saw Jukka's comments. I believe he's referring to stuff like:

mypy/mypy/semanal.py

Line 4106 in fc335cb

if n is not None and isinstance(n.node, (MypyFile, TypeInfo, TypeAlias)):

FuegoFro · 2022-05-10T21:26:56Z

Excellent, thank you so much for the merge! 😀

FuegoFro commented Jan 5, 2021

View reviewed changes

JukkaL reviewed Jan 6, 2021

View reviewed changes

FuegoFro commented Jan 6, 2021

View reviewed changes

mypy/checkmember.py Outdated Show resolved Hide resolved

FuegoFro requested a review from JukkaL January 11, 2021 19:43

JukkaL reviewed Jan 15, 2021

View reviewed changes

FuegoFro requested a review from JukkaL January 15, 2021 18:33

FuegoFro force-pushed the plugin_class_attribute_hook branch from 4b0f40f to 26f6cfe Compare March 9, 2021 18:53

FuegoFro force-pushed the plugin_class_attribute_hook branch from 26f6cfe to 98edc41 Compare April 5, 2021 23:33

FuegoFro force-pushed the plugin_class_attribute_hook branch from b8608f7 to 2b70840 Compare May 19, 2021 16:53

FuegoFro added 6 commits August 4, 2021 09:43

Don't import typing.Type at runtime to make mypyc happy

f1852e1

mypyc angy, no types 4 u

bbac561

Handle more exit cases, add more test cases. Some open questions rema…

379aa60

…in, marked with 'TODO RIGHT NOW NOMERGE'

Resolve open questions, mainly moving toward not using the hook for t…

987207a

…hem.

Update quotation marks for rebase

a976739

FuegoFro force-pushed the plugin_class_attribute_hook branch from 2b70840 to a976739 Compare August 4, 2021 16:52

FuegoFro mentioned this pull request Aug 4, 2021

Help getting https://github.com/python/mypy/pull/9881 merged #10931

Closed

Merge branch 'master' into plugin_class_attribute_hook

af49c12

97littleleaf11 added the priority-0-high label Dec 7, 2021

97littleleaf11 requested a review from hauntsaninja February 20, 2022 06:44

sobolevn self-requested a review February 20, 2022 09:53

sobolevn requested changes Feb 20, 2022

View reviewed changes

Add documentation and additional test

1816cf6

FuegoFro commented Feb 22, 2022

View reviewed changes

FuegoFro requested a review from sobolevn February 22, 2022 19:35

hauntsaninja approved these changes May 7, 2022

View reviewed changes

hauntsaninja merged commit 23e2a51 into python:master May 7, 2022

hauntsaninja mentioned this pull request May 24, 2022

Release 0.960 planning #12807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a class attribute hook to the plugin system #9881

Add a class attribute hook to the plugin system #9881

FuegoFro commented Jan 5, 2021

FuegoFro left a comment

FuegoFro Jan 5, 2021

JukkaL Jan 6, 2021

FuegoFro Jan 6, 2021

JukkaL left a comment

JukkaL Jan 6, 2021

FuegoFro left a comment

FuegoFro Jan 6, 2021

FuegoFro commented Jan 11, 2021

JukkaL left a comment

FuegoFro commented Jan 15, 2021

FuegoFro commented Jan 22, 2021

FuegoFro commented Feb 3, 2021

FuegoFro commented Feb 18, 2021

charmoniumQ commented Apr 15, 2021

mezz commented Jul 30, 2021

sobolevn left a comment

sobolevn Feb 20, 2022

FuegoFro Feb 22, 2022

sobolevn Feb 20, 2022

FuegoFro Feb 22, 2022

sobolevn Feb 20, 2022

FuegoFro Feb 22, 2022

FuegoFro left a comment

FuegoFro Feb 22, 2022

FuegoFro Feb 22, 2022

FuegoFro Feb 22, 2022

github-actions bot commented Feb 22, 2022

hauntsaninja left a comment

FuegoFro commented May 10, 2022

	elif isinstance(node.node, Var):
	mx.not_ready_callback(name, mx.context)
	return AnyType(TypeOfAny.special_form)

	if isinstance(node.node, TypeVarExpr):
	mx.msg.fail(message_registry.CANNOT_USE_TYPEVAR_AS_EXPRESSION.format(
	info.name, name), mx.context)
	return AnyType(TypeOfAny.from_error)


		Cls.x

		get_class_attribute_hook is called with '__main__.Cls.x'.

	x: Type[Cls]
	reveal_type(x.attr) # N: Revealed type is "builtins.int"

Add a class attribute hook to the plugin system #9881

Add a class attribute hook to the plugin system #9881

Conversation

FuegoFro commented Jan 5, 2021

Description

Test Plan

FuegoFro 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

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FuegoFro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FuegoFro commented Jan 11, 2021

JukkaL left a comment

Choose a reason for hiding this comment

FuegoFro commented Jan 15, 2021

FuegoFro commented Jan 22, 2021

FuegoFro commented Feb 3, 2021

FuegoFro commented Feb 18, 2021

charmoniumQ commented Apr 15, 2021

mezz commented Jul 30, 2021

sobolevn 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

Choose a reason for hiding this comment

FuegoFro 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

github-actions bot commented Feb 22, 2022

hauntsaninja left a comment

Choose a reason for hiding this comment

FuegoFro commented May 10, 2022