Can understand functools.total_ordering #7831

AWhetter · 2019-10-31T04:49:20Z

This change adds support for understanding the additional methods that are added by decorating a class with functools.total_ordering.

Limitations

There's still some potential improvements to make to this before merging:

There is no support for overloaded definitions. mypy/plugins/functools.py:_analyze_class looks specifically for FuncItems. I wasn't sure how to get the type of the arguments off of an overload?
mypy/plugins/functools.py:_find_other_type: I don't know if there is an existing function that I can use to find the value of the first argument instead?
The type of the input argument isn't really correct. For the input argument, it needs to support both what the argument in the implemented comparison supports and what the __eq__ supports (depending on which comparison method was implemented. For example if __ge__ was implemented, the argument to __le__ needs to be compatible with __ge__ and __eq__ (https://github.com/python/cpython/blob/bdac32e9fe25fdb97a7172a93aabd1ffead89462/Lib/functools.py#L146).
The return types also have room for improvement. Currently the generated methods inherit the return type of the implemented comparison method, but this is usually a Union of the __eq__ return type and the return type of the implemented method. Sometimes it's the return type of the __bool__ of those types. How can I condense the Union of the two potential return types if possible (eg how could the code know to use bool instead of Union[bool,bool]?

Closes #4610

aecay · 2019-11-14T10:38:01Z

Without realizing it, I made #7848 one day later to do exactly the same thing. Great minds think alike I guess 😝

I think the code in this PR is a better base to build on than the code in #7848, so once the questions above are answered and anything from my PR that isn't here is migrated over (see below), I think the way forward is to land this one and close mine.

What I see missing from this version of the change is the checking for the existence of the __eq__ function. It's not a runtime error to fail to define __eq__, because object has an implementation that will be used as a fallback, which uses reference equality. But I can't think of a case where this semantics would make sense with total_ordering. What I chose to do was emit a warning when we detect that there's no definition of __eq__ in the mro (besides object.__eq__). I wasn't super confident that was TRT to do, but it's definitely better than not looking at __eq__ at all.

aecay · 2019-11-14T10:39:04Z

mypy/plugins/functools.py

+    if isinstance(root_method.type, CallableType):
+        ret_type = root_method.type.ret_type
+    else:
+        ret_type = ctx.api.named_type('__builtins__.bool')


Shouldn't it just be an error if __lt__ (etc.) is not callable?

If __lt__ is not callable, it's okay to either generate an error or to fall back to some default behavior.

aecay · 2019-11-14T10:39:53Z

mypy/plugins/functools.py

+    for additional_op in _ORDERING_METHODS - {root}:
+        if additional_op not in comparison_methods:


for additional_op in _ORDERING_METHODS - set(comparison_methods.keys()) is an alternative. I guess it's debatable whether it's clearer, but it does avoid a level of indentation...

I like this better than using a set operation.

JukkaL

Thanks for the PR! total_ordering is a useful feature to support. Generally looks good; left various minor comments, mostly about various special cases.

JukkaL · 2019-11-19T15:53:18Z

mypy/plugins/functools.py

+
+    comparison_methods = _analyze_class(ctx)
+    if not comparison_methods:
+        ctx.api.fail('must define at least one ordering operation: < > <= >=', ctx.reason)


For consistency, error message should start with a capital letter.

Maybe also mention total_ordering in the error message?

JukkaL · 2019-11-19T15:53:47Z

mypy/plugins/functools.py

+                                            auto_attribs_default: bool = False) -> None:
+    """Add dunder methods to classes decorated with functools.total_ordering."""
+    if ctx.api.options.python_version < (3, 2):
+        ctx.api.fail("functools.total_ordering is not supported in Python 2 or 3.1", ctx.reason)


Nit: add double quotes around functools.total_ordering for consistency.

JukkaL · 2019-11-19T15:57:14Z

mypy/plugins/functools.py

+    cur_pos_arg = 0
+    other_arg = None
+    for arg in method.arguments:
+        if arg.kind == ARG_POS:


ARG_OPT seems valid here as well (though pretty unusual).

