From f61b61aad82ae4d37b23359ad5c5fed3d6a22eca Mon Sep 17 00:00:00 2001 From: Ajit Pratap Singh Date: Wed, 21 Oct 2026 20:57:16 +0530 Subject: [PATCH] fix(linter): report L006 column as a byte offset; tighten comment span test L006 (column alignment) compared indentation with a tab counted as 4 columns and then reported Location.Column as that tab-expanded width + 1, so a column indented with two tabs was reported at column 9 instead of 3. Indentation is still compared with tab = 4, but the reported column is now the 1-based byte column of the first non-whitespace character, matching the byte columns documented on models.Location (#797, #826). Also strengthen TestTokenPositions_CommentsKeepOwnPositions to assert the token after a comment starts at or after the comment's End (line, then column) and that no token overlaps the comment span, with a multi-line comment case. Refs #830 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9 --- docs/LINTING_RULES.md | 2 + pkg/linter/rules/style/column_alignment.go | 39 +++++++- .../rules/style/column_alignment_test.go | 94 +++++++++++++++++++ pkg/sql/tokenizer/token_position_test.go | 80 ++++++++++++---- 4 files changed, 191 insertions(+), 24 deletions(-) diff --git a/docs/LINTING_RULES.md b/docs/LINTING_RULES.md index 47a24b11d..ab98549a8 100644 --- a/docs/LINTING_RULES.md +++ b/docs/LINTING_RULES.md @@ -240,6 +240,8 @@ SELECT col3 ``` +When comparing indentation, a tab counts as 4 spaces, so a tab-indented column aligns with one indented by 4 spaces. The reported column is the 1-based byte column of the misaligned column's first character, and a tab counts as one byte. For example, a column indented with two tabs is reported at column 3. + --- ### L008: Comma Placement diff --git a/pkg/linter/rules/style/column_alignment.go b/pkg/linter/rules/style/column_alignment.go index 1f541bae2..eb565c292 100644 --- a/pkg/linter/rules/style/column_alignment.go +++ b/pkg/linter/rules/style/column_alignment.go @@ -52,6 +52,11 @@ import ( // // The rule finds the most common indentation level among columns and reports // columns that deviate from this pattern. +// +// Indentation widths are compared with a tab counting as 4 columns, so a tab +// and four spaces align. The reported Location.Column is the 1-based byte +// column of the column's first non-whitespace character (a tab is one byte), +// matching the byte columns documented on models.Location. type ColumnAlignmentRule struct { linter.BaseRule } @@ -91,6 +96,7 @@ func (r *ColumnAlignmentRule) Check(ctx *linter.Context) ([]linter.Violation, er selectLineNum := 0 columnIndents := []int{} columnLines := []int{} + columnCols := []int{} // 1-based byte column of each column's first character for lineNum, line := range ctx.Lines { trimmed := strings.TrimSpace(line) @@ -102,17 +108,21 @@ func (r *ColumnAlignmentRule) Check(ctx *linter.Context) ([]linter.Violation, er selectLineNum = lineNum + 1 columnIndents = []int{} columnLines = []int{} + columnCols = []int{} // Check if columns are on same line or start of multi-line afterSelect := strings.TrimPrefix(trimmed[6:], " ") if afterSelect != "" && !strings.HasPrefix(strings.ToUpper(afterSelect), "DISTINCT") { // Columns start on SELECT line indent := getIndentSize(line) + 7 // SELECT + space + col := leadingWhitespaceBytes(line) + 8 if strings.HasPrefix(upper, "SELECT DISTINCT") { indent = getIndentSize(line) + 16 // SELECT DISTINCT + space + col = leadingWhitespaceBytes(line) + 17 } columnIndents = append(columnIndents, indent) columnLines = append(columnLines, lineNum+1) + columnCols = append(columnCols, col) } continue } @@ -128,7 +138,7 @@ func (r *ColumnAlignmentRule) Check(ctx *linter.Context) ([]linter.Violation, er trimmed == "" { // End of columns, check alignment if len(columnIndents) > 1 { - violations = append(violations, r.checkColumnAlignment(columnIndents, columnLines, selectLineNum, ctx)...) + violations = append(violations, r.checkColumnAlignment(columnIndents, columnLines, columnCols, selectLineNum, ctx)...) } inSelectColumns = false continue @@ -138,12 +148,13 @@ func (r *ColumnAlignmentRule) Check(ctx *linter.Context) ([]linter.Violation, er indent := getIndentSize(line) columnIndents = append(columnIndents, indent) columnLines = append(columnLines, lineNum+1) + columnCols = append(columnCols, leadingWhitespaceBytes(line)+1) } } // Handle case where SELECT is at end of file if inSelectColumns && len(columnIndents) > 1 { - violations = append(violations, r.checkColumnAlignment(columnIndents, columnLines, selectLineNum, ctx)...) + violations = append(violations, r.checkColumnAlignment(columnIndents, columnLines, columnCols, selectLineNum, ctx)...) } return violations, nil @@ -156,8 +167,11 @@ func (r *ColumnAlignmentRule) Check(ctx *linter.Context) ([]linter.Violation, er // the one belonging to the earliest continuation column wins. The first column // is skipped as it may appear on the SELECT line with different indentation. // +// indents holds tab-expanded widths used for comparison; cols holds the +// matching 1-based byte columns used for the reported Location. +// // Returns a slice of violations for misaligned columns. -func (r *ColumnAlignmentRule) checkColumnAlignment(indents []int, lines []int, _ int, ctx *linter.Context) []linter.Violation { +func (r *ColumnAlignmentRule) checkColumnAlignment(indents, lines, cols []int, _ int, ctx *linter.Context) []linter.Violation { violations := []linter.Violation{} if len(indents) < 2 { @@ -190,7 +204,7 @@ func (r *ColumnAlignmentRule) checkColumnAlignment(indents []int, lines []int, _ RuleName: r.Name(), Severity: r.Severity(), Message: "Column not aligned with other columns", - Location: models.Location{Line: lineNum, Column: indent + 1}, + Location: models.Location{Line: lineNum, Column: cols[i+1]}, Line: ctx.GetLine(lineNum), Suggestion: "Align column to match other columns in the SELECT list", CanAutoFix: false, @@ -206,7 +220,9 @@ func (r *ColumnAlignmentRule) checkColumnAlignment(indents []int, lines []int, _ // Counts leading spaces (1 each) and tabs (4 each) to compute total indentation. // Stops at the first non-whitespace character. // -// Returns the total indentation size in space-equivalent units. +// Returns the total indentation size in space-equivalent units. The result is +// used only to compare alignment between columns; it is not a byte column (see +// leadingWhitespaceBytes for the reported position). func getIndentSize(line string) int { count := 0 for _, ch := range line { @@ -222,6 +238,19 @@ func getIndentSize(line string) int { return count } +// leadingWhitespaceBytes returns the number of leading space and tab bytes in +// line, i.e. the 0-based byte offset of its first non-whitespace character. +// Unlike getIndentSize, a tab counts as one byte, so the result plus one is the +// 1-based byte column documented on models.Location. +func leadingWhitespaceBytes(line string) int { + for i := 0; i < len(line); i++ { + if line[i] != ' ' && line[i] != '\t' { + return i + } + } + return len(line) +} + // Fix is not supported for this rule as it requires complex formatting logic. // // Auto-fixing column alignment would require: diff --git a/pkg/linter/rules/style/column_alignment_test.go b/pkg/linter/rules/style/column_alignment_test.go index 69b481329..dc8f84cf4 100644 --- a/pkg/linter/rules/style/column_alignment_test.go +++ b/pkg/linter/rules/style/column_alignment_test.go @@ -417,3 +417,97 @@ func TestColumnAlignmentRule_TieBreakDeterministic(t *testing.T) { }) } } + +// TestColumnAlignmentRule_TabColumnIsByteOffset verifies that L006 compares +// indentation with a tab counted as 4 columns but reports Location.Column as +// the 1-based byte column of the first non-whitespace character, where a tab +// is one byte (issue #830, models.Location docs). +func TestColumnAlignmentRule_TabColumnIsByteOffset(t *testing.T) { + type pos struct{ line, col int } + tests := []struct { + name string + sql string + want []pos + }{ + { + name: "tab aligns with four spaces", + sql: "SELECT a,\n b,\n\tc\nFROM t", + want: nil, + }, + { + name: "two tabs misaligned against four spaces", + sql: "SELECT a,\n b,\n c,\n\t\td\nFROM t", + want: []pos{{4, 3}}, // width 8 vs 4; first char at byte 3, not 9 + }, + { + name: "single tab misaligned against two spaces", + sql: "SELECT a,\n b,\n c,\n\td\nFROM t", + want: []pos{{4, 2}}, // width 4 vs 2; first char at byte 2, not 5 + }, + { + name: "mixed spaces and tab misaligned", + sql: "SELECT a,\n b,\n c,\n \t d\nFROM t", + want: []pos{{4, 6}}, // width 2+4+2=8; first char at byte 6, not 9 + }, + { + name: "spaces misaligned against tab majority", + sql: "SELECT a,\n\tb,\n\tc,\n d\nFROM t", + want: []pos{{4, 3}}, + }, + { + name: "tab-indented majority with mixed outlier and multi-byte name", + sql: "SELECT a,\n\t\tb,\n\t\tc,\n\t 名前\nFROM t", + want: []pos{{4, 4}}, // width 6 vs 8; first char at byte 4 + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rule := NewColumnAlignmentRule() + ctx := linter.NewContext(tt.sql, "test.sql") + vs, err := rule.Check(ctx) + if err != nil { + t.Fatalf("Check error: %v", err) + } + got := make([]pos, 0, len(vs)) + for _, v := range vs { + got = append(got, pos{v.Location.Line, v.Location.Column}) + } + if len(got) != len(tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Fatalf("got %v, want %v", got, tt.want) + } + // The reported column must address the first non-whitespace + // byte of the reported line. + line := ctx.GetLine(got[i].line) + if c := got[i].col; c < 1 || c > len(line) || line[c-1] == ' ' || line[c-1] == '\t' || + (c > 1 && line[c-2] != ' ' && line[c-2] != '\t') { + t.Errorf("column %d does not address the first non-whitespace byte of %q", c, line) + } + } + }) + } +} + +func TestLeadingWhitespaceBytes(t *testing.T) { + tests := []struct { + line string + want int + }{ + {"", 0}, + {"x", 0}, + {" x", 4}, + {"\tx", 1}, + {"\t\tx", 2}, + {" \t x", 5}, + {"\t名前", 1}, + {" \t ", 3}, + } + for _, tt := range tests { + if got := leadingWhitespaceBytes(tt.line); got != tt.want { + t.Errorf("leadingWhitespaceBytes(%q) = %d, want %d", tt.line, got, tt.want) + } + } +} diff --git a/pkg/sql/tokenizer/token_position_test.go b/pkg/sql/tokenizer/token_position_test.go index d6324eb04..5a2652f69 100644 --- a/pkg/sql/tokenizer/token_position_test.go +++ b/pkg/sql/tokenizer/token_position_test.go @@ -135,28 +135,70 @@ func TestTokenPositions_StartIsAfterTrivia(t *testing.T) { // A comment before a token must not move the token's Start, and the comment is // still collected with its own position. func TestTokenPositions_CommentsKeepOwnPositions(t *testing.T) { - tkz := pooledTokenizer(t) - tokens, err := tkz.Tokenize([]byte("a\tb /* x */ c")) - if err != nil { - t.Fatalf("Tokenize: %v", err) - } - if len(tkz.Comments) != 1 { - t.Fatalf("got %d comments, want 1", len(tkz.Comments)) + tests := []struct { + name string + sql string + commentStart models.Location + commentEnd models.Location + afterStart models.Location // Start of the token right after the comment ("c") + }{ + {"single-line block comment", "a\tb /* x */ c", loc(1, 5), loc(1, 12), loc(1, 13)}, + {"multi-line block comment", "a\tb /* x\n y */ c", loc(1, 5), loc(2, 6), loc(2, 7)}, } - c := tkz.Comments[0] - if c.Start != loc(1, 5) || c.End != loc(1, 12) { - t.Errorf("comment span = %+v-%+v, want %+v-%+v", c.Start, c.End, loc(1, 5), loc(1, 12)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tkz := pooledTokenizer(t) + tokens, err := tkz.Tokenize([]byte(tt.sql)) + if err != nil { + t.Fatalf("Tokenize: %v", err) + } + if len(tkz.Comments) != 1 { + t.Fatalf("got %d comments, want 1", len(tkz.Comments)) + } + c := tkz.Comments[0] + if c.Start != tt.commentStart || c.End != tt.commentEnd { + t.Errorf("comment span = %+v-%+v, want %+v-%+v", c.Start, c.End, tt.commentStart, tt.commentEnd) + } + if len(tokens) < 3 { + t.Fatalf("got %d tokens, want at least 3", len(tokens)) + } + // The token after the comment must not overlap the comment span: + // tokens[2].Start >= Comments[0].End, comparing line then column. + after := tokens[2] + if after.Token.Value != "c" { + t.Fatalf("tokens[2] = %q, want %q", after.Token.Value, "c") + } + if locBefore(after.Start, c.End) { + t.Errorf("tokens[2] (%q) Start %+v overlaps comment ending at %+v", after.Token.Value, after.Start, c.End) + } + if after.Start != tt.afterStart { + t.Errorf("tokens[2] (%q) Start = %+v, want %+v", after.Token.Value, after.Start, tt.afterStart) + } + // Every token lies wholly before or wholly after the comment, and + // has a non-empty span. + for i, tok := range tokens { + if tok.Token.Type == models.TokenTypeEOF { + continue + } + if tok.End.Line == tok.Start.Line && tok.End.Column <= tok.Start.Column { + t.Errorf("token %d (%q): empty span %+v-%+v", i, tok.Token.Value, tok.Start, tok.End) + } + // Half-open spans [Start, End) overlap iff each starts before the other ends. + if locBefore(tok.Start, c.End) && locBefore(c.Start, tok.End) { + t.Errorf("token %d (%q) span %+v-%+v overlaps comment %+v-%+v", + i, tok.Token.Value, tok.Start, tok.End, c.Start, c.End) + } + } + }) } - // Every token's Start is at or after the end of the preceding comment and - // before its own End. - for i, tok := range tokens { - if tok.Token.Type == models.TokenTypeEOF { - continue - } - if tok.End.Line == tok.Start.Line && tok.End.Column <= tok.Start.Column { - t.Errorf("token %d (%q): empty span %+v-%+v", i, tok.Token.Value, tok.Start, tok.End) - } +} + +// locBefore reports whether a precedes b, comparing line then column. +func locBefore(a, b models.Location) bool { + if a.Line != b.Line { + return a.Line < b.Line } + return a.Column < b.Column } // A trailing comment leaves no token after it; the EOF token sits at the end.