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

Fix metaclass check classcell #3986

Merged
merged 5 commits into from
Aug 13, 2022
Merged

Fix metaclass check classcell #3986

merged 5 commits into from
Aug 13, 2022

Conversation

yonmilk
Copy link
Contributor

@yonmilk yonmilk commented Jul 30, 2022

  • Fix __build_class__ check __class__ cell
  • Fix type.__new__ set classcell

When a class object is created with metaclass, the following operation is performed.
https://docs.python.org/3/reference/datamodel.html?highlight=classcell#:~:text=CPython implementation detail%3A In,a RuntimeError in Python 3.8.

CPython implementation detail: In CPython 3.6 and later, the __class__ cell is passed to the metaclass as a __classcell__ entry in the class namespace. If present, this must be propagated up to the type.__new__ call in order for the class to be initialised correctly. Failing to do so will result in a [RuntimeError] in Python 3.8.

  1. Error handling when __class__ (the class it belongs to) and metaclass are different (bltinmodule.c)
  2. If the result in step 1 is the same, set class in type.__new__ (typeobject.c)

But in Rust Python it sets classcell without comparing __class__.

if let Some(ref classcell) = classcell {
	  classcell.set(Some(class.clone()));
}

@yonmilk yonmilk force-pushed the metaclass branch 3 times, most recently from 0e7bc4d to cd0c9d0 Compare July 30, 2022 08:27
@youknowone youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 31, 2022
@Snowapril
Copy link
Contributor

Unless we locate the code for setting type to classcell in type.__new__, build failure will not be disappeared 😂

@yonmilk yonmilk force-pushed the metaclass branch 9 times, most recently from 79c9756 to e4c55a5 Compare August 7, 2022 08:41
@yonmilk yonmilk force-pushed the metaclass branch 2 times, most recently from 7f0301e to 6c85150 Compare August 11, 2022 04:38
@yonmilk yonmilk marked this pull request as ready for review August 11, 2022 05:13
Comment on lines 599 to 636
let __classcell__ = identifier!(vm, __classcell__);

// It is correct to get it from tp_dict, not attributes.
let cell = typ
.attributes
.write()
.get(__classcell__)
.map(|cell| PyCellRef::try_from_object(vm, cell.clone()));

if let Some(Ok(cell)) = cell {
cell.set(Some(typ.clone().to_pyobject(vm)));
}

Copy link
Contributor

@Snowapril Snowapril Aug 11, 2022

Choose a reason for hiding this comment

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

Suggested change
let __classcell__ = identifier!(vm, __classcell__);
// It is correct to get it from tp_dict, not attributes.
let cell = typ
.attributes
.write()
.get(__classcell__)
.map(|cell| PyCellRef::try_from_object(vm, cell.clone()));
if let Some(Ok(cell)) = cell {
cell.set(Some(typ.clone().to_pyobject(vm)));
}
typ.attributes
.write()
.get(identifier!(vm, __classcell__))
.map(|cell| {
if let Some(cell) = PyCellRef::try_from_object(vm, cell.clone()).ok() {
cell.set(Some(typ.clone().to_pyobject(vm)));
Ok(())
} else {
Err(vm.new_type_error(format!(
"__classcell__ must be a nonlocal cell, not {}",
cell.class()
)))
}
})
.transpose()?;

It seems that we miss to raise type error when the given __classcell__ is actually not a cell type

Copy link
Member

Choose a reason for hiding this comment

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

I think previous version is better. cell.set has side effect and the result of map is not used. This is more like control flow than data stream. Using if-let will show the flow better and will not hide side-effect of cell.set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I agree. Then we only need to add type error handling

Copy link
Contributor

@Snowapril Snowapril left a comment

Choose a reason for hiding this comment

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

BTW, without that change request, looks great !! 😊

cell.class().name()
))
})?;
cell.set(Some(typ.clone().to_pyobject(vm)));
Copy link
Member

Choose a reason for hiding this comment

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

This line has side-effect and the closure doesn't return anything. Using if-let statement would be better than map

Copy link
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.

The logic looks good, but I changed error-handling style in edb4a96

PyObject is not expected to be used as Display object because it generates not very helpful error messages.
This was on TODO list for long time but forgotten. Once #4045 is merged, please rebase this PR and fix error formatting arguments.

Thank you for fixing this hair-pulling problem! Now I am try to getting how it is working.

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.

None yet

3 participants