Skip to content

fix: remove javascript grammar - #626

Open
tean-lai wants to merge 9 commits into
mainfrom
tean/remove-javascript-grammar
Open

fix: remove javascript grammar#626
tean-lai wants to merge 9 commits into
mainfrom
tean/remove-javascript-grammar

Conversation

@tean-lai

@tean-lai tean-lai commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Resolves LANG-592.

Semgrep uses the typescript tree-sitter parser for javascript, so the semgrep-javascript grammar is outdated. This PR removes it.

In the future:

  • semgrep-javascript is deprecated
  • javascript is considered a sublang of typescript

Checklist

  • Any new parsing code was already published, integrated, and merged into Semgrep. DO NOT MERGE THIS PR BEFORE THE SEMGREP INTEGRATION WORK WAS COMPLETED.
  • Change has no security implications (otherwise, ping the security team)

@tean-lai
tean-lai marked this pull request as ready for review July 14, 2026 21:27
@tean-lai
tean-lai requested a review from a team as a code owner July 14, 2026 21:27
@tean-lai
tean-lai requested review from malaverdiere and nmote July 14, 2026 21:27
@tean-lai tean-lai self-assigned this Jul 14, 2026

@nmote nmote left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably also worth archiving https://github.com/semgrep/semgrep-javascript

@@ -1 +1 @@
../src/semgrep-javascript No newline at end of file
../src/semgrep-typescript/javascript No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably less confusing to just remove this symlink altogether

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan was to archive semgrep-javascript, but I seem to have insufficient perms at the moment.

Keeping the symlink seems to be more consistent with other stuff. Thinking particularly of tsx, which we treat as a sublang of typescript already, but that similarly symlinks to semgrep-typescript/tsx. It also seems like the test-lang script depends on the existence of this symlink.

@malaverdiere

Copy link
Copy Markdown
Contributor

Review: JavaScript-grammar removal

Checked whether automated tests still pass, whether the TypeScript grammar is affected, and swept for leftover JavaScript references. Verified all of this by actually running the scripts locally (initialized the tree-sitter-javascript/tree-sitter-typescript submodules, ran prep, ran the vendored tree-sitter 0.22.6 binary directly), not just by reading the diff.

✅ Already fixed by 8693d43 ("fix corpus symlink for javascript")

lang/semgrep-grammars/src/semgrep-typescript/prep originally pointed javascript/corpus/inherited at ../../../tree-sitter-javascript/javascript/corpus, which doesn't exist (tree-sitter-javascript is a flat repo — corpus lives at test/corpus). That's been corrected to ../../../tree-sitter-javascript/test/corpus. Confirmed the symlink resolves now. (For what it's worth, I also confirmed this specific corpus-test path is inert either way today — tree-sitter test only reads test/corpus, not bare corpus, and these sublang dirs use corpus/ directly — so this was latent either way, not an active regression. Still, good to have it correct.)

🔴 Still open — will break CI

.circleci/config.yml:159 still lists - javascript in the test-language matrix. That job runs ./test-lang javascript, whose very first line is cd semgrep-grammars/src/semgrep-javascript — a directory this PR deletes (c9e95e5 remove semgrep-javascript). I reproduced the failure locally:

$ cd lang && cd semgrep-grammars/src/semgrep-javascript
cd: semgrep-grammars/src/semgrep-javascript: No such file or directory

There's no allow_failure/continue-on-error on this job, so it'll hard-fail on every push. lang/Makefile's own coupling comment says any SUPPORTED_TS_LANGUAGES change must be mirrored here — javascript was correctly dropped from the Makefile lists but the CircleCI matrix entry wasn't. Since javascript is now a sublang of typescript (like tsx, which also isn't listed separately here), it should just be removed from this matrix.

🟡 Minor — stale JavaScript references left behind

  • doc/fix-semgrep-grammar-skill.md:9 and :24 still use javascript as the test-lang example (e.g. javascript, test-lang javascript pass), while the canonical .agents/skills/fix-semgrep-grammar/SKILL.md had its matching examples swapped to typescript in this PR (09c990f). Worth syncing so the two docs don't drift.
  • .agents/skills/fix-semgrep-grammar/SKILL.md:215 still says "semgrep-javascript's _jsx_child" as a worked example — that directory is gone; the rule now lives at lang/semgrep-grammars/src/semgrep-typescript/javascript/grammar.js. Worth rewording so it doesn't point at a directory that no longer exists.

TypeScript grammar

Unaffected. The prep script's typescript/tsx blocks are untouched, and none of the javascript-related additions can abort the script early (no early exit before the typescript/tsx blocks run).

malaverdiere added a commit to semgrep/semgrep-rules that referenced this pull request Jul 28, 2026
…e matching line

The `pattern: $X.div($Y)` match for `underlyingUnit.mul(balanceWithInvested()).div(totalSupply())`
is textually on the line with the `:` (else) branch of the ternary, not the
line where the enclosing assignment starts. Confirmed by parsing this
snippet directly: the `.div(...)` call_expression's span is entirely on
that line. The `// ruleid:` annotation was one line above the wrong branch,
which happened to line up with a less precise (pre-tree-sitter-solidity
v1.2.13) parse of this multi-line ternary. Move the annotation immediately
above the branch actually being matched.

Surfaced by semgrep/ocaml-tree-sitter-semgrep#626 (tree-sitter-solidity
bump to v1.2.13), which produces the correct AST here.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants