CLI: Refactor config parsing to use the new parsing constructs (#9389)

This change is a little odd, since many of the refactored functions in
`parser.go` now belong in `parsed.go`, but I elected to make the changes
first and then move them in a later PR so as to make viewing the diffs
easier when reviewing.

Primarily, this change:
- Directly uses the options-retrieval abilities of parsed.Args to
consume the rc file options
- Parses the rc files into `parsed.Config` structs in a map keyed by
config name and into a default config struct for unnamed configs (for
example, `common` or `build` as opposed to `common:foo` or `build:bar`)
- Uses these config structs to expand config options

Small behavior changes include that `common:` will no longer be
equivalent to `common`, which mirrors bazel's bahavior, and that the
`always` phase is now supported.

Once this is approved, a follow-on PR will move the functions indicated
with the relevant `TODO`s to their appropriate locations with minimal
changes as are necessary to move them as described.

---------

Co-authored-by: Brandon Duffany <brandon@buildbuddy.io>

Author: tempoz

cli/parser/bazelrc/bazelrc.go

@@ -3,6 +3,7 @@ package bazelrc
 import (
 	"bufio"
 	"fmt"
+	"maps"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -16,6 +17,9 @@ import (
 const (
 	EnablePlatformSpecificConfigFlag = "enable_platform_specific_config"
 	workspacePrefix                  = `%workspace%/`
+
+	CommonPhase = "common"
+	AlwaysPhase = "always"
 )
 
 var (
@@ -58,6 +62,18 @@ var (
 		"coverage": "test",
 	}
 
+	unconditionalCommandPhases = map[string]struct{}{
+		CommonPhase: {},
+		AlwaysPhase: {},
+	}
+
+	allPhases = func() map[string]struct{} {
+		v := maps.Clone(bazelCommands)
+		maps.Insert(v, maps.All(unconditionalCommandPhases))
+		v["startup"] = struct{}{}
+		return v
+	}()
+
 	StartupFlagNoRc = map[string]struct{}{
 		"ignore_all_rc_files": {},
 		"home_rc":             {},
@@ -69,45 +85,14 @@ var (
 
 // RcRule is a rule parsed from a bazelrc file.
 type RcRule struct {
-	Phase  string
-	Config string
+	Phase string
+	// Make Config a pointer to a string so we can distinguish default configs
+	// from configs with blank names.
+	Config *string
 	// Tokens contains the raw (non-canonicalized) tokens in the rule.
 	Tokens []string
 }
 
-type Rules struct {
-	All              []*RcRule
-	ByPhaseAndConfig map[string]map[string][]*RcRule
-}
-
-func StructureRules(rules []*RcRule) *Rules {
-	r := &Rules{
-		All:              rules,
-		ByPhaseAndConfig: map[string]map[string][]*RcRule{},
-	}
-	for _, rule := range rules {
-		byConfig := r.ByPhaseAndConfig[rule.Phase]
-		if byConfig == nil {
-			byConfig = map[string][]*RcRule{}
-			r.ByPhaseAndConfig[rule.Phase] = byConfig
-		}
-		byConfig[rule.Config] = append(byConfig[rule.Config], rule)
-	}
-	return r
-}
-
-func (r *Rules) ForPhaseAndConfig(phase, config string) []*RcRule {
-	byConfig := r.ByPhaseAndConfig[phase]
-	if byConfig == nil {
-		return nil
-	}
-	return byConfig[config]
-}
-
-func (r *RcRule) String() string {
-	return fmt.Sprintf("phase=%q,config=%q,tokens=%v", r.Phase, r.Config, r.Tokens)
-}
-
 // getPhases returns the command's inheritance hierarchy in increasing order of
 // precedence.
 //
@@ -128,35 +113,42 @@ func GetPhases(command string) (out []string) {
 	return
 }
 
-func appendRcRulesFromImport(workspaceDir, path string, opts []*RcRule, optional bool, importStack []string) ([]*RcRule, error) {
+func appendRcRulesFromImport(workspaceDir, path string, namedConfigs map[string]map[string][]string, defaultConfig map[string][]string, optional bool, importStack []string) error {
 	if strings.HasPrefix(path, workspacePrefix) {
 		path = filepath.Join(workspaceDir, path[len(workspacePrefix):])
 	}
-
-	file, err := os.Open(path)
+	realPath, err := Realpath(path)
 	if err != nil {
 		if optional {
-			return opts, nil
+			return nil
 		}
-		return nil, err
+		return fmt.Errorf("could not determine real path of bazelrc file: %s", err)
 	}
-	defer file.Close()
-	return appendRcRulesFromFile(workspaceDir, file, opts, importStack)
+
+	return AppendRcRulesFromFile(workspaceDir, realPath, namedConfigs, defaultConfig, importStack, optional)
 }
 
-func appendRcRulesFromFile(workspaceDir string, f *os.File, rules []*RcRule, importStack []string) ([]*RcRule, error) {
-	rpath, err := realpath(f.Name())
-	if err != nil {
-		return nil, fmt.Errorf("could not determine real path of bazelrc file: %s", err)
+// AppendRCRulesFromFile reads and lexes the provided rc file and appends the
+// args to the provided configs based on the detected phase and name.
+//
+// configs is a map keyed by config name where the values are maps keyed by
+// phase name where the values are lists containing all the rules for that
+// config in the order they are encountered.
+func AppendRcRulesFromFile(workspaceDir string, realPath string, namedConfigs map[string]map[string][]string, defaultConfig map[string][]string, importStack []string, optional bool) error {
+	if slices.Contains(importStack, realPath) {
+		return fmt.Errorf("circular import detected: %s -> %s", strings.Join(importStack, " -> "), realPath)
 	}
-	for _, path := range importStack {
-		if path == rpath {
-			return nil, fmt.Errorf("circular import detected: %s -> %s", strings.Join(importStack, " -> "), rpath)
+	importStack = append(importStack, realPath)
+	file, err := os.Open(realPath)
+	if err != nil {
+		if optional {
+			return nil
 		}
+		return err
 	}
-	importStack = append(importStack, rpath)
+	defer file.Close()
 
-	scanner := bufio.NewScanner(f)
+	scanner := bufio.NewScanner(file)
 	for scanner.Scan() {
 		line := scanner.Text()
 		// Handle line continuations (lines can end with "\" to effectively
@@ -175,9 +167,8 @@ func appendRcRulesFromFile(workspaceDir string, f *os.File, rules []*RcRule, imp
 		if tokens[0] == "import" || tokens[0] == "try-import" {
 			isOptional := tokens[0] == "try-import"
 			path := strings.TrimSpace(strings.TrimPrefix(line, tokens[0]))
-			rules, err = appendRcRulesFromImport(workspaceDir, path, rules, isOptional, importStack)
-			if err != nil {
-				return nil, err
+			if err := appendRcRulesFromImport(workspaceDir, path, namedConfigs, defaultConfig, isOptional, importStack); err != nil {
+				return err
 			}
 			continue
 		}
@@ -190,18 +181,29 @@ func appendRcRulesFromFile(workspaceDir string, f *os.File, rules []*RcRule, imp
 		if rule == nil {
 			continue
 		}
-		// Bazel doesn't support configs for startup options and ignores them if
-		// they appear in a bazelrc: https://bazel.build/run/bazelrc#config
-		if rule.Phase == "startup" && rule.Config != "" {
+		if rule.Config == nil {
+			defaultConfig[rule.Phase] = append(defaultConfig[rule.Phase], rule.Tokens...)
+			continue
+		}
+		// Bazel doesn't support named configs for startup options and ignores them
+		// if they appear in a bazelrc: https://bazel.build/run/bazelrc#config
+		if rule.Phase == "startup" {
 			continue
 		}
-		rules = append(rules, rule)
+		config, ok := namedConfigs[*rule.Config]
+		if !ok {
+			config = make(map[string][]string)
+			namedConfigs[*rule.Config] = config
+		}
+		config[rule.Phase] = append(config[rule.Phase], rule.Tokens...)
 	}
-	log.Debugf("Adding rc rules from %q: %v", rpath, rules)
-	return rules, scanner.Err()
+	log.Debugf("Added rc rules from %q; new configs: %#v", realPath, namedConfigs)
+	return scanner.Err()
 }
 
-func realpath(path string) (string, error) {
+// Realpath evaluates any symlinks in the given path and then returns the
+// absolute path.
+func Realpath(path string) (string, error) {
 	directPath, err := filepath.EvalSymlinks(path)
 	if err != nil {
 		return "", err
@@ -229,55 +231,24 @@ func parseRcRule(line string) (*RcRule, error) {
 		// bazel ignores .bazelrc lines consisting of a single shlex token
 		return nil, nil
 	}
-	if strings.HasPrefix(tokens[0], "-") {
-		return &RcRule{
-			Phase:  "common",
-			Tokens: tokens,
-		}, nil
-	}
-	if !strings.Contains(tokens[0], ":") {
-		return &RcRule{
-			Phase:  tokens[0],
-			Tokens: tokens[1:],
-		}, nil
-	}
-	phaseConfig := strings.Split(tokens[0], ":")
-	if len(phaseConfig) != 2 {
-		return nil, fmt.Errorf("invalid bazelrc syntax: %s", phaseConfig)
+	phase := tokens[0]
+	var configName *string
+	if colonIndex := strings.Index(tokens[0], ":"); colonIndex != -1 {
+		phase = tokens[0][:colonIndex]
+		v := tokens[0][colonIndex+1:]
+		configName = &v
 	}
+
 	return &RcRule{
-		Phase:  phaseConfig[0],
-		Config: phaseConfig[1],
+		Phase:  phase,
+		Config: configName,
 		Tokens: tokens[1:],
 	}, nil
 }
 
-func ParseRCFiles(workspaceDir string, filePaths ...string) ([]*RcRule, error) {
-	opts := make([]*RcRule, 0)
-	seen := map[string]bool{}
-	for _, filePath := range filePaths {
-		r, err := realpath(filePath)
-		if err != nil {
-			continue
-		}
-		if seen[r] {
-			continue
-		}
-		seen[r] = true
-
-		file, err := os.Open(filePath)
-		if err != nil {
-			continue
-		}
-		defer file.Close()
-		opts, err = appendRcRulesFromFile(workspaceDir, file, opts, nil /*=importStack*/)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return opts, nil
-}
-
+// GetBazelOS returns the os string that `enable_platform_specific_config` will
+// expect based on the detected runtime.GOOS.
+//
 // Mirroring the behavior here:
 // https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java#L41
 func GetBazelOS() string {
@@ -297,12 +268,28 @@ func GetBazelOS() string {
 	}
 }
 
+// IsBazelCommand returns whether the given string is recognized as a bazel
+// command.
 func IsBazelCommand(command string) bool {
 	_, ok := bazelCommands[command]
 	return ok
 }
 
+// Parent returns the parent command of the given command, if one exists.
 func Parent(command string) (string, bool) {
 	parent, ok := parentCommand[command]
 	return parent, ok
 }
+
+// IsUnconditionalCommandPhase returns whether or not this is a phase that should always
+// be evaluated, regardless of the command.
+func IsUnconditionalCommandPhase(phase string) bool {
+	_, ok := unconditionalCommandPhases[phase]
+	return ok
+}
+
+// IsPhase returns whether or not this is a valid phase for a bazel rc line.
+func IsPhase(phase string) bool {
+	_, ok := allPhases[phase]
+	return ok
+}

cli/parser/parsed/parsed.go

@@ -538,15 +538,14 @@ func (a *OrderedArgs) appendOption(option options.Option, startupOptionInsertInd
 	return startupOptionInsertIndex, commandOptionInsertIndex, fmt.Errorf("Failed to append Option: option '%s' is not a startup option and the command '%s' does not support it.", option.Name(), command)
 }
 
-// Offset exists as a temporary hack while we continue to move away from
-// passing arguments around as strings. It returns the offset in the
-// string-based arg slice where the argument at this index is located.
-func (a *OrderedArgs) Offset(index int) int {
-	offset := 0
-	for _, arg := range a.Args[:index] {
-		offset += len(arg.Format())
+type Config struct {
+	ByPhase map[string][]arguments.Argument
+}
+
+func NewConfig() *Config {
+	return &Config{
+		ByPhase: make(map[string][]arguments.Argument),
 	}
-	return offset
 }
 
 func Partition(args []arguments.Argument) *PartitionedArgs {
@@ -670,6 +669,8 @@ func (a *PartitionedArgs) RemoveOptions(optionNames ...string) []*IndexedOption
 func (a *PartitionedArgs) Append(args ...arguments.Argument) error {
 	for _, arg := range args {
 		switch arg := arg.(type) {
+		case *arguments.DoubleDash:
+			// skip
 		case *arguments.PositionalArgument:
 			switch {
 			case a.Command == nil: