Skip to content

None.__ne__(None) behavior is changed in 3.12 #5103

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
Closed
youknowone opened this issue Oct 23, 2023 · 7 comments · Fixed by #5124
Closed

None.__ne__(None) behavior is changed in 3.12 #5103

youknowone opened this issue Oct 23, 2023 · 7 comments · Fixed by #5124
Assignees
@evanrittenhouse
Labels
C-compat A discrepancy between RustPython and CPython good first issue Good for newcomers

Comments

@youknowone
Copy link
Member

Related test: https://github.com/youknowone/RustPython/blob/6e699d93fe9ed1f16921ef3ab4a83fc0a72c5473/extra_tests/snippets/builtin_none.py#L25
@youknowone youknowone added good first issue Good for newcomers C-compat A discrepancy between RustPython and CPython labels Oct 23, 2023
@evanrittenhouse
Copy link
Contributor

Feel free to assign this one to me

@evanrittenhouse
Copy link
Contributor

Just want to check - is there a way to run this test outside of installing Python 3.12 on my system and running python3 builtin_none.py? Doesn't look like it's in a test class, so PyTest can't pick it up.

@fanninpm
Copy link
Contributor

fanninpm commented Nov 2, 2023

installing Python 3.12 on my system

You can use pyenv or conda to install a newer version of Python than the one that's on your system.

That builtin_none.py should be fine as one of the extra_tests.

@youknowone
Copy link
Member Author

Even though the test is working under pytest, you sill need to install Python 3.12 to test with it.

Steps:

  1. Change test to the new behavior (you need Python 3.12 here)
  2. Fix RustPython to work like the new one. (See vm/src/builtins/singleton.rs)

@evanrittenhouse
Copy link
Contributor

evanrittenhouse commented Nov 24, 2023 •
edited
Loading

So I've been looking at this for a while now - I think that it ends up requiring a change here, but I'm not sure how to get the richcompare slot to resolve to PyNotImplemented for None. Any tips?

cc @youknowone

@youknowone
Copy link
Member Author

PyNone is here:

RustPython/vm/src/builtins/singletons.rs

Line 12 in 8fc2c39

pub struct PyNone;

You can implement Comparable for PyNone like other types. e.g.

RustPython/vm/src/builtins/range.rs

Lines 441 to 468 in 8fc2c39

impl Comparable for PyRange {
fn cmp(
zelf: &Py<Self>,
other: &PyObject,
op: PyComparisonOp,
_vm: &VirtualMachine,
) -> PyResult<PyComparisonValue> {
op.eq_only(|| {
if zelf.is(other) {
return Ok(true.into());
}
let rhs = class_or_notimplemented!(Self, other);
let lhs_len = zelf.compute_length();
let eq = if lhs_len != rhs.compute_length() {
false
} else if lhs_len.is_zero() {
true
} else if zelf.start.as_bigint() != rhs.start.as_bigint() {
false
} else if lhs_len.is_one() {
true
} else {
zelf.step.as_bigint() == rhs.step.as_bigint()
};
Ok(eq.into())
})
}
}

@evanrittenhouse
Copy link
Contributor

Ah got it, thanks for the pointer. Hadn't seen the Comparable trait, I'll keep that in mind for future issues.

@evanrittenhouse evanrittenhouse mentioned this issue Nov 25, 2023
Merged
@moreal moreal mentioned this issue Apr 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@evanrittenhouse evanrittenhouse

Labels
C-compat A discrepancy between RustPython and CPython good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

None.__ne__(None) should be NotImplemented evanrittenhouse/RustPython
3 participants
@youknowone @fanninpm @evanrittenhouse