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

[BUG] 'except?' started failing in 3.0.10 #6122

Closed
Baekalfen opened this issue Apr 1, 2024 · 16 comments · Fixed by #6124
Closed

[BUG] 'except?' started failing in 3.0.10 #6122

Baekalfen opened this issue Apr 1, 2024 · 16 comments · Fixed by #6124
Labels
Milestone

Comments

@Baekalfen
Copy link
Contributor

Baekalfen commented Apr 1, 2024

Describe the bug

Usage of except? -1 has seemingly started failing in 3.0.10. It works in all previous >3.0.0 versions.

Every place I use the "?"-version of except, it fails during cythonize with:

pyboy/utils.pxd:22:29: C method 'read_64bit' is declared but not defined

Note, it says "C method", but this is purely a Python/Cython function, it's not defining an external library.

Code to reproduce the behaviour:

I define a e.pxd file with:

from libc.stdint cimport uint64_t

cdef class IntIOInterface:
    cpdef uint64_t read_64bit(self) except? -1

And a e.py file with:

class IntIOInterface:

    def read_64bit(self):
        a = self.read()
        return a

Compile using following command:

cython --directive legacy_implicit_noexcept=True -3 e.py

Expected behaviour

Expected to compile cleanly, and use -1 to signal an exception like in previous versions.

OS

No response

Python version

No response

Cython version

3.0.10

Additional context

The code can be found here: https://github.com/Baekalfen/PyBoy/blob/ef25b6bb6445b6228069df57297620b0fe9487d8/pyboy/utils.pxd#L17

This bug report and fix is listed for Cython 3.0.10: #5709

@da-woods
Copy link
Contributor

da-woods commented Apr 1, 2024

I can't reproduce based on what you post here. I get

Error compiling Cython file:
------------------------------------------------------------
...
cpdef uint64_t read_64bit(self) except? -1
      ^
------------------------------------------------------------

e.pxd:1:6: 'uint64_t' is not a type identifier

Pretty irrespective of the Cython version.

If I add the line from libc.stdint cimport uint64_t then it compiles for me.

#5709 and #5710 shouldn't be relevant since they're only in the master branch and not a Cython release

@Baekalfen
Copy link
Contributor Author

Sorry for being too vague. You're right.

I narrowed it down, and it only coincides with legacy_implicit_noexcept:

$ cython e.py --directive legacy_implicit_noexcept=True
/opt/homebrew/lib/python3.11/site-packages/Cython/Compiler/Main.py:381: FutureWarning: Cython directive 'language_level' not set, using '3str' for now (Py3). This has changed from earlier releases! File: /Users/mads/Desktop/cytest/e.pxd
  tree = Parsing.p_module(s, pxd, full_module_name)
warning: e.py:5:4: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: e.py:5:4: Compatible but non-identical C method 'read_64bit' not redeclared in definition part of extension type 'IntIOInterface'.  This may cause incorrect vtables to be generated.
warning: e.pxd:6:29: Previous declaration is here

Error compiling Cython file:
------------------------------------------------------------
...
cimport cython
from libc.stdint cimport int64_t, uint8_t, uint16_t, uint32_t, uint64_t


cdef class IntIOInterface:
    cpdef uint64_t read_64bit(self) except? -1                             ^
------------------------------------------------------------

e.pxd:6:29: C method 'read_64bit' is declared but not defined

@Baekalfen
Copy link
Contributor Author

And you're right about #5709 and #5710, I mistook 3.1.0 for 3.0.10 in the changelog https://cython.readthedocs.io/en/latest/src/changes.html

@da-woods
Copy link
Contributor

da-woods commented Apr 1, 2024

Right - this looks to bisect to 4e9f730 (which is probably unsurprising)

@Baekalfen
Copy link
Contributor Author

Was it supposed to do that, or do you still think it's a bug?

@da-woods
Copy link
Contributor

da-woods commented Apr 1, 2024

It's a bug.

I think it moved a check earlier before it's applied the typing from the pxd file.

That probably suggests Cython doesn't have sufficient tests for legacy_implicit_noexcept and typing applied from pxd files.

@matusvalo
Copy link
Contributor

