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

Fix decimal #2323

Merged
merged 12 commits into from
Jul 19, 2018
Merged

Fix decimal #2323

merged 12 commits into from
Jul 19, 2018

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Jul 11, 2018

Fix various problems with decimal and add missing annotations.

@srittau
Copy link
Collaborator Author

srittau commented Jul 12, 2018

mypy crash reported as python/mypy#5347.

@srittau
Copy link
Collaborator Author

srittau commented Jul 13, 2018

Crash is now fixed in mypy master.

@JelleZijlstra
Copy link
Member

Thanks! I restarted Travis, will take another look after it passes.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Left a few comments.

_ComparableNum = Union[Decimal, int, float]
_DecimalNew = Union[Decimal, int, float, Text, Tuple[int, Sequence[int], int]]
if sys.version_info >= (3,):
_ComparableNum = Union[Decimal, numbers.Rational, float]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional definition makes it look confusingly like int is not supported in Python 3. In fact, it is redundant in Python 2 too, because it's automatically promoted to float. It would be clearer to have int either in both or in neither of the branches.

def __delattr__(self, name): ...
def __reduce__(self): ...
def clear_flags(self): ...
def __setattr__(self, name: str, value: Any) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should actually be omitted because the effect of having it is that the type checker will think setting any attribute is allowed. However, the implementation actually only allows the attributes that are known to exist on the class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced it with a comment saying so, for future reference.

def is_canonical(self, a: _Decimal) -> Decimal: ...
def is_finite(self, a: _Decimal) -> Decimal: ...
def is_infinite(self, a: _Decimal) -> Decimal: ...
def is_nan(self, a: _Decimal) -> Decimal: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these return bool, although I haven't checked all of those. I'm guessing divmod returns a Tuple[Decimal, Decimal].

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I am rechecking the return values of all Context methods.

@srittau
Copy link
Collaborator Author

srittau commented Jul 14, 2018

I am not sure why the travis build does not start automatically after the latest patches.

@JelleZijlstra
Copy link
Member

I closed and reopened, and now Travis seems to be running.

@JelleZijlstra JelleZijlstra merged commit 4b8c374 into python:master Jul 19, 2018
@srittau srittau deleted the fix-decimal branch July 19, 2018 08:42
@gvanrossum
Copy link
Member

I wonder if this broke something. I've got code that's essentially (though not quite)

float(Decimal() if bool() else 0)

(Though that example doesn't provoke it.)

The if/else expression's type is revealed as object, and
float(object()) is invalid. Maybe it was valid before? Or maybe the
join previously returned Union[Decimal, int] and now deteriorated to
object?

I kind of suspect #2323, since
it removed the SupportsFloat and SupportsInt base classes from Decimal
(the last typeshed sync before this afternoon's was
python/mypy#5373, which synced typeshed 3
commits before 2323).

@srittau
Copy link
Collaborator Author

srittau commented Jul 26, 2018

I think it's reasonable to assume that this change triggered the problem, although I suspect a bug in mypy. Since SupportsAbs and SupportsFloat are protocols, they should not be needed as base classes.

The following test cast demonstrates the problem:

from typing import SupportsFloat


class Floaty:
    def __float__(self): ...


class Supporter(SupportsFloat):
    def __float__(self): ...


reveal_type(Floaty() if bool() else 0)
reveal_type(Supporter() if bool() else 0)

The former is revealed as builtins.object, the latter as typing.SupportsFloat. As short term solution would be to add the base classes to Decimal again, but I believe this should be fixed in mypy.

@JukkaL
Copy link
Contributor

JukkaL commented Jul 27, 2018

@srittau Mypy works like by design, though it's clearly suboptimal in this case. It's not clear how this should be fixed in mypy. It's easy to imagine how this could work in simple cases, but a general solution seems difficult to define -- so that it's both practical and has the desired theoretical properties. Luckily the problems seem to arise pretty infrequently, and they can often be worked around by adding an explicit protocol base class. Can we add the base classes back? We should probably add a comment explaining why they are there so that they won't be removed again in the future.

If you want to propose a mypy fix, I'm happy to discuss it. I've looked into this a bit but gave up as it didn't seem worth the effort at the time.

@srittau
Copy link
Collaborator Author

srittau commented Jul 27, 2018

The problem I have with that is that it basically makes protocols useless, because the if .... else ... construct will not work with any protocol. This would mean we'd have to add all protocols to all classes again.

@JukkaL
Copy link
Contributor

JukkaL commented Jul 30, 2018

This is the first time I've seen this cause problems, so it's a bit unfair to say that it makes protocols basically useless. I proposed a potential mypy improvement in python/mypy#5392 (comment), which could partially address the issue.

@srittau
Copy link
Collaborator Author

srittau commented Jul 30, 2018

Well, useless in the sense that whenever someone comes up with a case where a protocol is used in an if ... else ... construct, we'd have to add the protocol as base class to the involved classes or add them preemptively. I suspect that it didn't come up much so far is because protocols are not used much yet in typeshed.

But just implementing the suggestion from typing/issues#564 would likely make this problem more pronounced.

yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
* Use Tuple field in DecimalTuple

* Remove unnecessary base classes from Decimal

* Decimal.__init__ -> __new__

* Decimal.__ne__ is not defined in Python 3

* Add Decimal.as_integer_ratio()

* Annotate DecimalException.handle()

* Correct types of Decimal method arguments

* Add missing arguments and optional markers to Decimal

* Add missing arguments to Context

* Remove spurious int from Unions with float

* Remove Context.__setattr__()

* Fix return types of Context methods
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.

4 participants