[CLI] Report errors for command line options that have no effect

[cameel]

Abstract

The compiler is pretty lax about command-line options whose values it does not use and just ignores many of them. It should report errors instead.

Over time we've started adding checks against this (e.g. #9075, #9364) they they're still very incomplete.

Motivation

Not getting an error when an option is invalid makes it harder for the user to realize that. It's especially problematic with options that seem to make sense together:

• --pretty-json sounds like it would affect --standard-json but it only works with --combined-json (Make --pretty-json work with Standard JSON output #11583.)
• --bin should be supported in the assembler mode but currently isn't ([CLI] Do not ignore the output selection in assembler mode #3870).

Specification

• There are several input modes (compiler, linker, assembler, standard JSON) and many options are not valid in one of these modes. The invalid ones should produce an error.
  • Take a look at tests for input modes in CommandLineParser. Basically, all options do not result in a change in CommandLineOptions in a specific mode should be invalid in that mode.
• Options that have equivalents and Standard JSON should be errors when --standard-json is used. For example:
  • Model checker settings.
  • Remappings.
  • --color/--no-color - Standard JSON output is not colorized and even when it will be (Support coloured messages in standard json #11507), this option should not affect it.
  • --libraries.
  • --error-codes.
• Please also check options whose values are not ignored by CommandLineParser but are still not used by CommandLineInterface.

Backwards Compatibility

This does not introduce any backwards-incompatible changes to the language itself but any command-line tools relying on the current behavior of options being ignored will be affected.

[Midhun07]

Hi @cameel I would like to work on this issue.
Also, Kindly point me to a good starting point. Thank you.

[cameel]

Great! There are many options so maybe it would be best to start with a PR that does this for only one of them and then, when you get the hang of it, continue with the rest in subsequent PRs. I think that --experimental-via-ir might be a good one to start with.

Start by running solc --help and looking at the option description and at the "Alternative Input Modes" section. These modes map to the InputMode enum in CommandLineParser. The only modes where the option makes sense are Compiler and CompilerWithASTImport. You can find that out by looking at solc/CommandLineInterface.cpp - there's only a single place in compile() that uses its value. In any other mode the option is ignored. The task is to modify CommandLineParser to return an error if the option is used in those other modes.

After you do this, tests in test/solc/CommandLineParser.cpp will start failing. Remove the option from them and instead add a new test case that loops over the modes where the option is invalid and checks the error message. You can use the existing no_options test case as a base.

Finally, don't forget to mention your change in the changelog.

If you need more help, drop by at the #solidity-dev Matrix/Gitter channel.

[Midhun07]

Thank you for the detailed explanation, It is really helpful. I will start with the said option.

[Midhun07]

@cameel I have made a pull request. #11853. Please check.

[cameel]

#11853 looks good overall. Here are some pointers on where to go from there once we merge it:

• CommandLineParser needs to have a list of forbidden options for each mode. There's already a stub of such a list for assembly mode. We need to complete that list and add a similar one for each mode.
  • We already have some existing checks for incompatible options (e.g. for --dialect and --machine outside of assembly mode but it's not the only one). You need to find them all and remove them. Please check CommandLineInterface.cpp as well - there might be some checks there.
• The error message in each case should be very similar. The only changing parts should be the names of all the options that are incompatible and the name of the input mode.
• We need a single test that loops over all modes and within each mode over all the incompatible options.

[Midhun07]

@cameel Thank you for merging the PR. I shall start with the other options. A couple of questions, Can I use the test case that I made in the previous PR for the rest of the options, maybe loop over each invalid option-input mode combination. Also, can I fix multiple option cases in a single PR.

[cameel]

Also, can I fix multiple option cases in a single PR.

Sure. I wanted a PR dealing with a single one mostly to work through potential issues on something small. Now I think we could do something bigger.

But I think it would be best to submit an incomplete PR as a draft for some feedback and add more stuff to it once we agree on the overall structure.

Can I use the test case that I made in the previous PR for the rest of the options, maybe loop over each invalid option-input mode combination.

Yeah, that's exactly what we need. That test should be refactored into something more generic. There are a lot of options and they should all be handled in a uniform way anyway so looping over them will save us a lot of code.

[Midhun07]

@cameel What should be the action if the user enters multiple invalid options for an input mode? Should it exit and return an error on the First invalid option or return errors for all invalid options? Please clarify.

[cameel]

I think it should report one error listing the names of all the invalid options and then exit.

[Midhun07]

@cameel So I assume when creating test cases we need to check for one invalid option with one input mode and multiple invalid options with one input mode separately. This would be tricky as there would be many combinations. I'm thinking of putting all invalid options for a particular input mode in one test loop. What do you think?

[cameel]

I think that checking all of the one invalid option with one input mode combinations would be enough. I think this will be enough to detect most errors and we'll never be able to test all possible combinations anyway. For the multiple invalid options with one input mode case you can test one or two such combinations but I'd also be fine with not testing them at all.

[Midhun07]

@cameel I have created fix for option error-recovery as a template for other options. Please check.

[cameel]

#11350 will soon be merged. When that happens, we need to remember to also validate its options (currently it does not take any so they all should be rejected as invalid).

[github-actions]

Hi everyone! This issue has been closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.