JukkaL · 2019-11-19T15:58:56Z

mypy/plugins/functools.py

+        ret_type = ctx.api.named_type('__builtins__.bool')
+    for additional_op in _ORDERING_METHODS - {root}:
+        if additional_op not in comparison_methods:
+            args = [Argument(Var('other', other_type), other_type, None, ARG_POS)]


If other_type is None, the argument type shoud probably be AnyType instead of None.

JukkaL · 2019-11-19T16:03:05Z

mypy/plugins/functools.py

+            if name in cls.names and name not in comparison_methods:
+                node = cls.names[name].node
+                if isinstance(node, FuncItem):
+                    comparison_methods[name] = node


What if the method is implemented using a variable (e.g. __lt__ = lambda ...)? More generally, if the dunder method exists as any kind of node in the symbol table (not just FuncItem), we probably don't want to generate a method.

Random idea: use Optional[FuncItem] as the dictionary value type. If it's None, the method is defined, but it's something unexpected. In this case it's okay to fall back to default types for the signature of a generated method.

JukkaL · 2019-11-19T16:04:21Z

test-data/unit/check-functools.test

+Ord() <= 1  # E: Unsupported operand types for <= ("Ord" and "int")
+Ord() == 1
+Ord() > 1  # E: Unsupported operand types for > ("Ord" and "int")
+Ord() >= 1  # E: Unsupported operand types for >= ("Ord" and "int")


Add reveal_type(Ord() <= Ord()) or similar to verify the type of the operation is as expected. Maybe use a non-bool return type in some test case.

JukkaL · 2019-11-19T16:06:34Z

test-data/unit/check-functools.test

+[builtins fixtures/ops.pyi]
+[builtins fixtures/dict.pyi]
+
+[case testTotalOrderingEqLe]


The remaining tests are a bit verbose. Maybe simplify some of them and only test cases that are expected to be materially different from other test cases? Another option is to vary the test cases in some aspects. Here are some ideas (some of these could be separate tests as well):

Omit the function type annotation in some test case (fall back to Any types).

Use a different argument type in some test case.

Use an unusual return type in some test case, such as str.

Test non-callable __lt__.

Test method defined as a variable (__lt__ = lambda: ..., for example).

JukkaL · 2019-11-19T16:09:31Z

mypy/plugins/functools.py

+    root_method = comparison_methods[root]
+    other_type = _find_other_type(root_method)
+    if isinstance(root_method.type, CallableType):
+        ret_type = root_method.type.ret_type


The generated methods seem to always return bool values at runtime. Copying of the return type thus seems incorrect. However, having inconsistent return types is problematic as well. Maybe use bool as the return type if the existing methods use bool, and otherwise fall back to Any to avoid trouble.

I've implemented this but it wasn't totally simple. The return type of "bool" is unbound when the plugin sees it so I've had to do some matching that feel a bit hacky. Let me know if you think there's a better way of doing it.

JukkaL · 2019-11-19T16:13:15Z

mypy/plugins/functools.py

+    if isinstance(root_method.type, CallableType):
+        ret_type = root_method.type.ret_type
+    else:
+        ret_type = ctx.api.named_type('__builtins__.bool')


If __lt__ is not callable, it's okay to either generate an error or to fall back to some default behavior.

JukkaL · 2019-11-19T16:13:39Z

mypy/plugins/functools.py

+    for additional_op in _ORDERING_METHODS - {root}:
+        if additional_op not in comparison_methods:


I like this better than using a set operation.

AWhetter · 2020-10-24T04:51:49Z

All the comments are addressed so this should be ready for another round of review.

JelleZijlstra

I have a couple of minor comments, but otherwise this looks good to go. Sorry for the long wait.

JelleZijlstra · 2020-11-25T02:11:41Z

mypy/plugins/functools.py

+                                            auto_attribs_default: bool = False) -> None:
+    """Add dunder methods to classes decorated with functools.total_ordering."""
+    if ctx.api.options.python_version < (3, 2):
+        ctx.api.fail('"functools.total_ordering" is not supported in Python 2 or 3.1', ctx.reason)


Or 3.0 I guess. It doesn't really matter since mypy doesn't support any of these early Python 3 versions, so this could just check for Python 2 too.

JelleZijlstra · 2020-11-25T02:12:08Z

mypy/plugins/functools.py

+    if root_method.type.ret_type != ctx.api.named_type('__builtins__.bool'):
+        proper_ret_type = get_proper_type(root_method.type.ret_type)
+        if not (isinstance(proper_ret_type, UnboundType)
+                and proper_ret_type.name.endswith('bool')):


endswith seems wrong, what if I make a type called "notabool"?

JelleZijlstra · 2020-11-25T02:12:26Z

mypy/plugins/functools.py

+
+
+def _find_other_type(method: _MethodInfo) -> Type:
+    """Find the type of the ``other`` argument in a comparision method."""


Suggested change

"""Find the type of the ``other`` argument in a comparision method."""

"""Find the type of the ``other`` argument in a comparison method."""

belm0 · 2021-02-09T00:34:07Z

@AWhetter are you available to follow up on the PR?

AWhetter · 2021-02-11T04:30:43Z

Sorry, this fell of my radar. I've made the requested changes.

ilevkivskyi mentioned this pull request Nov 13, 2019

Add plugin to typecheck functools.total_ordering #7848

Closed

aecay reviewed Nov 14, 2019

View reviewed changes

JukkaL reviewed Nov 19, 2019

View reviewed changes

jsirois mentioned this pull request Apr 13, 2020

Add a mypy plugin to support @total_ordering. pantsbuild/pants#9525

Merged

AWhetter force-pushed the fix_4610 branch from cf81f1c to cb8b028 Compare October 18, 2020 22:32

Can understand functools.total_ordering

ce87cc3

AWhetter force-pushed the fix_4610 branch from cb8b028 to ce87cc3 Compare October 24, 2020 04:18

JelleZijlstra approved these changes Nov 25, 2020

View reviewed changes

AWhetter force-pushed the fix_4610 branch from ce87cc3 to de6427b Compare February 9, 2021 04:24

AWhetter added 2 commits February 8, 2021 20:26

fixup! Can understand functools.total_ordering

15b51fc

Merge remote-tracking branch 'upstream/master' into fix_4610

52a1191

AWhetter force-pushed the fix_4610 branch from de6427b to 52a1191 Compare February 9, 2021 04:27

Update double quotes in error messages

33f80f5

JelleZijlstra merged commit c5e4e0f into python:master Apr 11, 2021

woodruffw mentioned this pull request Dec 11, 2024

Correctly support >= and <= for type checking woodruffw/ordered_enum#38

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Can understand functools.total_ordering #7831

Can understand functools.total_ordering #7831

AWhetter commented Oct 31, 2019 •

edited

Loading

aecay commented Nov 14, 2019

aecay Nov 14, 2019

JukkaL Nov 19, 2019

aecay Nov 14, 2019

JukkaL Nov 19, 2019

JukkaL left a comment

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

AWhetter Oct 18, 2020

JukkaL Nov 19, 2019

JukkaL Nov 19, 2019

AWhetter commented Oct 24, 2020

JelleZijlstra left a comment

JelleZijlstra Nov 25, 2020

JelleZijlstra Nov 25, 2020

JelleZijlstra Nov 25, 2020

belm0 commented Feb 9, 2021

AWhetter commented Feb 11, 2021

		for additional_op in _ORDERING_METHODS - {root}:
		if additional_op not in comparison_methods:



		def _find_other_type(method: _MethodInfo) -> Type:
		"""Find the type of the ``other`` argument in a comparision method."""

Can understand functools.total_ordering #7831

Can understand functools.total_ordering #7831

Conversation

AWhetter commented Oct 31, 2019 • edited Loading

Limitations

aecay commented Nov 14, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AWhetter commented Oct 24, 2020

JelleZijlstra 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

belm0 commented Feb 9, 2021

AWhetter commented Feb 11, 2021

AWhetter commented Oct 31, 2019 •

edited

Loading