Add a `match_class!` macro #771

OddCoincidence · 2019-03-31T22:32:58Z

Summary

Add a match_class! macro for implementing behavior that varies based on the built-in type of an object.

Detailed Explanation

Doing this currently with downcast is a bit awkward. Because it consumes the Rc,
you have two options:

Clone the object and then do a series of ifs.

impl PyFloat {
    fn eq(self: PyRef<Self>, other: PyObjectRef, vm: &VirtualMachine) -> PyResult {
        if let Some(i @ PyInt { .. }) = other.clone().downcast() {
            Ok(PyInt::new(&self.value + i.as_bigint()).into_ref(vm))
        } else if let Some(f @ PyFloat { .. }) = other.downcast() {{
            let output = self.value + f.value.to_bigint().unwrap();
            Ok(PyFloat::new(output))
        } else {
            Ok(vm.ctx.not_implemented())
        }
    }
}

Use the returned Err(obj) of downcast to avoid the clone.

match other.downcast() {
    Ok(i @ PyInt { ..}) => {
        Ok(PyInt::new(&self.value + ).into_ref(vm))
    },
    Err(obj) => match obj.downcast() {
        Ok(f @ PyFloat { .. }) => {
            let output = self.value + f.value.to_bigint().unwrap();
            Ok(PyFloat::new(output))
        },
        _ => Ok(vm.ctx.not_implemented()),
    }
}

This avoids a pointless clone at the cost of adding undesirable rightward drift. What we really want is to be able to use a match-like syntax that expands to the efficient version above:

impl PyFloat {
    fn eq(self: PyRef<Self>, other: PyObjectRef, vm: &VirtualMachine) -> PyResult {
        match_object!(other,
            i @ PyInt { .. } => {
                Ok(PyInt::new(&self.value + i.as_bigint()).into_ref(vm))
            },
            f @ PyFloat { .. } => {
                let output = self.value + f.value.to_bigint().unwrap();
                Ok(PyFloat::new(output))
            },
            _ => Ok(vm.ctx.not_implemented()),
        )
    }
}

Drawbacks, Rationale, and Alternatives

The main drawback is that it is another macro. It should be very straightforward to implement and to understand though.

Unresolved Questions

Will allowing a full pattern in each match clause encourage making struct fields public, when they really should not be? An alternative would be to just allow the type name in each match arm.

The text was updated successfully, but these errors were encountered:

coolreader18 · 2019-04-01T00:44:42Z

Another question is that this would further cement the unification of class and Rust type and would we want that? I'm not opposed to it personally, but it's something to think about.

Also, instead of using a pat matcher we could just do an $ident @ $path matcher, if we're concerned about public typestruct fields.

cthulahoops · 2019-04-02T13:46:01Z

This worries me a bit. Matching on the internals is something we should do rarely (if ever?) and with great care, so it seems strange to provide a convenient macro to make it easy.

I think the example use case in the PR is wrong. The correct implementation of to_int is something like:

pub fn to_int(vm: &VirtualMachine, obj: &PyObjectRef, base: u32) -> PyResult<BigInt> {
   Ok(vm.call_method(obj.clone(), "__int__")?.value)
}

(Plus whatever is needed to make the base special case work.)

OddCoincidence · 2019-04-02T16:16:05Z

Matching on the internals is something we should do rarely (if ever?) and with great care, so it seems strange to provide a convenient macro to make it easy.

What here are you viewing as "internals"? I would expect all the types and methods used here to (eventually) be part of our public API for extension.

I think the example use case in the PR is wrong. The correct implementation of to_int is something like:

Yeah, a better example would have been to something like complex.__eq__, which needs to work with complex, int, and float.

cthulahoops · 2019-04-02T16:47:41Z

What here are you viewing as "internals"? I would expect all the types and methods used here to (eventually) be part of our public API for extension.

My expectation is that only methods on the type should be able to access the value directly everything else has to go through methods. Otherwise I think you're liable to break subclasses by accessing something you shouldn't. From this perspective nothing should ever access the private data of another class.

I haven't thought much about how the shape of our public api, so I'm not sure how extensions are expected to work with values.

Yeah, a better example would have been to something like complex.__eq__, which needs to work with complex, int, and float.

Isn't that's still the same. int and float support imag and real so that interactions with complex work? (Oddly cpython doesn't seem to be duck-typing as I'd expect here, so I'm not sure how this works.)

OddCoincidence · 2019-04-02T17:02:59Z

My expectation is that only methods on the type should be able to access the value directly everything else has to go through methods.

Completely agree! I assume you're referring then to the full pattern matching in the proposal above (i.e. PyInt { .. })? I noted this in the "unresolved questions" section and in the PR I went with @coolreader18's suggestion to only support a more limited $ident @ $path syntax.

Or by methods did you mean vm.call_method(...)? My general feeling is that this should usually be circumvented for built-in types for performance reasons. I think we'll find that cpython does similar things, and a lot of edge cases that spring to mind won't actually work there.

Isn't that's still the same. int and float support imag and real so that interactions with complex work?

More generally, I think many implementations of __eq__ and other binary methods will have to match on class in one way or another. This just provides a cleaner, more readable, way.

cthulahoops · 2019-04-02T17:14:41Z

Ah, yes, I missed that distinction. In that case I guess this is equivalent to an isinstance check. (The examples in the issue call .value the actual implementation calls .as_bigint.)

Of course, this creates another complication: as_bigint now needs to check if it's a subclass, but that's a problem with or without this syntax.

I'm happier with the more limited implementation.

Structure taken from #771

coolreader18 added the RFC Request for comments label Apr 1, 2019

OddCoincidence mentioned this issue Apr 2, 2019

Add match_class! macro #774

Merged

OddCoincidence closed this as completed Apr 4, 2019

coolreader18 added a commit that referenced this issue Jun 8, 2019

Add RFC issue template

b0db7c1

Structure taken from #771

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add a `match_class!` macro #771

Add a `match_class!` macro #771

OddCoincidence commented Mar 31, 2019

coolreader18 commented Apr 1, 2019

Uh oh!

cthulahoops commented Apr 2, 2019

Uh oh!

OddCoincidence commented Apr 2, 2019

Uh oh!

cthulahoops commented Apr 2, 2019

Uh oh!

OddCoincidence commented Apr 2, 2019 •

edited

Loading

Uh oh!

cthulahoops commented Apr 2, 2019

Uh oh!

Add a match_class! macro #771

Add a match_class! macro #771

Comments

OddCoincidence commented Mar 31, 2019

Summary

Detailed Explanation

Drawbacks, Rationale, and Alternatives

Unresolved Questions

coolreader18 commented Apr 1, 2019

Uh oh!

cthulahoops commented Apr 2, 2019

Uh oh!

OddCoincidence commented Apr 2, 2019

Uh oh!

cthulahoops commented Apr 2, 2019

Uh oh!

OddCoincidence commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cthulahoops commented Apr 2, 2019

Uh oh!

Add a `match_class!` macro #771

Add a `match_class!` macro #771

OddCoincidence commented Apr 2, 2019 •

edited

Loading