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

[ENH] Restrict instance methods on extension types to the actual type #6127

Open
pitrou opened this issue Apr 4, 2024 · 2 comments
Open

Comments

@pitrou
Copy link
Contributor

pitrou commented Apr 4, 2024

Is your feature request related to a problem? Please describe.

Currently, it's possible to call instance methods of extension types with any self argument that's not of the desired type. For many extension types, this will result in a crash when accessing the C-level internals.

Example in PyArrow:

>>> import pyarrow as pa
>>> pa.Array.get_total_buffer_size(0)
Erreur de segmentation

Describe the solution you'd like.

Cython should, by default or when enabled through an option, guard against any call of extension type instance methods with a self that's not an instance of the type.

Describe alternatives you've considered.

  1. Manual type check in each instance method: works but extremely tedious.
  2. Cython type annotation in the method signature: this doesn't actually seem result in a runtime type check?
diff --git a/python/pyarrow/array.pxi b/python/pyarrow/array.pxi
index 59d2e91ef6..a2aa3288d4 100644
--- a/python/pyarrow/array.pxi
+++ b/python/pyarrow/array.pxi
@@ -1226,7 +1226,7 @@ cdef class Array(_PandasConvertible):
             size = GetResultValue(c_size_res)
         return size
 
-    def get_total_buffer_size(self):
+    def get_total_buffer_size(Array self):
         """
         The sum of bytes in each buffer referenced by the array.
 

Additional context

No response

@pitrou
Copy link
Contributor Author

pitrou commented Apr 4, 2024

Note that "regular" C extension classes do not exhibit this problem as their method implementations are automatically guarded by a type check:

>>> import numpy as np
>>> np.ndarray.byteswap(0)
Traceback (most recent call last):
  Cell In[5], line 1
    np.ndarray.byteswap(0)
TypeError: descriptor 'byteswap' for 'numpy.ndarray' objects doesn't apply to a 'int' object

@da-woods
Copy link
Contributor

da-woods commented Apr 4, 2024

Note that "regular" C extension classes do not exhibit this problem as their method implementations are automatically guarded by a type check [...]

This is probably why we don't check.

This issue only applies to cyfunctions (i.e. with the cython.binding directive set to True, which is now the default). Before that existing all Cython methods were "regular" C extension methods so had the check automatically built in. Which is presumably why we never added it in Cython.

I suspect it would be worth adding the check.

raulcd pushed a commit to apache/arrow that referenced this issue Apr 10, 2024
… for Cython 2 (#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in #40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in #40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: #41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
raulcd pushed a commit to apache/arrow that referenced this issue Apr 10, 2024
… for Cython 2 (#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in #40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in #40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: #41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
vibhatha pushed a commit to vibhatha/arrow that referenced this issue Apr 15, 2024
…_error for Cython 2 (apache#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in apache#40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in apache#40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
tolleybot pushed a commit to tmct/arrow that referenced this issue May 2, 2024
…_error for Cython 2 (apache#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in apache#40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in apache#40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
tolleybot pushed a commit to tmct/arrow that referenced this issue May 4, 2024
…_error for Cython 2 (apache#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in apache#40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in apache#40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
rok pushed a commit to tmct/arrow that referenced this issue May 8, 2024
…_error for Cython 2 (apache#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in apache#40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in apache#40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
rok pushed a commit to tmct/arrow that referenced this issue May 8, 2024
…_error for Cython 2 (apache#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in apache#40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in apache#40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
vibhatha pushed a commit to vibhatha/arrow that referenced this issue May 25, 2024
…_error for Cython 2 (apache#41059)

### Rationale for this change

`test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in apache#40976.

### What changes are included in this PR?

Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in apache#40976.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41043

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants