Skip to content
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.

Sign up for GitHub

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

Jump to bottom

require type checking on CustomConst #325

Merged
merged 3 commits into from
Aug 2, 2023
Merged

require type checking on CustomConst #325

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
102fef1
require type checking on `CustomConst`
ss2165 Aug 1, 2023
c4f3187
address review comments
ss2165 Aug 2, 2023
223f234
Merge branch 'main' into custom-const-check
ss2165 Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 61 additions & 6 deletions src/extensions/rotation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use std::collections::HashMap;
#[cfg(feature = "pyo3")]
use pyo3::prelude::*;

use crate::ops::constant::typecheck::ConstTypeError;
use crate::ops::constant::CustomConst;
use crate::resource::{OpDef, ResourceSet, TypeDef};
use crate::types::type_param::TypeArg;
Expand Down Expand Up @@ -93,6 +94,15 @@ pub enum Constant {
Quaternion(cgmath::Quaternion<f64>),
}

impl Constant {
fn rotation_type(&self) -> Type {
match self {
Constant::Angle(_) => Type::Angle,
Constant::Quaternion(_) => Type::Quaternion,
}
}
}

#[typetag::serde]
impl CustomConst for Constant {
fn name(&self) -> SmolStr {
Expand All @@ -103,12 +113,25 @@ impl CustomConst for Constant {
.into()
}

fn custom_type(&self) -> CustomType {
let t: Type = match self {
Constant::Angle(_) => Type::Angle,
Constant::Quaternion(_) => Type::Quaternion,
};
t.custom_type()
fn check_type(&self, typ: &CustomType) -> Result<(), ConstTypeError> {
let self_typ = self.rotation_type();

if &self_typ.custom_type() == typ {
Ok(())
} else {
Err(ConstTypeError::ValueCheckFail(
typ.clone().into(),
self.clone().into(),
))
}
}

fn equal_consts(&self, other: &dyn CustomConst) -> bool {
if let Some(other) = other.as_any().downcast_ref::<Constant>() {
self == other
} else {
false
}
}
}

Expand Down Expand Up @@ -315,4 +338,36 @@ mod test {
))
);
}

#[test]
fn test_type_check() {
let resource = resource();

let custom_type = resource
.types()
.get("angle")
.unwrap()
.instantiate_concrete([])
.unwrap();

let custom_value = Constant::Angle(AngleValue::F64(0.0));

// correct type
custom_value.check_type(&custom_type).unwrap();

let wrong_custom_type = resource
.types()
.get("quat")
.unwrap()
.instantiate_concrete([])
.unwrap();
let res = custom_value.check_type(&wrong_custom_type);
assert_eq!(
res,
Err(ConstTypeError::ValueCheckFail(
wrong_custom_type.into(),
custom_value.into(),
)),
);
}
}
11 changes: 5 additions & 6 deletions src/ops/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ pub enum ConstValue {
Tuple(Vec<ConstValue>),
/// An opaque constant value, with cached type
// Note: the extra level of tupling is to avoid https://github.com/rust-lang/rust/issues/78808
Opaque((CustomType, Box<dyn CustomConst>)),
Opaque((Box<dyn CustomConst>,)),
}

impl PartialEq for dyn CustomConst {
Expand All @@ -159,7 +159,7 @@ impl ConstValue {
match self {
Self::Int(value) => format!("const:int{value}"),
Self::F64(f) => format!("const:float:{f}"),
Self::Opaque((_, v)) => format!("const:{}", v.name()),
Self::Opaque((v,)) => format!("const:{}", v.name()),
Self::Sum(tag, val) => {
format!("const:sum:{{tag:{tag}, val:{}}}", val.name())
}
Expand Down Expand Up @@ -200,7 +200,7 @@ impl ConstValue {

impl<T: CustomConst> From<T> for ConstValue {
fn from(v: T) -> Self {
Self::Opaque((v.custom_type(), Box::new(v)))
Self::Opaque((Box::new(v),))
}
}

Expand All @@ -215,9 +215,8 @@ pub trait CustomConst:
/// An identifier for the constant.
fn name(&self) -> SmolStr;

/// Returns the type of the constant.
// TODO it would be good to ensure that this is a *classic* CustomType not a linear one!
fn custom_type(&self) -> CustomType;
/// Check the value is a valid instance of the provided type.
fn check_type(&self, typ: &CustomType) -> Result<(), ConstTypeError>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - so the I-have-some-YAML implementation of this (maybe there are two such: one which embeds a type, one without - I mean the one with) says:
if typ == myEmbeddedType {Ok(())} else {Err(....)}
right?

You could make this return String for errors, and then wrap those in CustomCheckFail when we call this? It'd make it clear whose code an error was coming from...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part one: yes

part two: yes, but with some wrapping i think, will do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah no actually, CustomCheckFail is only there as a catch all in case the existing error modes don't match up, in most cases ValueCheckFail is the correct mode and can be reused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no way for a CustomConst to synthesize its type, there's no way to tell that a CustomConst is Hashable or otherwise. If we don't want to add a synthesizing method, do we want to add a method returning a TypeTag ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need such (#322 will work without), but that doesn't mean we shouldn't have it ;). I guess it could return Option, and maybe should, whereupon we can't rely on it, so it doesn't reduce complexity (we still have to handle all the cases).


/// Compare two constants for equality, using downcasting and comparing the definitions.
fn equal_consts(&self, other: &dyn CustomConst) -> bool {
Expand Down
13 changes: 4 additions & 9 deletions src/ops/constant/typecheck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ pub enum ConstTypeError {
/// A mismatch between the type expected and the value.
#[error("Value {1:?} does not match expected type {0}")]
ValueCheckFail(ClassicType, ConstValue),
/// Error when checking a custom value.
#[error("Custom value type check error: {0:?}")]
CustomCheckFail(String),
}

lazy_static! {
Expand Down Expand Up @@ -140,15 +143,7 @@ pub(super) fn typecheck_const(typ: &ClassicType, val: &ConstValue) -> Result<(),
}
}
(Container::Sum(_), _) => Err(ConstTypeError::ValueCheckFail(ty.clone(), tm.clone())),
(Container::Opaque(ty), ConstValue::Opaque((ty_act, _val))) => {
if ty_act != ty {
return Err(ConstTypeError::TypeMismatch(
ty.clone().into(),
ty_act.clone().into(),
));
}
Ok(())
}
(Container::Opaque(ty), ConstValue::Opaque((val,))) => val.check_type(ty),
_ => Err(ConstTypeError::Unimplemented(ty.clone())),
},
(ClassicType::Hashable(HashableType::Container(c)), tm) => {
Expand Down