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.

Sign up for GitHub

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

Jump to bottom

Possible argument parsing error #58

Closed
jk-1 opened this issue Apr 8, 2023 · 1 comment · Fixed by #66
Closed

Possible argument parsing error #58

jk-1 opened this issue Apr 8, 2023 · 1 comment · Fixed by #66
Milestone
0.5.0

Comments

@jk-1
Copy link

jk-1 commented Apr 8, 2023

I have simple experimental scala script using mainargs.

#!/usr/bin/env amm

import $ivy.`ch.qos.logback:logback-classic:1.2.3`
import $ivy.`ch.qos.logback:logback-core:1.2.3`
import $ivy.`ch.qos.logback:logback-access:1.2.3`
import $ivy.`org.slf4j:slf4j-api:1.7.25`
import $ivy.`com.lihaoyi::os-lib:0.2.5`
import $ivy.`com.lihaoyi::sourcecode:0.2.7`
import $ivy.`com.lihaoyi::mainargs:0.4.0`

import java.nio.file.Files
import org.slf4j.{Logger, LoggerFactory}
import ammonite.ops
import scala.util.Try
import java.io.File
import mainargs.{main, arg, ParserForMethods, ParserForClass, Flag}

val logConfFile = new File("/home/jk/bin/logback.xml")
System.setProperty("logback.configurationFile", logConfFile.getCanonicalPath)
val log = LoggerFactory.getLogger(s"${sourcecode.File()}")
if (logConfFile.exists() == false) log.error(s"Log config file not found: ${logConfFile.getCanonicalPath}")
else log.debug(s"Using logback config: ${logConfFile.getCanonicalPath}")


@main(doc = "Do foo." )
def foo(): Unit = {
  log.debug(s"${sourcecode.Name()}(${sourcecode.Args()}) running...")
  log.debug(s"END")
}

@main(doc = "Print stuff into file. " )
def template(
           @arg(short = 'p', doc = "This is p-flag. ") pflag: Flag,
           @arg(short = 'h', doc = "This is h-flag. ") hflag: Flag,
           @arg(short = 'j', doc = "This is j-flag. ") jflag: Flag,
           @arg(short = 'f', doc = "Output file name. ") file: String = null,
         ): Unit = {
  log.debug(s"${sourcecode.Name()}(${sourcecode.Args()}) running...")
  log.debug(s"pflag: ${pflag.value}")
  log.debug(s"hflag: ${hflag.value}")
  log.debug(s"jflag: ${jflag.value}")
  log.debug(s"file: ${if (file != null) file else "<NULL>"}")
  log.debug(s"END")
}

When I run this with different arguments I got following results:

jk@t04:~/bin$ expmainargs.sc template 
2023-04-08_23:28:12.232 [main] DEBUG e.sc - Using logback config: /home/jk/bin/logback.xml
2023-04-08_23:28:12.260 [main] DEBUG e.sc - template(List(List(Text(Flag(false),pflag), Text(Flag(false),hflag), Text(Flag(false),jflag), Text(null,file)))) running...
2023-04-08_23:28:12.260 [main] DEBUG e.sc - pflag: false
2023-04-08_23:28:12.260 [main] DEBUG e.sc - hflag: false
2023-04-08_23:28:12.260 [main] DEBUG e.sc - jflag: false
2023-04-08_23:28:12.260 [main] DEBUG e.sc - file: <NULL>
2023-04-08_23:28:12.260 [main] DEBUG e.sc - END

This is expected result.

jk@t04:~/bin$ expmainargs.sc template -p 
2023-04-08_23:28:32.578 [main] DEBUG e.sc - Using logback config: /home/jk/bin/logback.xml
2023-04-08_23:28:32.606 [main] DEBUG e.sc - template(List(List(Text(Flag(true),pflag), Text(Flag(false),hflag), Text(Flag(false),jflag), Text(null,file)))) running...
2023-04-08_23:28:32.606 [main] DEBUG e.sc - pflag: true
2023-04-08_23:28:32.606 [main] DEBUG e.sc - hflag: false
2023-04-08_23:28:32.606 [main] DEBUG e.sc - jflag: false
2023-04-08_23:28:32.606 [main] DEBUG e.sc - file: <NULL>
2023-04-08_23:28:32.607 [main] DEBUG e.sc - END

This is expected result.

jk@t04:~/bin$ expmainargs.sc template -p foo
2023-04-08_23:28:39.022 [main] DEBUG e.sc - Using logback config: /home/jk/bin/logback.xml
2023-04-08_23:28:39.050 [main] DEBUG e.sc - template(List(List(Text(Flag(true),pflag), Text(Flag(true),hflag), Text(Flag(false),jflag), Text(null,file)))) running...
2023-04-08_23:28:39.050 [main] DEBUG e.sc - pflag: true
2023-04-08_23:28:39.050 [main] DEBUG e.sc - hflag: true
2023-04-08_23:28:39.050 [main] DEBUG e.sc - jflag: false
2023-04-08_23:28:39.050 [main] DEBUG e.sc - file: <NULL>
2023-04-08_23:28:39.050 [main] DEBUG e.sc - END

This is unexpected result. Why does the argument value "foo" set the hflag to true and file is still null?
I intuitively thought that the results would have been: pflag = true, hflag = false, jflag = false, file = "foo" but for some reason this was not the case.

