improve Interface::extends and ClassEntry::extends #190
Conversation
now accepts an Interface, like ClassEntry.implements() does
instead of accepting a fn to delay fetching ClassEntry, accept a String and look up during init()
phper/src/classes.rs
Outdated
| /// ``` | ||
| pub fn extends(&mut self, parent: impl Fn() -> &'static ClassEntry + 'static) { | ||
| self.parent = Some(Box::new(parent)); | ||
| pub fn extends(&mut self, parent_name: impl Into<String>) { |
There was a problem hiding this comment.
I think this would be better if it accepted a ClassEntry (which could come from a just-created class or defer looking up from globals, similar to Interface::from_name), but I couldn't make it work. I think this is at least more ergonomic than the previous way?
There was a problem hiding this comment.
How about StateClass<()>? Add method from_name just for StateClass<()>.
There was a problem hiding this comment.
The parameter can accept StateClass<T>, which can be transmute to StateClass<()> internally. Anyway, T is just phantom data in StateClass.
There was a problem hiding this comment.
Thanks for the pointer. Some progress, now we can either extends(StateClass<T>) or extends_name(String), the second one being useful for built-ins that we don't have a StateClass for. I could allow lazily creating a StateClass for built-ins, to unify those two methods - WDYT?
There was a problem hiding this comment.
In fact, I prefer to change it this way to be consistent with the implements method. What do you think?
There was a problem hiding this comment.
I like this, it's much nicer. I will try to cherry-pick this into my PR.
import ClassEntity::extends(StateClass) code from jmjoy:phper-fork:extends
these no longer have a use, since interfaces no longer implement based on a delaying function
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how class and interface extension are handled by removing deferred lookup functions and switching to a StateClass-based approach for obtaining ClassEntry references. Key changes include:
- Updating extension functions in ClassEntity and InterfaceEntity to use StateClass or Interface via name lookup.
- Modifying tests, documentation, and examples to reflect the new API.
- Removing closure-based deferral and replacing it with a more direct API that obtains ClassEntry from a global lookup if necessary.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/src/classes.rs | Updated tests to use StateClass-based extension and added a new helper for testing inheritance. |
| phper/src/classes.rs | Refactored API for class/interface extension; replaced closure with StateClass and added unsafe transmute. |
| phper-doc/doc/_06_module/_07_register_interface/index.md | Updated documentation examples for interface extension using Interface::from_name. |
| phper-doc/doc/_06_module/_06_register_class/index.md | Revised class extension examples to demonstrate StateClass usage. |
| phper-doc/doc/_02_quick_start/_02_write_a_simple_http_client/index.md | Adjusted quick start examples to reflect new exception inheritance. |
| examples/http-server/src/errors.rs | Changed exception class extension to use StateClass::from_name. |
| examples/http-client/src/errors.rs | Updated exception extension in client errors to use the new API. |
Files not reviewed (1)
- tests/integration/tests/php/classes.php: Language not supported
| pub fn extends(&mut self, parent: impl Fn() -> &'static ClassEntry + 'static) { | ||
| self.parent = Some(Box::new(parent)); | ||
| pub fn extends<S: 'static>(&mut self, parent: StateClass<S>) { | ||
| self.parent = Some(unsafe { transmute::<StateClass<S>, StateClass<()>>(parent) }); |
There was a problem hiding this comment.
The use of unsafe transmute here can lead to undefined behavior if the lifetime conversion between StateClass and StateClass<()> is not guaranteed to be safe. Consider using a safer, more explicit conversion method.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| let entry: &'static ClassEntry = | ||
| unsafe { std::mem::transmute(interface.as_class_entry()) }; |
There was a problem hiding this comment.
Using unsafe transmute in this context may introduce subtle lifetime issues when converting interface's ClassEntry. It is recommended to explore a safer alternative to handle the conversion explicitly.
| let entry: &'static ClassEntry = | |
| unsafe { std::mem::transmute(interface.as_class_entry()) }; | |
| let entry: &'static ClassEntry = interface.as_class_entry(); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
avoid providing functions to defer fetching ClassEntry when extending classes and interfaces.