Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new -stop-after-parse option: stop after the parsing phase #1945

Merged
merged 2 commits into from Sep 1, 2018

Conversation

Projects
None yet
4 participants
@gasche
Copy link
Member

commented Jul 30, 2018

This commit was part of the gigantic Menhir-parser PR (I need it for benchmarking purpose), but:

  • it is of independent interest
  • I got tired or rebasing it against the surprisingly-many changes to driver/
  • I expect some discussion on the option naming, so early debate is good

It sits on top of #1944 and serves to demonstrate why #1944 is required.

@gasche gasche force-pushed the gasche:stop-after-parse branch from f0b99b8 to 14ac840 Jul 30, 2018

@diml

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

Regarding the name of the option: what about -stop-after parse? This way, if in the future we want to add the possibility to stop after another phase, we don't need to add a new option.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

Yes; we had plans with @bobot to standardize pass options in this way (in particular, -i should be split into -dtypes -stop-after typing), but we never had the time to take a coherent look at it. I'm fine with -stop-after ARG as long as we accept to only support parse (or parsing, etc.) for now to, preferably, not overload the PR with unrelated concerns.

@diml

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

Agreed, I'm only suggesting a UI change

@gasche gasche force-pushed the gasche:stop-after-parse branch from 14ac840 to f0a6a10 Jul 31, 2018

@chambart

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

Would you consider producing an output ? It could be the marshalled ast similar to the output of camlp4.
Otherwise, this should be stated to be some kind of debug option.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2018

@chambart: the way I think of it, most passes of the compiler can produce some output (parsing can produce a serialized AST, typing can infer a .mli file, lambda or cmm outputs could be usefully serialized), and we should have separate flags to say "please make this specific pass produce a result" and "stop after this pass". -i says both things at once, at this is wrong, because it is useful to do one without the other (stop after typing to get warnings, produce .inferred.mli and keep compiling).

The present option says to stop after parsing, and we could have a different option, for example -dast, to produce the serialized AST as an output.

Note that the current flag is already useful (without -dast) for users who want to check that their file is syntactically correct, but nothing else.

@gasche gasche force-pushed the gasche:stop-after-parse branch from f0a6a10 to f08177d Aug 18, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2018

I just pushed a new commit that implements @diml's suggestion. I could not resist the temptation to generalize slightly, and the -stop-after option accepts either parsing or typing -- in the latter case, it behaves as -i, but without printing the interface.

@@ -376,6 +376,28 @@ let color = ref None ;; (* -color *)

let unboxed_types = ref false

(* This is used by the -stop-after option, which currently only supports
'parsing'. *)

This comment has been minimized.

Copy link
@diml

diml Aug 20, 2018

Member

This comment is no longer true

let mk_stop_after f =
"-stop-after", Arg.String f,
"<pass> Stop after the given compilation pass.\n\
\ Currently the only supported pass is 'parsing'."

This comment has been minimized.

Copy link
@xclerc

@gasche gasche force-pushed the gasche:stop-after-parse branch from f08177d to 9d8d0fa Aug 20, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

I fixed the two glitches that @diml and @xclerc reported. Thanks!

Note that I also need to edit the manpages and the manual with the new option. But before doing this I would like to get an approval in principle first, a sense of consensus (or at least the existence of the opinion) that the feature is desirable.

@diml

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Personally, this option seems good to me. I'm in favor of adding it.

@gasche gasche force-pushed the gasche:stop-after-parse branch from 9d8d0fa to c827ef0 Aug 21, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

Thanks! I updated the PR to include manpage and manual documentation.

.B \-safe\-string
Enforce the separation between types
.BR string \ and\ bytes ,
thereby making strings read-only. This is the default.
.TP
.B \-short\-paths
When a type is visible under several module-paths, use the shortest
one when printing the type's name in inferred interfaces and error and
one when printing the type'name in inferred interfaces and error and

This comment has been minimized.

Copy link
@xclerc

xclerc Aug 21, 2018

Contributor

This change looks suspicious.

This comment has been minimized.

Copy link
@gasche

