[ajitpratap0]

Description

Follow-up from #825. Several tokenizer error paths built locations from raw cursor fields instead of toSQLPosition:

• Quoted identifier broken by a newline: reported column 0, or the wrong column after tabs or multibyte characters.
• Unterminated dollar-quoted strings and SQL Server [bracket identifiers: reported the end of input instead of the opening delimiter.
• StringLiteralReader: had the same problem.

All error locations now use the same rule as token spans: 1-based line, byte column, tab counts as one column. Unterminated-literal errors point at the opener, and escape errors point at the character after the backslash.

Type of Change

• Bug fix (non-breaking change which fixes an issue). Error locations change, recorded as a Fixed entry in the CHANGELOG.

Changes Made

• pkg/sql/tokenizer/tokenizer.go and string_literal.go: all error locations now go through toSQLPosition or locationAt, which share one rule.
• New error_location_test.go: a table test with tabs, multibyte characters and multi-line input. Each location is mapped back to the opener byte.
• testdata/tokenizer_literal_golden.json: regenerated with -update-literal-golden.

Golden verification

For every golden entry whose digest changed, I printed the full tokenizer result for all 9 golden dialects, on main and on this branch, and compared them per entry and dialect:

• 216 results compared.
• 0 token differences and 0 error-code differences.
• Only error locations changed: 108 E1002 (unterminated string) and 45 E2004 (invalid escape).
• Example: '''é€𝄞''' 'c\q' moves from column 14 (a rune count) to column 20, the byte after the backslash.

Testing

• task check (fmt, vet, golangci-lint, race tests) and task test:cbinding
• Independent go-reviewer pass: no findings
• No hot-path changes; error paths only.

Breaking Changes

None to the API. Error columns and lines for the affected errors change; tools that snapshot error positions may need updating.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KTj1PEbxzVCETkeYrZKcrE

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
gosqlx-website	Ignored	Preview	Oct 21, 2026 9:41am UTC

[github-actions]

Claude finished @ajitpratap0's task in 28s —— View job

---

Review of PR #827

• Read the diff and the surrounding tokenizer code
• Checked the opener offsets and the shared location rule

I only read the code. I did not run the tests, and I didn't re-check your golden comparison.

Summary: This is a clean, well-scoped fix. It replaces the three different ways error locations were built (raw t.pos fields, rune counts, and the old getLocation) with one rule: 1-based line, byte column, tab counts as 4.

Correctness

• Opener offsets: tagStart - 1 is the leading $ of a dollar-quote (tokenizer.go:2239 slices from tagStart-1, which confirms it). contentStart - 1 is the [ of a bracket identifier. Both are right.
• Escape errors: your comment says handleEscapeSequence consumes only the backslash on failure, so t.pos is the offending character. string_literal.go uses r.pos.Index + 1 computed before the call, which gives the same result. The two paths agree.
• locationAt: it matches toSQLPosition. It clamps pos to len(input), which is safe. A negative pos can't occur from the current callers.
• getLocation: it now delegates to locationAt. Its old column was a plain byte offset with no tab handling, so this is a deliberate change. The CHANGELOG covers it ("Position.Location... follow the same rules").
• Position cache: in toSQLPosition, the posCacheIdx resume path only applies when pos.Index >= t.posCacheIdx. Reporting an earlier opener, as the new error sites do, therefore falls back to the line start and stays correct.

Performance: locationAt is O(n) per call, but it only runs on error paths and in Position.Location. That is fine as long as nothing calls it per token. A quick grep for getLocation callers on hot paths would confirm that.

Tests

• The table test with tabs, multibyte characters and multi-line input, mapping each location back to the opener byte, is a good invariant.
• The golden regeneration is backed by a per-dialect comparison showing 0 token or error-code differences, which is convincing.
• Optional: add a case for an unterminated dollar-quote or bracket after a previous token on the same line. That exercises the case where the opener is before the cache position.
• Optional: add a direct unit test comparing locationAt with toSQLPosition on random inputs. This would stop the two from drifting apart.

Nits

• The Column: 0 to Column: 1 change for NewWithKeywords(nil) and for input too large is reasonable. It is also a behavior change for anyone matching on those errors, and the CHANGELOG already mentions it.
• The PR description says "Independent go-reviewer pass: no findings". I have no objection either.

No blocking issues. LGTM from a code-review standpoint, pending CI.
· branch fix/tokenizer-error-locations

[github-actions]

Claude finished @ajitpratap0's task in 26s —— View job

---

Review of PR #827 (updated)

I read the diff and the surrounding tokenizer code. I did not run the tests.

Summary: The change is correct and well scoped. Error locations now go through one shared rule, lineLocation, which both toSQLPosition and locationAt use. That removes the drift risk my earlier review suggested testing for.

Correctness

• Opener offsets: tagStart - 1 is the leading $, and contentStart - 1 is the [. Both are right.
• Escape errors: t.getCurrentPosition() in the tokenizer and r.pos.Index + 1 in StringLiteralReader both point at the character after the backslash, so the two paths agree.
• Unterminated literals: StringLiteralReader now uses locationAt(r.input, startPos.Index). That fixes the rune-count column.
• locationAt: It clamps pos to the range [0, len(input)] for the prefix. lineLocation clamps again for the column.
• Performance: locationAt scans the input twice (bytes.Count and bytes.LastIndexByte) and runs in O(n). It is only used on error paths and in Position.Location, so this is fine.

Documentation mismatch (worth fixing before merge)

• The PR description says "tab counts as 4", and the title says "tab-aware columns".
• The code and the CHANGELOG say the opposite. The toSQLPosition doc comment says "every byte, a tab included, counts as one column", and the CHANGELOG says "a tab counting as one column".
• The tests agree with the code. For example, SELECT\t$$abc expects column 8.
• The behavior is consistent with editors and LSP clients, and I think it is the right one. Please correct the PR title and description so they don't promise tab-width-4 columns, since tools that snapshot error positions may read them.

Tests

• The table in error_location_test.go covers tabs, multibyte characters and multi-line input, and it maps each location back to the opener byte. That is a good invariant.
• Optional: add a case where the unterminated opener comes after an earlier token on the same line, for example SELECT 1, $$abc or SELECT a, [b. This exercises toSQLPosition with a position earlier than the cursor.
• Optional: add an locationAt vs toSQLPosition equivalence check. It is a smaller risk now that both call lineLocation.

Nits

• Column: 0 to Column: 1 for the nil-keywords and oversized-input errors is reasonable. The CHANGELOG covers it.

No blocking code issues. The only requested change is to correct the "tab counts as 4" wording in the PR title and description.
· branch fix/tokenizer-error-locations

[ajitpratap0]

Review notes (comment only, no pushes from my side).

Tab rule. #826 (merged, fixes #797) defines every position as a 1-based byte column where a tab is 1 column (toSQLPosition on main: column = (index - lineStart) + 1). Reading the diff, #827's code already matches that rule: lineLocation/locationAt count bytes, the new error_location_test.go cases use "SELECT\t..." -> column 8, and the CHANGELOG entry says "a tab counting as one column". The only place that says otherwise is the PR description ("tab counts as 4"). That sentence looks stale and could be corrected so it does not mislead reviewers. I found no tab=4 logic or tab=4 golden values in the diff.

Conflict. The merge conflict is textual, not semantic. #827 refactors the body of toSQLPosition into lineLocation and rewrites getLocation as locationAt, which touches the same lines as main's current toSQLPosition/getLocation (pkg/sql/tokenizer/tokenizer.go, around lines 2330-2380), plus the CHANGELOG hunk. After rebasing onto current main, it is worth confirming that toSQLPosition keeps the posCacheLine update and lineIndexAt use from #826. It is also worth regenerating testdata/tokenizer_literal_golden.json with -update-literal-golden on the rebased tree, since the golden digests depend on the final error locations.

Overlap with #830. #827 already covers #830 item 1: the readQuotedIdentifierSlow newline branch now uses t.toSQLPosition(startPos), string_literal.go uses locationAt, and a multibyte/tab regression table is added. It does not cover item 2 (L006 getIndentSize tab=4 / reported column) or item 3 (tightening TestTokenPositions_CommentsKeepOwnPositions with tokens[2].Start >= Comments[0].End). Item 3 is a one-line assertion in the tokenizer test package and could reasonably ride along here; item 2 is in pkg/linter and is probably better kept separate.

If #827 lands with item 1, #830 can be trimmed to items 2 and 3 (or just 2).

[github-actions]

Claude finished @ajitpratap0's task in 27s —— View job

---

Review of PR #827 (re-review after sync)

I read the diff against origin/main. I did not run the tests.

Summary: The change is correct and well scoped. All the touched error sites now go through the same rule, lineLocation: 1-based line, byte column, and a tab counts as one column.

Correctness

• Opener offsets:
  • tagStart - 1 is the leading $ of the dollar-quote.
  • contentStart - 1 is the [ of the bracket identifier.
  • startPos in readQuotedIdentifierSlow is the opening quote.
• Escape errors: t.getCurrentPosition() and locationAt(r.input, r.pos.Index+1) both point at the character after the backslash, so the tokenizer and StringLiteralReader agree.
• locationAt: the prefix is clamped to [0, len], so a negative or oversized pos is safe. The cost is O(n), and it only runs on error paths and in Position.Location.
• Cache with an earlier opener: toSQLPosition is called with a position before the cursor for the dollar-quote and bracket errors. I did not re-read the posCacheLine resume logic after the rebase. Please confirm that it still falls back to a correct line when the requested index is before the cached line.
• NewWithKeywords(nil) and oversized input: Column: 0 becomes Column: 1. This is consistent with the "never less than 1" rule.

Open items

1. Stale PR title and description. Both say "tab-aware columns" and "tab counts as 4". The code, tests and CHANGELOG all use tab = 1 column, matching fix(tokenizer): one column per tab and token Start after trivia (#797) #826. This was already raised in earlier comments and is still unfixed. Please change "tab counts as 4" to "tab counts as one column" in the title and body, because tools that snapshot error positions may rely on that wording.
2. Golden file after the rebase. Please regenerate testdata/tokenizer_literal_golden.json with -update-literal-golden on the rebased tree, since the digests depend on final error locations. The "tab counts as 4" claim in the description makes me want to confirm that the 216-result comparison was made with the tab = 1 rule.

Tests (optional)

• Add cases where the unterminated opener follows an earlier token on the same line (SELECT 1, $$abc, SELECT a, [b). These exercise toSQLPosition with an index behind the cursor.
• Item 3 from fix(positions): remaining column inconsistencies after #797 #830 is a one-line assertion (tokens[2].Start >= Comments[0].End in TestTokenPositions_CommentsKeepOwnPositions). It could ride along here.

No blocking code issues. The PR needs the description fix and a golden regeneration check after the rebase.
· branch fix/tokenizer-error-locations

[ajitpratap0]

Merged origin/main into this branch to resolve conflicts (normal merge commit, fast-forward push; no force-push, no rebase).

• CHANGELOG.md: kept both sides' [Unreleased] entries (union, no rewording).
• Tokenizer files and the literal golden: resolved by combining main's changes with this PR's toSQLPosition/locationAt error locations; golden fixture regenerated/checked.
• Re-verified: gofmt, go vet, go test -race on tokenizer and errors packages pass.

Also corrected the PR title/body wording: a tab counts as one column (not 4).

🤖 Generated with Claude Code

https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9

[github-actions]

Claude finished @ajitpratap0's task in 20s —— View job

---

Review of PR #827 (after sync with main)

I read the diff against origin/main and lineIndexAt/toSQLPosition. I did not run the tests.

Summary: The change is correct and well scoped. Every touched error site now goes through lineLocation: 1-based line, byte column, and a tab counts as one column.

Checked

• Rebase: toSQLPosition still updates posCacheLine and still uses lineIndexAt from fix(tokenizer): one column per tab and token Start after trivia (#797) #826. The refactor only moves the column arithmetic into lineLocation.
• Cache with an earlier opener: lineIndexAt only takes the cached line when lineStarts[li] <= idx (tokenizer.go:2309). For the dollar-quote and bracket errors, which report an index behind the cursor, it falls back to a search. This is safe. The error path also resets posCacheLine, but it is the last call in that tokenization, so nothing is affected.
• Opener offsets: tagStart-1 is the leading $, contentStart-1 is the [, and startPos is the opening quote.
• Escape errors: t.getCurrentPosition() and locationAt(r.input, r.pos.Index+1) both point at the character after the backslash.
• locationAt: the prefix is clamped to [0, len], and the cost is O(n) on error paths only.
• Column: 0 to Column: 1: for nil keywords and oversized input, this fits the "never less than 1" rule.

Open items (not code)

1. PR title and description say "tab-aware columns" and "tab counts as 4". The code, tests (SELECT\t$$abc gives column 8) and CHANGELOG all use tab = 1 column, matching fix(tokenizer): one column per tab and token Start after trivia (#797) #826. Please edit the title and body, for example to "…with byte columns (tab = 1)". I can't edit the PR description myself.
2. Golden file and the 216-result comparison. I did not check that testdata/tokenizer_literal_golden.json was regenerated after the merge from main. Please run go test ./pkg/sql/tokenizer -update-literal-golden and confirm the file doesn't change. The description's "tab counts as 4" wording also leaves it unclear which rule the comparison used. Please confirm it used tab = 1.

Optional tests

• Add cases where the unterminated opener follows an earlier token on the same line (SELECT 1, $$abc, SELECT a, [b). These exercise toSQLPosition with an index behind the cursor.
• Item 3 from fix(positions): remaining column inconsistencies after #797 #830 could ride along here. It is the one-line assertion tokens[2].Start >= Comments[0].End in TestTokenPositions_CommentsKeepOwnPositions. Item 2 (the L006 linter) is better kept separate.

No blocking code issues. This is ready once the description is corrected and the golden file is confirmed.
· branch fix/tokenizer-error-locations

[github-actions]

Claude finished @ajitpratap0's task in 17s —— View job

---

Review of PR #827 (after merge of main)

I read the diff against origin/main. I did not run the tests or regenerate the golden file.

Summary: The change is correct and well scoped. Every touched error site now goes through lineLocation: 1-based line, byte column, and a tab counts as one column. This matches #826.

Correctness

• Opener offsets:
  • tagStart-1 is the leading $ of the dollar-quote.
  • contentStart-1 is the [ of the bracket identifier.
  • startPos is the opening quote in readQuotedIdentifierSlow.
• Escape errors: t.getCurrentPosition() and locationAt(r.input, r.pos.Index+1) both point at the character after the backslash, so the tokenizer and StringLiteralReader agree.
• locationAt: The prefix is clamped to [0, len], and lineLocation clamps the column. A negative or oversized pos is safe.
• Cache with an earlier opener: toSQLPosition still sets posCacheLine and uses lineIndexAt from fix(tokenizer): one column per tab and token Start after trivia (#797) #826. The refactor only moves the column arithmetic into lineLocation. An index behind the cursor falls back to a search. The error path is the last call in that tokenization, so a reset cache does no harm.
• Column: 0 to Column: 1: This applies to nil keywords and oversized input. It fits the "never less than 1" rule, and the CHANGELOG covers it.

Performance: locationAt is O(n) (it calls bytes.Count and bytes.LastIndexByte). It only runs on error paths and in Position.Location, so this is fine.

Tests

• error_location_test.go covers tabs, multibyte characters and multi-line input, and it maps each location back to the opener byte. That is a good invariant.
• Optional: add cases where the opener follows an earlier token on the same line (SELECT 1, $$abc, SELECT a, [b). These exercise toSQLPosition with an index behind the cursor.
• Optional: add a locationAt vs toSQLPosition equivalence test. The risk is small now that both call lineLocation.
• Optional: the one-line assertion from fix(positions): remaining column inconsistencies after #797 #830 item 3 (tokens[2].Start >= Comments[0].End in TestTokenPositions_CommentsKeepOwnPositions) could ride along. Item 2 (the L006 linter) is better kept separate.

Open item: Before merge, run go test ./pkg/sql/tokenizer -update-literal-golden and confirm testdata/tokenizer_literal_golden.json doesn't change. You said you checked it after the merge, so this is only a CI-level confirmation.

No blocking issues. LGTM pending CI.
· branch fix/tokenizer-error-locations