Option type support #224
No reviewers
Labels
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/nac3#224
Loading…
Reference in New Issue
No description provided.
Delete Branch "option"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Option type is implemented as
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.4765b1b345
to7763dd36e3
@ -35,0 +42,4 @@
def is_some(self):
return not self.is_none()
def unwrap(self):
return self.nac3_option
Blank lines,
__str__
/__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, and
__str__
will return the same thing as__repr__
"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 PythonNone
)@ -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
"in nac3 python side"?
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
@ -906,4 +993,3 @@
})
.unwrap(),
};
println!("{:?}", result);
This shouldn't be in that PR. Just commit and push this sort of thing to master directly.
What happens when you
unwrap()
aNone
option?The behavior on the host should match.
@ -541,0 +565,4 @@
{
let field_data = match obj.getattr("nac3_option") {
Ok(d) => d,
// should be None, instantiate with type vars instead
Comment is unclear.
updated comment to be
None should be already handled above
@ -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")
...especially with this under it.
@ -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
acttached -> attached
@ -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")))
Okay, so
unwrap()
can return a NULL pointer which will crash the device later.I suggest checking for NULL here and raising an exception.
@ -1123,6 +1238,7 @@ pub fn get_builtins(primitives: &mut (PrimitiveStore, Unifier)) -> BuiltinInfo {
"min",
"max",
"abs",
"Some",
Does this really need special treatment?
Just
@portable def Some(...
inmin_artiq.py
wouldn't work?OK, I guess we need it like this for nac3standalone...
Add a test in
nac3standalone/demo
@ -54,6 +54,16 @@ pub enum RecordKey {
Int(i32),
}
impl Type {
Could you add a comment that documents what this does?
comment added
7763dd36e3
to3f1841bb15
@ -35,0 +52,4 @@
if self.is_none():
return "<Option: None>"
elif self.nac3_option is self:
return "<Option: Error(self recursion)>"
Python would report a stack overflow error already, and from its backtrace it should be clear enough what is happening.
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 theOption
.Thanks for pointing this out! I have force pushed to update the
__str__
and__repr__
, remove the recursion check and use_nac3_option
3f1841bb15
to7db5909f62
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 ofNone
, or maybe some other ways to handle this?none
seems fine. Then you can just use that in__str__
/__repr__
instead ofOption(None)
.Patching
None
seems too complicated and messy, and potentially with forward compatibility problems.7db5909f62
to66204cde5a
Option type supportto WIP: Option type support66204cde5a
to6e4b507d92
@ -35,0 +50,4 @@
def __repr__(self) -> str:
if self.is_none():
return "Option(none)"
Just
none
is fine.6e4b507d92
to4db68a201f
4db68a201f
to70d0b40a54
@ -35,0 +46,4 @@
return not self.is_none()
def unwrap(self):
return self._nac3_option
You want to raise an exception if it's
None
to match the device behavior.thanks! now it will raise a
ValueError("unwrap on none")
I would suggest a dedicated exception type.
added a new dedicated exception
UnwrapNoneError
@ -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
Remove "now" and "should"
@ -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
None
?@ -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
Why?
Because these two functions are all of type
() -> bool
, and are methods under the same classOption
Could you explain further in the comment?
updated the explaination in the new commit
70d0b40a54
toaebfd0a37d
WIP: Option type supportto Option type supportTests added, and now unwrap on
none
will beValueError("unwrap on none")
. The exception raising is also tested withartiq_run
with devices.aebfd0a37d
to7ede18429f