Extend BazelFlagInfo with additional metadata

[vogelsgesang]

This commit adds additional metadata to the protobuf dump produced by bazel help flag-as-proto.

The old name and the deprecation message are useful to provide diagnostics within bazelrc files using a language server.
The type_converter and enum_values are usful to validate argument values and to provide auto-completion.
The option expansions can be used, e.g., to warn on contradictions between explicitly specificed flags and expansion flags.

Fixes #25006

[vogelsgesang]

should we model this as a oneof and essentially mirror the complete Converter hierarchy from Java also in Protobuf?

Or should we rather use something like

/// The class name of the converter as returned by `converter.getClass().getName()`
string converter_class_name = 14;
/// For enum values: the list of enum values
repeated string values = 15;
/// For range-converters: the minimum value
repeated string range_min = 16;
/// For range-converters: the maximum value
repeated string range_min = 17;

[vogelsgesang]

This should be ready for review now.

@tempoz does this change also fulfill your requirements from #25006?

Note that you proposed

a map field listing the flags/values an expansion argument expands to (e.g. { "test_arg": "--wrapper_script_flag=--debug" "test_output": "streamed", "test_strategy": "exclusive", "test_timeout": "9999", "nocache_test_results": "1" }

while this commit is only adding a list instead of a map of expansion flag

[tempoz]

I guess a map is infeasible because an expansion flag with more than one of the same multi-value option can't be represented? This seems fine; I was just hoping to not need to do the (admittedly minimal) parsing necessary to partition the option name from the option value, but that's not a practical problem so much as an aesthetic one.

EDIT: I guess I'd vaguely prefer a repeated field of ExpansionOption or similar that has Name and Value fields.

[vogelsgesang]

EDIT: I guess I'd vaguely prefer a repeated field of ExpansionOption or similar that has Name and Value fields.

I quickly looked at how this could be achieved but couldn't find any utilities which would allow me to do so.

Note that expansions are specified as a single string containing both names and values as can be seen e.g. here. I couldn't find any readily reusable part of the command line parsing logic to reuse for parsing those expansions. If I missed it, I would appreciate a pointer to the relevant source code :)

[tempoz]

I quickly looked at how this could be achieved but couldn't find any utilities which would allow me to do so.

Yeah, that makes sense; the parsing code is pretty, ah, complex. I think what you (or perhaps more accurately, I) want is something like:

import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.ParsedOptionDescription;

...

  ImmutableList<Class<? extends OptionsBase>> startupOptionsClasses =
    BlazeCommandUtils.getStartupOptions(runtime.getBlazeModules());

  ImmutableSet<Class<? extends OptionsBase>> commonOptionsClasses =
    BlazeCommandUtils.getCommonOptions(runtime.getBlazeModules());

  BiConsumer<String, OptionDefinition> visitor =
    (commandName, option) -> {
      if (ImmutableSet.copyOf(option.getOptionMetadataTags())
        .contains(OptionMetadataTag.INTERNAL)) {
          return;
        }
        Iterable<Class<? extends OptionsBase>> optionsClasses;
        boolean allowResidue;
        if ("startup" == commandName) {
          optionClasses = startupOptionClasses;
          allowResidue = false;
        } else {
          optionClasses = commonOptionsClasses;
          allowResidue = true;
        }
        BazelFlagsProto.FlagInfo.Builder info =
          flags.computeIfAbsent(option.getOptionName(), key -> createFlagInfo(option, optionsClasses, allowResidue));
        info.addCommands(commandName);
      };

...

private static BazelFlagsProto.FlagInfo.Builder createFlagInfo(OptionDefinition option, Iterable<? extends Class<? extends OptionsBase>> optionsClasses, boolean allowResidue) {

...

    if (option.getOptionExpansion().length > 0) {
      OptionsParser parser =
        OptionsParser.builder().optionsClasses(optionsClasses).allowResidue(allowResidue).build();
      parser.parse(option.getOptionExpansion());
      for (ParsedOptionDescription po : parser.asCompleteListOfParsedOptions()) {
        BazelFlagsProto.ExpansionOption.Builder optionBuilder = BazelFlagsProto.ExpansionOption.newBuilder();
        optionBuilder.setName(po.getOptionDefinition().getOptionName());
        optionBuilder.setValue(po.getUnconvertedValue());
        flagBuilder.AddExpansionOption(optionBuilder.build());
      }
      flagBuilder.AddAllExpansionResidues(parser.getResidue());
    }

I mainly based this on the code found here:

bazel/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java

Lines 1219 to 1225 in 7d673a1

	ImmutableList<Class<? extends OptionsBase>> optionClasses =
	BlazeCommandUtils.getStartupOptions(modules);
	// First parse the command line so that we get the option_sources argument
	OptionsParser parser =
	OptionsParser.builder().optionsClasses(optionClasses).allowResidue(false).build();
	parser.parse(PriorityCategory.COMMAND_LINE, null, args);

But switched to using the parse(String... args) method signature for parse (found here), and I used asCompleteListOfParsedOptions() instead of <O extends OptionsBase> getOptions(Class<O> optionsClass) (found here).

Disclaimer: I have not run this, this is just the basic shape of the code I was hoping for.

Further disclaimer: It is perhaps overkill to allow for residues in expansions, but there's no reason it would be technically infeasible for an expansion to one day include a residue, so I decided to future-proof it.

EDIT: Also this line was pretty funny:

bazel/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandUtils.java

Line 78 in 5e2dfba

* <p>Why does metaprogramming always seem like such a bright idea in the beginning?

[vogelsgesang]

Thanks for those pointers!

Before adding this functionality to the PR, I would like to get some feedback from a Bazel maintainer on whether they would like to have this functionality or if they would consider it overly complex and prefer the individual client apps to do the parsing of expanded arguments by themselves

[vogelsgesang]

@sgowroji thanks for triaging this review to the team-CLI 🙂

Could you provide a rough estimate regarding the time it will take for this to be picked up?