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

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

Merged
merged 4 commits into from Jan 15, 2017

Conversation

Projects
None yet
5 participants
@Octachron
Contributor

Octachron commented Jan 9, 2017

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.

Show outdated Hide outdated stdlib/arg.ml
@@ -264,6 +263,14 @@ let parse_and_expand_argv_dynamic_aux allow_expand current argv speclist anonfun
end;
done
let parse_and_expand_argv_dynamic_aux allow_expand current argv speclist anonfun

This comment has been minimized.

@gasche

gasche Jan 9, 2017

Member

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

@gasche

gasche Jan 9, 2017

Member

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

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Jan 9, 2017

Contributor

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.

Contributor

bschommer commented Jan 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Jan 9, 2017

Contributor

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.

Contributor

Octachron commented Jan 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Jan 9, 2017

Contributor

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).

Contributor

bschommer commented Jan 9, 2017

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).

@lefessan lefessan added the bug label Jan 9, 2017

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Jan 9, 2017

Contributor

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).

Contributor

Octachron commented Jan 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Leonidas-from-XIV

Leonidas-from-XIV Jan 9, 2017

@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.

Leonidas-from-XIV commented Jan 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Jan 9, 2017

Contributor

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.

Contributor

bschommer commented Jan 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 9, 2017

Member

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

Member

gasche commented Jan 9, 2017

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

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Jan 9, 2017

Contributor

@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.

Contributor

Octachron commented Jan 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 13, 2017

Member

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

Member

gasche commented Jan 13, 2017

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

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Jan 13, 2017

Contributor

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

Contributor

Octachron commented Jan 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Jan 13, 2017

Contributor

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.

Contributor

bschommer commented Jan 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 13, 2017

Member

@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.

Member

gasche commented Jan 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Jan 13, 2017

Contributor

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

Contributor

bschommer commented Jan 13, 2017

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

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Jan 13, 2017

Contributor

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

Contributor

Octachron commented Jan 13, 2017

@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).

Show outdated Hide outdated 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

This comment has been minimized.

@gasche

gasche Jan 13, 2017

Member

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

@gasche

gasche Jan 13, 2017

Member

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

This comment has been minimized.

@Octachron

Octachron Jan 13, 2017

Contributor

True. Fixed.

@Octachron

Octachron Jan 13, 2017

Contributor

True. Fixed.

Show outdated Hide outdated stdlib/arg.ml
in
let no_arg () =
match follow with
| None -> ()
| Some arg -> stop (Wrong (s, arg, "no argument")) in
| Some arg -> raise (Stop (Wrong (s, arg, "no argument"))) in

This comment has been minimized.

@gasche

gasche Jan 13, 2017

Member

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).

@gasche

gasche Jan 13, 2017

Member

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).

This comment has been minimized.

@Octachron

Octachron Jan 13, 2017

Contributor

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.

@Octachron

Octachron Jan 13, 2017

Contributor

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.

This comment has been minimized.

@gasche

gasche Jan 13, 2017

Member

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

@gasche

gasche Jan 13, 2017

Member

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

Show outdated Hide outdated 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

This comment has been minimized.

@gasche

gasche Jan 13, 2017

Member

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

@gasche

gasche Jan 13, 2017

Member

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

This comment has been minimized.

@Octachron

Octachron Jan 13, 2017

Contributor

Good point.

@Octachron

Octachron Jan 13, 2017

Contributor

Good point.

Octachron added some commits Jan 13, 2017

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Jan 13, 2017

Contributor

@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?

Contributor

Octachron commented Jan 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 13, 2017

Member

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

Member

gasche commented Jan 13, 2017

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Fix Stdlib.Arg: do no repeat usage_msg thrice (#999)
Fix Stdlib.Arg: do no repeat usage_msg thrice (GPR#999)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment