-
Notifications
You must be signed in to change notification settings - Fork 2.2k
docs+build: introduce ast-grep to encode some of the style guide elements not captured by the linter #10393
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
base: master
Are you sure you want to change the base?
docs+build: introduce ast-grep to encode some of the style guide elements not captured by the linter #10393
Changes from all commits
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 |
|---|---|---|
|
|
@@ -352,7 +352,23 @@ lint-config-check: | |
| $(GOTOOL) $(GOLINT_PKG) config verify -v | ||
|
|
||
| #? lint: Run static code analysis | ||
| lint: check-go-version lint-config-check lint-source | ||
| lint: check-go-version lint-config-check lint-source | ||
|
|
||
| # Globs to exclude generated files from ast-grep. | ||
| AST_GREP_EXCLUDE := --globs '!**/*.pb.go' --globs '!**/*.pb.gw.go' --globs '!**/*.pb.json.go' --globs '!**/sqlc/*.go' | ||
|
|
||
| # Optional directory/package filter for ast-grep (e.g., make ast-lint pkg=lnwallet). | ||
| AST_GREP_PATH := $(if $(pkg),$(pkg),.) | ||
|
|
||
| #? ast-lint: Run ast-grep style checks. Use pkg=<dir> to focus on a specific directory. | ||
| ast-lint: | ||
| @$(call print, "Running ast-grep style checks.") | ||
| sg scan $(AST_GREP_EXCLUDE) $(AST_GREP_PATH) | ||
|
|
||
| #? ast-grep-fix: Auto-fix ast-grep style issues. Use pkg=<dir> to focus on a specific directory. | ||
| ast-grep-fix: | ||
|
Member
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. Tried |
||
| @$(call print, "Auto-fixing ast-grep style issues.") | ||
| sg scan --update-all $(AST_GREP_EXCLUDE) $(AST_GREP_PATH) | ||
|
|
||
| #? protolint: Lint proto files using protolint | ||
| protolint: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| # Go formatting rules for multi-line function calls and struct literals. | ||
| # Based on docs/development_guidelines.md. | ||
| # | ||
| # These rules detect asymmetric closing braces/parens where the closing | ||
| # character is on the same line as content instead of its own line. | ||
|
|
||
| # Rule: Multi-line struct literal with closing brace on same line as field. | ||
| # This catches patterns like: | ||
| # &Struct{ | ||
| # Field: value} // WRONG - } on same line as field | ||
| # | ||
| # Instead of: | ||
| # &Struct{ | ||
| # Field: value, | ||
| # } // RIGHT - } on its own line | ||
| id: struct-literal-asymmetric-close | ||
| language: go | ||
| severity: warning | ||
| message: "Struct literal closing brace should be on its own line" | ||
| note: | | ||
| Per docs/development_guidelines.md: When a struct literal spans multiple lines, | ||
| the closing } should be on its own line at the same indentation level | ||
| as the opening line. | ||
| WRONG: | ||
| foo(&Bar{ | ||
| Field: value}) | ||
| RIGHT: | ||
| foo(&Bar{ | ||
| Field: value, | ||
| }) | ||
| rule: | ||
| kind: composite_literal | ||
| all: | ||
| # Must span multiple lines (contains newline) | ||
| - regex: "\\{[\\s\\S]*\\n[\\s\\S]*\\}" | ||
| # Must have content immediately before closing brace (no trailing comma) | ||
| - regex: "[a-zA-Z0-9_\"')}]\\}$" | ||
| --- | ||
| # Rule: Multi-line function call with asymmetric wrapping. | ||
| # Detects calls where first arg is on same line as opening paren but | ||
| # closing paren is on same line as last arg (asymmetric). | ||
| # | ||
| # EXCLUDES log/error calls which have different formatting rules per guidelines. | ||
| id: func-call-asymmetric-wrap | ||
| language: go | ||
| severity: warning | ||
| message: "Multi-line function call has asymmetric wrapping" | ||
| note: | | ||
| Per docs/development_guidelines.md: When wrapping function calls, | ||
| if the first arg is on the opening line, the closing paren | ||
| should also be on its own line OR all args should fit on one line. | ||
| WRONG: | ||
| value, err := bar(a, | ||
| b, c) | ||
| RIGHT (option 1 - all on new lines): | ||
| value, err := bar( | ||
| a, b, c, | ||
| ) | ||
| RIGHT (option 2 - all on one line if fits): | ||
| value, err := bar(a, b, c) | ||
| NOTE: Log and error calls are excluded from this rule per guidelines. | ||
| rule: | ||
| kind: call_expression | ||
| all: | ||
| # Must span multiple lines | ||
| - regex: "\\n" | ||
| # Must have first arg on same line as open paren: foo(arg | ||
| - regex: "\\([^\\n\\)]+," | ||
| # Must end with arg immediately before close paren (no trailing comma on own line) | ||
| - regex: "[a-zA-Z0-9_\"'\\)\\]]\\)$" | ||
| not: | ||
| any: | ||
| # Exclude log calls (compact form is correct for these) | ||
| - regex: "\\.(Trace|Debug|Info|Warn|Error|Fatal|Critical)(f|S)?\\(" | ||
| # Exclude fmt.Errorf and fmt.Printf | ||
| - regex: "fmt\\.(Errorf|Printf|Sprintf)\\(" | ||
| # Exclude testing log calls (t.Logf, h.Logf, etc.) | ||
| - regex: "\\.(Logf|Log)\\(" | ||
| --- | ||
| # Rule: Log/error calls using expanded form instead of compact. | ||
| # Per guidelines, log and error formatting should minimize lines. | ||
| id: log-error-expanded-form | ||
| language: go | ||
| severity: hint | ||
|
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. The severity for this rule is set to severity: warningStyle Guide ReferencesFootnotes
|
||
| message: "Log/error calls should use compact form, not expanded" | ||
| note: | | ||
| Per docs/development_guidelines.md: Log and error formatting should | ||
| minimize lines while staying under 80 chars. | ||
| WRONG: | ||
| return fmt.Errorf( | ||
| "message", | ||
| arg, | ||
| ) | ||
| RIGHT: | ||
| return fmt.Errorf("message "+ | ||
| "continued", arg) | ||
| rule: | ||
| kind: call_expression | ||
| all: | ||
| - any: | ||
| - pattern: fmt.Errorf($$$ARGS) | ||
| - pattern: fmt.Printf($$$ARGS) | ||
| - pattern: $LOG.Debugf($$$ARGS) | ||
| - pattern: $LOG.Infof($$$ARGS) | ||
| - pattern: $LOG.Warnf($$$ARGS) | ||
| - pattern: $LOG.Errorf($$$ARGS) | ||
| - pattern: $LOG.Tracef($$$ARGS) | ||
| # Starts with open paren followed by newline (expanded form) | ||
| - regex: "f\\(\\s*\\n" | ||
| --- | ||
| # Rule: Switch/select cases should be separated by blank lines. | ||
| # Detects consecutive case clauses without blank line spacing. | ||
| id: switch-case-needs-spacing | ||
| language: go | ||
| severity: hint | ||
|
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. The severity for this rule is severity: warningStyle Guide ReferencesFootnotes
|
||
| message: "Switch/select cases should be separated by blank lines" | ||
| note: | | ||
| Per docs/development_guidelines.md: We favor spacing between stanzas within | ||
| switch case statements and select statements. | ||
| WRONG: | ||
| switch x { | ||
| case a: | ||
| <code> | ||
| case b: | ||
| <code> | ||
| } | ||
| RIGHT: | ||
| switch x { | ||
| // Comment for case a. | ||
| case a: | ||
| <code> | ||
| case b: | ||
| <code> | ||
| } | ||
| rule: | ||
| any: | ||
| - kind: expression_switch_statement | ||
| - kind: type_switch_statement | ||
| # Detects case/default following } or ) without blank line | ||
| # Pattern: closing brace/paren, newline, tab, then case/default keyword | ||
| regex: "[)}]\\n\\t(case|default)" | ||
| --- | ||
| # Rule: Select cases should be separated by blank lines. | ||
| id: select-case-needs-spacing | ||
| language: go | ||
| severity: hint | ||
|
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. Similar to the severity: warningStyle Guide ReferencesFootnotes
|
||
| message: "Select cases should be separated by blank lines" | ||
| note: | | ||
| Per docs/development_guidelines.md: We favor spacing between stanzas within | ||
| select statements. | ||
| WRONG: | ||
| select { | ||
| case <-ch1: | ||
| <code> | ||
| case <-ch2: | ||
| <code> | ||
| } | ||
| RIGHT: | ||
| select { | ||
| case <-ch1: | ||
| <code> | ||
| case <-ch2: | ||
| <code> | ||
| } | ||
| rule: | ||
| kind: select_statement | ||
| # Detects case/default following } or ) without blank line | ||
| regex: "[)}]\\n\\t(case|default)" | ||
| --- | ||
| # Rule: No inline comments - comments should be on their own line. | ||
| # Detects inline comments by matching composite literals that contain | ||
| # lines with code followed by // comment on the same line. | ||
| id: no-inline-comments | ||
| language: go | ||
| severity: warning | ||
| message: "Inline comment should be on its own line above the code" | ||
| note: | | ||
| Comments should be on their own line above the code they describe, | ||
| not at the end of a line. | ||
| WRONG: | ||
| ChanStatusDefault, // Default status | ||
| Amount: 100, // The amount in satoshis. | ||
| RIGHT: | ||
| // Default status. | ||
| ChanStatusDefault, | ||
| // The amount in satoshis. | ||
| Amount: 100, | ||
| rule: | ||
| kind: literal_value | ||
| has: | ||
| kind: comment | ||
| # Match literals containing inline comments: code followed by // on same line. | ||
| # The key is [^\n] before // ensures they're on the same line. | ||
| # Pattern: (something other than newline/whitespace), optional spaces, // comment, newline | ||
| regex: "[a-zA-Z0-9_)\"'\\]},][ \\t]*//[^\\n]*\\n" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| # Go function definition formatting rules. | ||
| # Based on docs/development_guidelines.md. | ||
| # | ||
| # These rules detect improper wrapping patterns in function definitions. | ||
|
|
||
| # Rule: Function parameters ending with dangling comma before close paren. | ||
| id: func-def-dangling-param-comma | ||
| language: go | ||
| severity: warning | ||
| message: "Function parameters should not end with dangling comma before close paren" | ||
| note: | | ||
| Per docs/development_guidelines.md: Function definition wrapping should | ||
| continue parameters on the next line, not leave a dangling comma. | ||
|
|
||
| WRONG: | ||
| func foo(a, b, c, | ||
| ) (d, error) { | ||
|
|
||
| RIGHT: | ||
| func foo(a, b, | ||
| c) (d, error) { | ||
| rule: | ||
| kind: function_declaration | ||
| all: | ||
| # Has parameters that end with comma-newline-close-paren-open-paren (for returns) | ||
| - regex: ",\\s*\\n\\s*\\)\\s*\\(" | ||
| --- | ||
| # Rule: Function return types starting with open paren on its own line. | ||
| id: func-def-dangling-return-paren | ||
| language: go | ||
| severity: warning | ||
| message: "Function return types should not start with open paren on its own line" | ||
| note: | | ||
| Per docs/development_guidelines.md: Return type wrapping should continue | ||
| on the same line, not start a new line with open paren. | ||
|
|
||
| WRONG: | ||
| func bar(a, b, c) ( | ||
| d, error, | ||
| ) { | ||
|
|
||
| RIGHT: | ||
| func baz(a, b, c) (d, | ||
| error) { | ||
| rule: | ||
| kind: function_declaration | ||
| all: | ||
| # Return type starts with close-paren-open-paren-newline | ||
| - regex: "\\)\\s*\\(\\s*\\n" | ||
| # NOTE: The "multi-line function needs blank line before body" rule is disabled. | ||
| # It's difficult to implement correctly with ast-grep because: | ||
| # 1. The regex matches ANY block inside the function (switch, if, etc.), not | ||
| # just the function's opening brace | ||
| # 2. AST doesn't represent blank lines as nodes - they're only in text | ||
| # 3. The rule would need to match the exact position of text after the { | ||
| # This is a style hint in the guidelines, not a hard requirement. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ruleDirs: | ||
| - rules |
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.
Looks like it giving false positives, or should we update our guidelines to be more specific?