-
Notifications
You must be signed in to change notification settings - Fork 1
feat(allowedsymbols): replace AST checker internals with go/analysis.Analyzer #160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ac0b72c
35d8b77
4d97ff1
a437e30
406df81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| // Unless explicitly stated otherwise all files in this repository are licensed | ||
| // under the Apache License Version 2.0. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| // Copyright 2026-present Datadog, Inc. | ||
|
|
||
| // Package allowedsymbols provides a go/analysis.Analyzer that enforces | ||
| // symbol-level import restrictions on Go source files. | ||
| // | ||
| // The analyzer checks that every imported symbol is in a given allowlist, that | ||
| // no permanently banned packages are imported, and that every symbol in the | ||
| // allowlist is actually used. It reports violations with file:line:col | ||
| // diagnostics. | ||
| package allowedsymbols | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "go/ast" | ||
| "go/token" | ||
| "strings" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| ) | ||
|
|
||
| // AnalyzerConfig configures a single instance of the allowed-symbols analyzer. | ||
| type AnalyzerConfig struct { | ||
| // Symbols is the allowlist to enforce (e.g. builtinAllowedSymbols). | ||
| // Each entry must be in "importpath.Symbol" form. | ||
| Symbols []string | ||
| // ExemptImport returns true for import paths that are auto-allowed and | ||
| // should not be checked against the allowlist (e.g. same-module imports). | ||
| ExemptImport func(importPath string) bool | ||
| // ListName is used in diagnostic messages (e.g. "builtinAllowedSymbols"). | ||
| ListName string | ||
| } | ||
|
|
||
| // NewAnalyzer returns a go/analysis.Analyzer that enforces the symbol-level | ||
| // import restrictions described by cfg. Violations are reported via | ||
| // pass.Reportf and appear as diagnostics with proper file:line:col positions. | ||
| // | ||
| // NewAnalyzer panics if any entry in cfg.Symbols is malformed (no dot | ||
| // separator), matching the behaviour of the test-harness variant. | ||
| func NewAnalyzer(cfg AnalyzerConfig) *analysis.Analyzer { | ||
| for _, entry := range cfg.Symbols { | ||
| if strings.LastIndexByte(entry, '.') <= 0 { | ||
| panic(fmt.Sprintf("allowedsymbols.NewAnalyzer: malformed allowlist entry (no dot): %q", entry)) | ||
| } | ||
| } | ||
|
|
||
| run := func(pass *analysis.Pass) (any, error) { | ||
| allowedSyms, allowedPkgs := buildAllowlistSets(cfg.Symbols) | ||
| usedSymbols := make(map[string]bool, len(cfg.Symbols)) | ||
|
|
||
| for _, f := range pass.Files { | ||
| localToPath := checkFileImports(f, allowedPkgs, cfg.ExemptImport, | ||
| func(pos token.Pos, format string, args ...any) { | ||
| pass.Reportf(pos, format, args...) | ||
| }) | ||
|
|
||
| checkFileSelectors(f, localToPath, allowedSyms, usedSymbols, | ||
| func(pos token.Pos, format string, args ...any) { | ||
| pass.Reportf(pos, format, args...) | ||
| }) | ||
| } | ||
|
|
||
| if len(pass.Files) > 0 { | ||
| reportUnused(cfg.Symbols, usedSymbols, func(entry string) { | ||
| pass.Reportf(pass.Files[0].Package, | ||
| "allowlist symbol %q is not used by any file — remove it from %s", | ||
| entry, cfg.ListName) | ||
| }) | ||
| } | ||
|
|
||
| return nil, nil | ||
| } | ||
|
|
||
| return &analysis.Analyzer{ | ||
| Name: "allowedsymbols", | ||
| Doc: "enforces symbol-level import restrictions via an allowlist", | ||
| Run: run, | ||
| } | ||
| } | ||
|
|
||
| // buildAllowlistSets constructs lookup maps from a flat allowlist. | ||
| // It returns (allowedSymbols, allowedPackages) where allowedSymbols maps | ||
| // "pkg.Symbol" → true and allowedPackages maps "importpath" → true. | ||
| func buildAllowlistSets(symbols []string) (map[string]bool, map[string]bool) { | ||
| allowedSyms := make(map[string]bool, len(symbols)) | ||
| allowedPkgs := make(map[string]bool) | ||
| for _, entry := range symbols { | ||
| dot := strings.LastIndexByte(entry, '.') | ||
| if dot <= 0 { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Claude Sonnet 4.6] Fixed — |
||
| continue | ||
| } | ||
| allowedSyms[entry] = true | ||
| allowedPkgs[entry[:dot]] = true | ||
| } | ||
| return allowedSyms, allowedPkgs | ||
| } | ||
|
|
||
| // checkFileImports validates each import in f against the permanently banned | ||
| // list, the exempt predicate, and allowedPkgs. It calls report for each | ||
| // violation and returns a localName→importPath map for the file's valid, | ||
| // non-exempt imports. | ||
| // | ||
| // report is called with a token.Pos and a pre-formatted message. Callers that | ||
| // use the file:line string form (e.g. fileLineReporter) set pos to | ||
| // token.NoPos; callers that surface diagnostics via pass.Reportf pass the | ||
| // actual source position. | ||
| func checkFileImports( | ||
| f *ast.File, | ||
| allowedPkgs map[string]bool, | ||
| exemptImport func(string) bool, | ||
| report func(pos token.Pos, format string, args ...any), | ||
| ) map[string]string { | ||
| localToPath := make(map[string]string) | ||
|
|
||
| for _, imp := range f.Imports { | ||
| importPath := strings.Trim(imp.Path.Value, `"`) | ||
|
|
||
| banned := false | ||
| for key, reason := range permanentlyBanned { | ||
| if strings.HasSuffix(key, "/") { | ||
| if strings.HasPrefix(importPath, key) { | ||
| report(imp.Pos(), "import of %q is permanently banned (%s)", importPath, reason) | ||
| banned = true | ||
| break | ||
| } | ||
| } else if importPath == key { | ||
| report(imp.Pos(), "import of %q is permanently banned (%s)", importPath, reason) | ||
| banned = true | ||
| break | ||
| } | ||
| } | ||
| if banned { | ||
| continue | ||
| } | ||
|
|
||
| if exemptImport != nil && exemptImport(importPath) { | ||
| continue | ||
| } | ||
|
|
||
| var localName string | ||
| if imp.Name != nil { | ||
| localName = imp.Name.Name | ||
| } else { | ||
| parts := strings.Split(importPath, "/") | ||
| localName = parts[len(parts)-1] | ||
| } | ||
|
|
||
| if localName == "_" || localName == "." { | ||
| report(imp.Pos(), "blank/dot import of %q is not allowed", importPath) | ||
| continue | ||
| } | ||
|
|
||
| if !allowedPkgs[importPath] { | ||
| report(imp.Pos(), "import of %q is not in the allowlist", importPath) | ||
| continue | ||
| } | ||
|
|
||
| localToPath[localName] = importPath | ||
| } | ||
|
|
||
| return localToPath | ||
| } | ||
|
|
||
| // checkFileSelectors walks all ast.SelectorExpr nodes in f and checks each | ||
| // package-qualified symbol against allowedSyms. Allowed symbols are recorded | ||
| // in usedSymbols. Violations are sent to report. | ||
| func checkFileSelectors( | ||
| f *ast.File, | ||
| localToPath map[string]string, | ||
| allowedSyms map[string]bool, | ||
| usedSymbols map[string]bool, | ||
| report func(pos token.Pos, format string, args ...any), | ||
| ) { | ||
| ast.Inspect(f, func(n ast.Node) bool { | ||
| sel, ok := n.(*ast.SelectorExpr) | ||
| if !ok { | ||
| return true | ||
| } | ||
| ident, ok := sel.X.(*ast.Ident) | ||
| if !ok { | ||
| return true | ||
| } | ||
| importPath, ok := localToPath[ident.Name] | ||
| if !ok { | ||
| return true // not a package-level selector | ||
| } | ||
| key := importPath + "." + sel.Sel.Name | ||
| if !allowedSyms[key] { | ||
| report(sel.Pos(), "%s is not in the allowlist", key) | ||
| } else { | ||
| usedSymbols[key] = true | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
|
|
||
| // reportUnused calls onUnused for each symbol in symbols that is not present | ||
| // in usedSymbols. | ||
| func reportUnused(symbols []string, usedSymbols map[string]bool, onUnused func(entry string)) { | ||
| for _, entry := range symbols { | ||
| if !usedSymbols[entry] { | ||
| onUnused(entry) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| // Unless explicitly stated otherwise all files in this repository are licensed | ||
| // under the Apache License Version 2.0. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| // Copyright 2026-present Datadog, Inc. | ||
|
|
||
| package allowedsymbols | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
| ) | ||
|
|
||
| // TestNewAnalyzer_Metadata verifies that the returned Analyzer has the correct | ||
| // name and does not declare unnecessary framework dependencies. | ||
| func TestNewAnalyzer_Metadata(t *testing.T) { | ||
| a := NewAnalyzer(AnalyzerConfig{ | ||
| Symbols: []string{"fmt.Println"}, | ||
| ListName: "test", | ||
| }) | ||
| if a.Name != "allowedsymbols" { | ||
| t.Errorf("Name = %q, want \"allowedsymbols\"", a.Name) | ||
| } | ||
| if len(a.Requires) != 0 { | ||
| t.Errorf("Requires has %d entries, want 0", len(a.Requires)) | ||
| } | ||
| } | ||
|
|
||
| // TestNewAnalyzer_MalformedEntry verifies that NewAnalyzer panics immediately | ||
| // when given an allowlist entry with no dot separator, matching the test-harness | ||
| // behaviour (t.Fatalf on the same condition). | ||
| func TestNewAnalyzer_MalformedEntry(t *testing.T) { | ||
| defer func() { | ||
| if r := recover(); r == nil { | ||
| t.Error("expected panic for malformed allowlist entry, got none") | ||
| } | ||
| }() | ||
| NewAnalyzer(AnalyzerConfig{ | ||
| Symbols: []string{"nodot"}, | ||
| ListName: "test", | ||
| }) | ||
| } | ||
|
|
||
| // TestNewAnalyzer_AllowedImport runs the analyzer against a package that uses | ||
| // only allowlisted symbols; no diagnostics should be emitted. | ||
| func TestNewAnalyzer_AllowedImport(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| a := NewAnalyzer(AnalyzerConfig{ | ||
| Symbols: []string{"fmt.Println"}, | ||
| ListName: "test", | ||
| }) | ||
| analysistest.Run(t, testdata, a, "good") | ||
| } | ||
|
|
||
| // TestNewAnalyzer_BannedImport verifies that a permanently banned import is | ||
| // reported at the import site. | ||
| func TestNewAnalyzer_BannedImport(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| a := NewAnalyzer(AnalyzerConfig{ | ||
| Symbols: []string{"fmt.Println"}, | ||
| ListName: "test", | ||
| }) | ||
| analysistest.Run(t, testdata, a, "bannedimport") | ||
| } | ||
|
|
||
| // TestNewAnalyzer_DisallowedImport verifies that an import not in the | ||
| // allowlist is reported at the import site. | ||
| func TestNewAnalyzer_DisallowedImport(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| a := NewAnalyzer(AnalyzerConfig{ | ||
| Symbols: []string{"fmt.Println"}, | ||
| ListName: "test", | ||
| }) | ||
| analysistest.Run(t, testdata, a, "disallowedimport") | ||
| } | ||
|
|
||
| // TestNewAnalyzer_UnusedSymbol verifies that an allowlisted symbol that is | ||
| // never referenced in the package is reported at the package declaration. | ||
| func TestNewAnalyzer_UnusedSymbol(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| a := NewAnalyzer(AnalyzerConfig{ | ||
| Symbols: []string{"fmt.Println", "fmt.Sprintf"}, | ||
| ListName: "test", | ||
| }) | ||
| analysistest.Run(t, testdata, a, "unusedsymbol") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewAnalyzerhas no tests viaanalysistest.Runor any other mechanism. The unique code paths in theRunfunction (especially thepass.Files[0].Packageposition used for unused-symbol reporting at line 59) are not covered by any existing test. Consider adding at least oneanalysistest-based test before this API is considered production-ready.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Claude Sonnet 4.6] Fixed — added
analyzer_test.gowith sixanalysistest.Run-based tests covering: metadata (correct Name, empty Requires), malformed-entry panic, allowed imports (no diagnostics), banned imports, disallowed imports, and unused allowlist symbols. (commit a437e30)