diff --git a/.github/workflows/selftest.yml b/.github/workflows/selftest.yml index 72720bb..5dc3f25 100644 --- a/.github/workflows/selftest.yml +++ b/.github/workflows/selftest.yml @@ -9,6 +9,11 @@ on: pull_request: branches: ["master"] workflow_dispatch: + inputs: + coverage-ref: + description: "Exact commit SHA for a one-time tools coverage baseline (empty = current commit)" + required: false + type: string permissions: read-all @@ -30,9 +35,20 @@ jobs: runs-on: ubuntu-22.04 permissions: contents: read + id-token: write steps: - uses: actions/checkout@v5 + with: + ref: ${{ inputs.coverage-ref || github.sha }} - uses: actions/setup-go@v6 with: go-version: "1.19.x" - - run: go test -race ./... + - run: go test -race -coverprofile=coverage-tools.txt ./... + - name: Upload tools coverage + uses: codecov/codecov-action@v5 + with: + use_oidc: true + files: coverage-tools.txt + override_commit: ${{ inputs.coverage-ref }} + disable_search: true + fail_ci_if_error: true diff --git a/doccov/README.md b/doccov/README.md index b8ce0e0..ef2e23f 100644 --- a/doccov/README.md +++ b/doccov/README.md @@ -61,3 +61,11 @@ jobs: GoDoc completeness (a doc comment on every exported symbol) is a separate concern, covered by `revive`'s `exported` rule in the same workflow's Analyze step. + +Config visibility follows the complete `base` contract: public options expose +`get_` and `set_`, secret options only `set_`, host-only options only `get_`, +and secret + host-only options neither. `-config` recognizes multiline Go +factory chains and the final literal `SetSecret(true/false)` / +`SetHostOnly(true/false)` value. Non-literal visibility flags are rejected so +an unknown contract cannot silently pass. Config names follow the existing +`configKeyName = "name"` convention. diff --git a/doccov/main.go b/doccov/main.go index 0416c4e..f956c51 100644 --- a/doccov/main.go +++ b/doccov/main.go @@ -162,70 +162,134 @@ func scanSurface(dir string) ([]string, error) { return out, nil } -var ( - configKeyConst = regexp.MustCompile(`(configKey\w+)\s*=\s*"([^"]+)"`) - configDeclLine = regexp.MustCompile(`ConfigOption\(\s*(configKey\w+)`) -) +// configVisibility mirrors base's independent script getter/setter controls. +type configVisibility struct { + key string + secret bool + hostOnly bool +} -// scanConfig returns the sorted config-accessor builtin names that `base` -// auto-generates for a module's config options: set_ for every option, -// plus get_ for non-secret options. It is convention-based (best-effort): -// it reads the `configKey = ""` constants and the -// gen[Secret]ConfigOption(configKey, …) declarations; a declaration line -// containing "Secret" (genSecretConfigOption or a chained .SetSecret(true)) -// marks that option secret, so it gets no get_ accessor. Returns nil if the -// module follows neither convention. +// scanConfig recognizes the ecosystem's configKey constants and ConfigOption +// factory chains. Parse Go syntax so multiline calls, comments, and explicit +// false overrides have exactly the same visibility semantics as single lines. func scanConfig(dir string) ([]string, error) { entries, err := os.ReadDir(dir) if err != nil { return nil, err } - keyValue := map[string]string{} // configKey ident -> option name - declared := map[string]bool{} // configKey ident -> registered as an option - secret := map[string]bool{} // configKey ident -> secret (set_ only) - for _, e := range entries { - name := e.Name() - if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + keys := map[string]string{} + options := map[string]configVisibility{} + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { continue } - data, err := os.ReadFile(filepath.Join(dir, name)) + file, err := parser.ParseFile(token.NewFileSet(), filepath.Join(dir, name), nil, 0) if err != nil { return nil, err } - text := string(data) - for _, m := range configKeyConst.FindAllStringSubmatch(text, -1) { - keyValue[m[1]] = m[2] - } - for _, line := range strings.Split(text, "\n") { - m := configDeclLine.FindStringSubmatch(line) - if m == nil { - continue + var scanErr error + ast.Inspect(file, func(node ast.Node) bool { + if spec, ok := node.(*ast.ValueSpec); ok { + for i, ident := range spec.Names { + if i < len(spec.Values) && strings.HasPrefix(ident.Name, "configKey") { + if value := stringLit(spec.Values[i]); value != "" { + keys[ident.Name] = value + } + } + } } - declared[m[1]] = true - if strings.Contains(line, "Secret") { - secret[m[1]] = true + expr, ok := node.(ast.Expr) + if !ok { + return true + } + option, found, err := configOption(expr) + if err != nil { + scanErr = err + return false } + if found { + options[option.key] = option + return false + } + return true + }) + if scanErr != nil { + return nil, fmt.Errorf("%s: %w", name, scanErr) } } set := map[string]bool{} - for ident := range declared { - val, ok := keyValue[ident] + for key, option := range options { + name, ok := keys[key] if !ok { continue } - set["set_"+val] = true - if !secret[ident] { - set["get_"+val] = true + if !option.hostOnly { + set["set_"+name] = true + } + if !option.secret { + set["get_"+name] = true } } out := make([]string, 0, len(set)) - for k := range set { - out = append(out, k) + for name := range set { + out = append(out, name) } sort.Strings(out) return out, nil } +func configOption(expr ast.Expr) (configVisibility, bool, error) { + call, ok := expr.(*ast.CallExpr) + if !ok { + return configVisibility{}, false, nil + } + if selector, ok := call.Fun.(*ast.SelectorExpr); ok { + option, found, err := configOption(selector.X) + if err != nil || found { + if err == nil && (selector.Sel.Name == "SetHostOnly" || selector.Sel.Name == "SetSecret") { + if len(call.Args) != 1 { + return option, true, fmt.Errorf("%s needs a literal boolean", selector.Sel.Name) + } + flag, ok := call.Args[0].(*ast.Ident) + if !ok || (flag.Name != "true" && flag.Name != "false") { + return option, true, fmt.Errorf("%s needs a literal boolean", selector.Sel.Name) + } + if selector.Sel.Name == "SetHostOnly" { + option.hostOnly = flag.Name == "true" + } else { + option.secret = flag.Name == "true" + } + } + return option, found, err + } + } + name := configFactoryName(call.Fun) + if !strings.HasSuffix(name, "ConfigOption") { + return configVisibility{}, false, nil + } + for _, arg := range call.Args { + if key, ok := arg.(*ast.Ident); ok && strings.HasPrefix(key.Name, "configKey") { + return configVisibility{key: key.Name, secret: strings.Contains(name, "Secret")}, true, nil + } + } + return configVisibility{}, false, nil +} + +func configFactoryName(expr ast.Expr) string { + switch expr := expr.(type) { + case *ast.Ident: + return expr.Name + case *ast.SelectorExpr: + return expr.Sel.Name + case *ast.IndexExpr: + return configFactoryName(expr.X) + case *ast.IndexListExpr: + return configFactoryName(expr.X) + } + return "" +} + // stringLit extracts a string constant from a builtin's first argument. It // resolves a plain literal ("module.fn") and the common "ModuleName + \".fn\"" // concatenation, returning the literal portion. diff --git a/doccov/main_test.go b/doccov/main_test.go index cee628e..0447645 100644 --- a/doccov/main_test.go +++ b/doccov/main_test.go @@ -5,6 +5,7 @@ package main import ( + "fmt" "go/parser" "os" "path/filepath" @@ -139,3 +140,97 @@ func TestRunMissingReadme(t *testing.T) { t.Fatal("a missing documentation file should fail") } } + +// The accessor contract is orthogonal: host-only hides setters, secret hides +// getters. Test the whole product, multiline chains, and later false overrides. +func TestConfigVisibilityContract(t *testing.T) { + for _, secret := range []bool{false, true} { + for _, host := range []bool{false, true} { + for _, factory := range []string{"genConfigOption", "genSecretConfigOption"} { + dir := t.TempDir() + src := fmt.Sprintf("package m\nconst configKeyLimit = `limit`\nvar option = %s(\nconfigKeyLimit, \"Secret in description is not a flag\", 1).\nSetSecret(%v).\nSetHostOnly(%v)\n", factory, secret, host) + if err := os.WriteFile(filepath.Join(dir, "mod.go"), []byte(src), 0o600); err != nil { + t.Fatal(err) + } + got, err := scanConfig(dir) + if err != nil { + t.Fatal(err) + } + want := []string{} + if !secret { + want = append(want, "get_limit") + } + if !host { + want = append(want, "set_limit") + } + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("secret=%v host=%v factory=%s: %v, want %v", secret, host, factory, got, want) + } + } + } + } + for _, tc := range []struct{ chain, want string }{ + {`.SetHostOnly(true).SetHostOnly(false).SetSecret(true).SetSecret(false)`, "get_limit,set_limit"}, + {`.SetHostOnly(true)`, "get_limit"}, + } { + dir := t.TempDir() + src := "package m\nconst configKeyLimit = \"limit\"\nvar option = genConfigOption(configKeyLimit, \"Secret\", 0)" + tc.chain + if err := os.WriteFile(filepath.Join(dir, "mod.go"), []byte(src), 0o600); err != nil { + t.Fatal(err) + } + got, err := scanConfig(dir) + if err != nil { + t.Fatal(err) + } + if strings.Join(got, ",") != tc.want { + t.Fatalf("%s: %v, want %s", tc.chain, got, tc.want) + } + } +} + +func TestConfigDeclarationSyntax(t *testing.T) { + for _, tc := range []struct{ expression, want string }{ + {`genConfigOption[int](configKeyLimit, "limit", 0)`, "get_limit,set_limit"}, + {`pkg.genConfigOption[int, string](configKeyLimit, "limit", 0).WithDescription("text").SetHostOnly(true)`, "get_limit"}, + {`base.NewNamedConfigOption("module", configKeyLimit, "limit", 0).SetSecret(true)`, "set_limit"}, + {`genSecretConfigOption(configKeyLimit, "limit", 0)`, "set_limit"}, + {`genConfigOption(configKeyUnknown, "limit", 0)`, ""}, + {`genConfigOption("literal without key convention", 0)`, ""}, + {`unrelated(configKeyLimit)`, ""}, + {`func() interface{} { return nil }()`, ""}, + {`pkg.unrelated(configKeyLimit)`, ""}, + } { + dir := t.TempDir() + src := "package m\nconst configKeyLimit = \"limit\"\nvar option = " + tc.expression + if err := os.WriteFile(filepath.Join(dir, "mod.go"), []byte(src), 0o600); err != nil { + t.Fatal(err) + } + got, err := scanConfig(dir) + if err != nil { + t.Fatal(err) + } + if strings.Join(got, ",") != tc.want { + t.Fatalf("%s: %v, want %s", tc.expression, got, tc.want) + } + } + for _, suffix := range []string{`.SetSecret()`, `.SetSecret(flag)`, `.SetHostOnly(1)`, `.SetHostOnly(true).SetSecret(flag)`} { + dir := t.TempDir() + src := "package m\nconst configKeyLimit = \"limit\"\nvar option = genConfigOption(configKeyLimit, 0)" + suffix + if err := os.WriteFile(filepath.Join(dir, "mod.go"), []byte(src), 0o600); err != nil { + t.Fatal(err) + } + if _, err := scanConfig(dir); err == nil || !strings.Contains(err.Error(), "literal boolean") { + t.Fatalf("%s: %v", suffix, err) + } + } + if _, err := scanConfig(filepath.Join(t.TempDir(), "missing")); err == nil { + t.Fatal("missing directory accepted") + } + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "bad.go"), []byte("not go"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := scanConfig(dir); err == nil { + t.Fatal("malformed Go accepted") + } +}