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

Infer float in "safe" mode #5234

Merged
merged 3 commits into from Feb 24, 2023
Merged

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Jan 28, 2023

It currently infers to object, and double seems better than object.

I've picked "double" on the basis that this will always give the same precision answer as Python so it seems to match the semantics of "safe" mode better.

Now does the simpler thing and just keeps it as a C float

Closes #5202

It currently infers to object, and double seems better than
object.

I've picked "double" on the basis that this will always give the
same precision answer as Python so it seems to match the
semantics of "safe" mode better.
Comment on lines 535 to 537
cdef float fl = 5.0
from_fl = fl
assert typeof(from_fl) == "double", typeof(from_fl)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems … surprising. Why should we infer double for a clear C float? It's one thing to reduce Python float to C double, but C float should not be affected by this.

@da-woods da-woods changed the title Infer float->double in "safe" mode Infer float in "safe" mode Jan 30, 2023
@scoder scoder merged commit e4857a2 into cython:master Feb 24, 2023
@scoder scoder added this to the 3.0 milestone Feb 24, 2023
@da-woods da-woods deleted the float_safe_type_inference branch February 24, 2023 10:34
@scoder
Copy link
Contributor

scoder commented Feb 24, 2023

There seems to be a conflict of this PR with #5058. CI fails with

======================================================================
FAIL: test_tuple_without_typing (pep526_variable_annotations)
Doctest: pep526_variable_annotations.test_tuple_without_typing
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/doctest.py", line 2205, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for pep526_variable_annotations.test_tuple_without_typing
  File "/home/runner/work/cython/cython/TEST_TMP/2/run/c/pep526_variable_annotations/pep526_variable_annotations.cpython-39-x86_64-linux-gnu.so", line unknown line number, in test_tuple_without_typing

----------------------------------------------------------------------
File "/home/runner/work/cython/cython/TEST_TMP/2/run/c/pep526_variable_annotations/pep526_variable_annotations.cpython-39-x86_64-linux-gnu.so", line ?, in pep526_variable_annotations.test_tuple_without_typing
Failed example:
    test_tuple_without_typing((1, 1.0), (1, 1.0), (1, 1.0))
Expected:
    int
    int
    Python object
    Python object
    (int, float)
    tuple object
    tuple object
    tuple object
    tuple object
Got:
    int
    int
    float
    Python object
    (int, float)
    tuple object
    tuple object
    tuple object
    tuple object

The failing test code is:

# because tuple is specifically special cased to go to ctuple where possible
def test_tuple_without_typing(a: tuple[cython.int, cython.float], b: tuple[cython.int, ...],
               c: tuple[cython.int, object]  # cannot be a ctuple
               ):
    """
    >>> test_tuple_without_typing((1, 1.0), (1, 1.0), (1, 1.0))
    int
    int
    Python object
    Python object
    (int, float)
    tuple object
    tuple object
    tuple object
    tuple object
    """
    x: tuple[int, float] = (a[0], a[1])  # note: Python int/float, not cython.int/float
    y: tuple[cython.int, ...] = (1,2.)
    plain_tuple: tuple = ()
    z = a[0]  # should infer to C int
    p = x[1]  # should infer to Python float -> C double

    print(cython.typeof(z))
    print("int" if cython.compiled and cython.typeof(x[0]) == "Python object" else cython.typeof(x[0]))  # FIXME: infer Python int
    print(cython.typeof(p) if cython.compiled or cython.typeof(p) != 'float' else "Python object")  # FIXME: infer C double
    ...

I think we should be inferring Python float or C double instead of C float here. Might be a mismatch in the "looking for C type or Python type here" context.

@da-woods
Copy link
Contributor Author

da-woods commented Feb 24, 2023

@scoder here's what I think is happening:

def test_tuple_without_typing(a: tuple[cython.int, cython.float], b: tuple[cython.int, ...],
               c: tuple[cython.int, object]  # cannot be a ctuple
               ):

a is a c-tuple here or (C int, C float)

    x: tuple[int, float] = (a[0], a[1])

x is just typed as a regular tuple from the annotation, since it can't understand the Py-type subscripts

    p = x[1]

There's a bit of typing logic that says "x was packed from a C int and C float so unpacking it gives the same thing".
It's infer_sequence_item_type in ExprNodes.py. I think it's right here but smarter than what we were expecting. And we should update the test.

(I actually made this change in test_tuple above because I decided it was inferring things correctly)

@0dminnimda
Copy link
Contributor

0dminnimda commented Feb 24, 2023

Shouldn't x: tuple[int, float] force a[1] to be converted to float (or object if tuple subscripts are not analyzed) and thus be accessible from x[1] as float (or object) and not cython.float?

@da-woods
Copy link
Contributor Author

So the current situation is:

x: tuple[int, float]

is just interpreted as tuple (the subscripts aren't analysed unless they're all C types).

However, it knows the a[1] when the tuple was created was cython.float so it infers that indexing a[1] also gives a Cython float.

I could see the argument for inferring a C double instead, but the current behaviour also seems pretty correct to me (and requires less awkward special-casing)

scoder added a commit that referenced this pull request Feb 25, 2023
…lly be inferring either Python float or C double from the Python type annotation of 'x' instead, but as it stands, we infer the types from the ctuple assignment.
@scoder
Copy link
Contributor

scoder commented Feb 25, 2023

The missing type inference for Python container types is really a missing feature, more than a bug. I've changed the test to match the current behaviour. The test is a bit contrived anyway. Mixing Python float declarations arbitrarily with C float/double declarations should best be avoided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Incorrect type inference for floats
3 participants