Implement debug expressions #684

ggreif · 2019-09-24T13:14:46Z

Implements #683.

fixes #683 see also ACT-118

it sadly dies there...

this cries for implicit variables Next: look into - functorising for release being true/false (sode size duplication?) - using a function reading a mutable cell both bring their own ugliness, but what is the best?

ggreif

Just a few things to be aware of.

src/lowering/desugar.ml

src/pipeline/pipeline.ml

src/exes/asc.ml

rossberg

Frontend stuff LGTM, leaving backend to Joachim.

ggreif · 2019-09-24T18:18:13Z

I was looking for a blog post that I read about the functor configuration pattern in OCaml, but can't seem to find it. This comes close: https://discuss.ocaml.org/t/is-idomatic-in-ocaml-to-take-configuration-and-return-a-module-that-uses-it/2628

Maybe a first class module is even easier: https://v1.realworldocaml.org/v1/en/html/first-class-modules.html

This reverts commit 62210a5.

matthewhammer · 2019-09-24T21:41:19Z

src/as_frontend/typing.ml

@@ -779,6 +779,9 @@ and infer_exp'' env exp : T.typ =
    let t = check_typ env typ in
    if not env.pre then check_exp (add_lab env id.it t) t exp1;
    t
+  | DebugE exp1 ->
+    if not env.pre then check_exp env T.unit exp1;
+    T.unit


I guess we didn't discuss the typing rule(s) for debug blocks.

It makes sense to insist that they return unit type.

