Fix -i broken by PR #8938 by gretay-js · Pull Request #9090 · ocaml/ocaml

gretay-js · 2019-11-05T12:27:31Z

With -i command line option, the compiler should stop after printing the interface, without generating any files. This behavior was broken by PR #8938, and this patch fixes it.

Octachron · 2019-11-08T10:20:08Z

I think that the fix belongs to the should_stop_after function:
the users should_stop_after Typing should not have to remember to check the print_types flags.

gretay-js · 2019-11-08T10:45:58Z

ok, thanks! fixed.

driver/compile_common.ml

Octachron · 2019-11-08T12:50:14Z

utils/clflags.ml

+  if pass = Compiler_pass.Typing && !print_types then true
+  else
+    match !stop_after with
+    | None -> false


With this implementation, -i has a higher priority than -stop-after.
A slightly different implementation would be to reverse the priority and let -i stop the compilation after typing only if stop_after was not set itself.

The way I think of it, ideally -i should be decomposable in several simpler options, and in particular it implies -stop-after typing. Now if two different -stop-after <pass> are passed on the command-line, I would expect to stop at the earliest pass specified. This means that if -i -stop-after <pass> is specified, I would expect to stop at the earliest pass of typing and <pass>. Does that make sense?

Currently, it's an error to pass two different -stop-after options.

ocaml/driver/main_args.ml

Lines 1834 to 1843 in a5e1216

let _stop_after pass =

let module P = Compiler_pass in

match P.of_string pass with

| None -> () (* this should not occur as we use Arg.Symbol *)

| Some pass ->

match !stop_after with

| None -> stop_after := (Some pass)

| Some p ->

if not (p = pass) then

fatal "Please specify at most one -stop-after <pass>."

Apropos #8938 (comment), remember that -i needs to decompose to three options (or there needs to be another micro-step before stop-after typing) to ensure that .cmi files are not written.

-i already has very high priority - it overrides both -c and -o (in the sense that ocamlopt -o foo foo.ml -i doesn't produce an executable or any other files).

I didn't remember this, thanks! This suggests that we could also disallow passing -i and -stop-after at the same time. (But then that would suggest making the useful side-effect of -i available with for example a -dtypes or -dsignature option.) But personally I think that the intersection behavior would also be sensible, and better than giving priority to one option or the other.

Meta-level point: I personally don't care very strongly about what -i does or does not, it's a fairly minor feature that is probably not worth spending too much time on it. I applaud your (@gretay-js) care to leave the details of the command-line options well-rounded, but it would also probably be fine to leave it as is, or even slightly changed.

This suggests that we could also disallow passing -i and -stop-after at the same time.

I considered it, but -i already silently ignores some other options, such as -c and -S, so it could also silently ignore -stop-after when pass is after Typing.

Yes, I think it would be fine that way.

Regarding the meta-level point, I agree with you in principle - the only problem is that there's an expectation that ocamlc -i foo.ml > foo.mli should give you a reasonable start for an .mli file (I can't remember if it's a valid one - i.e. if there are cases where it's not true). I don't know if any bad build systems have capitalised on that (and so might be broken by -i starting to produce other artefacts).

ocamlbuild has a foo.inferred.mli target that creates the file using this process, but it is only invoked manually as a user-provided target, and in any case I don't think anything would break if .cmi files were suddenly produced. (I don't know if any build systems is able to track precisely which files are produced as a byproduct of certain command-line flag choices, but neither ocamlbuild nor make try it.)

gretay-js · 2019-11-20T15:42:19Z

I think this was OK in principle and is ready to go, right?

gasche · 2019-11-20T15:51:44Z

Sorry, I had missed the fact that you did change the combined semantics of -i -stop-after foo as we discussed.

It may be a nitpick but I'm not sold on the implementation if pass = Compiler_pass.Typing && !print_types then true; the should_stop_after pass function has the property of being monotonic (if you should stop at a pass, you should also stop at later passes), and I intuitively think we should preserve it. So I would rather use Compiler_pass.(rank Typing <= rank pass).

…ts pass argument

gretay-js · 2019-11-20T17:32:32Z

the should_stop_after pass function has the property of being monotonic (if you should stop at a pass, you should also stop at later passes), and I intuitively think we should preserve it. So I would rather use Compiler_pass.(rank Typing <= rank pass).

Yes, it is safer to check the order of passes here explicitly. I updated the PR. Thank you!

gasche

I'm happy with the implementation.

Octachron · 2019-11-26T15:56:00Z

Merged, thanks for the fix!

ocaml#8938 from upstream trunk (cherry-pick 4353c75) ocaml#9090 from upstream trunk (cherry-pick fe7c8ed) ocaml#9097 from upstream trunk (cherry-pick 6daaf62) ocaml#8939 from gretay-js/save-linear (cherry-pick 1200de9) ocaml#9003 from gretay-js/start-from-emit (cherry-pick 21d5d26)

ocaml#8938 from upstream trunk (cherry-pick 4353c75) ocaml#9090 from upstream trunk (cherry-pick fe7c8ed) ocaml#9097 from upstream trunk (cherry-pick 6daaf62) ocaml#8939 from gretay-js/save-linear (cherry-pick 1200de9) ocaml#9003 from gretay-js/start-from-emit (cherry-pick 21d5d26) Add linear_format to dune file

ocaml#8938 from upstream trunk (cherry-pick 4353c75) ocaml#9090 from upstream trunk (cherry-pick fe7c8ed) ocaml#9097 from upstream trunk (cherry-pick 6daaf62) ocaml#8939 from gretay-js/save-linear (cherry-pick 1200de9) ocaml#9003 from gretay-js/start-from-emit (cherry-pick 21d5d26) Add linear_format to dune file Update .depend

Octachron reviewed Nov 8, 2019

View reviewed changes

driver/compile_common.ml Outdated Show resolved Hide resolved

Octachron reviewed Nov 8, 2019

View reviewed changes

gretay-js added 2 commits November 8, 2019 13:17

Fix -i broken by PR ocaml#8938

3fe58f9

Check print_types in should_stop after Typing

b52e812

gretay-js force-pushed the fix_i branch from 491d542 to b52e812 Compare November 8, 2019 13:27

Compare rank instead of equals makes should_stop_after monotonic in i…

ef77296

…ts pass argument

gasche approved these changes Nov 20, 2019

View reviewed changes

Octachron merged commit fe7c8ed into ocaml:trunk Nov 26, 2019

stedolan added a commit to janestreet/ocaml that referenced this pull request Mar 17, 2020

ocaml#9090 from upstream trunk (cherry-pick fe7c8ed)

b150755

gretay-js added a commit to gretay-js/ocaml that referenced this pull request Jun 4, 2020

ocaml#9090 from upstream trunk (cherry-pick fe7c8ed)

e46ccc0

	let _stop_after pass =
	let module P = Compiler_pass in
	match P.of_string pass with
	\| None -> () (* this should not occur as we use Arg.Symbol *)
	\| Some pass ->
	match !stop_after with
	\| None -> stop_after := (Some pass)
	\| Some p ->
	if not (p = pass) then
	fatal "Please specify at most one -stop-after <pass>."

Conversation

gretay-js commented Nov 5, 2019

Uh oh!

Octachron commented Nov 8, 2019

Uh oh!

gretay-js commented Nov 8, 2019

Uh oh!

Uh oh!

Octachron Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

gasche Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

gretay-js Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dra27 Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

gasche Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gretay-js Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

gasche Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

dra27 Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

gasche Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

gretay-js commented Nov 20, 2019

Uh oh!

gasche commented Nov 20, 2019

Uh oh!

gretay-js commented Nov 20, 2019

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Octachron commented Nov 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gretay-js Nov 8, 2019 •

edited

Loading

gasche Nov 8, 2019 •

edited

Loading