Docteur-Lalla / mlinterp

Interpreter of a dynamically typed OCaml subset for research purpose
BSD 2-Clause "Simplified" License
5 stars 1 forks source link

[junior job] print bindings as they are declared #1

Open gasche opened 5 years ago

gasche commented 5 years ago

(Context: this is an idea for an easy feature to add to mlinterp to anyone interested in playing with OCaml interpreters, not just the mlinterp maintainers.)

Currently mlinterp's interactive toplevel will display the result of expressions, but not of bindings added to the environment:

# 1 + 2;;
val 3 : int
# let x = 1 + 2;;
val () : sumtype

Compare to the OCaml toplevel's output:

# 1 + 2;;
- : int = 3
# let x = 1 + 2;;
val x : int = 3

The feature suggestion is to print val x = 3 : int or let x : int = 3 when given the input sentence let x = 1 + 2;;. This could be generalized to richer module items, but as a first step just handling let would be nice.

ulugbekna commented 5 years ago

A related issue: defining mutually recursive values returns a single-line generic result in mlinterp

# let a = 1 and b = 2;;
val () : sumtype

and two-line result in ocaml toplevel:

# let a = 1 and b =1;;
val a : int = 1
val b : int = 1
ulugbekna commented 5 years ago

I've been studying the codebase for two days now. Completing the task seems to entail a series of changes in the flow of how repl phrases are internally handled by the interpreter, especially if we aim to solve both issues.

Before making those changes, I thought if we first need to write tests to make sure that the changes do not change/break the interpreter's current working functionality.

gasche commented 5 years ago

A slightly imperfect/hackish but pragmatic way to do it would be as follows: line 59 of Main.ml, where toplevel phrases are evaluated, you have access of both ctx, the environment before the phrase is evaluated, and ctx', the environment after if it is evaluated. You could compute a difference between the two environments (as a new function exposed in Context.mli), and then just list the new bindings and their type.

ulugbekna commented 5 years ago

@gasche that's a really good idea, thank you! 😃 It was fast implement and fixed both issues. I will polish the code a little and make a pull request. Also, converting to dune was a very good idea; it made the dev process so pleasant.

I have two issues in which I could use some advice though:

1) I am concerned about the case when we redeclare the values (we get same old val (): sumtype). My solution, as of now, counts those vars same even if their values are different, but this can be fixed. However, it'll be hard to fix the case when both name and value of the previously declared and the redeclared values are same (let me put some thought into it, though). Do you think such a solution good to go, given I fix the different-value redeclared values?

2) I have several print_endlines throughout the code for personal debugging purposes. Do you think it would be okay to remove those after the code review?

3) I think the project does not use any indentation tool. What do you think about introducing ocp-indent if the author agrees, of course?

edit: added 3)