brilirs: Add call support

[yati-sagade]

This is a simple recursive implementation of the call operation (both
in the value and effect contexts). I originally tried an iterative version,
but the borrow checker gave me a tough fight. This is much more readable
than tracking a stack of function call environments directly anyway.

[yati-sagade]

This fixes #100

[Pat-Lafon]

I'm kind of surprised that you don't need to do anything for the arguments of a function. Looking at the tests cases, does this assume that a variable you pass as an argument has the same name in the callee function as it did in the caller context?

[yati-sagade]

@Pat-Lafon good catch, this was working exactly why you note -- the test happened to use the same parameter name as the argument names at the callsite. I changed the param names in the test and sure enough the test failed. The latest commit fixes that. Thanks!

[Pat-Lafon]

@yati-sagade one other thing I was thinking about. The interesting thing about this implementation is that the code for all of the functions is in one large vec. I would imagine this would be faster than what brili and I attempted with having each function contain their vec of instructions. However, I was wondering what do you do if two functions use the same label name.

[yati-sagade]

You are right, that would break, labels seem to be function scoped:

@main {
  jmp .bar;
}
@foo {
  x: int = const 1;
.bar:
  print x;
}

Does not work with the reference interpreter. So the label index needs to be per function (i.e., moved into FuncInfo).

[Pat-Lafon]

bril/brilirs/src/interp.rs

Lines 345 to 353 in f7ca8bf

	let vals = get_values(vars, call_args.len(), call_args)?;
	Ok(
	func_info
	.args
	.iter()
	.map(|a| a.name.clone())
	.zip(vals.into_iter().cloned())
	.collect::<HashMap<String, Value>>(),
	)

@yati-sagade Should the arguments to a function call be type checked? I assume that if you pass an argument of the wrong type it will error the first time it is used though maybe I can come up with a weird program that could abuse this.

@foo(x:int) : bool { y : bool = not x; return y; }

Would this work if I called it as foo(true)?

[yati-sagade]

Good call to type-check args, added that.

I think your suggestion of keeping a map {func_name => func_blocks} makes the code much simpler, and I would be surprised if there was a perf hit we'd care about. Given what bril is for, I think we should weigh simplicity heavier than performance -- I've changed the code like so. I also got rid of the cfg module, and am now always computing the CFG, since we need that for execution anyway.

I realized that it would be nice to unify the error messages emitted by reference interpreter and this one at some point.

[Pat-Lafon]

I think your suggestion of keeping a map {func_name => func_blocks} makes the code much simpler, and I would be surprised if there was a perf hit we'd care about. Given what bril is for, I think we should weigh simplicity heavier than performance -- I've changed the code like so. I also got rid of the cfg module, and am now always computing the CFG, since we need that for execution anyway.

Design wise, it seems this has converged with what I did in #104. That should make it easy to rebase off of these changes if/when they get merged in.

I realized that it would be nice to unify the error messages emitted by reference interpreter and this one at some point.

The test cases for brili errors only check that it returns an error code of 2 though we could implement Display to get similar messages. I've been thinking about how to tackle #52 since that would help with usability a great deal.

[sampsyo]

Lovely! Thanks for the implementation, @yati-sagade, and for the robust discussion, @Pat-Lafon. I like the simpler setup here where functions get their own namespaces, which is certainly truer to the original representation and less likely to cause weird problems than the "flat" version. Wahoo!

As for error messages: it's a good point. It could make differential testing easier. However, I don't think we need to worry about it too much… I have written the original TypeScript brili to catch & report as many errors as possible, but I don't think it's necessary for every interpreter to be quite so thorough. That is, it is allowed for implementations to do weird/unexpected things on ill-formed input, and it's infeasible to catch every error, so it's OK to just totally break if something particularly weird happens. Anyway, happy to review a PR that carries out @Pat-Lafon's vision of adding Display for errors if anyone's interested!