Stricter type checking for Python integers and floats #1554

heinrich5991 · 2023-05-23T21:37:38Z

The error messages mirror the ones used in Python itself. E.g.:

>>> chr(1.0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'float' object cannot be interpreted as an integer
>>> class A:
...     def __index__(self):
...             return 1.0
...
>>> chr(A())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __index__ returned non-int (type float)

and

>>> math.sqrt("1")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: must be real number, not str
>>> class A:
...     def __float__(self):
...             return "1"
...
>>> math.sqrt(A())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: A.__float__ returned non-float (type str)

Fixes #1544.

Also add some tests checking the stricter type checking plus a few floating point roundtrip tests.

heinrich5991 · 2023-05-23T21:40:01Z

This is a breaking change and should probably get a CHANGELOG entry, if it were accepted.

bendk

I love the deletion of all the coerce() calls, but I'm not sure about how to implement the check.

bendk · 2023-05-24T13:54:09Z

uniffi_bindgen/src/bindings/python/templates/RustBufferHelper.py

        return value

+    @classmethod
+    def write(cls, value, buf):
+        cls.writeChecked(value, buf)


I'm not understanding the need for a separate writeChecked method. What if this stayed as just write()?

Suggested change

cls.writeChecked(value, buf)

cls.writeChecked(cls.check(value), buf)

It's missing a cls.check call around the value. Seems like I'm missing some tests…

Added that cls.check call.

Ahh, I understand now. What about write_unchecked instead? That follows the Rust meaning of "unchecked" and also follows PEP 8.

What about write_unchecked instead?

write_unchecked/writeUnchecked sounds good.

and also follows PEP 8.

I also prefer that naming style. I tried to fit in though; most of the file is using camelCase, e.g. all of RustBuffer's methods: allocWithBuilder, consumeWithStream, … Should I still change it?

I changed it to writeUnchecked for now. If you want to have it as write_unchecked instead, please say so.

I agree what you have now is more consistent with the existing style, even though pep-8 does recommend otherwise.

uniffi_bindgen/src/bindings/python/templates/RustBufferHelper.py

mhammond

This seems fine to me

mhammond · 2023-05-25T17:07:41Z

uniffi_bindgen/src/bindings/python/templates/RustBufferHelper.py

+        try:
+            value = value.__index__()
+        except Exception:
+            raise TypeError("'{}' object cannot be interpreted as an integer".format(type(value).__name__))


As an aside, I'd probably be in favour of declaring we only work with 3.6 and up (even 3.7 TBH, seeing 3.6 is getting no security updates) which would mean f-strings could be used.

side-note: Glean supports 3.6 and that's also the minimum for mozilla-central.

mhammond · 2023-05-25T17:12:49Z

uniffi_bindgen/src/bindings/python/templates/RustBufferHelper.py

        return value

+    @classmethod
+    def write(cls, value, buf):
+        cls.writeChecked(value, buf)


I agree what you have now is more consistent with the existing style, even though pep-8 does recommend otherwise.

heinrich5991 · 2023-05-31T08:30:42Z

What's still missing to get this merged? @bendk

badboy · 2023-05-31T09:33:54Z

What's still missing to get this merged? @bendk

I'll give this a look tomorrow

mhammond · 2023-05-31T12:55:29Z

TBH I think it's ready and the only reason I didn't merge it is because Ben explicitly requested changes and I didn't want to speak for whether he thought the updates were ok.

badboy

Code looks good. I'll check with Ben.

@heinrich5991 In the meantime can you add the changelog entry?

heinrich5991 · 2023-06-01T09:46:25Z

@heinrich5991 In the meantime can you add the changelog entry?

Done. EDIT: Now also conflicts resolved. EDIT2: Now also rebase mistake fixed.

Kotlin and Swift don't need it either way because they're statically typed. By moving it directly to the lowering/writing, it avoids a bunch of no-ops when no coercing is actually needed, e.g. when recursively coercing lists.

The error messages mirror the ones used in Python itself. E.g.: ```python >>> chr(1.0) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: 'float' object cannot be interpreted as an integer >>> class A: ... def __index__(self): ... return 1.0 ... >>> chr(A()) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: __index__ returned non-int (type float) ``` and ```python >>> math.sqrt("1") Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: must be real number, not str >>> class A: ... def __float__(self): ... return "1" ... >>> math.sqrt(A()) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: A.__float__ returned non-float (type str) ``` Fixes mozilla#1544. Also add some tests checking the stricter type checking plus a few floating point roundtrip tests.

bendk

Looks good to me. I agree it's better to match the current naming style. We can tackle the PEP-8 update in a different PR.

badboy · 2023-06-02T08:07:59Z

Thank you, @heinrich5991!

heinrich5991 requested a review from a team as a code owner May 23, 2023 21:37

heinrich5991 requested review from jhugman and removed request for a team May 23, 2023 21:37

heinrich5991 force-pushed the pr_less_coercing branch from d8fe29f to 61634a4 Compare May 23, 2023 21:53

jhugman requested a review from a team May 23, 2023 22:02

bendk requested changes May 24, 2023

View reviewed changes

heinrich5991 force-pushed the pr_less_coercing branch 2 times, most recently from 6fbd1e9 to 39ee669 Compare May 24, 2023 16:16

mhammond reviewed May 25, 2023

View reviewed changes

badboy approved these changes Jun 1, 2023

View reviewed changes

heinrich5991 force-pushed the pr_less_coercing branch from 39ee669 to 2d4e890 Compare June 1, 2023 09:46

heinrich5991 force-pushed the pr_less_coercing branch from 2d4e890 to b0a8cf2 Compare June 1, 2023 09:47

heinrich5991 added 3 commits June 1, 2023 11:48

Move type-limits Python test to unittest module

7616748

heinrich5991 force-pushed the pr_less_coercing branch from b0a8cf2 to 0f44aef Compare June 1, 2023 09:49

bendk approved these changes Jun 1, 2023

View reviewed changes

badboy merged commit 154739d into mozilla:main Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stricter type checking for Python integers and floats #1554

Stricter type checking for Python integers and floats #1554

heinrich5991 commented May 23, 2023

heinrich5991 commented May 23, 2023

bendk left a comment

bendk May 24, 2023

heinrich5991 May 24, 2023

heinrich5991 May 24, 2023

bendk May 24, 2023

heinrich5991 May 24, 2023

heinrich5991 May 24, 2023

mhammond May 25, 2023

mhammond left a comment

mhammond May 25, 2023

badboy May 26, 2023

mhammond May 25, 2023

heinrich5991 commented May 31, 2023

badboy commented May 31, 2023

mhammond commented May 31, 2023

badboy left a comment

heinrich5991 commented Jun 1, 2023 •

edited

Loading

bendk left a comment

badboy commented Jun 2, 2023

	cls.writeChecked(value, buf)
	cls.writeChecked(cls.check(value), buf)

Stricter type checking for Python integers and floats #1554

Stricter type checking for Python integers and floats #1554

Conversation

heinrich5991 commented May 23, 2023

heinrich5991 commented May 23, 2023

bendk 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

mhammond 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

heinrich5991 commented May 31, 2023

badboy commented May 31, 2023

mhammond commented May 31, 2023

badboy left a comment

Choose a reason for hiding this comment

heinrich5991 commented Jun 1, 2023 • edited Loading

bendk left a comment

Choose a reason for hiding this comment

badboy commented Jun 2, 2023

heinrich5991 commented Jun 1, 2023 •

edited

Loading