-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Introduce IntoPyStringRef to make vm.get_attribute more usable. #705
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
Conversation
The wasm test is failing. Anyone understand why? |
It's broken for master too. I think it was something related to imports, but it should be fixed with #701. It's not related to anything you did, I'm pretty sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine, but I do wonder if we could generalize it a bit (see other comments).
vm/src/obj/objstr.rs
Outdated
@@ -33,7 +34,36 @@ impl<T: ToString> From<T> for PyString { | |||
} | |||
} | |||
|
|||
pub type PyStringRef = PyRef<PyString>; | |||
pub trait IntoPyStringRef { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this returns a PyResult
, should it actually be called TryIntoPyStringRef
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. If it has to be try_into_pystringref
this gets a bit unwieldy, but I guess there won't too many callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, had the same concern for TryIntoObject
(that's why I dropped the Py
and Ref
, since in practice you know it's referring to a PyObject
from context and working with the value types instead of refs is pretty exceptional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha! The analogy to TryIntoObject
is what I need to get the right shape of this. It's TryIntoRef<T>
. I this will be really useful eventually, and could replace TryFromObject
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this look now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I this will be really useful eventually, and could replace TryFromObject?
TryFromObject
is a bit different, e.g. it wouldn't make sense TryIntoRef
for isize
, but that is done for TryFromObject
so we can accept it as a fn parameter. (Sorry if I caused confusion by typoing TryFromObject
as TryIntoObject
above).
But if my plan for PyObjectRef
to eventually be a type alias for PyRef<dyn PyObjectPayload>
works out, then this would prevent us from every needing a TryIntoObject
and TryFromObject
could be replaced with a TryFromRef
.
How does this look now?
This looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think creating impl TryIntoRef<PyInt> for isize
does make sense.
They're sort of doing the same thing, in one case allowing us to write flexible parameters for functions that can be called from rust, in the other flexible paremeters for functions that can be called from python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry that isize
example was poorly thought out. I basically just meant that they are going in opposite directions:
TryIntoRef
: Self -> Ref
TryFromObject
: Ref -> Self
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now. They overlap in the one specific case where I've used TryFromObject (Self is PyRef), but there are other implementations.
vm/src/obj/objstr.rs
Outdated
} | ||
} | ||
|
||
impl IntoPyStringRef for String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These impls do seem a bit redundant since we already have similar ones for PyString
. Could we do something like:
impl<T: Into<PyString>> IntoPyStringRef for T
Or to generalize it further:
impl<V: PyValue, T: Into<V>> IntoPyRef<V> for T
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it works.
Assuming you meant:
impl<T: Into<PyString>> TryIntoPyStringRef for T
{
fn into_pystringref(self, vm: &mut VirtualMachine) -> PyResult<PyStringRef> {
Ok(PyString::from(self).into_ref(vm))
}
}
It complains because:
help: the trait `std::fmt::Display` is not implemented for `T`
And fixing that:
47 | impl TryIntoPyStringRef for PyStringRef {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `pyobject::PyRef<obj::objstr::PyString>`
if you allow it to use the version it derives then it ends up formatting the object using the default formatter and you get: "'str' object"
instead of the string. (I was expected to lose object identity in that case, surprised to lose object value.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default implementation for ToString is the issue:
impl<T> ToString for T where
T: Display + ?Sized,
3d14ae1
to
e66b507
Compare
Codecov Report
@@ Coverage Diff @@
## master #705 +/- ##
==========================================
- Coverage 52.9% 50.72% -2.18%
==========================================
Files 81 81
Lines 14617 15031 +414
Branches 3441 3628 +187
==========================================
- Hits 7733 7625 -108
- Misses 5072 5592 +520
- Partials 1812 1814 +2
Continue to review full report at Codecov.
|
A stand-alone piece of my AttributeProtocol work.
I don't know if an equivalent of
IntoPyObjectRef
can be pieced together from the existing function machinery, but I couldn't see how.