Skip to content

Commit

Permalink
refactor, fix(nolint): Enhance Inline Detection (#128)
Browse files Browse the repository at this point in the history
# Description

Improve `nolint` comment scope detection and inline comment handling.

**Key changes:**

1. Enhanced inline comment detection
- Updated `isInlineComment` function to determine inline status based on
file offsets
- A comment is considered inline if it appears after a statement on the
same line
- Example: In `fmt.Println("foo") //nolint:rule1`, the comment is
properly
     detected as inline since it follows the statement

2. Refined scope determination rules
- For standalone comments (without code), the scope extends from the
comment line
     through the end of the next statement
- Comments appearing before the package declaration now apply to the
entire file

3. Refactored
    - Changed some type and variable names
  • Loading branch information
notJoon authored Feb 7, 2025
1 parent 50689d1 commit 35576c6
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 50 deletions.
107 changes: 60 additions & 47 deletions internal/nolint/nolint.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,33 +11,34 @@ const nolintPrefix = "//nolint"

// Manager manages nolint scopes and checks if a position is nolinted.
type Manager struct {
scopes map[string][]scope // filename to scopes
// scopes maps filename to a slice of nolint scopes.
scopes map[string][]nolintScope
}

// scope represents a range in the code where nolint applies.
type scope struct {
// nolintScope represents a range in the code where nolint applies.
type nolintScope struct {
rules map[string]struct{}
start token.Position
end token.Position
}

// ParseComments parses nolint comments in the given AST file and returns a nolintManager.
// ParseComments parses nolint comments in the given AST file and returns a Manager.
func ParseComments(f *ast.File, fset *token.FileSet) *Manager {
manager := Manager{
scopes: make(map[string][]scope, len(f.Comments)),
scopes: make(map[string][]nolintScope, len(f.Comments)),
}
stmtMap := indexStatementsByLine(f, fset)
packageLine := fset.Position(f.Package).Line

for _, cg := range f.Comments {
for _, comment := range cg.List {
scope, err := parseComment(comment, f, fset, stmtMap, packageLine)
ns, err := parseComment(comment, f, fset, stmtMap, packageLine)
if err != nil {
// ignore invalid nolint comments
continue
}
filename := scope.start.Filename
manager.scopes[filename] = append(manager.scopes[filename], scope)
filename := ns.start.Filename
manager.scopes[filename] = append(manager.scopes[filename], ns)
}
}
return &manager
Expand All @@ -50,83 +51,82 @@ func parseComment(
fset *token.FileSet,
stmtMap map[int]ast.Stmt,
packageLine int,
) (scope, error) {
var scope scope
) (nolintScope, error) {
var ns nolintScope
text := comment.Text

if !strings.HasPrefix(text, nolintPrefix) {
return scope, fmt.Errorf("invalid nolint comment")
return ns, fmt.Errorf("invalid nolint comment")
}

prefixLen := len(nolintPrefix)
rest := text[prefixLen:]

// A nolint comment can either have a list of rules after a colon (:)
// or if no rules are specified, it applies to all rules
if len(rest) > 0 && rest[0] != ':' {
return scope, fmt.Errorf("invalid nolint comment format")
return ns, fmt.Errorf("invalid nolint comment format")
}

if len(rest) > 0 && rest[0] == ':' {
rest = strings.TrimPrefix(rest, ":")
rest = strings.TrimSpace(rest)
if rest == "" {
return scope, fmt.Errorf("invalid nolint comment: no rules specified after colon")
return ns, fmt.Errorf("invalid nolint comment: no rules specified after colon")
}
} else if len(rest) > 0 {
return scope, fmt.Errorf("invalid nolint comment: expected colon after 'nolint'")
}

scope.rules = parseIgnoreRuleNames(rest)
ns.rules = parseIgnoreRuleNames(rest)
pos := fset.Position(comment.Slash)

// check if the comment is before the package declaration
// If the comment appears before the package declaration, apply it to the entire file
if isBeforePackageDecl(pos.Line, packageLine) {
scope.start = fset.Position(f.Pos())
scope.end = fset.Position(f.End())
return scope, nil
ns.start = fset.Position(f.Pos())
ns.end = fset.Position(f.End())
return ns, nil
}

// check if the comment is at the end of a line (inline comment)
if pos.Line == fset.File(comment.Slash).Line(comment.Slash) {
// Inline comment, applies to the statement on the same line
// Check if the comment is inline (appears after code on the same line)
if isInlineComment(fset, comment, stmtMap) {
if stmt, exists := stmtMap[pos.Line]; exists {
scope.start = fset.Position(stmt.Pos())
scope.end = fset.Position(stmt.End())
return scope, nil
// For inline comments, apply to the scope of the current statement
ns.start = fset.Position(stmt.Pos())
ns.end = fset.Position(stmt.End())
return ns, nil
}
}

// check if the comment is above a statement
// For standalone comments: if there's a statement on the next line,
// apply to that statement's scope while including the comment line itself
nextLine := pos.Line + 1
if stmt, exists := stmtMap[nextLine]; exists {
scope.start = fset.Position(stmt.Pos())
scope.end = fset.Position(stmt.End())
return scope, nil
ns.start = pos // Apply from the comment line
ns.end = fset.Position(stmt.End())
return ns, nil
}

// check if the comment is above a function declaration
// If no immediate statement follows, look for a function declaration to apply to
if decl := findFunctionAfterLine(fset, f, pos.Line); decl != nil {
funcPos := fset.Position(decl.Pos())
if funcPos.Line == pos.Line+1 {
scope.start = funcPos
scope.end = fset.Position(decl.End())
return scope, nil
ns.start = pos
ns.end = fset.Position(decl.End())
return ns, nil
}
}

// Default case: apply to the line of the comment
scope.start = pos
scope.end = pos
return scope, nil
// default behavior:
// apply only to the comment line
ns.start = pos
ns.end = pos
return ns, nil
}

// parseIgnoreRuleNames parses the rule list from the nolint comment more efficiently.
// parseIgnoreRuleNames parses the rule list from the nolint comment.
func parseIgnoreRuleNames(text string) map[string]struct{} {
rulesMap := make(map[string]struct{})

if text == "" {
return rulesMap
}

rules := strings.Split(text, ",")
for _, rule := range rules {
rule = strings.TrimSpace(rule)
Expand All @@ -138,6 +138,7 @@ func parseIgnoreRuleNames(text string) map[string]struct{} {
}

// indexStatementsByLine traverses the AST once and maps each line to its corresponding statement.
// If multiple statements exist on a single line, only the first statement is recorded.
func indexStatementsByLine(f *ast.File, fset *token.FileSet) map[int]ast.Stmt {
stmtMap := make(map[int]ast.Stmt)
ast.Inspect(f, func(n ast.Node) bool {
Expand All @@ -146,7 +147,6 @@ func indexStatementsByLine(f *ast.File, fset *token.FileSet) map[int]ast.Stmt {
}
if stmt, ok := n.(ast.Stmt); ok {
line := fset.Position(stmt.Pos()).Line
// save only the first statement of each line
if _, exists := stmtMap[line]; !exists {
stmtMap[line] = stmt
}
Expand Down Expand Up @@ -174,20 +174,33 @@ func findFunctionAfterLine(fset *token.FileSet, f *ast.File, line int) *ast.Func
return nil
}

// isInlineComment determines if a comment is inline with a statement.
// The comment is considered inline if it appears on the same line as a statement
// and its file offset is greater than the statement's starting offset.
func isInlineComment(fset *token.FileSet, comment *ast.Comment, stmtMap map[int]ast.Stmt) bool {
pos := fset.Position(comment.Slash)
if stmt, exists := stmtMap[pos.Line]; exists {
stmtPos := fset.Position(stmt.Pos())
return pos.Offset > stmtPos.Offset
}
return false
}

// IsNolint checks if a given position and rule are nolinted.
func (m *Manager) IsNolint(pos token.Position, ruleName string) bool {
scopes, exists := m.scopes[pos.Filename]
if !exists {
return false
}
for _, scope := range scopes {
if pos.Line < scope.start.Line || pos.Line > scope.end.Line {
for _, ns := range scopes {
if pos.Line < ns.start.Line || pos.Line > ns.end.Line {
continue
}
if len(scope.rules) == 0 {
// If the rules list is empty, nolint applies to all rules
if len(ns.rules) == 0 {
return true
}
if _, exists := scope.rules[ruleName]; exists {
if _, exists := ns.rules[ruleName]; exists {
return true
}
}
Expand Down
6 changes: 3 additions & 3 deletions internal/nolint/nolint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ func main() {
line int
expected bool
}{
{"anyrule", 5, true}, // Line 5 is covered by nolint without rules
{"anyrule", 5, true}, // Line 5 is covered by nolint (from comment on line 4)
{"anyrule", 6, false}, // Line 6 is not covered
{"rule1", 7, true}, // Line 7 is covered by nolint:rule1
{"rule2", 9, true}, // Line 9 is covered by nolint:rule2
{"rule1", 7, true}, // Line 7 is covered by nolint:rule1 (inline)
{"rule2", 9, true}, // Line 9 is covered by nolint:rule2 (from comment on line 8)
{"rule3", 9, false}, // Line 9 is not covered for rule3
}

Expand Down

0 comments on commit 35576c6

Please sign in to comment.