Skip to content

Conversation

tlively
Copy link
Member

@tlively tlively commented Oct 13, 2025

This will be useful for the validation of new instructions. That
validation requires being able to look up the descriptor and described
types of expanded deftypes.

Update existing uses of expand_deftype to add an additional
comptype_of_desctype.

This will be useful for the validation of new instructions. That
validation requires being able to look up the descriptor and described
types of expanded deftypes.

Update existing uses of expand_deftype to add an additional
comptype_of_desctype.
@tlively tlively requested a review from rossberg October 13, 2025 23:28

let func_type (c : context) x =
match expand_deftype (Lib.List32.nth c.types.ctx x.it) with
match comptype_of_desctype (expand_deftype (Lib.List32.nth c.types.ctx x.it)) with
Copy link
Member Author

Choose a reason for hiding this comment

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

Splitting this line by introducing an intermediate variable somehow causes a test in func.wast to fail. I would have thought this was impossible, so I have no idea what's going on there.

Copy link
Member

Choose a reason for hiding this comment

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

Odd, what is the error you're seeing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I uploaded commit 0296c5e to demonstrate the problem. Before that commit the tests all pass and after that commit I get this error:

interpreter/wasm: uncaught exception Failure("nth")
Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
Called from Wasm__Parser.func_type in file "text/parser.mly", line 171, characters 26-59
Called from Wasm__Parser.inline_functype_explicit in file "text/parser.mly", line 251, characters 16-29
Called from Wasm__Parser._menhir_action_118 in file "text/parser.mly", line 984, characters 14-61
Called from Wasm__Parser._menhir_action_261.(fun) in file "text/parser.mly", line 1368, characters 38-43
Called from Wasm__Parser._menhir_action_261.(fun) in file "text/parser.mly", line 1368, characters 55-60
Called from Wasm__Parser._menhir_action_261.(fun) in file "text/parser.mly", line 1368, characters 55-60
Called from Wasm__Parser._menhir_action_261.(fun) in file "text/parser.mly", line 1368, characters 55-60
Called from Wasm__Parser._menhir_action_189 in file "text/parser.mly", line 1412, characters 14-41
Called from Wasm__Parser._menhir_run_0851 in file "text/parser.ml", line 8483, characters 15-62
Called from Wasm__Parse.make.parse in file "text/parse.ml", line 41, characters 12-36
Called from Wasm__Js.of_definition in file "script/js.ml", line 758, characters 27-72
Called from Wasm__Js.of_assertion in file "script/js.ml", line 807, characters 26-43
Called from Wasm__Js.of_command in file "script/js.ml", line 854, characters 4-24
Called from Stdlib__List.map in file "list.ml", line 92, characters 20-23
Called from Wasm__Js.of_script in file "script/js.ml", line 859, characters 19-55
Called from Wasm__Run.create_js_file in file "script/run.ml", line 72, characters 11-39
Called from Wasm__Run.run_meta in file "script/run.ml", line 614, characters 6-116
Called from Stdlib__List.iter in file "list.ml", line 110, characters 12-15
Called from Wasm__Run.input_from in file "script/run.ml", line 104, characters 4-14
Called from Dune__exe__Wasm.(fun) in file "wasm.ml", line 67, characters 33-53
Called from Stdlib__List.iter in file "list.ml", line 110, characters 12-15

On this test case from func.wast:

(assert_malformed
  (module quote
    "(func $f (result f64) (f64.const 0))"  ;; adds implicit type definition
    "(func $g (param i32))"                 ;; reuses explicit type definition
    "(func $h (result f64) (f64.const 1))"  ;; reuses implicit type definition
    "(type $t (func (param i32)))"

    "(func (type 2) (param i32))"  ;; does not exist
  )
  "unknown type"
)

Comment on lines 171 to 175
let dt = expand_deftype (Lib.List32.nth c.types.ctx x.it) in
match comptype_of_desctype dt with
| FuncT (ts1, ts2) -> ts1, ts2
| _ -> error x.at ("non-function type " ^ Int32.to_string x.it)
| exception Failure _ -> error x.at ("unknown type " ^ Int32.to_string x.it)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, the match has an exception clause, which acts as an exception handler. This change is moving the call to nth outside that handler.

The following would work, but I don't think it's worthwhile:

Suggested change
let dt = expand_deftype (Lib.List32.nth c.types.ctx x.it) in
match comptype_of_desctype dt with
| FuncT (ts1, ts2) -> ts1, ts2
| _ -> error x.at ("non-function type " ^ Int32.to_string x.it)
| exception Failure _ -> error x.at ("unknown type " ^ Int32.to_string x.it)
match
let dt = expand_deftype (Lib.List32.nth c.types.ctx x.it) in
comptype_of_desctype dt
with
| FuncT (ts1, ts2) -> ts1, ts2
| _ -> error x.at ("non-function type " ^ Int32.to_string x.it)
| exception Failure _ -> error x.at ("unknown type " ^ Int32.to_string x.it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! That makes sense. I thought I was losing my marbles. I'll go ahead and just revert that last change, then.

This reverts commit 0296c5e.
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

To be honest, given that almost all uses of this function will continue to need to convert on to comptype, I'm not sure that changing the signature of this function is the best approach. Rather, I'd introduce another function, say describe_deftype (or something more appropriate), that produces desctype and then redefine expand_deftype in terms of that. WDYT?

@tlively
Copy link
Member Author

tlively commented Oct 16, 2025

Yeah, that sounds much less intrusive. Maybe expand_deftype_as_desctype? But that's fairly verbose :/

@rossberg
Copy link
Member

Yeah... expand_deftype'? :)

@tlively
Copy link
Member Author

tlively commented Oct 17, 2025

I can't tell if that's a serious suggestion 😅

Anyway, whatever we call the new function, the change is tiny enough that I've gone ahead and included it as part of #68.

@tlively tlively closed this Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants