wrong "Incompatible types" error with generic classes #228

New Issue

sb10q · 2022-03-20T22:28:28+08:00

sb10q commented

2022-03-20 22:28:28 +08:00

Based on https://github.com/m-labs/artiq/issues/1207

from typing import TypeVar, Generic

from numpy import int32

from artiq.experiment import *
from artiq.coredevice.core import Core


@nac3
class A:
    @kernel
    def do(self) -> int32:
        return 0


@nac3
class B:
    @kernel
    def do(self) -> int32:
        return 1


T = TypeVar("T", A, B)


@nac3
class C(Generic[T]):
    sub: Kernel[T]

    def __init__(self, which):
        if which:
            self.sub = B()
        else:
            self.sub = A()

@nac3
class Types(EnvExperiment):
    core: KernelInvariant[Core]
    c0: Kernel[C[A]]
    c1: Kernel[C[B]]
    
    def build(self):
        self.setattr_device("core")
        self.c0 = C(False)
        self.c1 = C(True)
    
    @kernel
    def run(self):
        pass

nac3artiq.CompileError: type error inside object launching kernel: error when getting type of field `c1` (Incompatible types: artiq_run_typedef.A and artiq_run_typedef.B)

Commenting out either the c0 or the c1 field makes the error disappear.

Based on https://github.com/m-labs/artiq/issues/1207 ```python from typing import TypeVar, Generic from numpy import int32 from artiq.experiment import * from artiq.coredevice.core import Core @nac3 class A: @kernel def do(self) -> int32: return 0 @nac3 class B: @kernel def do(self) -> int32: return 1 T = TypeVar("T", A, B) @nac3 class C(Generic[T]): sub: Kernel[T] def __init__(self, which): if which: self.sub = B() else: self.sub = A() @nac3 class Types(EnvExperiment): core: KernelInvariant[Core] c0: Kernel[C[A]] c1: Kernel[C[B]] def build(self): self.setattr_device("core") self.c0 = C(False) self.c1 = C(True) @kernel def run(self): pass ``` ``` nac3artiq.CompileError: type error inside object launching kernel: error when getting type of field `c1` (Incompatible types: artiq_run_typedef.A and artiq_run_typedef.B) ``` Commenting out either the ``c0`` or the ``c1`` field makes the error disappear.

ychenfo commented

2022-03-22 05:49:26 +08:00

There should still be some deeper bugs since this patch only non-deterministically fix the bug (other times fails with another bug thread '<unnamed>' panicked at 'internal error: entered unreachable code: T', nac3core/src/codegen/mod.rs:323:18..).. but hope this can be of some help: I think the type cache for recursive type should be inserted with key python object id instead of the type id for generic classes, the when cache should be updated when the previously obtained generic type is resolved to a concrete type

