Repository navigation
[spectec] Update meta-interpreter for descriptors - #128
Conversation
Update the SpecTec meta-interpreter backend (construct.ml, host.ml, interpreter.ml, relation.ml, runner.ml) and il2al variable renaming (postprocess.ml) to support exact heap types, exact external function types, descriptor types, struct instance descriptors, and the custom descriptors instructions, while keeping --test-version 3 compatible with specification/wasm-3.0.
|
|
||
| let replace_name old_name new_name algo = | ||
| let replace_name' walker e = | ||
| let replace_name' (walker: Walk.walker) e = |
There was a problem hiding this comment.
AI explanation of why this change is necessary:
simplify_var_name in postprocess.ml renames suffixed variables (such as dcs_0 -> dcs in $subst_desctype) across an AL algorithm using replace_name. Previously, replace_name relied solely on Walk.base_walker.walk_expr, which for IterE (inner_e, (iter, xes)) walks inner_e and the RHS expressions in xes : (id * expr) list, but not the LHS bound variable string x ("dcs_0"), leaving dcs?{dcs_0 <- dcs?}. When backend-interpreter executed that IterE, it bound "dcs_0" in the environment and then evaluated inner_e (VarE "dcs"), failing with an unbound variable error. Updating replace_name to also rename matching keys x in xes fixes this.
Someone with more context will have to say whether that makes sense.
| path | ||
| |> Sys.readdir | ||
| |> Array.to_list | ||
| |> List.filter (fun filename -> not (!Construct.version <= 3 && filename = "custom-descriptors")) |
There was a problem hiding this comment.
This is a bit of a hack. wasm-latest still mirrors wasm-3.0 and they use the same test directory, but wasm-3.0 cannot run the custom descriptors tests. Maybe we need to start mirroring the tests into a separate "latest" directory that proposals can update, similar to what we do for the .spectec sources.
There was a problem hiding this comment.
Yes, good point. Though unlike with the specs, we do not keep the old versions of the test suite around (maybe we should?). So it's less clear what should happen when merging the feature.
Alternatively, instead of hard-coding a single test name inline here, perhaps add some kind of blacklist that is easier to update?
There was a problem hiding this comment.
I'll land this for now, but I'm happy to adopt a different scheme here if you decide on one. I don't have a strong feeling about what is best. I guess it's not the end of the world if each new proposal just adds this line to skip its own tests if necessary.
|
|
||
| let replace_name old_name new_name algo = | ||
| let replace_name' walker e = | ||
| let replace_name' (walker: Walk.walker) e = |
| path | ||
| |> Sys.readdir | ||
| |> Array.to_list | ||
| |> List.filter (fun filename -> not (!Construct.version <= 3 && filename = "custom-descriptors")) |
There was a problem hiding this comment.
Yes, good point. Though unlike with the specs, we do not keep the old versions of the test suite around (maybe we should?). So it's less clear what should happen when merging the feature.
Alternatively, instead of hard-coding a single test name inline here, perhaps add some kind of blacklist that is easier to update?
Update the SpecTec meta-interpreter backend (construct.ml, host.ml, interpreter.ml, relation.ml, runner.ml) and il2al variable renaming (postprocess.ml) to support exact heap types, exact external function types, descriptor types, struct instance descriptors, and the custom descriptors instructions, while keeping --test-version 3 compatible with specification/wasm-3.0.