jk@t04:~/bin$ expmainargs.sc template -p foo bar
2023-04-08_23:28:49.660 [main] DEBUG e.sc - Using logback config: /home/jk/bin/logback.xml
2023-04-08_23:28:49.688 [main] DEBUG e.sc - template(List(List(Text(Flag(true),pflag), Text(Flag(true),hflag), Text(Flag(true),jflag), Text(null,file)))) running...
2023-04-08_23:28:49.689 [main] DEBUG e.sc - pflag: true
2023-04-08_23:28:49.689 [main] DEBUG e.sc - hflag: true
2023-04-08_23:28:49.689 [main] DEBUG e.sc - jflag: true
2023-04-08_23:28:49.689 [main] DEBUG e.sc - file: <NULL>
2023-04-08_23:28:49.689 [main] DEBUG e.sc - END

This is also unexpected result. Why does the argument values "foo" and "bar" set the hflag and jflag to true?
I intuitively thought that this command line would have been rejected because there were 2 string arguments "foo" and "bar".

jk@t04:~/bin$ expmainargs.sc template -p foo bar baf
2023-04-08_23:28:59.663 [main] DEBUG e.sc - Using logback config: /home/jk/bin/logback.xml
2023-04-08_23:28:59.691 [main] DEBUG e.sc - template(List(List(Text(Flag(true),pflag), Text(Flag(true),hflag), Text(Flag(true),jflag), Text(baf,file)))) running...
2023-04-08_23:28:59.691 [main] DEBUG e.sc - pflag: true
2023-04-08_23:28:59.692 [main] DEBUG e.sc - hflag: true
2023-04-08_23:28:59.692 [main] DEBUG e.sc - jflag: true
2023-04-08_23:28:59.692 [main] DEBUG e.sc - file: baf
2023-04-08_23:28:59.692 [main] DEBUG e.sc - END

This is again unexpected result. Why does the argument values "foo" and "bar" set the hflag and jflag to true?
I intuitively thought that this command line would have been rejected because there were 3 string arguments "foo", "bar" and "baf".

jk@t04:~/bin$ expmainargs.sc template -p foo bar baf blaa
2023-04-08_23:29:18.136 [main] DEBUG e.sc - Using logback config: /home/jk/bin/logback.xml
Unknown argument: "blaa"
Expected Signature: template
Print stuff into file.
  -p --pflag       This is p-flag.
  -h --hflag       This is h-flag.
  -j --jflag       This is j-flag.
  -f --file <str>  Output file name.

This is expected result, too many arguments.

Is this an error in mainargs code or something missing from the user documentation or what is the problem?

Used versions:

mainargs:0.4.0

scala -version
Scala code runner version 2.12.8 -- Copyright 2002-2018, LAMP/EPFL and Lightbend, Inc.

java -version
openjdk version "11.0.18" 2023-01-17
OpenJDK Runtime Environment (build 11.0.18+10-post-Ubuntu-0ubuntu120.04.1)
OpenJDK 64-Bit Server VM (build 11.0.18+10-post-Ubuntu-0ubuntu120.04.1, mixed mode, sharing)

amm
Loading...
Welcome to the Ammonite Repl 2.4.0 (Scala 2.12.13 Java 11.0.18)

Thank you for your support!
@lihaoyi
Copy link
Member

lihaoyi commented Apr 29, 2023

Can you minimize this further? If this is a mainargs issue, I'd like a repro without dozens of lines of irrelevant logback initialization code and file io handling

@lihaoyi lihaoyi mentioned this issue Apr 29, 2023
Merged
lihaoyi added a commit that referenced this issue Apr 29, 2023
@lihaoyi
Fix argument parsing of flags in the presence of allowPositional=true (…
a9e4c5e 
…#66)

Should fix #58 and
#60

Previously, we allowed any arg to take positional arguments if
`allowPositional = true` (which is the case for Ammonite and Mill
user-defined entrypoints.), even `mainargs.Flag`s. for which being
positional doesn't make sense.

```scala
    val positionalArgSigs = argSigs
      .filter {
        case x: ArgSig.Simple[_, _] if x.reader.noTokens => false
        case x: ArgSig.Simple[_, _] if x.positional => true
        case x => allowPositional
      }
```

The relevant code path was rewritten in
#62, but the buggy behavior
was preserved before and after that change. This wasn't caught in other
uses of `mainargs.Flag`, e.g. for Ammonite/Mill's own flags, because
those did not set `allowPositional=true`

This PR tweaks `TokenGrouping.groupArgs` to be more discerning about how
it selects positional, keyword, and missing arguments:

1. Now, only `TokenReader.Simple[_]`s with `.positional` or
`allowPositional` can be positional; `Flag`s, `Leftover`, etc. cannot

2. Keyword arguments are limited only to `Flag`s and `Simple` with
`!a.positional`

Added `mainargs.IssueTests.issue60` as a regression test, that fails on
main and passes on this PR. Existing tests all pass
@lefou lefou added this to the 0.5.0 milestone Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.0
Development

Successfully merging a pull request may close this issue.

Fix argument parsing of flags in the presence of allowPositional=true com-lihaoyi/mainargs
3 participants
@lihaoyi @lefou @jk-1