Option type support #224

Merged
sb10q merged 8 commits from option into master 2022-03-26 15:09:15 +08:00
Collaborator

Option type is implemented as

T = TypeVar('T')
class Option(Generic[T]):
	...

with its own representation as a nullable pointer, instead of union type of SomeClass | None originally in python, because that should require a more significant modification to the current type system.

Option type is implemented as ```python T = TypeVar('T') class Option(Generic[T]): ... ``` with its own representation as a nullable pointer, instead of union type of `SomeClass | None` originally in python, because that should require a more significant modification to the current type system.
ychenfo force-pushed option from 4765b1b345 to 7763dd36e3 2022-03-20 05:02:27 +08:00 Compare
sb10q reviewed 2022-03-20 07:53:59 +08:00
@ -35,0 +42,4 @@
def is_some(self):
return not self.is_none()
def unwrap(self):
return self.nac3_option
Owner

Blank lines, __str__/__repr__

Blank lines, ``__str__``/``__repr__``
Author
Collaborator

I am not fully sure the best content to put into the returned the string.. currently a simple implementation based on something like this, and __str__ will return the same thing as __repr__

I am not fully sure the best content to put into the returned the string.. currently a simple implementation based on something like [this](https://github.com/m-labs/artiq/blob/nac3/artiq/language/scan.py#L270-L274), and `__str__` will return the same thing as `__repr__`
Owner

"The str() function returns a user-friendly description of an object. The repr() method returns a developer-friendly string representation of an object."

I propose:

str: Some(x) / None (like Rust)
repr: Some(x) / Option(None) (to clarify the difference with the Python None)

"The str() function returns a user-friendly description of an object. The repr() method returns a developer-friendly string representation of an object." I propose: str: ``Some(x)`` / ``None`` (like Rust) repr: ``Some(x)`` / ``Option(None)`` (to clarify the difference with the Python ``None``)
sb10q reviewed 2022-03-20 07:54:38 +08:00
@ -538,6 +559,33 @@ impl InnerResolver {
let types = types?;
Ok(types.map(|types| unifier.add_ty(TypeEnum::TTuple { ty: types })))
}
// special handling for option type since it has special layout in nac3 python side
Owner

"in nac3 python side"?

"in nac3 python side"?
Author
Collaborator

updated comment to be special handling for option type since its class member layout in python side is special and cannot be mapped directly to a nac3 type as below

updated comment to be `special handling for option type since its class member layout in python side is special and cannot be mapped directly to a nac3 type as below`
sb10q reviewed 2022-03-20 07:58:38 +08:00
@ -906,4 +993,3 @@
})
.unwrap(),
};
println!("{:?}", result);
Owner

This shouldn't be in that PR. Just commit and push this sort of thing to master directly.

This shouldn't be in that PR. Just commit and push this sort of thing to master directly.
ychenfo marked this conversation as resolved
Owner

What happens when you unwrap() a None option?
The behavior on the host should match.