diff --git a/nac3artiq/src/symbol_resolver.rs b/nac3artiq/src/symbol_resolver.rs
index 0966d6a..bd5448b 100644
--- a/nac3artiq/src/symbol_resolver.rs
+++ b/nac3artiq/src/symbol_resolver.rs
@@ -477,8 +477,8 @@ impl InnerResolver {
         primitives: &PrimitiveStore,
     ) -> PyResult<Result<Type, String>> {
         let ty = self.helper.type_fn.call1(py, (obj,)).unwrap();
-        let ty_id: u64 = self.helper.id_fn.call1(py, (ty.clone(),))?.extract(py)?;
-        if let Some(ty) = self.pyid_to_type.read().get(&ty_id) {
+        let py_obj_id: u64 = self.helper.id_fn.call1(py, (obj,)).unwrap().extract(py)?;
+        if let Some(ty) = self.pyid_to_type.read().get(&py_obj_id) {
             return Ok(Ok(*ty))
         }
         let (extracted_ty, inst_check) = match self.get_pyty_obj_type(
@@ -538,9 +538,22 @@ impl InnerResolver {
                 let types = types?;
                 Ok(types.map(|types| unifier.add_ty(TypeEnum::TTuple { ty: types })))
             }
-            (TypeEnum::TObj { params: var_map, fields, .. }, false) => {
-                self.pyid_to_type.write().insert(ty_id, extracted_ty);
+            (TypeEnum::TObj { params, fields, obj_id }, false) => {
+                self.pyid_to_type.write().insert(py_obj_id, extracted_ty);
                 let mut instantiate_obj = || {
+                    let var_map = params
+                        .iter()
+                        .map(|(id_var, ty)| {
+                            if let TypeEnum::TVar { id, range, name, loc, .. } =
+                                &*unifier.get_ty(*ty)
+                            {
+                                assert_eq!(*id, *id_var);
+                                (*id, unifier.get_fresh_var_with_range(range, *name, *loc).0)
+                            } else {
+                                unreachable!()
+                            }
+                        })
+                        .collect::<HashMap<_, _>>();
                     // loop through non-function fields of the class to get the instantiated value
                     for field in fields.iter() {
                         let name: String = (*field.0).into();
@@ -577,12 +590,17 @@ impl InnerResolver {
                             return Ok(Err("object is not of concrete type".into()));
                         }
                     }
-                    Ok(Ok(extracted_ty))
+                    let final_ty = unifier.subst(extracted_ty, &var_map).unwrap_or_else(|| {
+                        extracted_ty
+                    });
+                    self.pyid_to_type.write().insert(py_obj_id, final_ty);
+                    Ok(Ok(final_ty))
                 };
                 let result = instantiate_obj();
                 // do not cache the type if there are errors
                 if matches!(result, Err(_) | Ok(Err(_))) {
-                    self.pyid_to_type.write().remove(&ty_id);
+                    self.pyid_to_type.write().remove(&py_obj_id);
                 }
                 result
             }

There should still be some deeper bugs since this patch only non-deterministically fix the bug (other times fails with another bug `thread '<unnamed>' panicked at 'internal error: entered unreachable code: T', nac3core/src/codegen/mod.rs:323:18`..).. but hope this can be of some help: I think the type cache for recursive type should be inserted with key python object id instead of the type id for generic classes, the when cache should be updated when the previously obtained generic type is resolved to a concrete type ```diff diff --git a/nac3artiq/src/symbol_resolver.rs b/nac3artiq/src/symbol_resolver.rs index 0966d6a..bd5448b 100644 --- a/nac3artiq/src/symbol_resolver.rs +++ b/nac3artiq/src/symbol_resolver.rs @@ -477,8 +477,8 @@ impl InnerResolver { primitives: &PrimitiveStore, ) -> PyResult<Result<Type, String>> { let ty = self.helper.type_fn.call1(py, (obj,)).unwrap(); - let ty_id: u64 = self.helper.id_fn.call1(py, (ty.clone(),))?.extract(py)?; - if let Some(ty) = self.pyid_to_type.read().get(&ty_id) { + let py_obj_id: u64 = self.helper.id_fn.call1(py, (obj,)).unwrap().extract(py)?; + if let Some(ty) = self.pyid_to_type.read().get(&py_obj_id) { return Ok(Ok(*ty)) } let (extracted_ty, inst_check) = match self.get_pyty_obj_type( @@ -538,9 +538,22 @@ impl InnerResolver { let types = types?; Ok(types.map(|types| unifier.add_ty(TypeEnum::TTuple { ty: types }))) } - (TypeEnum::TObj { params: var_map, fields, .. }, false) => { - self.pyid_to_type.write().insert(ty_id, extracted_ty); + (TypeEnum::TObj { params, fields, obj_id }, false) => { + self.pyid_to_type.write().insert(py_obj_id, extracted_ty); let mut instantiate_obj = || { + let var_map = params + .iter() + .map(|(id_var, ty)| { + if let TypeEnum::TVar { id, range, name, loc, .. } = + &*unifier.get_ty(*ty) + { + assert_eq!(*id, *id_var); + (*id, unifier.get_fresh_var_with_range(range, *name, *loc).0) + } else { + unreachable!() + } + }) + .collect::<HashMap<_, _>>(); // loop through non-function fields of the class to get the instantiated value for field in fields.iter() { let name: String = (*field.0).into(); @@ -577,12 +590,17 @@ impl InnerResolver { return Ok(Err("object is not of concrete type".into())); } } - Ok(Ok(extracted_ty)) + let final_ty = unifier.subst(extracted_ty, &var_map).unwrap_or_else(|| { + extracted_ty + }); + self.pyid_to_type.write().insert(py_obj_id, final_ty); + Ok(Ok(final_ty)) }; let result = instantiate_obj(); // do not cache the type if there are errors if matches!(result, Err(_) | Ok(Err(_))) { - self.pyid_to_type.write().remove(&ty_id); + self.pyid_to_type.write().remove(&py_obj_id); } result } ```

pca006132 commented

2022-03-23 00:33:58 +08:00

41d62f7325

This one is a bit tricky... There are multiple reasons, including the fix Anto mentioned above, and a bug in composer. The composer will put a dummy typevar for each field, and unify the dummy typevar with a concrete type later. However, the field may contain typevars that we need to substitute, and we cannot substitute it if we only have the dummy typevar, thus the non-deterministic behavior (this depends on the order we perform the unifications). The fix is to put those classes with generic type parameters at the end,

https://git.m-labs.hk/M-Labs/nac3/commit/41d62f7325bb86cb057e5909865151127ecd7756 This one is a bit tricky... There are multiple reasons, including the fix Anto mentioned above, and a bug in composer. The composer will put a dummy typevar for each field, and unify the dummy typevar with a concrete type later. However, the field may contain typevars that we need to substitute, and we cannot substitute it if we only have the dummy typevar, thus the non-deterministic behavior (this depends on the order we perform the unifications). The fix is to put those classes with generic type parameters at the end,

👍 1

pca006132 closed this issue

2022-03-23 00:37:04 +08:00

Sign in to join this conversation.