Generalize main annotations

[timotheeandres]

Work in progress

Working on abstracting @main annotations in Scala3

[nicolasstucki]

Suggested change

	Select(instanciateAnnotation(mainAnnot), defn.MainAnnot_command.name),
	Select(instanciateAnnotation(mainAnnot), nme.main),

[timotheeandres]

That won't work, nme.main is "main" whereas defn.MainAnnot_command is "command"

[nicolasstucki]

Then we should add command to StdNames.

[nicolasstucki]

Suggested change

	val run = Apply(Select(Ident(cmdName), defn.MainAnnotCommand_run.name), mainCall)
	val run = Apply(Select(Ident(cmdName), nme.run), mainCall)

[timotheeandres]

Important points from LAMP meeting, 03/02/2022:

• rename @main.Name to @main.Alias or @main.AliasName
• merge @main.Name and @main.ShortName
• implement exit codes for parsing errors
• allow --help and -h to display help, but if a parameter is called "help" or if one of the aliases is "h", do not use them for help display
• remove max width argument in @main
• prepare post for Scala Contributors forum to ask about argument passing rules
  • from Aleksander's email:

    So I think that actually, the precedent is clear:
    1) We should allow mixing flags and positional arguments
    2) In order to allow reliably passing strings that start with - as positional arguments, we need to support -- and we need to allow positional arguments to come after it
    3) B/c of the above, if we want to maintain backwards compatibility, we should support sequence and boolean flags from the start. We should also do that from the start, since these are the most commonplace.
    Reg. sequence/multivalue flags, possibly the most commonplace way to support them is by repeating the flag (like in ssh -o $OPT). If an optional argument is a Seq, we should allow repeating it.
    This raises the question of what should be done if a positional argument is a Seq. My first intuition would be to just disallow it. It should be always better for that argument to either be a vararg, or a repeated flag.
    Reg. boolean flags, we should probably negate the default value when they are passed explicitly.

[smarter]

Continuing the discussion from @abgruszecki's mail here:

Reg. boolean flags, we should probably negate the default value when they are passed explicitly.

That seems confusing. A convention I've seen in some cli programs like ripgrep (https://github.com/BurntSushi/ripgrep) is that for every boolean flag --foo there is a complimentary flag --no-foo, and if a combination of these flags is passed, the last one win (this is very convenient since it lets you define an alias with some flags but then override them on the command line).

[som-snytt]

s/complimentary/complementary

There's another thread about making sure option processing is uniform. Consensus isn't easy.

[abgruszecki]

Continuing the discussion from @abgruszecki's mail here:

Reg. boolean flags, we should probably negate the default value when they are passed explicitly.

That seems confusing. A convention I've seen in some cli programs like ripgrep (https://github.com/BurntSushi/ripgrep) is that for every boolean flag --foo there is a complimentary flag --no-foo, and if a combination of these flags is passed, the last one win (this is very convenient since it lets you define an alias with some flags but then override them on the command line).

Hm, these seem like orthogonal problems. What I mean is only that:

1. All arguments that can be passed as a flag must have a default value
2. Therefore, if the argument is a boolean, then having the boolean flag passed means that we call the main function with the reverse of the default value and not a hardcoded true/false value.

In general, if an argument is not repeated, then what I would expect is that if I pass multiple instances of it, the last one wins, no matter if the flag is boolean or not. We can do that later on though, if in the first version passing a flag multiple times will be an error (which is now the case, IIRC).

[timotheeandres]

• implement exit codes for parsing errors

There is a slight issue with this: as much as I think it could provide something good for scripting, it makes testing significantly harder, as the test harness does not seem to have a nice way of handling return codes. Should I implement it anyway, and remove tests that expect an error to occur? Or leave it for the experimental version to implement?

[abgruszecki]

• implement exit codes for parsing errors

There is a slight issue with this: as much as I think it could provide something good for scripting, it makes testing significantly harder, as the test harness does not seem to have a nice way of handling return codes. Should I implement it anyway, and remove tests that expect an error to occur? Or leave it for the experimental version to implement?

Wait, what is the exact problem? Is it difficult to check the exact error code returned, or is the test harness not detecting that some non-zero exit codes are an error? The latter is a bug to be fixed; if it's the earlier, I think not having tests that check for the exact error code returned is not great, but acceptable.

[smarter]

If you wish to test the exact exit code you could write a test in the following shell script which is part of our CI: https://github.com/lampepfl/dotty/blob/main/project/scripts/bootstrapCmdTests

Therefore, if the argument is a boolean, then having the boolean flag passed means that we call the main function with the reverse of the default value and not a hardcoded true/false value.

That's the part I find confusing, imo if we have:

@main def run(color: Boolean = true, verbose: Boolean = false) = ...

Then we should generate four flags: --color, --no-color, --verbose, --no-verbose, with the default equivalent to passing --color --no-verbose.

[abgruszecki]

Therefore, if the argument is a boolean, then having the boolean flag passed means that we call the main function with the reverse of the default value and not a hardcoded true/false value.

That's the part I find confusing, imo if we have:

@main def run(color: Boolean = true, verbose: Boolean = false) = ...

Then we should generate four flags: --color, --no-color, --verbose, --no-verbose, with the default equivalent to passing --color --no-verbose.

Ah, that's just simpler than what I had in mind, I agree this is the better way.

[nicolasstucki]

This name is redundant because we already have it in parameterInfoss. I would pass the parameter index to argGetter instead. This way we can access the name with parameterInfoss[idx].name but also can access any of the other ParameterInfos in a simple way.

Suggested change

	def argGetter[T](name: String, optDefaultGetter: Option[() => T])(using fromString: ArgumentParser[T]): () => T
	def argGetter[T](idx: Int, optDefaultGetter: Option[() => T])(using fromString: ArgumentParser[T]): () => T

[nicolasstucki]

The double s in parameterInfoss is a bit confusing because we usually use it for List[List[T]]. I would propose dropping the second s from parameterInfoss and the s of ParameterInfos

Suggested change

	def command(args: Array[String], commandName: String, documentation: String, parameterInfoss: MainAnnotation.ParameterInfos*): MainAnnotation.Command[ArgumentParser, MainResultType]
	def command(args: Array[String], commandName: String, documentation: String, parameterInfos: MainAnnotation.ParameterInfo*): MainAnnotation.Command[ArgumentParser, MainResultType]

[nicolasstucki]

If you wish to test the exact exit code you could write a test in the following shell script which is part of our CI: https://github.com/lampepfl/dotty/blob/main/project/scripts/bootstrapCmdTests

Actually, it should be in https://github.com/lampepfl/dotty/blob/main/project/scripts/cmdTests but we need the fix of #14428 to make the CI run it on PRs. We will need to rebase on main once #14428 is fixed.

[nicolasstucki]

Extracted MainAnnotation in #14558.

[nicolasstucki]

Replaced with #14558 and #14841