From bc79d0f493eb0eeac7c1c15d462f78af51ee0cb2 Mon Sep 17 00:00:00 2001 From: Daniel Ostrow Date: Wed, 18 Feb 2026 18:30:06 -0500 Subject: [PATCH] Add standalone mangle-lint CLI tool for static analysis of .mg files Introduces a new lint/ package and cmd/mangle-lint binary that reuses the existing analysis infrastructure and adds new quality checks: unused predicates, missing documentation, naming conventions, singleton variables, rule complexity, and dead code detection. Supports text and JSON output, configurable severity, and per-rule enable/disable flags. --- cmd/mangle-lint/main.go | 138 +++++++++++++ lint/check_complexity.go | 43 +++++ lint/check_dead_code.go | 57 ++++++ lint/check_missing_doc.go | 46 +++++ lint/check_naming.go | 91 +++++++++ lint/check_singleton.go | 49 +++++ lint/check_unused_pred.go | 67 +++++++ lint/lint.go | 231 ++++++++++++++++++++++ lint/lint_test.go | 394 ++++++++++++++++++++++++++++++++++++++ lint/output.go | 43 +++++ lint/rules.go | 127 ++++++++++++ 11 files changed, 1286 insertions(+) create mode 100644 cmd/mangle-lint/main.go create mode 100644 lint/check_complexity.go create mode 100644 lint/check_dead_code.go create mode 100644 lint/check_missing_doc.go create mode 100644 lint/check_naming.go create mode 100644 lint/check_singleton.go create mode 100644 lint/check_unused_pred.go create mode 100644 lint/lint.go create mode 100644 lint/lint_test.go create mode 100644 lint/output.go create mode 100644 lint/rules.go diff --git a/cmd/mangle-lint/main.go b/cmd/mangle-lint/main.go new file mode 100644 index 0000000..33e6bfd --- /dev/null +++ b/cmd/mangle-lint/main.go @@ -0,0 +1,138 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Binary mangle-lint is a standalone linter for Mangle programs. +package main + +import ( + "flag" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/google/mangle/lint" +) + +var ( + format = flag.String("format", "text", "output format: text or json") + severity = flag.String("severity", "info", "minimum severity to report: info, warning, or error") + disable = flag.String("disable", "", "comma-separated list of rule names to disable") + enable = flag.String("enable", "", "comma-separated list of rule names to enable (all others disabled)") + listRules = flag.Bool("list-rules", false, "list all available lint rules and exit") + maxPremises = flag.Int("max-premises", 8, "threshold for overly-complex-rule check") +) + +func main() { + flag.Usage = func() { + fmt.Fprintf(os.Stderr, "Usage: mangle-lint [flags] [file.mg...]\n\n") + fmt.Fprintf(os.Stderr, "A linter for the Mangle Datalog language.\n\n") + fmt.Fprintf(os.Stderr, "Flags:\n") + flag.PrintDefaults() + fmt.Fprintf(os.Stderr, "\nExit codes:\n") + fmt.Fprintf(os.Stderr, " 0 No findings (or only info)\n") + fmt.Fprintf(os.Stderr, " 1 Warnings found\n") + fmt.Fprintf(os.Stderr, " 2 Errors found\n") + } + flag.Parse() + + if *listRules { + fmt.Println("Available lint rules:") + fmt.Println() + for _, r := range lint.AllRules() { + fmt.Printf(" %-25s [%s] %s\n", r.Name(), r.DefaultSeverity(), r.Description()) + } + os.Exit(0) + } + + args := flag.Args() + if len(args) == 0 { + flag.Usage() + os.Exit(2) + } + + // Expand glob patterns. + var files []string + for _, arg := range args { + matches, err := filepath.Glob(arg) + if err != nil || len(matches) == 0 { + files = append(files, arg) // treat as literal path + } else { + files = append(files, matches...) + } + } + + config := lint.DefaultConfig() + config.MaxPremises = *maxPremises + config.MinSeverity = lint.ParseSeverity(*severity) + + if *disable != "" { + for _, name := range strings.Split(*disable, ",") { + config.DisabledRules[strings.TrimSpace(name)] = true + } + } + if *enable != "" { + // Disable all rules first, then enable only the specified ones. + for _, r := range lint.AllRules() { + config.DisabledRules[r.Name()] = true + } + for _, name := range strings.Split(*enable, ",") { + delete(config.DisabledRules, strings.TrimSpace(name)) + } + } + + linter := lint.NewLinter(config) + var allResults []lint.LintResult + hasParseError := false + + for _, path := range files { + results, err := linter.LintFile(path) + if err != nil { + fmt.Fprintf(os.Stderr, "%s: %v\n", path, err) + hasParseError = true + continue + } + allResults = append(allResults, results...) + } + + // Output. + switch *format { + case "json": + if err := lint.FormatJSON(os.Stdout, allResults); err != nil { + fmt.Fprintf(os.Stderr, "error writing JSON: %v\n", err) + os.Exit(2) + } + default: + lint.FormatText(os.Stdout, allResults) + } + + // Exit code. + if hasParseError { + os.Exit(2) + } + maxSev := lint.SeverityInfo + for _, r := range allResults { + if r.Severity > maxSev { + maxSev = r.Severity + } + } + switch { + case maxSev >= lint.SeverityError: + os.Exit(2) + case maxSev >= lint.SeverityWarning: + os.Exit(1) + default: + os.Exit(0) + } +} diff --git a/lint/check_complexity.go b/lint/check_complexity.go new file mode 100644 index 0000000..7283e45 --- /dev/null +++ b/lint/check_complexity.go @@ -0,0 +1,43 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import "fmt" + +// OverlyComplexRule flags rules with too many premises. +type OverlyComplexRule struct{} + +func (r *OverlyComplexRule) Name() string { return "overly-complex-rule" } +func (r *OverlyComplexRule) Description() string { return "Flags rules with too many premises" } +func (r *OverlyComplexRule) DefaultSeverity() Severity { return SeverityInfo } + +func (r *OverlyComplexRule) Check(input *LintInput, config LintConfig) []LintResult { + var results []LintResult + threshold := config.MaxPremises + if threshold <= 0 { + threshold = 8 + } + for _, clause := range input.ProgramInfo.Rules { + if len(clause.Premises) > threshold { + results = append(results, LintResult{ + RuleName: r.Name(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("rule for %q has %d premises (threshold: %d); consider breaking into intermediate predicates", clause.Head.Predicate.Symbol, len(clause.Premises), threshold), + Predicate: clause.Head.Predicate.Symbol, + }) + } + } + return results +} diff --git a/lint/check_dead_code.go b/lint/check_dead_code.go new file mode 100644 index 0000000..7603d3a --- /dev/null +++ b/lint/check_dead_code.go @@ -0,0 +1,57 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import ( + "fmt" + + "github.com/google/mangle/ast" +) + +// DeadCodeRule flags IDB predicates whose derived facts are never consumed +// by any other rule's premises. +type DeadCodeRule struct{} + +func (r *DeadCodeRule) Name() string { return "dead-code" } +func (r *DeadCodeRule) Description() string { return "Flags derived predicates whose results are never consumed" } +func (r *DeadCodeRule) DefaultSeverity() Severity { return SeverityInfo } + +func (r *DeadCodeRule) Check(input *LintInput, config LintConfig) []LintResult { + // Build set of predicates consumed in premises (excluding self-references). + consumed := make(map[ast.PredicateSym]bool) + for _, clause := range input.ProgramInfo.Rules { + for _, p := range clause.Premises { + for _, pred := range extractPredicatesFromTerm(p) { + consumed[pred] = true + } + } + } + + var results []LintResult + for pred := range input.ProgramInfo.IdbPredicates { + if !isUserPredicate(pred) { + continue + } + if !consumed[pred] { + results = append(results, LintResult{ + RuleName: r.Name(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("predicate %q derives facts but results are never consumed by another rule", pred.Symbol), + Predicate: pred.Symbol, + }) + } + } + return results +} diff --git a/lint/check_missing_doc.go b/lint/check_missing_doc.go new file mode 100644 index 0000000..e70055c --- /dev/null +++ b/lint/check_missing_doc.go @@ -0,0 +1,46 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import "fmt" + +// MissingDocRule flags predicates that have no documentation descriptors. +type MissingDocRule struct{} + +func (r *MissingDocRule) Name() string { return "missing-doc" } +func (r *MissingDocRule) Description() string { return "Flags predicates without documentation" } +func (r *MissingDocRule) DefaultSeverity() Severity { return SeverityInfo } + +func (r *MissingDocRule) Check(input *LintInput, config LintConfig) []LintResult { + var results []LintResult + for pred, decl := range input.ProgramInfo.Decls { + if !isUserPredicate(pred) { + continue + } + if decl.IsSynthetic() { + continue + } + docs := decl.Doc() + if len(docs) == 0 || (len(docs) == 1 && docs[0] == "") { + results = append(results, LintResult{ + RuleName: r.Name(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("predicate %q has no documentation", pred.Symbol), + Predicate: pred.Symbol, + }) + } + } + return results +} diff --git a/lint/check_naming.go b/lint/check_naming.go new file mode 100644 index 0000000..ceb64b4 --- /dev/null +++ b/lint/check_naming.go @@ -0,0 +1,91 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import ( + "fmt" + "regexp" + + "github.com/google/mangle/ast" +) + +var ( + // predicateNameRe matches valid snake_case predicate names, optionally with package prefixes. + predicateNameRe = regexp.MustCompile(`^([a-z][a-z0-9_]*\.)*[a-z][a-z0-9_]*$`) + // variableNameRe matches valid variable names: uppercase start, alphanumeric + underscore. + variableNameRe = regexp.MustCompile(`^[A-Z][A-Za-z0-9_]*$`) +) + +// NamingConventionRule checks that predicate names are snake_case and variables +// are uppercase. +type NamingConventionRule struct{} + +func (r *NamingConventionRule) Name() string { return "naming-convention" } +func (r *NamingConventionRule) Description() string { return "Checks predicate and variable naming conventions" } +func (r *NamingConventionRule) DefaultSeverity() Severity { return SeverityWarning } + +func (r *NamingConventionRule) Check(input *LintInput, config LintConfig) []LintResult { + var results []LintResult + + // Check predicate names. + checked := make(map[ast.PredicateSym]bool) + allClauses := append(input.ProgramInfo.Rules, factsAsClauses(input)...) + for _, clause := range allClauses { + pred := clause.Head.Predicate + if checked[pred] || !isUserPredicate(pred) { + continue + } + checked[pred] = true + if !predicateNameRe.MatchString(pred.Symbol) { + results = append(results, LintResult{ + RuleName: r.Name(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("predicate %q does not follow snake_case naming convention", pred.Symbol), + Predicate: pred.Symbol, + }) + } + } + + // Check variable names in rules. + for _, clause := range input.ProgramInfo.Rules { + vars := make(map[ast.Variable]bool) + ast.AddVarsFromClause(clause, vars) + for v := range vars { + name := v.Symbol + if name == "_" { + continue + } + if !variableNameRe.MatchString(name) { + results = append(results, LintResult{ + RuleName: r.Name(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("variable %q in rule for %q does not follow naming convention (should start with uppercase)", name, clause.Head.Predicate.Symbol), + Predicate: clause.Head.Predicate.Symbol, + }) + } + } + } + + return results +} + +// factsAsClauses wraps initial facts from ProgramInfo into Clause values. +func factsAsClauses(input *LintInput) []ast.Clause { + var clauses []ast.Clause + for _, fact := range input.ProgramInfo.InitialFacts { + clauses = append(clauses, ast.Clause{Head: fact}) + } + return clauses +} diff --git a/lint/check_singleton.go b/lint/check_singleton.go new file mode 100644 index 0000000..b38f95c --- /dev/null +++ b/lint/check_singleton.go @@ -0,0 +1,49 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import ( + "fmt" + "strings" +) + +// SingletonVariableRule flags variables that appear in only one term within a clause. +type SingletonVariableRule struct{} + +func (r *SingletonVariableRule) Name() string { return "singleton-variable" } +func (r *SingletonVariableRule) Description() string { return "Flags variables appearing only once in a clause (likely typos)" } +func (r *SingletonVariableRule) DefaultSeverity() Severity { return SeverityWarning } + +func (r *SingletonVariableRule) Check(input *LintInput, config LintConfig) []LintResult { + var results []LintResult + for _, clause := range input.ProgramInfo.Rules { + counts := collectVariablesByTerm(clause) + for v, count := range counts { + // Skip wildcards and variables prefixed with underscore. + if v.Symbol == "_" || strings.HasPrefix(v.Symbol, "_") { + continue + } + if count == 1 { + results = append(results, LintResult{ + RuleName: r.Name(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("variable %q appears only once in rule for %q; use _ if intentionally unused", v.Symbol, clause.Head.Predicate.Symbol), + Predicate: clause.Head.Predicate.Symbol, + }) + } + } + } + return results +} diff --git a/lint/check_unused_pred.go b/lint/check_unused_pred.go new file mode 100644 index 0000000..bbc8b12 --- /dev/null +++ b/lint/check_unused_pred.go @@ -0,0 +1,67 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import ( + "fmt" + + "github.com/google/mangle/ast" +) + +// UnusedPredicateRule flags predicates that are declared but never referenced. +type UnusedPredicateRule struct{} + +func (r *UnusedPredicateRule) Name() string { return "unused-predicate" } +func (r *UnusedPredicateRule) Description() string { return "Flags declared predicates that are never referenced" } +func (r *UnusedPredicateRule) DefaultSeverity() Severity { return SeverityWarning } + +func (r *UnusedPredicateRule) Check(input *LintInput, config LintConfig) []LintResult { + // Build set of all referenced predicates. + referenced := make(map[ast.PredicateSym]bool) + + // From rules: head and premises. + for _, clause := range input.ProgramInfo.Rules { + referenced[clause.Head.Predicate] = true + for _, p := range clause.Premises { + for _, pred := range extractPredicatesFromTerm(p) { + referenced[pred] = true + } + } + } + + // From initial facts. + for _, fact := range input.ProgramInfo.InitialFacts { + referenced[fact.Predicate] = true + } + + var results []LintResult + for pred, decl := range input.ProgramInfo.Decls { + if !isUserPredicate(pred) { + continue + } + if decl.IsSynthetic() { + continue + } + if !referenced[pred] { + results = append(results, LintResult{ + RuleName: r.Name(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("predicate %q is declared but never referenced", pred.Symbol), + Predicate: pred.Symbol, + }) + } + } + return results +} diff --git a/lint/lint.go b/lint/lint.go new file mode 100644 index 0000000..6c2b736 --- /dev/null +++ b/lint/lint.go @@ -0,0 +1,231 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package lint provides a standalone linter for Mangle programs. It reuses the +// existing analysis infrastructure and adds additional style and quality checks. +package lint + +import ( + "bufio" + "fmt" + "os" + + "github.com/google/mangle/analysis" + "github.com/google/mangle/ast" + "github.com/google/mangle/parse" +) + +// Severity levels for lint findings. +type Severity int + +const ( + // SeverityInfo is for informational findings that may not indicate a problem. + SeverityInfo Severity = iota + // SeverityWarning is for findings that likely indicate a problem. + SeverityWarning + // SeverityError is for findings that definitely indicate a problem. + SeverityError +) + +// MarshalJSON encodes severity as a JSON string. +func (s Severity) MarshalJSON() ([]byte, error) { + return []byte(`"` + s.String() + `"`), nil +} + +// String returns the human-readable name of a severity level. +func (s Severity) String() string { + switch s { + case SeverityInfo: + return "info" + case SeverityWarning: + return "warning" + case SeverityError: + return "error" + default: + return "unknown" + } +} + +// ParseSeverity parses a severity string. Returns SeverityInfo if unrecognized. +func ParseSeverity(s string) Severity { + switch s { + case "warning": + return SeverityWarning + case "error": + return SeverityError + default: + return SeverityInfo + } +} + +// LintResult represents a single finding from a lint check. +type LintResult struct { + // RuleName is the machine-readable name of the lint rule. + RuleName string `json:"rule"` + // Severity of the finding. + Severity Severity `json:"severity"` + // File is the source file path. + File string `json:"file,omitempty"` + // Message is a human-readable description of the finding. + Message string `json:"message"` + // Predicate is the predicate involved, if applicable. + Predicate string `json:"predicate,omitempty"` +} + +// LintConfig holds the toggleable configuration for all lint rules. +type LintConfig struct { + // MaxPremises is the threshold for the overly-complex-rule check. + MaxPremises int + // DisabledRules is a set of rule names to skip. + DisabledRules map[string]bool + // MinSeverity: findings below this severity are suppressed from output. + MinSeverity Severity +} + +// DefaultConfig returns a LintConfig with sensible defaults. +func DefaultConfig() LintConfig { + return LintConfig{ + MaxPremises: 8, + DisabledRules: map[string]bool{}, + MinSeverity: SeverityInfo, + } +} + +// LintInput bundles everything a lint check needs. +type LintInput struct { + // File is the source file path. + File string + // Unit is the parsed source unit. + Unit parse.SourceUnit + // ProgramInfo is the result of analysis.Analyze. + ProgramInfo *analysis.ProgramInfo + // Strata from analysis.Stratify (nil if stratification failed). + Strata []analysis.Nodeset + // PredToStratum maps each predicate to its stratum number (nil if stratification failed). + PredToStratum map[ast.PredicateSym]int +} + +// Linter orchestrates parsing, analysis, and lint checks. +type Linter struct { + config LintConfig + rules []LintRule +} + +// NewLinter creates a Linter with the given config and all registered rules. +func NewLinter(config LintConfig) *Linter { + return &Linter{ + config: config, + rules: AllRules(), + } +} + +// LintFile parses and lints a single .mg file. Returns results and any hard +// errors (parse failure, etc.). Analysis errors are surfaced as lint findings +// rather than hard errors when possible. +func (l *Linter) LintFile(path string) ([]LintResult, error) { + f, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("open %s: %w", path, err) + } + defer f.Close() + unit, err := parse.Unit(bufio.NewReaderSize(f, 4096)) + if err != nil { + return nil, fmt.Errorf("parse %s: %w", path, err) + } + return l.LintUnit(path, unit) +} + +// LintUnit lints a pre-parsed source unit. +func (l *Linter) LintUnit(file string, unit parse.SourceUnit) ([]LintResult, error) { + // Run existing analysis. + programInfo, err := analysis.AnalyzeOneUnit(unit, nil) + if err != nil { + // Return analysis error as a lint finding so we still report something useful. + return []LintResult{{ + RuleName: "analysis", + Severity: SeverityError, + File: file, + Message: fmt.Sprintf("analysis error: %v", err), + }}, nil + } + + var results []LintResult + + // Surface temporal warnings from existing analysis. + for _, w := range programInfo.Warnings { + sev := convertTemporalSeverity(w.Severity) + if sev < l.config.MinSeverity { + continue + } + results = append(results, LintResult{ + RuleName: "temporal-recursion", + Severity: sev, + File: file, + Message: w.Message, + Predicate: w.Predicate.Symbol, + }) + } + + // Run stratification check. + program := analysis.Program{ + EdbPredicates: programInfo.EdbPredicates, + IdbPredicates: programInfo.IdbPredicates, + Rules: programInfo.Rules, + } + strata, predToStratum, stratErr := analysis.Stratify(program) + if stratErr != nil { + results = append(results, LintResult{ + RuleName: "stratification", + Severity: SeverityError, + File: file, + Message: stratErr.Error(), + }) + } + + // Build input for lint rules. + input := &LintInput{ + File: file, + Unit: unit, + ProgramInfo: programInfo, + Strata: strata, + PredToStratum: predToStratum, + } + + // Run each enabled lint rule. + for _, rule := range l.rules { + if l.config.DisabledRules[rule.Name()] { + continue + } + findings := rule.Check(input, l.config) + for _, f := range findings { + if f.Severity >= l.config.MinSeverity { + f.File = file + results = append(results, f) + } + } + } + + return results, nil +} + +func convertTemporalSeverity(s analysis.WarningSeverity) Severity { + switch s { + case analysis.SeverityWarning: + return SeverityWarning + case analysis.SeverityCritical: + return SeverityError + default: + return SeverityInfo + } +} diff --git a/lint/lint_test.go b/lint/lint_test.go new file mode 100644 index 0000000..ba2d249 --- /dev/null +++ b/lint/lint_test.go @@ -0,0 +1,394 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/google/mangle/parse" +) + +func lintSource(t *testing.T, source string, config ...LintConfig) []LintResult { + t.Helper() + unit, err := parse.Unit(strings.NewReader(source)) + if err != nil { + t.Fatalf("parse error: %v", err) + } + cfg := DefaultConfig() + if len(config) > 0 { + cfg = config[0] + } + linter := NewLinter(cfg) + results, err := linter.LintUnit("test.mg", unit) + if err != nil { + t.Fatalf("lint error: %v", err) + } + return results +} + +func filterByRule(results []LintResult, rule string) []LintResult { + var filtered []LintResult + for _, r := range results { + if r.RuleName == rule { + filtered = append(filtered, r) + } + } + return filtered +} + +// --- Complexity Rule Tests --- + +func TestOverlyComplexRule_Triggers(t *testing.T) { + // A rule with 3 premises, threshold set to 2. + source := ` +bar(/x). +baz(/y). +qux(/z). +foo(X) :- bar(X), baz(X), qux(X). +` + cfg := DefaultConfig() + cfg.MaxPremises = 2 + results := lintSource(t, source, cfg) + filtered := filterByRule(results, "overly-complex-rule") + if len(filtered) != 1 { + t.Errorf("got %d findings, want 1: %v", len(filtered), filtered) + } +} + +func TestOverlyComplexRule_NoTrigger(t *testing.T) { + source := ` +bar(/x). +foo(X) :- bar(X). +` + results := lintSource(t, source) + filtered := filterByRule(results, "overly-complex-rule") + if len(filtered) != 0 { + t.Errorf("got %d findings, want 0: %v", len(filtered), filtered) + } +} + +// --- Missing Doc Rule Tests --- + +func TestMissingDoc_Triggers(t *testing.T) { + source := ` +Decl bar(X) bound [/name]. +bar(/x). +` + results := lintSource(t, source) + filtered := filterByRule(results, "missing-doc") + if len(filtered) != 1 { + t.Errorf("got %d findings, want 1: %v", len(filtered), filtered) + } +} + +func TestMissingDoc_WithDoc(t *testing.T) { + source := ` +Decl bar(X) descr[doc("a documented predicate")] bound [/name]. +bar(/x). +` + results := lintSource(t, source) + filtered := filterByRule(results, "missing-doc") + if len(filtered) != 0 { + t.Errorf("got %d findings, want 0: %v", len(filtered), filtered) + } +} + +// --- Naming Convention Rule Tests --- + +func TestNamingConvention_BadPredicate(t *testing.T) { + source := ` +Decl pointsTo(X, Y) bound [/name, /name]. +pointsTo(/a, /b). +` + results := lintSource(t, source) + filtered := filterByRule(results, "naming-convention") + if len(filtered) != 1 { + t.Errorf("got %d naming findings, want 1: %v", len(filtered), filtered) + } +} + +func TestNamingConvention_GoodPredicate(t *testing.T) { + source := ` +Decl points_to(X, Y) bound [/name, /name]. +points_to(/a, /b). +` + results := lintSource(t, source) + filtered := filterByRule(results, "naming-convention") + if len(filtered) != 0 { + t.Errorf("got %d naming findings, want 0: %v", len(filtered), filtered) + } +} + +// --- Singleton Variable Rule Tests --- + +func TestSingletonVariable_Triggers(t *testing.T) { + source := ` +bar(/x, /y). +foo(X) :- bar(X, Typo). +` + results := lintSource(t, source) + filtered := filterByRule(results, "singleton-variable") + // "Typo" appears only in one premise and nowhere else. + if len(filtered) != 1 { + t.Errorf("got %d findings, want 1: %v", len(filtered), filtered) + } + if len(filtered) > 0 && !strings.Contains(filtered[0].Message, "Typo") { + t.Errorf("expected message to mention 'Typo', got: %s", filtered[0].Message) + } +} + +func TestSingletonVariable_Wildcard(t *testing.T) { + // Wildcards should not trigger. + source := ` +bar(/x, /y). +foo(X) :- bar(X, _). +` + results := lintSource(t, source) + filtered := filterByRule(results, "singleton-variable") + if len(filtered) != 0 { + t.Errorf("got %d findings, want 0: %v", len(filtered), filtered) + } +} + +func TestSingletonVariable_TransformOutput(t *testing.T) { + // Transform output variables used in head should not be singletons. + source := ` +Decl score(X, Y) bound [/name, /number]. +score(/alice, 10). +score(/alice, 20). +Decl total_score(X, Y) bound [/name, /number]. +total_score(Name, Total) :- score(Name, _) |> do fn:group_by(Name), let Total = fn:sum(Name). +` + results := lintSource(t, source) + filtered := filterByRule(results, "singleton-variable") + // Total is in transform output AND head, Name is in premises+head+transform. + // None should be singletons. + for _, f := range filtered { + if strings.Contains(f.Message, "Total") { + t.Errorf("transform output variable 'Total' should not be flagged: %s", f.Message) + } + } +} + +// --- Unused Predicate Rule Tests --- + +func TestUnusedPredicate_Triggers(t *testing.T) { + source := ` +Decl unused_pred(X) bound [/name]. +Decl used_pred(X) bound [/name]. +used_pred(/x). +foo(X) :- used_pred(X). +` + results := lintSource(t, source) + filtered := filterByRule(results, "unused-predicate") + if len(filtered) != 1 { + t.Errorf("got %d findings, want 1: %v", len(filtered), filtered) + } + if len(filtered) > 0 && !strings.Contains(filtered[0].Message, "unused_pred") { + t.Errorf("expected message to mention 'unused_pred', got: %s", filtered[0].Message) + } +} + +func TestUnusedPredicate_AllUsed(t *testing.T) { + source := ` +Decl bar(X) bound [/name]. +bar(/x). +foo(X) :- bar(X). +` + results := lintSource(t, source) + filtered := filterByRule(results, "unused-predicate") + if len(filtered) != 0 { + t.Errorf("got %d findings, want 0: %v", len(filtered), filtered) + } +} + +// --- Dead Code Rule Tests --- + +func TestDeadCode_Triggers(t *testing.T) { + source := ` +bar(/x). +dead(X) :- bar(X). +` + results := lintSource(t, source) + filtered := filterByRule(results, "dead-code") + if len(filtered) != 1 { + t.Errorf("got %d findings, want 1: %v", len(filtered), filtered) + } + if len(filtered) > 0 && !strings.Contains(filtered[0].Message, "dead") { + t.Errorf("expected message to mention 'dead', got: %s", filtered[0].Message) + } +} + +func TestDeadCode_Consumed(t *testing.T) { + source := ` +bar(/x). +intermediate(X) :- bar(X). +result(X) :- intermediate(X). +` + results := lintSource(t, source) + filtered := filterByRule(results, "dead-code") + // "intermediate" is consumed by "result", so only "result" is dead code. + for _, f := range filtered { + if strings.Contains(f.Message, "intermediate") { + t.Errorf("intermediate should not be flagged as dead code: %s", f.Message) + } + } +} + +// --- Config Tests --- + +func TestDisableRule(t *testing.T) { + source := ` +bar(/x, /y). +foo(X) :- bar(X, Typo). +` + cfg := DefaultConfig() + cfg.DisabledRules = map[string]bool{"singleton-variable": true} + results := lintSource(t, source, cfg) + filtered := filterByRule(results, "singleton-variable") + if len(filtered) != 0 { + t.Errorf("disabled rule should produce 0 findings, got %d", len(filtered)) + } +} + +func TestMinSeverity(t *testing.T) { + source := ` +bar(/x). +dead(X) :- bar(X). +` + cfg := DefaultConfig() + cfg.MinSeverity = SeverityWarning + results := lintSource(t, source, cfg) + for _, r := range results { + if r.Severity < SeverityWarning { + t.Errorf("found result below min severity: %v", r) + } + } +} + +// --- Output Tests --- + +func TestFormatText(t *testing.T) { + results := []LintResult{ + {RuleName: "test-rule", Severity: SeverityWarning, File: "foo.mg", Message: "test message"}, + } + var buf bytes.Buffer + FormatText(&buf, results) + out := buf.String() + if !strings.Contains(out, "foo.mg") || !strings.Contains(out, "warning") || !strings.Contains(out, "test message") { + t.Errorf("unexpected text output: %s", out) + } +} + +func TestFormatJSON(t *testing.T) { + results := []LintResult{ + {RuleName: "test-rule", Severity: SeverityWarning, File: "foo.mg", Message: "test message"}, + } + var buf bytes.Buffer + if err := FormatJSON(&buf, results); err != nil { + t.Fatal(err) + } + out := buf.String() + if !strings.Contains(out, `"warning"`) || !strings.Contains(out, `"test message"`) { + t.Errorf("unexpected JSON output: %s", out) + } +} + +func TestFormatJSON_Empty(t *testing.T) { + var buf bytes.Buffer + if err := FormatJSON(&buf, nil); err != nil { + t.Fatal(err) + } + out := strings.TrimSpace(buf.String()) + if out != "[]" { + t.Errorf("expected empty JSON array, got: %s", out) + } +} + +// --- Integration Tests --- + +func TestExamplesNoErrors(t *testing.T) { + // All example .mg files should parse and lint without hard errors. + // We allow info/warning findings but not analysis errors. + examplesDir := "../examples" + if _, err := os.Stat(examplesDir); os.IsNotExist(err) { + t.Skip("examples directory not found, skipping integration test") + } + matches, err := filepath.Glob(filepath.Join(examplesDir, "*.mg")) + if err != nil { + t.Fatal(err) + } + if len(matches) == 0 { + t.Skip("no .mg files found in examples directory") + } + + linter := NewLinter(DefaultConfig()) + for _, path := range matches { + t.Run(filepath.Base(path), func(t *testing.T) { + results, err := linter.LintFile(path) + if err != nil { + t.Fatalf("lint error on %s: %v", path, err) + } + // Check that no result is an analysis error (which would indicate + // the linter pipeline broke on valid code). + for _, r := range results { + if r.RuleName == "analysis" { + t.Errorf("analysis error on valid example %s: %s", path, r.Message) + } + } + }) + } +} + +// --- Severity Tests --- + +func TestParseSeverity(t *testing.T) { + tests := []struct { + input string + want Severity + }{ + {"info", SeverityInfo}, + {"warning", SeverityWarning}, + {"error", SeverityError}, + {"unknown", SeverityInfo}, + } + for _, tt := range tests { + got := ParseSeverity(tt.input) + if got != tt.want { + t.Errorf("ParseSeverity(%q) = %v, want %v", tt.input, got, tt.want) + } + } +} + +func TestSeverityString(t *testing.T) { + tests := []struct { + sev Severity + want string + }{ + {SeverityInfo, "info"}, + {SeverityWarning, "warning"}, + {SeverityError, "error"}, + } + for _, tt := range tests { + got := tt.sev.String() + if got != tt.want { + t.Errorf("Severity(%d).String() = %q, want %q", tt.sev, got, tt.want) + } + } +} diff --git a/lint/output.go b/lint/output.go new file mode 100644 index 0000000..4501d0f --- /dev/null +++ b/lint/output.go @@ -0,0 +1,43 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import ( + "encoding/json" + "fmt" + "io" +) + +// FormatText writes lint results in human-readable text format. +func FormatText(w io.Writer, results []LintResult) { + for _, r := range results { + loc := r.File + if loc == "" { + loc = "" + } + fmt.Fprintf(w, "%s: [%s] %s: %s\n", loc, r.Severity, r.RuleName, r.Message) + } +} + +// FormatJSON writes lint results in JSON format. +func FormatJSON(w io.Writer, results []LintResult) error { + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + // Ensure we output an empty array rather than null for zero results. + if results == nil { + results = []LintResult{} + } + return enc.Encode(results) +} diff --git a/lint/rules.go b/lint/rules.go new file mode 100644 index 0000000..35217ad --- /dev/null +++ b/lint/rules.go @@ -0,0 +1,127 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lint + +import ( + "github.com/google/mangle/ast" +) + +// LintRule is the interface every lint check implements. +type LintRule interface { + // Name returns the unique, hyphen-separated rule name. + Name() string + // Description returns a one-line description suitable for --list-rules. + Description() string + // DefaultSeverity returns the severity level when no override is configured. + DefaultSeverity() Severity + // Check runs the lint check against the input and returns zero or more findings. + Check(input *LintInput, config LintConfig) []LintResult +} + +// AllRules returns all built-in lint rules. +func AllRules() []LintRule { + return []LintRule{ + &UnusedPredicateRule{}, + &MissingDocRule{}, + &NamingConventionRule{}, + &SingletonVariableRule{}, + &OverlyComplexRule{}, + &DeadCodeRule{}, + } +} + +// extractPredicatesFromTerm returns all predicate symbols referenced in a term. +func extractPredicatesFromTerm(term ast.Term) []ast.PredicateSym { + switch p := term.(type) { + case ast.Atom: + return []ast.PredicateSym{p.Predicate} + case ast.NegAtom: + return []ast.PredicateSym{p.Atom.Predicate} + case ast.TemporalLiteral: + return extractPredicatesFromTerm(p.Literal) + case ast.TemporalAtom: + return []ast.PredicateSym{p.Atom.Predicate} + default: + return nil + } +} + +// isUserPredicate returns true if the predicate is user-defined. +func isUserPredicate(pred ast.PredicateSym) bool { + return !pred.IsBuiltin() && !pred.IsInternalPredicate() && + pred.Symbol != "Package" && pred.Symbol != "Use" +} + +// collectVariablesByTerm counts how many distinct terms reference each variable +// in a clause. This counts at the term level: head counts as 1, each premise +// counts as 1, each transform statement counts as 1. +func collectVariablesByTerm(clause ast.Clause) map[ast.Variable]int { + counts := make(map[ast.Variable]int) + + // Count variables in head. + headVars := make(map[ast.Variable]bool) + ast.AddVars(clause.Head, headVars) + if clause.HeadTime != nil { + addIntervalVars(*clause.HeadTime, headVars) + } + for v := range headVars { + counts[v]++ + } + + // Count variables in each premise separately. + for _, p := range clause.Premises { + premVars := make(map[ast.Variable]bool) + ast.AddVars(p, premVars) + for v := range premVars { + counts[v]++ + } + } + + // Count variables in transform. + if clause.Transform != nil { + addTransformVars(clause.Transform, counts) + } + + return counts +} + +func addTransformVars(t *ast.Transform, counts map[ast.Variable]int) { + for _, stmt := range t.Statements { + // Count the output variable (e.g., Sum in "Sum = fn:sum(Z)"). + if stmt.Var != nil { + counts[*stmt.Var]++ + } + // Count the input variables in the function arguments. + stmtVars := make(map[ast.Variable]bool) + for _, arg := range stmt.Fn.Args { + ast.AddVars(arg, stmtVars) + } + for v := range stmtVars { + counts[v]++ + } + } + if t.Next != nil { + addTransformVars(t.Next, counts) + } +} + +func addIntervalVars(interval ast.Interval, m map[ast.Variable]bool) { + if interval.Start.Type == ast.VariableBound { + m[interval.Start.Variable] = true + } + if interval.End.Type == ast.VariableBound { + m[interval.End.Variable] = true + } +}