Properly handles `del val.__dict__`. by hbina · Pull Request #5576 · RustPython/RustPython

hbina · 2025-03-01T12:12:52Z

youknowone · 2025-03-02T04:00:31Z

see also #5509

hbina · 2025-03-02T14:51:37Z

Thanks for that PR, I have updated the PR based on that. If everything is good I will try to make my PR on top of the original one. Its much easier for me to work with squashed commits but I don't want to lose the work of the original PR.

youknowone · 2025-03-03T00:52:16Z

@hbina Sure! That will be a good way. Thank you!

youknowone · 2025-03-09T18:03:06Z

vm/src/object/core.rs

@@ -711,14 +712,37 @@ impl PyObject {

    /// Set the dict field. Returns `Err(dict)` if this object does not have a dict field


The doc-comment doesn't fit in actual implementation anymore

vm/src/object/core.rs

youknowone · 2025-03-09T18:14:41Z

vm/src/object/core.rs

+    pub fn set_dict(&self, dict: PySetterValue<PyDictRef>) -> Option<()> {
+        // NOTE(hanif) - So far, this is the only error condition that I know of so we can use Option
+        // for now.
+        if self.payload_is::<crate::builtins::function::PyFunction>() {


I am sorry. I don't get how PyFunction specifically requires to be checked yet. Is this same in CPython?

Because deleting dict of a function is not allowed.
I can't find the equivalent check inside of cpython...

class A: pass def ff(): return 0 # Deleting __dict__ of class is fine a = A() print(a.__dict__) del a.__dict__ # Deleting __dict__ of function is not allowed print(ff.__dict__) del ff.__dict__

Yields

hbina085@akarin-thinkpad ~/g/RustPython (hbina-fix-issue-5355) [1]> python ggg_issue_5355.py (numbadev) {} {} Traceback (most recent call last): File "/home/hbina085/git/RustPython/ggg_issue_5355.py", line 16, in <module> del ff.__dict__ ^^^^^^^^^^^ TypeError: cannot delete __dict__

Perhaps I should have added comments.

Instead of PyFunction, isn't it a similar, but different rule? e.g. static types, immutable types etc

Maybe instant_dict() can be simple None for those types.

youknowone · 2025-03-09T18:17:42Z

vm/src/object/core.rs

+                // self.0.dict = Some(InstanceDict::new(dict));
+                unsafe {
+                    let ptr = self as *const _ as *mut PyObject;
+                    (*ptr).0.dict = Some(InstanceDict::new(dict));
+                }


Is this necessarily to be unsafe? Is this guaranteed to be actually safe?

I have no idea how to implement this without unsafe...All the methods only have immutable access to self. Can't figure out the head or tails of PyObjectRef either.

When it matched to None, I guess this object never can have __dict__. Then returning None will fit to it.

This PR is based on the work by key262yek. The original PR is here: RustPython#5509

- Add support for deleting object dictionaries - Modify object_set_dict to handle dictionary deletion - Update setter value handling to support delete operations

- Simplify object_set_dict method in object.rs - Update set_dict method in core.rs to handle both assignment and deletion - Consolidate dictionary modification logic

- Modify set_dict method to allow assigning dict to objects without existing dict - Update error handling in object_set_dict to provide more precise error message - Refactor type module to correctly handle dictionary assignment

This PR is based on the work by key262yek. Signed-off-by: Hanif Ariffin <hanif.ariffin.4326@gmail.com>

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>

youknowone · 2025-03-24T04:57:40Z

vm/src/object/core.rs

+                // self.0.dict = Some(InstanceDict::new(dict));
+                unsafe {
+                    let ptr = self as *const _ as *mut PyObject;
+                    (*ptr).0.dict = Some(InstanceDict::new(dict));
+                }


When it matched to None, I guess this object never can have __dict__. Then returning None will fit to it.

youknowone · 2025-03-24T04:58:33Z

vm/src/object/core.rs

+            (Some(_), PySetterValue::Delete) => {
+                // self.0.dict = None;
+                unsafe {
+                    let ptr = self as *const _ as *mut PyObject;
+                    (*ptr).0.dict = None;
+                }
+            }


Suggested change

(Some(_), PySetterValue::Delete) => {

// self.0.dict = None;

unsafe {

let ptr = self as *const _ as *mut PyObject;

(*ptr).0.dict = None;

}

}

(Some(dict), PySetterValue::Delete) => {

dict.write().clear();

}

Can't do this because it only have &self.

youknowone · 2025-03-24T04:59:05Z

vm/src/object/core.rs

+    pub fn set_dict(&self, dict: PySetterValue<PyDictRef>) -> Option<()> {
+        // NOTE(hanif) - So far, this is the only error condition that I know of so we can use Option
+        // for now.
+        if self.payload_is::<crate::builtins::function::PyFunction>() {


Maybe instant_dict() can be simple None for those types.

hbina force-pushed the hbina-fix-issue-5355 branch 2 times, most recently from dbe0d21 to 16113a4 Compare March 2, 2025 14:24

hbina force-pushed the hbina-fix-issue-5355 branch from 16113a4 to d964098 Compare March 8, 2025 11:19

youknowone reviewed Mar 9, 2025

View reviewed changes

hbina and others added 6 commits March 23, 2025 11:18

Implemented the ability to del __dict__

2010890

This PR is based on the work by key262yek. The original PR is here: RustPython#5509

Implement delete object dictionary

1abd204

- Add support for deleting object dictionaries - Modify object_set_dict to handle dictionary deletion - Update setter value handling to support delete operations

Refactor object dictionary setter to handle delete and assign operations

c43327e

- Simplify object_set_dict method in object.rs - Update set_dict method in core.rs to handle both assignment and deletion - Consolidate dictionary modification logic

Implemented the ability to del __dict__

02ce85a

This PR is based on the work by key262yek. Signed-off-by: Hanif Ariffin <hanif.ariffin.4326@gmail.com>

Update vm/src/object/core.rs

425fd4f

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>

hbina force-pushed the hbina-fix-issue-5355 branch from a693b3b to 425fd4f Compare March 23, 2025 03:18

youknowone reviewed Mar 24, 2025

View reviewed changes

		@@ -711,14 +712,37 @@ impl PyObject {

		/// Set the dict field. Returns `Err(dict)` if this object does not have a dict field

Conversation

hbina commented Mar 1, 2025

Uh oh!

youknowone commented Mar 2, 2025

Uh oh!

hbina commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowone commented Mar 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hbina commented Mar 2, 2025 •

edited

Loading