Skip to content

fix(grammar): stop the Swift try-bang entry shifting past its own type - #1986

Open
CaptainMittens wants to merge 2 commits into
DeusData:mainfrom
CaptainMittens:fix/swift-scanner-try-bang-shift
Open

fix(grammar): stop the Swift try-bang entry shifting past its own type#1986
CaptainMittens wants to merge 2 commits into
DeusData:mainfrom
CaptainMittens:fix/swift-scanner-try-bang-shift

Conversation

@CaptainMittens

@CaptainMittens CaptainMittens commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

One token in the same vendored file as #1977. This is the second of the two shifts in #1978, and the one that is undefined on Windows.

Stacked on #1977, because both changes rewrite the same recorded hash in scripts/vendored-checksums.txt. Two branches off main would collide there. Until #1977 merges this PR shows both commits; after it merges the diff narrows to the one token below.

The bug

internal/cbm/vendored/grammars/swift/scanner.c:131 builds the entry that suppresses the ! of a try!:

1UL << FAKE_TRY_BANG, // BANG,

FAKE_TRY_BANG is the last member of a 33-entry TokenType enum, so its value is 32.

unsigned long is 64 bits on Linux and macOS. On Windows it is 32 bits, so the shift count equals the width of the type, which is undefined. 1ULL is 64 bits on every target this project builds for.

Why nothing catches it

Three things line up, and the third is the one I would not have guessed:

  1. The expression is a compile-time constant. The compiler folds it, so no sanitizer sees it at run time. UBSan cannot report what never executes.
  2. Makefile.cbm:719 compiles every vendored grammar with -w. That switches off -Wshift-count-overflow, the one diagnostic that names this exact defect. The comment on line 718 gives the reason: upstream code has warnings.
  3. The affected platform is not exotic. CLANG64 on windows-latest sits in CORE_WIN (.github/workflows/_test.yml:63), so it builds on every pull request and reports nothing.

What I did and did not verify

I have no Windows machine, so I did not observe a symptom in the product. What I did do is compile the same shape locally, using unsigned int because it is 32 bits everywhere:

uint64_t m = 1u << 32;

cc -Wall warns shift count >= width of type. Adding -w, as the grammar build does, silences it.

I expected the folded value to be 0 and said so in #1978 before testing. It is not. The variable is simply never assigned, and three runs of one binary printed 4338156640, 4364321888 and 4377183328; at -O2 it printed 8447164672. The correct form, (uint64_t)1 << 32, prints 4294967296 every time. So the practical effect on Windows is a suppressor entry holding whatever happens to be there, not a predictable zero.

No test, and why

A test here would pass before and after the change on every machine CI can run it on. unsigned long is already 64 bits on Linux and macOS, so the expression is correct there; the defect only exists on a target the test suite does not execute. Adding a test that cannot fail would suggest a guarantee that does not exist. The existing swift_force_unwrap_scanner_shift from #1977 already keeps a force-unwrap parsing at all.

Upstream

Not my own reading alone. alex-pinkus/tree-sitter-swift made the same one-token change in 6ab8d1d74ebd (2026-08-10). Our copy is pinned at 8abb3e8b3325 (2026-03-20), so that fix landed after our pin. I checked our copy byte-for-byte against upstream at that pin: it is a faithful vendor, not a local edit that drifted.

Re-vendoring picks up both fixes and five months of other work, but TOKEN_COUNT moves from 33 to 34 upstream, so the grammar itself changed. CONTRIBUTING.md:134 puts a vendored dependency change behind a design discussion, so I have not proposed it here.

Files

File Change
internal/cbm/vendored/grammars/swift/scanner.c 1UL to 1ULL on line 131
scripts/vendored-checksums.txt the one recorded hash for that file, as scripts/security-vendored.sh --update writes it

Layer 8 of the security gate compares vendored content against that manifest, so the edit and its recorded hash land together. scripts/security-vendored.sh exits 0 on this branch.

One thing worth your call

-w on the grammar build is what hid this. Turning on the single warning -Wshift-count-overflow for vendored grammars — one narrow diagnostic, not -Wall -Werror — would catch this class across all 104 vendored scanners without touching the rest of the noise. I have not made that change here. Say the word and I will open it as its own issue.

Fixes #1978

Refs #1892
@CaptainMittens
fix(grammar): stop the Swift scanner shifting past the width of an int
de2ed3b 
The Swift scanner keeps a 64-bit mask of the symbols that suppress a
match -- the rule that stops `try!` emitting its `!` as a token of its
own. It tests one bit per candidate:

    uint64_t suppressing_symbols = OP_SYMBOL_SUPPRESSOR[full_match];
    for (uint64_t suppressor = 0; suppressor < TOKEN_COUNT; suppressor++) {
        if (!(suppressing_symbols & 1 << suppressor)) {

The mask is uint64_t but the literal `1` is an int, so the shift is an
int shift. TOKEN_COUNT is larger than 32, so once suppressor reaches 31
the shift runs past the width of the type. That is undefined behavior,
and every bit above 31 is tested against a value the standard does not
define.

Nothing caught it because nothing in the tree reached the suppressor
path. Any Swift force-unwrap does: `cached!` is enough.

UBSan reports it as:

    scanner.c:514:47: runtime error: left shift of 1 by 31 places
    cannot be represented in type 'int'

`1ULL` makes the literal as wide as the mask it is tested against.

The new test in tests/test_extraction.c cannot go red on its own. The
normal test build prints the UBSan message and carries on, which is why
this survived. The Windows CLANGARM64 leg runs UBSan in trap mode, and
there the same shift is an illegal-instruction crash -- so the test
exists to make sure that leg keeps parsing a force-unwrap at all.

scripts/vendored-checksums.txt records the new hash for the one changed
file, as scripts/security-vendored.sh --update writes it. Layer 8 of the
security gate compares vendored content against that manifest, so the
edit and its recorded hash belong in the same commit.

Found while adding Swift URL extraction in DeusData#1892 / DeusData#1976, and split out
of that PR so the vendored change can be reviewed on its own.

Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
@CaptainMittens
fix(grammar): stop the Swift try-bang entry shifting past its own type
9d91891 
scanner.c line 131 shifts a literal 1UL by 32 to build the entry that
suppresses the bang of a try!. unsigned long is 64 bits on Linux and
macOS but 32 bits on Windows, so there the shift count equals the width
of the type, which is undefined.

1ULL is 64 bits on every target this project builds for.

Nothing reports it today. The expression is a compile-time constant, so
no sanitizer sees it run, and Makefile.cbm:719 builds vendored grammars
with -w, which switches off -Wshift-count-overflow.

Upstream tree-sitter-swift made the same change in 6ab8d1d74ebd, after
the commit this grammar is pinned to.

Refs DeusData#1892

Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
@CaptainMittens CaptainMittens mentioned this pull request Sep 1, 2026
Open
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@CaptainMittens @CadanMeers