gasche Aug 21, 2018

Author Member

Indeed... Fixed, thanks. (I had tried to get rid of single apostrophes in the text that Emacs groff-mode has trouble with, but I'm not sure what's the proper way to escape them so I gave up in between.)

@gasche gasche force-pushed the gasche:stop-after-parse branch from c827ef0 to a4a8970 Aug 21, 2018

fatal "Please specify at most one of -pack, -a, -c, -output-obj";
| Some (P.Parsing | P.Typing) ->
fatal "Options -i and -stop-after (parsing|typing)\

This comment has been minimized.

Copy link
@diml

diml Aug 21, 2018

Member

I suggest to use something like String.concat "|" P.names rather than hard-code the list in various places. This way things are more likely to stay in sync when we add more constructors to P.t.

This comment has been minimized.

Copy link
@gasche

gasche Aug 21, 2018

Author Member

We still have those present in the documentation, which is not procedurally generated. But your idea works well in combination with Arg.Symbol, will do, thanks.

@@ -83,6 +83,12 @@ let mk_dllpath f =
"<dir> Add <dir> to the run-time search path for shared libraries"
;;

let mk_stop_after f =
"-stop-after", Arg.String f,

This comment has been minimized.

Copy link
@diml

diml Aug 21, 2018

Member

We could also use Arg.Symbol (Compile_pass.names, f), this way we don't need to validate the input in f

@gasche gasche force-pushed the gasche:stop-after-parse branch from a4a8970 to d6ce7d1 Aug 22, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

@diml I changed the code as you suggested. A buildup commit factorizing the behavior on recoverable parsing errors in driver/compenv.ml snuck in as 4370d84.

@gasche gasche force-pushed the gasche:stop-after-parse branch from d6ce7d1 to f004914 Aug 22, 2018

let module P = Compiler_pass in
begin match P.of_string pass with
| None ->
let error msg = raise (Arg.Bad msg) in

This comment has been minimized.

Copy link
@diml

diml Aug 23, 2018

Member

Note that this code can never be reached now that we use Symbol, so we could effectively just use assert false

let mk_stop_after f =
"-stop-after", Arg.Symbol (Clflags.Compiler_pass.pass_names, f),
"<pass> Stop after the given compilation pass.\n\
\ ('parsing', 'typing')"

This comment has been minimized.

Copy link
@diml

diml Aug 23, 2018

Member

We can remove the listing in the doc string, the Arg module already generates it

let module P = Compiler_pass in
begin match P.parse pass with
| None ->
let error msg = raise (Arg.Bad msg) in

This comment has been minimized.

Copy link
@diml

diml Aug 23, 2018

Member

This is dead code as well

@diml

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

A buildup commit factorizing the behavior on recoverable parsing errors in driver/compenv.ml snuck in as 4370d84

ack

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

@diml I made the changes in a new commit (I will rebase later), but I'm having second thoughts about the use of Arg.Symbol here. My worry is that people may use -stop-after ... in their build script, and get a broken build on older OCaml versions that don't support the pass they use. With my previous implementation, we actually had a soft warning and the option was ignored in that case, which I believe is a safe failover.

@gasche gasche force-pushed the gasche:stop-after-parse branch from b24a4bd to d9bf119 Aug 25, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

@diml: I would be interested by getting this approved if you have time for a last bit of ping-pong, as this is a dependency of #292. Thanks a lot for the copious amount of feedback you have given so far.

@gasche gasche force-pushed the gasche:stop-after-parse branch from d9bf119 to ddfb31c Aug 27, 2018

@diml

diml approved these changes Aug 27, 2018

@diml

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

No problem. I see your point about the soft warning but personally I think a hard error is better: it is a bit more work if you want to support older versions of the compiler, but at least there are no surprises. For instance a script using -stop-after parsing might make the assumption that the compiler creates no file and execute it in a place where it is not allowed to create files.

@gasche gasche force-pushed the gasche:stop-after-parse branch from ddfb31c to 2949342 Aug 31, 2018

@gasche gasche merged commit 6e2891c into ocaml:trunk Sep 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.