matusvalo commented Apr 1, 2024

I can't reproduce based on what you post here. I get

Error compiling Cython file:
------------------------------------------------------------
...
cpdef uint64_t read_64bit(self) except? -1
      ^
------------------------------------------------------------

e.pxd:1:6: 'uint64_t' is not a type identifier

Pretty irrespective of the Cython version.

If I add the line from libc.stdint cimport uint64_t then it compiles for me.

#5709 and #5710 shouldn't be relevant since they're only in the master branch and not a Cython release

I am not able to reproduce it too. I tried following:

  • e.pxd:
from libc.stdint cimport uint64_t

cpdef uint64_t read_64bit(self) except? -1
  • e.py:
def read_64bit(self):
        a = self.read()
        return a

I compiled it using:

cython --directive legacy_implicit_noexcept=True -3 e.py

but I did not get any error... (just warning about noexcept). Am I doing something wrong?

@Baekalfen
Copy link
Contributor Author

but I did not get any error... (just warning about noexcept). Am I doing something wrong?

Could you double check, that you're on Cython 3.0.10?

@matusvalo
Copy link
Contributor

matusvalo commented Apr 1, 2024

% cat e.py
def read_64bit(self):
        a = self.read()
        return a

% cat e.pxd
from libc.stdint cimport uint64_t

cpdef uint64_t read_64bit(self) except? -1

% git status
HEAD detached at 3.0.10
Untracked files:
  (use "git add <file>..." to include in what will be committed)
	./

nothing added to commit but untracked files present (use "git add" to track)

% cython -V
Cython version 3.0.10

% cython --directive legacy_implicit_noexcept=True -3 e.py
warning: e.py:1:0: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.

@da-woods
Copy link
Contributor

da-woods commented Apr 1, 2024

@matusvalo it may need to be in a cdef class. I modified it to be like that when I tested it but I'm not sure if it's a necessary part of the issue

@Baekalfen
Copy link
Contributor Author

Confirmed, it has to be in a class.

New pxd:

cimport cython
from libc.stdint cimport int64_t, uint8_t, uint16_t, uint32_t, uint64_t


cdef class IntIOInterface:
    cpdef uint64_t read_64bit(self) except? -1

New py:

class IntIOInterface:
    def __init__(self, buf):
        pass

    def read_64bit(self):
        a = self.read()
        b = self.read()
        c = self.read()
        d = self.read()
        e = self.read()
        f = self.read()
        g = self.read()
        h = self.read()
        return a | (b << 8) | (c << 16) | (d << 24) | (e << 32) | (f << 40) | (g << 48) | (h << 56)

@matusvalo
Copy link
Contributor

I confirm.

@matusvalo
Copy link
Contributor

Let me update the description to contain reproducible example.

@matusvalo
Copy link
Contributor

matusvalo commented Apr 2, 2024

I think possible solution is to replace line:

self.exception_check = False

with following:

            if not (self.declared_name() in env.entries and not in_pxd):
                self.exception_check = False

The reason is because when .pxd file is defined there are two runs CFuncDeclaratorNode.analyse:

  1. run is executed for .pxd file and is triggered by the parser
  2. run is executed for .py file and is triggered by ParseTreeTransform.AlignFunctionDefinitions.visit_DefNode

The problem is causing 2nd run because it does uses information from entry which misses correct value of has_explicit_exc_clause (default value False is used).

Possible solution (shown above) is to not override exception_check when CFuncDeclaratorNode is already declared by .pxd file since it already contains correct value of exception_check from 1st run. Other solution would be to add has_explicit_exc_clause information to CFuncType which I don't like.

Maybe there is some other solution but currently I don't see any.

@da-woods
Copy link
Contributor

da-woods commented Apr 2, 2024

I think possible solution is to replace line:

That looks reasons based on a quick look.

@Baekalfen
Copy link
Contributor Author

I think possible solution is to replace line:

self.exception_check = False

with following:

            if not (self.declared_name() in env.entries and not in_pxd):
                self.exception_check = False

This fixes the issue on my end. Can't comment on the soundness internally in Cython.

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

Successfully merging a pull request may close this issue.

3 participants