Fix Stdlib.Arg: do no repeat usage_msg thrice by Octachron · Pull Request #999 · ocaml/ocaml

Octachron · 2017-01-09T00:46:03Z

Currently, when reporting an error during the parsing of argument, the parsing functions in Arg repeat the usage_msg thrice (and the error message twice). For instance, invoking ocamllex -o yields

ocamllex: option '-o' needs an argument.
usage: ocamllex [options] sourcefile
  -ml  Output code that does not use the Lexing module built-in automata interpreter
  -o  <file>  Set output file name to <file>
  -q  Do not display informational messages
  -v  Print version and exit
  -version  Print version and exit
  -vnum  Print version number and exit
  -help  Display this list of options
  --help  Display this list of options
ocamllex: ocamllex: option '-o' needs an argument.
usage: ocamllex [options] sourcefile
  -ml  Output code that does not use the Lexing module built-in automata interpreter
  -o  <file>  Set output file name to <file>
  -q  Do not display informational messages
  -v  Print version and exit
  -version  Print version and exit
  -vnum  Print version number and exit
  -help  Display this list of options
  --help  Display this list of options
.
usage: ocamllex [options] sourcefile
  -ml  Output code that does not use the Lexing module built-in automata interpreter
  -o  <file>  Set output file name to <file>
  -q  Do not display informational messages
  -v  Print version and exit
  -version  Print version and exit
  -vnum  Print version number and exit
  -help  Display this list of options
  --help  Display this list of options

This PR fixes this problem by avoiding long-ranged side-effect when constructing error messages in Arg.parse_and_expand_argv_dynamic_aux. Similarly, this PR also suppresses the repetition of the program name by not prefixing already constructed error messages with the program name.
when reporting error.

gasche · 2017-01-09T03:06:55Z

stdlib/arg.ml

    end;
  done

+let parse_and_expand_argv_dynamic_aux allow_expand current argv speclist anonfun


Is this a shadowing definition? There are some many parse_ functions now that I think it would be best avoided for readability.

bschommer · 2017-01-09T07:41:34Z

To me that seems to be more of a problem of ocamllex. The behavior is the same for 4.04 which has the old functions.

Octachron · 2017-01-09T08:09:45Z

To me that seems to be more of a problem of ocamllex. The behavior is the same for 4.04 which has the >old functions.

I am sorry, I was unclear. The problem is in no way specific to ocamllex: ocamlc, ocamlopt, …, all programs using Arg are also affected: an example as simple as

Arg.parse ["-arg", String (fun _ -> ()), "arg"] ignore "Message"

is enough to trigger the problem, which was already here in 4.04.

bschommer · 2017-01-09T08:27:54Z

