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

Arg module sometimes misbehaves instead of rejecting invalid -keyword=arg inputs #1923

Merged
merged 2 commits into from Jul 22, 2018

Conversation

Projects
None yet
2 participants
@sliquister
Copy link
Contributor

commented Jul 22, 2018

When -a is defined as Unit, accepted -a=1.
When -a is defined as Tuple [_; _], accepted -a=1 as -a 1 1.
When -a is defined as Rest, looped infinitely on -a=1.

Arg module sometimes misbehaved instead of rejecting invalid -keyword…
…=arg inputs

When -a is defined as Unit, accepted -a=1.
When -a is defined as Tuple, accepted -a=1 b c as -a b=1 c=1.
When -a is defined as Rest, looped infinitely on -a=1.
@gasche

This comment has been minimized.

Copy link
Member

commented Jul 22, 2018

This is a bit tricky. I am convinced by the Unit¹ and Tuple changes. I don't know how to reason about the Rest case, as it calls both no_arg() and consume_arg() in the same case -- of course, this is related to the evolution of current. In particular, all other calls to consume_arg() are preceded by a call to get_arg(), why is the one in Rest (that was already there) correct?

¹: the Unit change calls no_arg () before the user function, while all other actions are careful to call consume_arg () only after the user function (that may matter if it raises an exception), but no_arg() actually does not mutate the Arg state, it only performs a sanity-check, so this looks ok.

I would like to see two things to be fully reassured:

  • specifications (as comments) of what those *_arg() functions are doing, that includes {pre,post}conditions on current, so that we can reason about the correctness of user code.
  • tests in the testsuite, in particular regression tests for this problem, and tests for other nonsensical usages of the k=v form -- if those exists.
@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2018

The code for Rest is in principle an inlining of get_arg:

while
  match get_arg () with
  | exception Stop _ -> false
  | arg -> f arg; consume_arg (); true
do () done

but that's essentially as broken. Though this one could have worked, in a weird way: if follow was a ref, then consume_arg could have done:

let consume_arg () =
  match !follow with
  | None -> same as now
  | Some _ -> follow := None

and then I think the code would have treated --rest=1 as --rest 1, and --tuple=1 as --tuple 1.

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2018

I added tests, but I didn't add comments. it doesn't seem like any explanation is going to be simpler than the code. There is definitely no invariant on !current, as that can be modified by user code.

@gasche

gasche approved these changes Jul 22, 2018

Copy link
Member

left a comment

In the interest of not blocking for the unattainable perfect (yes, sometimes I know to say that), let's get it merged now. Thanks for the tests!

@gasche gasche merged commit b0ebe69 into ocaml:trunk Jul 22, 2018

2 checks passed

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

@sliquister sliquister deleted the sliquister:broken-arg branch Jul 22, 2018

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2018

Thanks.

Does it mess up github's notion of review if I rewrite commits? Because here, I would have squashed the two commits, but I don't know if it means reviewers now need to either rereview the whole thing (which is small here, but not always), or have to trust the author from stating accurately everything they changed after pushing the rewrite.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 22, 2018

No, I think github attaches review comments to diff in some clever way that makes it work. Sorry for not being very attentive wrt. the commit structure.

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.