What happens when you ``unwrap()`` a ``None`` option? The behavior on the host should match.
sb10q reviewed 2022-03-20 08:02:13 +08:00
@ -541,0 +565,4 @@
{
let field_data = match obj.getattr("nac3_option") {
Ok(d) => d,
// should be None, instantiate with type vars instead
Owner

Comment is unclear.

Comment is unclear.
Author
Collaborator

updated comment to be None should be already handled above

updated comment to be `None should be already handled above`
sb10q reviewed 2022-03-20 08:02:29 +08:00
@ -541,0 +566,4 @@
let field_data = match obj.getattr("nac3_option") {
Ok(d) => d,
// should be None, instantiate with type vars instead
Err(_) => unreachable!("cannot be None")
Owner

...especially with this under it.

...especially with this under it.
sb10q reviewed 2022-03-20 08:03:06 +08:00
@ -759,0 +833,4 @@
}
} else if ty_id == self.primitive_ids.none {
// for option type, just a null ptr, whose type needs to be casted in codegen
// according to the type info acttached in the ast
Owner

acttached -> attached

acttached -> attached
ychenfo marked this conversation as resolved
sb10q reviewed 2022-03-20 08:05:55 +08:00
@ -183,0 +265,4 @@
|ctx, obj, _, _, generator| {
let obj_val = obj.unwrap().1.clone().to_basic_value_enum(ctx, generator)?;
if let BasicValueEnum::PointerValue(ptr) = obj_val {
Ok(Some(ctx.builder.build_load(ptr, "unwrap_some")))
Owner

Okay, so unwrap() can return a NULL pointer which will crash the device later.
I suggest checking for NULL here and raising an exception.

Okay, so ``unwrap()`` can return a NULL pointer which will crash the device later. I suggest checking for NULL here and raising an exception.
sb10q reviewed 2022-03-20 08:07:51 +08:00
@ -1123,6 +1238,7 @@ pub fn get_builtins(primitives: &mut (PrimitiveStore, Unifier)) -> BuiltinInfo {
"min",
"max",
"abs",
"Some",
Owner

Does this really need special treatment?
Just @portable def Some(... in min_artiq.py wouldn't work?

Does this really need special treatment? Just ``@portable def Some(...`` in ``min_artiq.py`` wouldn't work?
Owner

OK, I guess we need it like this for nac3standalone...

OK, I guess we need it like this for nac3standalone...
Owner

Add a test in nac3standalone/demo

Add a test in ``nac3standalone/demo``
sb10q reviewed 2022-03-20 08:18:52 +08:00
@ -54,6 +54,16 @@ pub enum RecordKey {
Int(i32),
}
impl Type {
Owner

Could you add a comment that documents what this does?

Could you add a comment that documents what this does?
Author
Collaborator

comment added

comment added
ychenfo marked this conversation as resolved
ychenfo force-pushed option from 7763dd36e3 to 3f1841bb15 2022-03-21 04:37:33 +08:00 Compare
sb10q reviewed 2022-03-21 09:56:10 +08:00
@ -35,0 +52,4 @@
if self.is_none():
return "<Option: None>"
elif self.nac3_option is self:
return "<Option: Error(self recursion)>"
Owner

Python would report a stack overflow error already, and from its backtrace it should be clear enough what is happening.

Python would report a stack overflow error already, and from its backtrace it should be clear enough what is happening.
Owner

And by the way your proposal does not find all cases. For example, you could have option A containing option B, and option B containing option A, and it would overflow the stack and not print "Error(self recursion)".

Anyway those infinite recursions seem rare (you would have to try hard to shoot yourself in the foot by mutating the Option object) and produce a good enough backtrace without anything special.

You could put an underscore _nac3_option (see Python naming conventions) to highlight the fact that the user is not supposed to mutate the Option.

And by the way your proposal does not find all cases. For example, you could have option A containing option B, and option B containing option A, and it would overflow the stack and not print "Error(self recursion)". Anyway those infinite recursions seem rare (you would have to try hard to shoot yourself in the foot by mutating the ``Option`` object) and produce a good enough backtrace without anything special. You could put an underscore ``_nac3_option`` (see Python naming conventions) to highlight the fact that the user is not supposed to mutate the ``Option``.
Author
Collaborator

Thanks for pointing this out! I have force pushed to update the __str__ and __repr__, remove the recursion check and use _nac3_option

Thanks for pointing this out! I have force pushed to update the `__str__` and `__repr__`, remove the recursion check and use `_nac3_option`
ychenfo force-pushed option from 3f1841bb15 to 7db5909f62 2022-03-22 05:23:58 +08:00 Compare
Author
Collaborator

During the process of add tests found this problem that it would be hard to patch a builtin class/object (for the use of a = None; a.is_some()) unless using something like this.. And this would also cause the nac3 option None not being able to be used in an @portable function..

So should we use something like none instead of None, or maybe some other ways to handle this?

During the process of add tests found this problem that it would be hard to patch a builtin class/object (for the use of `a = None; a.is_some()`) unless using something like [this](https://github.com/clarete/forbiddenfruit).. And this would also cause the nac3 option None not being able to be used in an `@portable` function.. So should we use something like `none` instead of `None`, or maybe some other ways to handle this?
Owner

none seems fine. Then you can just use that in __str__ / __repr__ instead of Option(None).

Patching None seems too complicated and messy, and potentially with forward compatibility problems.

``none`` seems fine. Then you can just use that in ``__str__`` / ``__repr__`` instead of ``Option(None)``. Patching ``None`` seems too complicated and messy, and potentially with forward compatibility problems.
ychenfo force-pushed option from 7db5909f62 to 66204cde5a 2022-03-23 03:50:30 +08:00 Compare
ychenfo changed title from Option type support to WIP: Option type support 2022-03-23 04:35:45 +08:00
ychenfo force-pushed option from 66204cde5a to 6e4b507d92 2022-03-23 05:14:05 +08:00 Compare
sb10q reviewed 2022-03-23 08:56:37 +08:00
@ -35,0 +50,4 @@
def __repr__(self) -> str:
if self.is_none():
return "Option(none)"
Owner

Just none is fine.

Just ``none`` is fine.
ychenfo marked this conversation as resolved
ychenfo force-pushed option from 6e4b507d92 to 4db68a201f 2022-03-24 06:45:07 +08:00 Compare
ychenfo force-pushed option from 4db68a201f to 70d0b40a54 2022-03-24 07:23:43 +08:00 Compare
sb10q reviewed 2022-03-24 10:04:26 +08:00
@ -35,0 +46,4 @@
return not self.is_none()
def unwrap(self):
return self._nac3_option
Owner

You want to raise an exception if it's None to match the device behavior.

You want to raise an exception if it's ``None`` to match the device behavior.
Author
Collaborator

thanks! now it will raise a ValueError("unwrap on none")

thanks! now it will raise a `ValueError("unwrap on none")`
Owner

I would suggest a dedicated exception type.

I would suggest a dedicated exception type.
Author
Collaborator

added a new dedicated exception UnwrapNoneError

added a new dedicated exception `UnwrapNoneError`
ychenfo marked this conversation as resolved
sb10q reviewed 2022-03-24 10:05:13 +08:00
@ -541,0 +549,4 @@
{
let field_data = match obj.getattr("_nac3_option") {
Ok(d) => d,
// we use `none = Option(None)` now, so the obj should always have
Owner

Remove "now" and "should"

Remove "now" and "should"
ychenfo marked this conversation as resolved
sb10q reviewed 2022-03-24 10:05:56 +08:00
@ -909,3 +993,3 @@
}
if let Ok(t) = sym_ty {
self.0.pyid_to_type.write().insert(*id, t);
// do not cache for option type since None can have same pyid but different type
Owner

None ?

``None`` ?
ychenfo marked this conversation as resolved
sb10q reviewed 2022-03-24 10:06:58 +08:00
@ -105,6 +105,19 @@ pub fn get_builtins(primitives: &mut (PrimitiveStore, Unifier)) -> BuiltinInfo {
("__param2__".into(), int64, true),
];
// for Option, is_some and is_none share the same type
Owner

Why?

Why?
Author
Collaborator

Because these two functions are all of type () -> bool, and are methods under the same class Option

Because these two functions are all of type `() -> bool`, and are methods under the same class `Option`
Owner

Could you explain further in the comment?

Could you explain further in the comment?
Author
Collaborator

updated the explaination in the new commit

updated the explaination in the new commit
ychenfo force-pushed option from 70d0b40a54 to aebfd0a37d 2022-03-26 02:49:40 +08:00 Compare
ychenfo changed title from WIP: Option type support to Option type support 2022-03-26 02:49:49 +08:00
Author
Collaborator

Tests added, and now unwrap on none will be ValueError("unwrap on none"). The exception raising is also tested with artiq_run with devices.

Tests added, and now unwrap on `none` will be `ValueError("unwrap on none")`. The exception raising is also tested with `artiq_run` with devices.
ychenfo force-pushed option from aebfd0a37d to 7ede18429f 2022-03-26 14:11:15 +08:00 Compare
sb10q merged commit 80631fc92b into master 2022-03-26 15:09:15 +08:00
sb10q deleted branch option 2022-03-26 15:09:15 +08:00
sb10q referenced this issue from a commit 2022-03-26 15:09:16 +08:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#224
No description provided.