A more permissive approach, which I think we should skip for now, would permit each debug block to have a return type that becomes optional via the debug block (the debug block would introduce the optional type around the block expression's type), where the debug-on and debug-off cases correspond with returning the some (?_) value and returning the none (null) value, respectively.

An argument in favor (and also against!) this optional typing rule is that it permits a program to dynamically know the debug status within the program's dynamic logic, should that ever be needed for conditional logic that can't be simply turned on/off via conditional compilation. I don't have concrete use cases in mind, admittedly. It also seems like it may be a little too powerful for the common case.

We discuss the typing in #683. You can check the debug status by setting a mutable variable from within the debug block. Isn't that enough?

Ah, thanks for the pointer; I'll copy the comment above there to continue.

[After writing the comment further up, I thought of a use case and have developed a stronger preference toward optional-valued debug blocks over this version. #683 has that discussion.]

ggreif · 2019-09-24T21:51:31Z

@rossberg @crusso @nomeata @matthewhammer I have adapted the "configuration by functor" idea. Still not sure if this is the best way. PTAL.

adds `-r` flag to run.sh and a test/run-release directory where the release tests live

nomeata

No backend changes to speak of, so happy with this.

src/as_interpreter/interpret.ml

src/pipeline/pipeline.ml

rossberg · 2019-09-25T07:47:42Z

src/pipeline/pipeline.ml


 (* Interpretation (Source) *)

 let interpret_prog denv prog : (Value.value * Interpret.scope) option =
  let open Interpret in
  phase "Interpreting" prog.Source.note;
  let flags = { trace = !Flags.trace; print_depth = !Flags.print_depth } in
-  let result = Interpret.interpret_prog flags denv prog in
+  let module Interpreter = MakeInterpreter(val conf ()) in


Why (repeatedly) instantiate this locally and not simply up where you define the conf argument?

Trying that next. My gut feeling is that it'll evaluate let release = !Flags.release_mode too early. I'll see.

See commit 862f40b, which fails the tests. The commit message contains a possible way forward. Is this what you are hinting at @rossberg?

I see, doing it eagerly is reading the flag too early. I think the mix of state and parameterisation as the PR now does isn't a healthy mix. If you want to parameterise, then you'll need to do so on all levels, such that you can get rid of the global state. That is, the main module, after configuring the flags, has to instantiate the pipeline etc., e.g., as a functor. That's possible but somewhat tedious and intrusive, which is why the preference was resorting to state, as with existing flags.

I suggest sticking to state for this PR and leaving a broader configuration refactoring to a later PR (that we can do when there is an urgent enough need for change).

Yep, the weight-to-gain ratio is pretty bad. I have committed (e6f5e69) a workable solution wich would still need factoring (library as_config out). Anyway I'll back out all the functor mumbo-jumbo, and we can bring it back any time from this PR. I'll simply use bool refs for this purpose.

This is now implemented in d551c87 (and a small follow-up). PTAL.

src/pipeline/pipeline.ml

Still to do: - avoid repeated `module type Conf`( in 4 places now) - avoide repeated `struct`

Causes regression due to premature evaluation of !Flags.release_mode. This commit will be reverted unless the following remedy is applied: make `release` in the parameter signature a function `unit -> bool`. This swould mean that - the parameter gets more weight (being passed around recursively) - churn in the many copies of `Conf` signatures The latter has a cure, by moving it into an `as_config` library.

Removes the fancy functor stuff for now.

…rint

ggreif · 2019-09-25T18:01:41Z

@rossberg ok to go in?

…rint

rossberg · 2019-09-26T13:47:56Z

src/exes/asc.ml

+  Arg.Unit
+    (fun () ->
+      As_interpreter.Interpret.release_mode := true;
+      Lowering.Desugar.release_mode := true),


Hm, it doesn't seem good to duplicate the state. Can't we have just one? Pipeline.Flags seems like the obvious place for now.

Well, lowering or as_interpreter can’t depend on pipeline.

That’s why I argued for a dedicated flags or config library earlier.

Yeah, now that we have subdirs, why not simply pull out pipeline/Pipeline.Flags into pipeline/Flags?

Totally agree (that redundant state is suboptimal), but how will we distribute the central state? By passing the value to the interpreter and desugarer? Then what?

in the respective routines pass down the flag recursively. This is what I did in the very first implementation for the desugarer, and it is highly tedious. I did not repeat the same exercise for the interpreter, as I expected even more ugliness.

we could also set module local state based on that input to the bottleneck routines and use that confined flag locally.

I chose with the current implementation the same pragmatics as the backend uses for selection of the target embedder (compile_mode). There only one reference cell exists, which is directly set from the main program asc.ml. It is not orchestrated by pipeline.ml. The situation here is only different in the regard that the same information is needed by two modules.

Do you suggest to implement one to the above two avenues? I have I overlooked another possibility. To me none of the two seem to bring particular benefits.

All I'm saying is: make Pipeline.Flags a separate module instead of a nested one and put the flag in there.

@ggreif, sorry, I don't follow. AFAICS, there should not be any need for introducing a new dir or a new library, just add a new module.

@rossberg We are dealing with dune-managed entities here. There are library dependencies. pipeline lib depends on lowering lib (as stated in its dune file). dune (the build tool) won't let you grab a library's module unless you depend on it. E.g. for lowering to use library pipeline's module Flags there would be a circular library dependency. Which is forbidden in dune. @nomeata please correct me if I'm wrong (or just dense).

Oh, I see, you are saying that this would make the dune libraries recursive. Sorry, I misread what you and Joachim were saying.

Okay, in that case, yeah, we'd need a new dir, or perhaps the simplest thing is to create a new flags module in as_def?

Please look at my recent commit, and keep bike shedding :-) Of course we can have Flags module in as_def. I am pretty much neutral on this one.

Actually, looking at as_def it is very much just the AST of terms, so flags would be rather alien in there, as they also have codegen/execution aspects.

and thius eliminate redundantly duplicated state `release_mode`

…rint

This reverts commit 20f40d8.

ggreif · 2019-09-27T19:10:11Z

Commit 20f40d8 will be part of a different PR. It slipped in unintentionally and has been reverted.

ggreif added 5 commits September 24, 2019 15:07

implement debug blocks

0334fed

fixes #683 see also ACT-118

accept output

d24446c

fixup px

1c5673f

WIP: define the release flag, and pass it to desugar

1c2df49

it sadly dies there...

WIP: walk the valley of tears passing around the release mode

62210a5

this cries for implicit variables Next: look into - functorising for release being true/false (sode size duplication?) - using a function reading a mutable cell both bring their own ugliness, but what is the best?

ggreif commented Sep 24, 2019

View reviewed changes

src/lowering/desugar.ml Outdated Show resolved Hide resolved

src/pipeline/pipeline.ml Outdated Show resolved Hide resolved

src/exes/asc.ml Outdated Show resolved Hide resolved

src/exes/asc.ml Outdated Show resolved Hide resolved

rossberg reviewed Sep 24, 2019

View reviewed changes

ggreif added 5 commits September 24, 2019 20:31

Revert "WIP: walk the valley of tears passing around the release mode"

a926ac4

This reverts commit 62210a5.

WIP: testing first class module, but this is still statically created

5de5ac5

WIP: this seems to work

12d787e

WIP: simplify

aa039cd

WIP: functorially configure the interpreter too

519b707

matthewhammer reviewed Sep 24, 2019

View reviewed changes

ggreif added 2 commits September 24, 2019 23:43

comment only

5b2e183

fix the flag

21b8cb1

ggreif added 2 commits September 25, 2019 00:41

tests for release mode

246093f

adds `-r` flag to run.sh and a test/run-release directory where the release tests live

run the run-release tests

36af741

nomeata approved these changes Sep 25, 2019

View reviewed changes

rossberg reviewed Sep 25, 2019

View reviewed changes

ggreif added 4 commits September 25, 2019 13:05

Avoid first-class modules.

098d1c3

Still to do: - avoid repeated `module type Conf`( in 4 places now) - avoide repeated `struct`

spell out that --debug is the default

7e2417e

lost whitespace

6f86f9c

ggreif changed the title ~~Gabor/conditional print~~ Implement debug expressions Sep 25, 2019

ggreif added 2 commits September 25, 2019 16:19

WIP: make release a (side-effecting) function

8011b17

WIP: Simplify a bit more

e6f5e69

matthewhammer mentioned this pull request Sep 25, 2019

Specify debug blocks: ACT-118 #683

Closed

implement low-tech solution, state is distributed but updated centrally

d551c87

Removes the fancy functor stuff for now.

remove unnecessary data

d7ed2eb

ggreif force-pushed the gabor/conditional-print branch from 73275f7 to d7ed2eb Compare September 25, 2019 16:55

Merge remote-tracking branch 'origin/master' into gabor/conditional-p…

22b6746

…rint

ggreif marked this pull request as ready for review September 25, 2019 18:00

Merge remote-tracking branch 'origin/master' into gabor/conditional-p…

0f18ef4

…rint

ggreif requested a review from rossberg September 26, 2019 11:26

rossberg reviewed Sep 26, 2019

View reviewed changes

ggreif added 3 commits September 26, 2019 18:49

factor out flags into library as_config

02be9ed

and thius eliminate redundantly duplicated state `release_mode`

clean up

81995fd

consolidate flags

20f40d8

ggreif requested a review from rossberg September 27, 2019 10:48

rossberg approved these changes Sep 27, 2019

View reviewed changes

ggreif added 2 commits September 27, 2019 20:40

Merge remote-tracking branch 'origin/master' into gabor/conditional-p…

2d92569

…rint

Revert "consolidate flags"

3e22154

This reverts commit 20f40d8.

ggreif merged commit 97a77eb into master Sep 27, 2019

ggreif mentioned this pull request Sep 27, 2019

Consolidate flags from codegen into as_config #692

Merged

ggreif deleted the gabor/conditional-print branch September 30, 2019 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement debug expressions #684

Implement debug expressions #684

ggreif commented Sep 24, 2019 •

edited

Loading

ggreif left a comment

rossberg left a comment

ggreif commented Sep 24, 2019

matthewhammer Sep 24, 2019

ggreif Sep 24, 2019

matthewhammer Sep 25, 2019

matthewhammer Sep 25, 2019

ggreif commented Sep 24, 2019

nomeata left a comment

rossberg Sep 25, 2019

ggreif Sep 25, 2019

ggreif Sep 25, 2019

rossberg Sep 25, 2019 •

edited

Loading

ggreif Sep 25, 2019 •

edited

Loading

ggreif Sep 25, 2019

ggreif commented Sep 25, 2019

rossberg Sep 26, 2019

nomeata Sep 26, 2019

rossberg Sep 26, 2019

ggreif Sep 26, 2019 •

edited

Loading

rossberg Sep 26, 2019

rossberg Sep 26, 2019

ggreif Sep 26, 2019

rossberg Sep 26, 2019

ggreif Sep 26, 2019

ggreif Sep 27, 2019 •

edited

Loading

ggreif commented Sep 27, 2019

Implement debug expressions #684

Implement debug expressions #684

Conversation

ggreif commented Sep 24, 2019 • edited Loading

ggreif left a comment

Choose a reason for hiding this comment

rossberg left a comment

Choose a reason for hiding this comment

ggreif commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreif commented Sep 24, 2019

nomeata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossberg Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

ggreif Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreif commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreif Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreif Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

ggreif commented Sep 27, 2019

ggreif commented Sep 24, 2019 •

edited

Loading

rossberg Sep 25, 2019 •

edited

Loading

ggreif Sep 25, 2019 •

edited

Loading

ggreif Sep 26, 2019 •

edited

Loading

ggreif Sep 27, 2019 •

edited

Loading