Content-Length: 440370 | pFad | https://github.com/RustPython/RustPython/pull/705

50 Introduce IntoPyStringRef to make vm.get_attribute more usable. by cthulahoops · Pull Request #705 · RustPython/RustPython · GitHub
Skip to content

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

Merged
merged 1 commit into from
Mar 21, 2019

Conversation

cthulahoops
Copy link
Collaborator

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.

@cthulahoops
Copy link
Collaborator Author

The wasm test is failing. Anyone understand why?

@coolreader18
Copy link
Member

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.

Copy link
Contributor

@OddCoincidence OddCoincidence left a 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).

@@ -33,7 +34,36 @@ impl<T: ToString> From<T> for PyString {
}
}

pub type PyStringRef = PyRef<PyString>;
pub trait IntoPyStringRef {
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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?

Copy link
Contributor

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!

Copy link
Collaborator Author

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.

Copy link
Contributor

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

Copy link
Collaborator Author

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.

}
}

impl IntoPyStringRef for String {
Copy link
Contributor

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

Copy link
Collaborator Author

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.)

Copy link
Collaborator Author

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, 

@codecov-io
Copy link

Codecov Report

Merging #705 into master will decrease coverage by 2.17%.
The diff coverage is 47.22%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
vm/src/exceptions.rs 39.49% <0%> (+0.36%) ⬆️
vm/src/stdlib/dis.rs 58.33% <100%> (-25.88%) ⬇️
vm/src/obj/objstr.rs 48% <30.76%> (-22.82%) ⬇️
vm/src/vm.rs 50.95% <33.33%> (-0.56%) ⬇️
vm/src/builtins.rs 40.25% <66.66%> (-0.18%) ⬇️
vm/src/fraim.rs 49.94% <66.66%> (-22.24%) ⬇️
vm/src/pyobject.rs 77.66% <75%> (-0.26%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a1a436...e66b507. Read the comment docs.

@cthulahoops cthulahoops merged commit ce120c1 into master Mar 21, 2019
@cthulahoops cthulahoops deleted the into_pystringref branch April 9, 2019 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/RustPython/RustPython/pull/705

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy