Skip to content

DEP: FutureWarning for void.item(): Will return bytes #10044

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 26, 2017
Merged

DEP: FutureWarning for void.item(): Will return bytes #10044

merged 1 commit into from
Nov 26, 2017

Conversation

ahaldane
Copy link
Member

This PR holds off on the changes of #8157 (to fix #8129), and instead issues a FutureWarning if anyone calls void.item(). We would push off #8157 to 1.15.

@ahaldane ahaldane force-pushed the futurewarn_voiditem branch from 9451ff9 to 4821ab8 Compare November 17, 2017 17:48
@ahaldane ahaldane added this to the 1.14.0 release milestone Nov 17, 2017
@ahaldane ahaldane force-pushed the futurewarn_voiditem branch from 4821ab8 to 40e599a Compare November 17, 2017 17:51
@ahaldane ahaldane changed the title MAINT: FutureWarning for void.item(): Will return bytes DEP: FutureWarning for void.item(): Will return bytes Nov 17, 2017
@eric-wieser
Copy link
Member

I don't like the idea of an unconditional warning, especially when numpy has a habit of calling .item internally.

@ahaldane
Copy link
Member Author

ahaldane commented Nov 17, 2017 •
edited by eric-wieser
Loading

While it would be ideal to make it configurable, it also seems tricky, and the cost/benefit ratio of coding it up seems high.

Some points in favor of an unconditional warning:

  1. Even internal uses of void.item() are probably bugs which we would want to hear about.
  2. None of the unit test code ever calls void.item(), even internally.
  3. Any casting or other code that tried to do arr.setitem(void.getitem()) already fails since buffers can't be converted to voids.

@eric-wieser
Copy link
Member

Running int(np.void(b'10')) results in void.item being called, but maybe that's a bug too.

@ahaldane
Copy link
Member Author

ahaldane commented Nov 17, 2017 •
edited
Loading

I'm not sure it's a bug, but I'd also add that it raises an error like in my case 3. (In fact, it gives different errors in Python3 and Python2).

It might not be a bug because once we switch to returning bytes it will "work" (eg, int(np.void(b'10').tobytes() gives 10).

eric-wieser
eric-wieser reviewed Nov 19, 2017
View reviewed changes
numpy/core/src/multiarray/arraytypes.c.src Outdated
if (DEPRECATE_FUTUREWARNING(
"the `.item()` method of unstructured void types will return "
"an immutable `bytes` object in the near future instead of the "
"mutable memoryview or integer array returned in numpy 1.13.")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth noting that this is already possible with bytes(void_obj) and/or bytearray(void_obj)

@ahaldane ahaldane force-pushed the futurewarn_voiditem branch from 40e599a to c4c3e79 Compare November 19, 2017 18:24
@charris
Copy link
Member

charris commented Nov 22, 2017

Let's go with the FutureWarning, that will at least get us some feedback.

eric-wieser
eric-wieser reviewed Nov 22, 2017
View reviewed changes
numpy/core/tests/test_regression.py Outdated
@@ -2233,7 +2233,15 @@ def test_scalar_copy(self):
item = sctype(values.get(sctype, 1))
item2 = copy.copy(item)
assert_equal(item, item2)

def test_void_item_memview(self):
Copy link
Member

Choose a reason for hiding this comment

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

Missing newline

charris
charris reviewed Nov 25, 2017
View reviewed changes
numpy/core/src/multiarray/arraytypes.c.src Outdated
}
/*
* In the future all the code below will be replaced by
* // For unstructured void types like V4, return a bytes object (copy).
Copy link
Member

Choose a reason for hiding this comment

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

No C++ comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically this is not a C++ comment :) I'll fix it though.

charris
charris reviewed Nov 26, 2017
View reviewed changes
numpy/core/src/multiarray/arraytypes.c.src Outdated
@@ -730,6 +732,24 @@ VOID_getitem(void *input, void *vap)
return (PyObject *)ret;
}

if (!future_warning_emitted){
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, Is this restriction needed? For that matter, is it wanted? What if someone wants to find all the places that need fixing?

@ahaldane
Copy link
Member Author

Yeah I think I should remove the filter_waring_emitted as you said.

As to the question of how to turn off the warning if it is not needed, I think the user should just use warnings.filterwarnings - the warnings module already provides a way of silencing it.

@ahaldane ahaldane force-pushed the futurewarn_voiditem branch from c4c3e79 to 1f78eab Compare November 26, 2017 16:12
@charris
Copy link
Member

charris commented Nov 26, 2017

Merged, thanks Allan. We'll see how it goes ...

@charris charris merged commit b13c705 into numpy:master Nov 26, 2017
@ahaldane ahaldane mentioned this pull request Nov 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charris charris charris left review comments

@eric-wieser eric-wieser eric-wieser left review comments

Assignees
No one assigned
Labels
07 - Deprecation component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Constructing object array from void array leads to use-after-free
3 participants
@ahaldane @eric-wieser @charris