You are right, I just did not see it for ocamlopt since there are so many options. The problem is now only if there exist some other ocaml tool relying on the old behavior (I can't image one).

Octachron · 2017-01-09T10:34:19Z

I have updated the fix to avoid changing the behavior with error raised by user functions: the patch now simply avoids to convert internal Stop exception to external Bad/Help exception too early (which caused these exceptions to be recaught a second time as user function errors, which cumulated with the lack of buffer clearing resulted to the triple repetition of the usage message).

Leonidas-from-XIV · 2017-01-09T10:48:39Z

@bschommer

The problem is now only if there exist some other ocaml tool relying on the old behavior (I can't image one).

There should not be one, since 4.02.3 does not have this issue, it's a regression in 4.03.

bschommer · 2017-01-09T11:31:56Z

Ah okay, I only tried it with 4.04 and since I was pretty sure that there were no changes from 4.03 to 4.04 I thought it was no regression.

gasche · 2017-01-09T12:21:15Z

I believe d431b5b is the change that introduced the regression (I just bisected).

Octachron · 2017-01-09T12:31:10Z

@gasche d431b5b does introduce an "exception type" error by raising a Bad exception in get_arg rather than a Stop exception. I have tried to rename few key functions in the last version of the PR to avoid future misunderstandings.

gasche · 2017-01-13T18:15:40Z

Is this related to / does this affect MPR#7460?

Octachron · 2017-01-13T18:29:11Z

It sounds similar, but I would say no: MPR#7460 exception is raised from outside of Arg: https://github.com/ocaml/ocaml/blob/trunk/driver/main.ml#L132

bschommer · 2017-01-13T18:41:04Z

But it could have the same origin. The main changes to Arg were my adding of Expand and the PR which introduced this regression. Since both trunk and 4.04 are affected I think the other PR will be the culprit.

gasche · 2017-01-13T18:47:43Z

@Octachron : would you add a test to the testsuite for the behavior fixed in your PR? I think the two regressions show that we need more testing there.

bschommer · 2017-01-13T19:24:35Z

I found the source of MPR#7460 and opened a PR #1011 for it.

Octachron · 2017-01-13T20:38:43Z

@gasche : Done, I added a test for all major variations on error message raised in arg.ml .

gasche

I think your added tests are nice, but they only test one of the Arg.parse* functions and I'm not sure I fully trust that other will behave in the same way; I think it would be helpful (rather than trying to test all of these functions) to also have an integration test that directly invokes the ocamlc compiler from the command-line (since this is what most users will see, and where we should therefore be most careful about avoiding regressions).

gasche · 2017-01-13T21:01:40Z

stdlib/arg.ml

+       *or* add the program name as a prefix and the usage message as a suffix
+       to an user-raised Bad exception.
+    *)
+    let () = Buffer.clear b in


I think that declaring b to be local to this function, instead of carefully clearing it, would make the code simpler and more robust.

True. Fixed.

gasche · 2017-01-13T21:01:55Z

stdlib/arg.ml

        match follow with
        | None -> ()
-        | Some arg -> stop (Wrong (s, arg, "no argument")) in
+        | Some arg -> raise (Stop (Wrong (s, arg, "no argument"))) in


When reading the patch (without having tried to understand the code first) I see no particular reason why this raise site would not use convert_error, while the one above does. I think that this code could be restructured to be easier to understand. In particular, I would like all raises that do (respectively, don't) need the convert_error wrapping should be close together, instead of both categories being interleaved with no clear scoping. It would be even better, ideally, if the types could make it more clear that double-handling is impossible (for example by having the inputs of convert_error use a different error-description type than its output).

I have simplified the code by moving it around and fusionning the different try clause in the main while loop.
Conversion now happens only at one point.

Thanks, the new code makes much more sense to me.

gasche · 2017-01-13T21:02:19Z

testsuite/tests/lib-arg/testerror.ml

+  let argv = Array.of_list ("testerror" :: argv) in
+  try Arg.parse_argv ~current:(ref 0) argv spec anon usage with
+  | Arg.Bad s-> Printf.printf "(%d/%d) Bad:%s\n" (i+1) total s
+  | Arg.Help s -> Printf.printf "(%d/%d) Help:%s\n" (i+1) total s


I think that Help:\n%s\n (and similarly for Bad) would make the reference file easier to read.

Good point.

Octachron · 2017-01-13T22:09:36Z

@gasche All the parse functions are currently defined in term of parse_and_expand_argv_dynamic_aux, testing ocamlc would test just a different parameter.

I have nothing against an integration test for ocaml ({,c,opt} etc…) argument but is such test really the object of this PR?

gasche · 2017-01-13T22:10:29Z

Ok; I'm willing to merge as-is after we get CI returns.

Fix Stdlib.Arg: do no repeat usage_msg thrice (GPR#999)

* refactor package urls, routes, and handlers * move package redirects to redirection.ml

@cuihtlauac

* make package documentation link more obvious (ocaml#828) * make documentation link more obvious * minor style improvement Co-Authored-By: Sabine Schmaltz <[email protected]> * improve authors/maintainers display on package overview (ocaml#1001) * refactor package urls, routes, and handlers (ocaml#999) * refactor package urls, routes, and handlers * move package redirects to redirection.ml * Unify package overview and documentation layout (ocaml#1015) * unified package overview/docs layout Rearranges package overview and documentation such that: * package_layout.eml defines the two sidebars and the content area * there is a navigation element to switch between Overview (About) and Documentation (Docs) * there is a placeholder element for the upcoming in-package search (to remind us how important this feature is and to show users that we have a plan where it goes) Consequences: * package overview page now has a collapsing sidebar with a button to slide it in on small screens * package documentation now has a tablet (md) layout that shows the sidebar on-screen, instead of collapsed * add text-sm which was lost during unification * "Overview" instead of "About" since we have the space * Add support for sitemap.xml * Generate sitemap.xml by dream * Apply suggestions from @cuihtlauac code review Co-authored-by: Cuihtlauac Alvarado <[email protected]> * Convert lists of urls to sequences and change names of functions * Apply suggestions from @cuihtlauac code review 2 Co-authored-by: Cuihtlauac Alvarado <[email protected]> * Delete subdomains URLs and URLs returning status code other than 200 * Apply suggestions from @cuihtlauac code review 3 Co-authored-by: Cuihtlauac Alvarado <[email protected]> * Update PR * formating * do not touch playground asset * Apply suggestions from @cuihtlauac code review 4 * Apply suggestions from @cuihtlauac code review 5 * Apply suggestions from @cuihtlauac code review 6 * Apply suggestions from @cuihtlauac code review 7 * Apply suggestions from @cuihtlauac code review 8 --------- Co-authored-by: Sabine Schmaltz <[email protected]> Co-authored-by: sabine <[email protected]> Co-authored-by: Cuihtlauac Alvarado <[email protected]>

gasche reviewed Jan 9, 2017

View reviewed changes

lefessan added the bug label Jan 9, 2017

Octachron force-pushed the arg_stuttering branch 2 times, most recently from 2856cfc to 6d3e942 Compare January 9, 2017 10:27

Octachron force-pushed the arg_stuttering branch from 6d3e942 to 0890ae9 Compare January 9, 2017 12:19

copy mentioned this pull request Jan 9, 2017

Not passing a pid after -p causes usage to be printed multiple times copy/gdbprofiler#7

Closed

Fix stdlib arg stuttering

675bbfb

Octachron force-pushed the arg_stuttering branch from 0890ae9 to 675bbfb Compare January 13, 2017 20:33

gasche reviewed Jan 13, 2017

View reviewed changes

Octachron added 2 commits January 13, 2017 22:41

Improved test

94764d9

Reorganize arg.ml

02613c6

Changes for GPR#999

a18107f

gasche merged commit 48546a7 into ocaml:trunk Jan 15, 2017

pirbo mentioned this pull request Mar 31, 2017

Invalid command line options print help 3 times Kappa-Dev/KappaTools#382

Closed

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Fix Stdlib.Arg: do no repeat usage_msg thrice (ocaml#999)

5094932

Fix Stdlib.Arg: do no repeat usage_msg thrice (GPR#999)

This was referenced Mar 14, 2019

bug in Arg when an option does not have its argument #7523

Closed

Arg.parse prints error message multiple times #7578

Closed

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

refactor package urls, routes, and handlers (ocaml#999)

3160b61

* refactor package urls, routes, and handlers * move package redirects to redirection.ml

Conversation

Octachron commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bschommer commented Jan 9, 2017

Uh oh!

Octachron commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bschommer commented Jan 9, 2017

Uh oh!

Octachron commented Jan 9, 2017

Uh oh!

Leonidas-from-XIV commented Jan 9, 2017

Uh oh!

bschommer commented Jan 9, 2017

Uh oh!

gasche commented Jan 9, 2017

Uh oh!

Octachron commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasche commented Jan 13, 2017

Uh oh!

Octachron commented Jan 13, 2017

Uh oh!

bschommer commented Jan 13, 2017

Uh oh!

gasche commented Jan 13, 2017

Uh oh!

bschommer commented Jan 13, 2017

Uh oh!

Octachron commented Jan 13, 2017

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented Jan 13, 2017

Uh oh!

gasche commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Octachron commented Jan 9, 2017 •

edited

Loading

Octachron commented Jan 9, 2017 •

edited

Loading