Skip to content

Fix PyFloat::py_new always returning new float object issue#3979

Merged
youknowone merged 6 commits intoRustPython:mainfrom
jopemachine:edit-float-new-behavior
Jul 31, 2022
Merged

Fix PyFloat::py_new always returning new float object issue#3979
youknowone merged 6 commits intoRustPython:mainfrom
jopemachine:edit-float-new-behavior

Conversation

@jopemachine
Copy link
Copy Markdown
Contributor

@jopemachine jopemachine commented Jul 28, 2022

Fixes #3978.

>>>>> import math
>>>>> a = float(math.nan)
>>>>> b = float(math.nan)
>>>>> a is b
>>>>> true

Reference

@jopemachine jopemachine marked this pull request as draft July 28, 2022 01:10
@jopemachine jopemachine marked this pull request as ready for review July 28, 2022 01:11
@Snowapril
Copy link
Copy Markdown
Contributor

It seems below incompatibility make extra_test failed.

class F(float):
    def __int__(self):
        return 3
# rustpython
>>>>> type(F(1.2))
<class 'float'>
# cpython
>>> type(F(1.2))
<class '__main__.F'>

@Snowapril
Copy link
Copy Markdown
Contributor

We need to move the exact float type check into PyNumber::float and return its payload with ref. Please refer to what payload_if_exact return and use into_ref 😊

@jopemachine
Copy link
Copy Markdown
Contributor Author

We need to move the exact float type check into PyNumber::float and return its payload with ref. Please refer to what payload_if_exact return and use into_ref 😊

number::float is not called when the argument is math.nan because try_float_opt early returned in this line.

So I tried to edit try_float_opt instead like below, but it seems it is not working as expected. (id(float(math.nan)) prints different oid each time)

pub fn try_float_opt(&self, vm: &VirtualMachine) -> PyResult<Option<PyRef<PyFloat>>> {
	let value = if let Some(float) = self.payload_if_exact::<PyFloat>(vm) {
			Some(float.into_ref(vm))
	} else {
			let number = self.to_number();
			#[allow(clippy::manual_map)]
			if let Some(f) = number.float(vm)? {
					Some(f)
			} else if let Some(i) = self.try_index_opt(vm) {
					let value = int::try_to_float(i?.as_bigint(), vm)?;
					Some(vm.ctx.new_float(value))
			} else if let Some(value) = self.downcast_ref::<PyFloat>() {
					Some(vm.ctx.new_float(value.to_f64()))
			} else {
					None
			}
	};
	Ok(value)
}

I guess this is because below line returned new float instance instead of existing one.

https://github.com/RustPython/RustPython/blob/main/vm/src/builtins/float.rs#L152-L154

So, I think maybe we need to update the py_new function somehow.

@youknowone youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 28, 2022
@youknowone
Copy link
Copy Markdown
Member

you are right. you will find PyInt have a similar logic you are looking for

@youknowone
Copy link
Copy Markdown
Member

looks good, would you add a related test to extra_tests?

@jopemachine jopemachine force-pushed the edit-float-new-behavior branch from 2eefd2c to f894029 Compare July 31, 2022 09:55
Copy link
Copy Markdown
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you!

@youknowone youknowone merged commit e8ed8aa into RustPython:main Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

z-ca-2022 Tag to track contrubution-academy 2022

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PyFloat::py_new always return new float object even if the argument's oid is same.

3 participants