zksecurity / noname

Noname: a programming language to write zkapps
https://zksecurity.github.io/noname/
161 stars 35 forks source link

make the type checker pass more explicit in its function signature #140

Open mimoo opened 3 weeks ago

mimoo commented 3 weeks ago

it would be nice to have an actual

fn typecheck(nast: NAST<B>) -> Result<TAST<B>>

to really show the flow between the different compiler passes (AST -> NAST -> TAST)

instead of what we have now: https://github.com/zksecurity/noname/blob/main/src/type_checker/mod.rs#L182

fn analyze(&mut self, nast: NAST<B>, is_lib: bool) -> Result<()>
katat commented 1 week ago

Each TAST could be bound to multiple NAST, as this test case demonstrated: https://github.com/zksecurity/noname/blob/3a07d2a1339d3c7de6aa417b97dd2914352b3171/src/tests/modules.rs#L85

So we would allow the TypeChecker to init with all the NAST to loop through during type checking.

pub struct TypeChecker<B>
where
    B: Backend,
{
    /// the functions present in the scope
    /// contains at least the set of builtin functions (like assert_eq)
    functions: HashMap<FullyQualified, FnInfo<B>>,

    /// Custom structs type information and ASTs for methods.
    structs: HashMap<FullyQualified, StructInfo>,

    /// Constants declared in this module.
    constants: HashMap<FullyQualified, ConstInfo<B::Field>>,

    /// Mapping from node id to TyKind.
    /// This can be used by the circuit-writer when it needs type information.
    // TODO: I think we should get rid of this if we can
    node_types: HashMap<usize, TyKind>,

+    /// The list of all the NASTs that we have to type check.
+    /// next node id / NAST
+    #[serde(skip)]
+    nasts: Vec<(usize, NAST<B>)>,
}

UPDATE: Actually this will lead to borrowing issue when it tries to do this:

pub fn typecheck(&mut self) -> Result<()> {
    for (nast, node_id) in &self.nasts {
        self.analyze(&nast, nast.is_lib)?;
    }

    Ok(())
}

So letting the TypeChecker to contain nasts seems not to be a good idea.

mimoo commented 1 week ago

^ yeah not a good idea IMO. I think having two functions could help:

fn init_analyze(nast: NAST<B>, is_lib: bool) -> Result<TAST>;
fn analyze(self: TAST, nast: NAST<B>, is_lib: bool) -> Result<TAST>;