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

DEP: Remove deprecated numeric style dtype strings #19539

Merged
merged 11 commits into from Aug 10, 2021
Merged

Conversation

NectDz
Copy link
Contributor

@NectDz NectDz commented Jul 21, 2021

See #18993
This issue was presented by @Daybreak2019

When the function strncmp was used it did not account for a null byte character meaning it is an incomplete comparison.

The elements in the array dep_tps on line 1727 in the descriptor.c file did not match the elements in the deprecated_types array in test_deprecations.py line 324 therefore a change in the dep_tps array had to be made so the null byte character could be compared correctly.

The null byte character is compared on line 1732 in the descriptor.c file by the change strlen(dep_tp)+1 which includes the strings length but adds +1 so the null byte character can also be compared making the strncmp function compare both strings completely.

@NectDz NectDz changed the title Insecure String Comparison Fix BUG: Insecure String Comparison Fix Jul 21, 2021
@seberg
Copy link
Member

@seberg seberg commented Jul 22, 2021

Thanks, would be fine to just put this in. But... if you are to it, now that I see the deprecation comment: it would be much nicer to finish the deprecation and delete the whole branch as was started in gh-16554.

The code effectively always did the right thing, the warnings were given. The typical timelin for deprecation is one year/two releases, which the next release will be. And there is no reason to delay in this specific case.

@eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 22, 2021

Is this code even reachable? I can't work out how to trigger the warning.

@seberg
Copy link
Member

@seberg seberg commented Jul 22, 2021

Use the exact string as dtype: np.dtype("Uint64"), etc. it is a bit confusing, because pre 1.20 deprecated a whole bunch of other ones, and 1.20+ has only a few of these remaining. EDIT: And they are missing on <1.20, so you can't reproduce it there.

@eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 22, 2021

I'm trying this out on 1.19.4 and getting no warning - is the deprecation more recent than that? edit: yes.

@NectDz
Copy link
Contributor Author

@NectDz NectDz commented Jul 27, 2021

Thanks, would be fine to just put this in. But... if you are to it, now that I see the deprecation comment: it would be much nicer to finish the deprecation and delete the whole branch as was started in gh-16554.

The code effectively always did the right thing, the warnings were given. The typical timelin for deprecation is one year/two releases, which the next release will be. And there is no reason to delay in this specific case.

Thanks for the feedback! Based on your linked PR, here are the things I think I have to do. Does this make sense / is anything missing from my list?

  1. Remove 32 and 64 from the arrays in the _type_aliases.py file on lines 212 and 213
  2. Remove uint32 and uint64 in the numerictypes.py file on line 127
  3. Remove uint32 and uint64 in the test_deprecations.py file on line 627

@seberg
Copy link
Member

@seberg seberg commented Jul 27, 2021

Remove 32 and 64 from the arrays in the _type_aliases.py file on lines 212 and 213

No, I think those are fine, note that the main problem here is the capital letter, uint32 is fine, but Uint32 is not.

Remove uint32 and uint64 in the numerictypes.py file on line 127

Yeah, that looks right.

Remove uint32 and uint64 in the test_deprecations.py file on line 627

You should delete the whole TestNumericStyleTypecodes testcase/class. Then add all of them to the test_numeric_style_types_are_invalid test in numpy/core/tests/test_dtype.py

Copy link
Member

@seberg seberg left a comment

Thanks for doing the more complete fix! Just two small things also to make the linting CI happy. It would be nice to have a very brief (single bullet point) release note for this. doc/release/upcoming_changes/README.rst explains how to do this. Just to note that the deprecation is finalized.

But basically, this is good to go :).

numpy/core/tests/test_dtype.py Outdated Show resolved Hide resolved
numpy/core/src/multiarray/descriptor.c Outdated Show resolved Hide resolved
@NectDz
Copy link
Contributor Author

@NectDz NectDz commented Aug 2, 2021

Thanks for doing the more complete fix! Just two small things also to make the linting CI happy. It would be nice to have a very brief (single bullet point) release note for this. doc/release/upcoming_changes/README.rst explains how to do this. Just to note that the deprecation is finalized.

But basically, this is good to go :).

I am currently doing the release note and I am wondering on what PR type I should list this as?

@seberg
Copy link
Member

@seberg seberg commented Aug 2, 2021

It would be expired.

@NectDz
Copy link
Contributor Author

@NectDz NectDz commented Aug 4, 2021

hmm, do you have any idea on why the test is failing @seberg ? I don't know what is going wrong

@seberg
Copy link
Member

@seberg seberg commented Aug 4, 2021

Those unfortunately are flaky, don't worry about them. The changes are all good I think. I would modify the release note to something like:

* Using the strings ``"Bytes0"``, <???>, and <???> as a dtype will now raise a ``TypeError``.

np.Bytes0, etc. never actually existed. (I had intended to just do that and merge, but forgot if you want to update the release note please do. Mostly to use the strings, maybe shorten it a bit, and use a bullet point. If it spans multiple lines, you have to indent the second line, I think.)

@NectDz
Copy link
Contributor Author

@NectDz NectDz commented Aug 4, 2021

Those unfortunately are flaky, don't worry about them. The changes are all good I think. I would modify the release note to something like:

* Using the strings ``"Bytes0"``, <???>, and <???> as a dtype will now raise a ``TypeError``.

np.Bytes0, etc. never actually existed. (I had intended to just do that and merge, but forgot if you want to update the release note please do. Mostly to use the strings, maybe shorten it a bit, and use a bullet point. If it spans multiple lines, you have to indent the second line, I think.)

ok sounds good but just to clear things up, would I also have to include Uint32 and Uint64 as strings in the release note?

@seberg
Copy link
Member

@seberg seberg commented Aug 4, 2021

Yeah, I think listing them all makes sense, it is not that long of a list.

@seberg seberg changed the title BUG: Insecure String Comparison Fix DEP: Remove deprecated numeric style dtype strings Aug 5, 2021
numpy/core/numerictypes.py Outdated Show resolved Hide resolved
@seberg
Copy link
Member

@seberg seberg commented Aug 10, 2021

Cancelled the last job, I suspect things were just slow due to the github issues today. In effect, all jobs have run with only style change differences anyway.

Thanks @NectDz, especially for taking the time to finish the deprecation when the issue was just about the smaller fix!

@seberg seberg merged commit eeef9d4 into numpy:main Aug 10, 2021
18 of 19 checks passed
scratchmex pushed a commit to scratchmex/numpy that referenced this issue Aug 13, 2021
Finishes the deprecation, and effectively closes numpygh-18993

* Insecure String Comparison

* Finished Deprecations

* Breaks numpy types

* Removed elements in dep_tps

* Delete Typecode Comment

* Deleted for loop

* Fixed 80 characters or more issue

* Expired Release Note

* Updated Release Note

* Update numpy/core/numerictypes.py

* Update numpy/core/tests/test_deprecations.py

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@rgommers rgommers added this to the 1.22.0 release milestone Dec 22, 2021
dbalchev pushed a commit to dbalchev/numpy that referenced this issue Apr 19, 2022
Finishes the deprecation, and effectively closes numpygh-18993

* Insecure String Comparison

* Finished Deprecations

* Breaks numpy types

* Removed elements in dep_tps

* Delete Typecode Comment

* Deleted for loop

* Fixed 80 characters or more issue

* Expired Release Note

* Updated Release Note

* Update numpy/core/numerictypes.py

* Update numpy/core/tests/test_deprecations.py

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
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.

None yet

4 participants