diff --git a/docs/checks.md b/docs/checks.md index 81ecf71..a84be69 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -89,7 +89,7 @@ Each top-level boolean enables or disables an entire check family. `quality_rules.local_precision` controls the local-quality precision subset currently cataloged in the [Local quality precision glossary](#local-quality-precision-glossary), including the shipped `naming.*`, `function.*`, `error.*`, `defensive.*`, -selected `maintainability.*`, and history-aware `smell.*` rule IDs. It defaults +selected `maintainability.*`, and structural/history-aware `smell.*` rule IDs. It defaults to enabled, but repositories can set it to `false` while they refactor legacy hotspots or avoid broad historical noise in full self-scans. @@ -1105,7 +1105,7 @@ Current detector rollout: - Implemented `Change Safety / Testability` detectors: `testing.behavior-change-without-test`, `testing.failure-path-missing`, `testing.hardwired-dependency`, `testing.nondeterministic-domain-logic`, and `testing.legacy-hotspot-uncovered` for Go, Python, TypeScript, JavaScript, and C++ path/text evidence. `testing.legacy-hotspot-uncovered` uses bounded local git history and skips when reliable history/hotspot inputs are unavailable. - Implemented `Change Safety / Refactors` detectors: the direct `refactor.*` family below has stable metadata, language coverage, fix templates, config toggles, and diff-mode safe-refactor detector tests. - Implemented local-quality support rules live in the `Code Quality` section and are cataloged in the local precision glossary below, including shipped `naming.*`, `function.*`, `error.*`, `defensive.*`, and `maintainability.*` IDs. -- Implemented structural smell rules live in the `Code Quality` section: `smell.god-object`, `smell.feature-envy`, `smell.middle-man`, `smell.message-chain`, `smell.data-clump`, and `smell.switch-on-type`. +- Implemented structural smell rules live in the `Code Quality` section: `smell.god-object`, `smell.feature-envy`, `smell.middle-man`, `smell.message-chain`, `smell.data-clump`, `smell.switch-on-type`, and `smell.refused-bequest`. - Implemented history-aware maintainability/smell rules live in `Code Quality`-adjacent report sections and skip when git history is unavailable: `maintainability.hotspot`, `maintainability.high-churn-hotspot`, `maintainability.repeat-defect-area`, `maintainability.unstable-interface`, `maintainability.change-amplification`, `smell.shotgun-surgery-history`, and `smell.divergent-change-history`. Cataloged rule glossary: @@ -1200,6 +1200,7 @@ These rules live outside the repository-wide `Change Safety` section in report o | Structural smell | `smell.message-chain` | warn | Code reaches through a long chain of collaborators, increasing coupling to object structure. | | Structural smell | `smell.data-clump` | warn | The same group of primitive/domain parameters appears repeatedly across functions. | | Structural smell | `smell.switch-on-type` | warn | Behavior repeatedly branches on type/kind/discriminator checks that should move behind polymorphism or dispatch. | +| Structural smell | `smell.refused-bequest` | warn | A derived/embedded type inherits a contract but refuses multiple inherited-style methods with no-op or unsupported implementations. | | Maintainability delta | `maintainability.public-surface-growth` | warn | A changed file exports more public symbols than it did at the base ref. | | Maintainability delta | `maintainability.dependency-growth` | warn | A changed file imports or includes more direct dependencies than it did at the base ref. | | Maintainability history | `maintainability.hotspot` | warn | A changed file has high recent churn, defect history, or both. | diff --git a/docs/features.md b/docs/features.md index bd16368..99bfc23 100644 --- a/docs/features.md +++ b/docs/features.md @@ -133,7 +133,8 @@ Imported reports are never passed to AI triage. - Diff-mode change safety - uses the `checks.change` family to report implemented change-safety, cleanup, testability, and safe-refactor findings - emits PR-summary fields such as `change_safety`, `refactor_confidence`, and `maintainability_delta` only as artifact evidence; they do not create extra annotations or change per-rule severities - - the local-quality precision rollout supports the same review goal through the exact `naming.*`, `function.*`, `error.*`, `defensive.*`, selected `maintainability.*`, and history-aware `smell.*` IDs listed by `codeguard rules` on the active build + - the local-quality precision rollout supports the same review goal through the exact `naming.*`, `function.*`, `error.*`, `defensive.*`, selected `maintainability.*`, and structural/history-aware `smell.*` IDs listed by `codeguard rules` on the active build + - structural smell rules such as `smell.refused-bequest` are reported as change-quality evidence: they explain when a design shape makes future changes less safe or harder to isolate, not just whether an individual line is syntactically suspicious ## Parsers diff --git a/internal/codeguard/checks/quality/quality_smells.go b/internal/codeguard/checks/quality/quality_smells.go index 8025140..fda398c 100644 --- a/internal/codeguard/checks/quality/quality_smells.go +++ b/internal/codeguard/checks/quality/quality_smells.go @@ -22,25 +22,25 @@ const ( ) var ( - pythonClassPattern = regexp.MustCompile(`^(\s*)class\s+([A-Za-z_]\w*)\b`) - pythonMethodPattern = regexp.MustCompile(`^(\s*)(?:async\s+)?def\s+([A-Za-z_]\w*)\s*\(([^)]*)\)\s*:`) - clikeClassPattern = regexp.MustCompile(`(?m)^[ \t]*(?:export[ \t]+)?(?:default[ \t]+)?(?:class|struct)[ \t]+([A-Za-z_$][\w$]*)[^{;]*\{`) - clikeMethodLinePattern = regexp.MustCompile(`^[ \t]*(?:(?:public|private|protected|static|async|virtual|override|inline|constexpr|const|explicit|final)\s+)*(?:[~A-Za-z_$][\w$:<>,*&\s]+\s+)?([~A-Za-z_$][\w$]*)\s*\(([^)]*)\)\s*(?:const\s*)?(?:override\s*)?(?:noexcept\s*)?\{`) - clikeFieldLinePattern = regexp.MustCompile(`^[ \t]*(?:(?:public|private|protected|static|readonly|mutable|const|let|var|final)\s+)*(?:[A-Za-z_$][\w$:<>,.?*&\[\]]+\s+)?([A-Za-z_$][\w$]*)\s*(?::[^=;]+)?(?:=[^;]+)?;`) - delegateReceiverPattern = regexp.MustCompile(`(?:return\s+)?(?:self|this|[a-zA-Z_]\w*)[.\->]+(_?[A-Za-z_]\w*)[.\->]+[A-Za-z_]\w*\s*\(`) - delegateLocalPattern = regexp.MustCompile(`(?:return\s+)?(_?[A-Za-z_]\w*)[.\->]+[A-Za-z_]\w*\s*\(`) - goKindSwitchPattern = regexp.MustCompile(`(?m)switch\s+[^{}\n]*(?:\.|_)?(?:kind|type|Kind|Type)\b`) - pythonKindBranchPattern = regexp.MustCompile(`(?m)\b(?:if|elif)\s+[^:\n]*(?:\.|_)?(?:kind|type)\b[^:\n]*(?:==| in )`) - scriptKindSwitchPattern = regexp.MustCompile(`(?m)switch\s*\([^)]*(?:\.|_)?(?:kind|type|Kind|Type)\b[^)]*\)`) - cppKindSwitchPattern = regexp.MustCompile(`(?m)switch\s*\([^)]*(?:\.|_)?(?:kind|type|Kind|Type)\b[^)]*\)`) - typeBranchPattern = regexp.MustCompile(`(?m)(?:\.\(type\)|\btypeid\s*\(|\bdynamic_cast\s*<|\binstanceof\b|\btypeof\b|\bisinstance\s*\(|\btype\s*\()`) - refusedBequestNoopRegexp = regexp.MustCompile(`(?i)\b(unsupported|not\s+implemented|notimplemented|throw\s+new\s+error|raise\s+notimplemented|panic\s*\()`) + pythonClassPattern = regexp.MustCompile(`^(\s*)class\s+([A-Za-z_]\w*)\s*(?:\(([^)]*)\))?\s*:`) + pythonMethodPattern = regexp.MustCompile(`^(\s*)(?:async\s+)?def\s+([A-Za-z_]\w*)\s*\(([^)]*)\)\s*:`) + clikeClassPattern = regexp.MustCompile(`(?m)^[ \t]*(?:export[ \t]+)?(?:default[ \t]+)?(?:class|struct)[ \t]+([A-Za-z_$][\w$]*)([^{;]*)\{`) + clikeMethodLinePattern = regexp.MustCompile(`^[ \t]*(?:(?:public|private|protected|static|async|virtual|override|inline|constexpr|const|explicit|final)\s+)*(?:[~A-Za-z_$][\w$:<>,*&\s]+\s+)?([~A-Za-z_$][\w$]*)\s*\(([^)]*)\)\s*(?:const\s*)?(?:override\s*)?(?:noexcept\s*)?\{`) + clikeFieldLinePattern = regexp.MustCompile(`^[ \t]*(?:(?:public|private|protected|static|readonly|mutable|const|let|var|final)\s+)*(?:[A-Za-z_$][\w$:<>,.?*&\[\]]+\s+)?([A-Za-z_$][\w$]*)\s*(?::[^=;]+)?(?:=[^;]+)?;`) + delegateReceiverPattern = regexp.MustCompile(`(?:return\s+)?(?:self|this|[a-zA-Z_]\w*)[.\->]+(_?[A-Za-z_]\w*)[.\->]+[A-Za-z_]\w*\s*\(`) + delegateLocalPattern = regexp.MustCompile(`(?:return\s+)?(_?[A-Za-z_]\w*)[.\->]+[A-Za-z_]\w*\s*\(`) + goKindSwitchPattern = regexp.MustCompile(`(?m)switch\s+[^{}\n]*(?:\.|_)?(?:kind|type|Kind|Type)\b`) + pythonKindBranchPattern = regexp.MustCompile(`(?m)\b(?:if|elif)\s+[^:\n]*(?:\.|_)?(?:kind|type)\b[^:\n]*(?:==| in )`) + scriptKindSwitchPattern = regexp.MustCompile(`(?m)switch\s*\([^)]*(?:\.|_)?(?:kind|type|Kind|Type)\b[^)]*\)`) + cppKindSwitchPattern = regexp.MustCompile(`(?m)switch\s*\([^)]*(?:\.|_)?(?:kind|type|Kind|Type)\b[^)]*\)`) + typeBranchPattern = regexp.MustCompile(`(?m)(?:\.\(type\)|\btypeid\s*\(|\bdynamic_cast\s*<|\binstanceof\b|\btypeof\b|\bisinstance\s*\(|\btype\s*\()`) ) type structuralClass struct { Name string StartLine int EndLine int + Bases []string Fields []string Methods []structuralFunction } @@ -80,6 +80,7 @@ func structuralSmellFindings(env support.Context, file string, source string, la findings = append(findings, messageChainFindings(env, file, source, language)...) findings = append(findings, dataClumpFindings(env, file, functions)...) findings = append(findings, switchOnTypeFindings(env, file, source, language)...) + findings = append(findings, refusedBequestFindings(env, file, classes, language)...) return findings } @@ -98,7 +99,9 @@ func goStructuralModel(fset *token.FileSet, parsed *ast.File, data []byte) ([]st if structType, ok := typeSpec.Type.(*ast.StructType); ok && structType.Fields != nil { for _, field := range structType.Fields.List { if len(field.Names) == 0 { - class.Fields = append(class.Fields, goExprText(field.Type)) + embedded := goExprText(field.Type) + class.Fields = append(class.Fields, embedded) + class.Bases = append(class.Bases, strings.TrimPrefix(strings.TrimPrefix(embedded, "*"), "[]")) continue } for _, name := range field.Names { @@ -190,7 +193,7 @@ func pythonStructuralClasses(masked string) []structuralClass { continue } classIndent := len(match[1]) - class := structuralClass{Name: match[2], StartLine: idx + 1, EndLine: len(maskedLines)} + class := structuralClass{Name: match[2], StartLine: idx + 1, EndLine: len(maskedLines), Bases: parseBaseList(match[3])} end := len(maskedLines) for scan := idx + 1; scan < len(maskedLines); scan++ { trimmed := strings.TrimSpace(maskedLines[scan]) @@ -267,6 +270,7 @@ func clikeStructuralClasses(source string, masked string) []structuralClass { Name: masked[match[2]:match[3]], StartLine: startLine, EndLine: support.LineNumberForOffset(source, bodyEnd), + Bases: clikeBaseList(masked[match[4]:match[5]]), } class.Fields = clikeClassFields(bodyMasked) class.Methods = clikeClassMethods(bodySource, bodyMasked, startLine, class.Name) diff --git a/internal/codeguard/checks/quality/quality_smells_refused_bequest.go b/internal/codeguard/checks/quality/quality_smells_refused_bequest.go new file mode 100644 index 0000000..c7c1608 --- /dev/null +++ b/internal/codeguard/checks/quality/quality_smells_refused_bequest.go @@ -0,0 +1,156 @@ +package quality + +import ( + "fmt" + "regexp" + "sort" + "strings" + + "github.com/devr-tools/codeguard/internal/codeguard/checks/support" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +const smellRefusedBequestRuleID = "smell.refused-bequest" + +var ( + refusedBequestNoopRegexp = regexp.MustCompile(`(?i)\b(unsupported|not\s+implemented|notimplemented|throw\s+new\s+error|raise\s+notimplemented|panic\s*\()`) + refusedBequestBodyRegexp = regexp.MustCompile(`(?i)\b(?:unsupported|not\s+implemented|notimplemented|not\s+supported|todo\s*\(|panic\s*\(|throw\s+(?:new\s+)?(?:Error|Unsupported|std::runtime_error)|raise\s+(?:NotImplemented|NotImplementedError|RuntimeError))\b`) + refusedBequestNoopBody = regexp.MustCompile(`(?i)^\s*(?://.*|/\*.*\*/|#.*)?\s*(?:pass|return\s+None|return\s+nil|return\s+null|return\s+undefined|return\s*;?|continue\s*;?|break\s*;?)\s*$`) +) + +func refusedBequestFindings(env support.Context, file string, classes []structuralClass, language string) []core.Finding { + findings := make([]core.Finding, 0) + for _, class := range classes { + bases := uniqueStrings(class.Bases) + if len(bases) == 0 || len(class.Methods) < 2 { + continue + } + refused := make([]string, 0) + for _, method := range class.Methods { + if method.Name == "" || isConstructorLikeMethod(method.Name, class.Name, language) { + continue + } + if methodRefusesInheritedContract(method.Body) { + refused = append(refused, method.Name) + } + } + refused = uniqueStrings(refused) + if len(refused) < 2 { + continue + } + sort.Strings(refused) + bases = sanitizedEvidenceNames(bases) + methods := sanitizedEvidenceNames(refused) + findings = append(findings, env.NewFinding(support.FindingInput{ + RuleID: smellRefusedBequestRuleID, + Level: "warn", + Path: file, + Line: class.StartLine, + Column: 1, + Message: fmt.Sprintf("derived type %s inherits from %s but refuses %d inherited-style methods; prefer composition or split the contract so changes remain safer", class.Name, strings.Join(bases, ", "), len(refused)), + Confidence: core.ConfidenceHigh, + Metadata: map[string]string{ + "bases": strings.Join(bases, ","), + "refused_methods": strings.Join(methods, ","), + "refused_count": fmt.Sprintf("%d", len(refused)), + "change_signal": "inheritance-contract-friction", + }, + })) + } + return findings +} + +func methodRefusesInheritedContract(body string) bool { + trimmedLines := make([]string, 0) + for _, line := range strings.Split(body, "\n") { + trimmed := strings.TrimSpace(strings.TrimSuffix(line, ";")) + if trimmed == "" || trimmed == "{" || trimmed == "}" { + continue + } + trimmedLines = append(trimmedLines, trimmed) + } + if len(trimmedLines) == 0 { + return false + } + bodyText := strings.Join(trimmedLines, " ") + if refusedBequestBodyRegexp.MatchString(bodyText) { + return true + } + if len(trimmedLines) <= 2 { + for _, line := range trimmedLines { + if refusedBequestNoopBody.MatchString(line) { + return true + } + } + } + return false +} + +func isConstructorLikeMethod(methodName string, className string, language string) bool { + switch language { + case "python": + return strings.HasPrefix(methodName, "__") && strings.HasSuffix(methodName, "__") + default: + return methodName == className || methodName == "constructor" || strings.HasPrefix(methodName, "~") + } +} + +func sanitizedEvidenceNames(values []string) []string { + out := make([]string, 0, len(values)) + for _, value := range values { + value = strings.TrimSpace(value) + value = strings.Trim(value, "*&") + value = strings.TrimPrefix(value, "public ") + value = strings.TrimPrefix(value, "private ") + value = strings.TrimPrefix(value, "protected ") + if value == "" { + continue + } + out = append(out, value) + } + return uniqueStrings(out) +} + +func parseBaseList(text string) []string { + text = strings.TrimSpace(text) + if text == "" { + return nil + } + parts := splitTopLevelStructuralArgs(text) + bases := make([]string, 0, len(parts)) + for _, part := range parts { + part = strings.TrimSpace(part) + if part == "" { + continue + } + if strings.HasPrefix(part, "object") || strings.HasPrefix(part, "ABC") { + continue + } + bases = append(bases, part) + } + return bases +} + +func clikeBaseList(suffix string) []string { + suffix = strings.TrimSpace(suffix) + if suffix == "" { + return nil + } + baseText := "" + if idx := strings.Index(suffix, "extends"); idx >= 0 { + baseText = suffix[idx+len("extends"):] + if impl := strings.Index(baseText, "implements"); impl >= 0 { + baseText = baseText[:impl] + } + } else if idx := strings.Index(suffix, ":"); idx >= 0 { + baseText = suffix[idx+1:] + } else { + return nil + } + baseText = strings.ReplaceAll(baseText, "public ", "") + baseText = strings.ReplaceAll(baseText, "private ", "") + baseText = strings.ReplaceAll(baseText, "protected ", "") + baseText = strings.ReplaceAll(baseText, "virtual ", "") + baseText = strings.TrimSpace(baseText) + return parseBaseList(baseText) +} diff --git a/internal/codeguard/rules/catalog_fix_templates_quality.go b/internal/codeguard/rules/catalog_fix_templates_quality.go index 88da9e4..8cfeead 100644 --- a/internal/codeguard/rules/catalog_fix_templates_quality.go +++ b/internal/codeguard/rules/catalog_fix_templates_quality.go @@ -61,4 +61,5 @@ var qualityFixTemplates = map[string]core.FixTemplate{ "smell.message-chain": {Kind: guided, Text: "Hide deep traversal behind a named operation at the boundary.\n\nBefore:\ncountry := user.Account().Profile().Address().Country().Code()\n\nAfter:\ncountry := user.CountryCode()"}, "smell.data-clump": {Kind: guided, Text: "Extract repeated primitive parameters into a named value object or options type.\n\nBefore:\nfunc create(customerID string, orderID string, currency string)\nfunc update(customerID string, orderID string, currency string)\n\nAfter:\ntype OrderKey struct { CustomerID, OrderID, Currency string }\nfunc create(key OrderKey)"}, "smell.switch-on-type": {Kind: guided, Text: "Centralize type/kind dispatch or move behavior into polymorphic implementations.\n\nBefore:\nswitch event.Kind { case Created: handleCreated(event); case Updated: handleUpdated(event) }\n\nAfter:\nhandlers[event.Kind].Handle(event)\n// or event.HandleWith(handler) when the variants own behavior."}, + "smell.refused-bequest": {Kind: guided, Text: "Replace inheritance that refuses inherited behavior with composition or smaller contracts.\n\nBefore:\nclass ReadOnlyFile extends File {\n write() { throw new Error(\"unsupported\") }\n truncate() { throw new Error(\"unsupported\") }\n}\n\nAfter:\ninterface ReadableFile { read(): Data }\nclass ReadOnlyFile implements ReadableFile { read(): Data { ... } }\n// Write behavior lives only on a separate WritableFile capability."}, } diff --git a/internal/codeguard/rules/catalog_quality_smells.go b/internal/codeguard/rules/catalog_quality_smells.go index 1d6eee5..e7fe5e7 100644 --- a/internal/codeguard/rules/catalog_quality_smells.go +++ b/internal/codeguard/rules/catalog_quality_smells.go @@ -99,4 +99,20 @@ var qualitySmellCatalog = map[string]core.RuleMetadata{ Description: "Warns when behavior repeatedly branches on type/kind/discriminator checks that should move behind polymorphism or dispatch.", HowToFix: "Replace repeated type/kind branches with polymorphic methods, strategy objects, or a centralized dispatch table.", }, + "smell.refused-bequest": { + ID: "smell.refused-bequest", + Section: "Code Quality", + DefaultLevel: "warn", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + LanguageCoverage: core.FixedRuleLanguageCoverage( + core.RuleLanguageCPP, + core.RuleLanguageGo, + core.RuleLanguageJavaScript, + core.RuleLanguagePython, + core.RuleLanguageTypeScript, + ), + Title: "Refused bequest", + Description: "Warns when a derived/embedded type inherits a contract but explicitly refuses multiple inherited-style methods with no-op or unsupported implementations.", + HowToFix: "Prefer composition, split the inherited contract into smaller interfaces/classes, or move optional behavior behind capabilities so future changes do not rely on unsupported overrides.", + }, } diff --git a/tests/checks/quality_smells_test.go b/tests/checks/quality_smells_test.go index b14fd8b..17fa0b0 100644 --- a/tests/checks/quality_smells_test.go +++ b/tests/checks/quality_smells_test.go @@ -15,6 +15,7 @@ var structuralSmellRuleIDs = []string{ "smell.message-chain", "smell.data-clump", "smell.switch-on-type", + "smell.refused-bequest", } type structuralSmellCase struct { @@ -141,6 +142,10 @@ func goStructuralSmellPositiveCase() structuralSmellCase { "func cancelOrder(customerID string, orderID string, currency string) {}", "func handleOne(event Event) { switch event.Kind { case \"created\": case \"updated\": } }", "func handleTwo(event Event) { switch event.Kind { case \"deleted\": case \"archived\": } }", + "type Store struct{}", + "type ReadOnlyStore struct { Store }", + "func (ReadOnlyStore) Save() { panic(\"unsupported\") }", + "func (ReadOnlyStore) Delete() { panic(\"not implemented\") }", "type Customer struct { Profile Profile; Account Account }", "type Profile struct { Name string; Email string }", "type Account struct { Region string; Plan string; Status string }", @@ -207,6 +212,11 @@ func pythonStructuralSmellPositiveCase() structuralSmellCase { "def handle_two(event):", " if event.kind == 'deleted': pass", " elif event.kind == 'archived': pass", + "class ReadOnlyFile(File):", + " def write(self, value):", + " raise NotImplementedError('unsupported')", + " def truncate(self):", + " raise NotImplementedError('not supported')", }, "\n"), } } @@ -251,6 +261,10 @@ func cppStructuralSmellPositiveCase() structuralSmellCase { "void cancelOrder(String customerId, String orderId, String currency) {}", "void handleOne(Event event) { switch (event.kind) { case Created: break; case Updated: break; } }", "void handleTwo(Event event) { switch (event.kind) { case Deleted: break; case Archived: break; } }", + "class ReadOnlyFile : public File {", + " void write() override { throw std::runtime_error(\"unsupported\"); }", + " void truncate() override { throw std::runtime_error(\"not supported\"); }", + "};", }, "\n"), } } @@ -301,6 +315,11 @@ func scriptStructuralSmellSource(typed bool) string { "", "function handleOne(event) { switch (event.kind) { case 'created': break; case 'updated': break } }", "function handleTwo(event) { switch (event.kind) { case 'deleted': break; case 'archived': break } }", + "", + "class ReadOnlyFile extends File {", + " write(value" + paramType + ") { throw new Error('unsupported') }", + " truncate() { throw new Error('not supported') }", + "}", }, "\n") } diff --git a/tests/cli/features_metadata_test.go b/tests/cli/features_metadata_test.go index 0b8994b..d529e2b 100644 --- a/tests/cli/features_metadata_test.go +++ b/tests/cli/features_metadata_test.go @@ -199,6 +199,7 @@ func TestSDKRuleMetadataForStructuralSmellRules(t *testing.T) { "smell.message-chain", "smell.data-clump", "smell.switch-on-type", + "smell.refused-bequest", } { t.Run(ruleID, func(t *testing.T) { rule := requireRuleMetadata(t, ruleID)