Skip to content

JS-1506 Support TypeScript 7 with a Go-based JS/TS analyzer#6458

Draft
vdiez wants to merge 58 commits into
masterfrom
feat/tsgolint-grpc-poc
Draft

JS-1506 Support TypeScript 7 with a Go-based JS/TS analyzer#6458
vdiez wants to merge 58 commits into
masterfrom
feat/tsgolint-grpc-poc

Conversation

@vdiez
Copy link
Copy Markdown
Contributor

@vdiez vdiez commented Feb 25, 2026

Scope

This PR now tracks JS-1506: support TypeScript 7 with a Go-based JS/TS analyzer, while keeping Node.js as the primary analyzer for the rest of SonarJS.

The branch has moved well beyond the original JS-1379 / tsgolint gRPC PoC. It should now be read as the integration branch for the current jsts-go sidecar and the Go-side runtime needed to approach Node analyze-project parity for the migrated rule slice.

Current Branch State

  • Adds the Sonar-owned Go analyzer runtime under server-go/sonar-server.
  • Reuses the shared AnalyzeProject gRPC contract from master, including AnalyzeProject, AnalyzeProjectUnary, CancelAnalysis, and Lease.
  • Keeps WebSensor as the orchestrator: Node.js still runs the main analysis, and only selected JS/TS rules are routed to jsts-go.
  • Removes the old tsgolint submodule/runtime dependency. The live upstream inputs are server-go/typescript-go, server-go/patches/typescript-go, server-go/shim, and Sonar-owned code in server-go/sonar-server.
  • Generates Go rule metadata and default options from the Node rule metadata so per-file gating and configuration stay aligned across runtimes.
  • The current branch references are docs/node-vs-jsts-go-equivalence-assessment.md, docs/jsts-go-migration-progress.md, docs/jsts-go-typescript-go-api-surface.md, and server-go/UPSTREAM.md.

Routed Rules

JSTS_GO_RULES currently routes these 16 Sonar keys to the Go sidecar:

Sonar key Go rule
S131 switch-exhaustiveness-check
S2870 no-array-delete
S2933 prefer-readonly
S4123 await-thenable
S4137 consistent-type-assertions
S4157 no-unnecessary-type-arguments
S4325 no-unnecessary-type-assertion
S6544 no-misused-promises
S6551 no-base-to-string
S6557 prefer-string-starts-ends-with
S6565 prefer-return-this-type
S6571 no-redundant-type-constituents
S6582 prefer-optional-chain
S6583 no-mixed-enums
S6606 prefer-nullish-coalescing
S6671 prefer-promise-reject-errors

Notes:

  • S131 stays Sonar-owned on the Go side by combining switch-exhaustiveness-check with Go-side defaults and issue relocation.
  • S6544 stays composite on the Go side by pairing no-misused-promises with the Go companion no-async-promise-executor.
  • S2870 stays Sonar-owned on the Go side as no-array-delete.
  • The current Go routing preserves JS and TS variants, so migrated rules can still run on JavaScript files when the rule metadata allows it.

Analyze-Project And Runtime Work In This Branch

  • Request normalization for config, rules, files, pathMap, VirtualFiles, bundles, and rulesWorkdir.
  • Overlay VFS support for inline file content and canAccessFileSystem=false.
  • Project-tree discovery and file-store parity when the request omits files.
  • JS/TS source-test filtering, inclusions, exclusions, suffix handling, and per-file rule gating.
  • Discovery of tsconfig.json, package.json, deno.json, and deno.jsonc, plus dependency/module/ECMAScript signals used by migrated rules.
  • SonarQube batch typed programs, SonarLint-oriented incremental typed flows, orphan typed-program creation, and AST-only fallback for the migrated subset that can run without type services.
  • disableTypeChecking routing through the AST-only lane for the migrated subset that supports it.
  • Configured globals / environments handling for scope-sensitive migrated rules.
  • skipNodeModuleLookupOutsideBaseDir support in Go program creation.
  • Targeted SonarLint FsEvents invalidation for cached configured and orphan programs.
  • A Node-vs-Go differential harness under packages/parity/corpus, with npm run jsts-go-parity:diff-all, currently covering all 16 routed Go rules.

Build Notes

  • Keep skip-nodejs from master.
  • Use -Pskip-jsts-go to skip Go build and compress steps when they are not under test.
  • Build and packaging now revolve around jsts-go under server-go.

Remaining Gaps And Non-Goals

  • Node remains authoritative for non-migrated rules, Vue, Babel fallback, CSS, and the broader analyze-project output surface.
  • The Go sidecar is still issue-only; Node still owns parse/runtime errors, AST, metrics, highlights, symbols, CPD, quick fixes, and telemetry.
  • Accepted non-goals for the current sidecar scope remain startup lease-acquisition timeout semantics, content-based prefilters such as minified / bundle / max-file-size skips, parser-selection parity such as Babel / Vue fallback behavior, and non-issue outputs.
  • JS-1741 remains the CI / packaging / cross-platform stabilization ticket.
  • JS-1743 now owns further parity-corpus expansion plus the remaining Node-vs-Go diagnostic-shape gaps on the shared rule set.
  • JS-1744 covers benchmarking.
  • JS-1746 is now primarily the typed unicorn slice (S7727, S7728, S7729, S7755, S7778, S7785).

Summary by Gitar

  • Rule Migration:
    • Routed 15 additional hard type-service rules to the Go sidecar, bringing the total to 92.
    • Implemented new Go-side analysis rules including unused-import, inconsistent-return, no-implicit-dependencies, and several regex-specific rules (unused-named-groups, regex-complexity, slow-regex, etc.).
  • Parity Corpus:
    • Added 15 new test projects to packages/parity/corpus to validate the newly implemented Go rules.

This will update automatically on new commits.

@vdiez vdiez requested a review from a team February 25, 2026 16:34
@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title feat: tsgolint gRPC integration PoC JS-1379 feat: tsgolint gRPC integration PoC Feb 25, 2026
@hashicorp-vault-sonar-prod
Copy link
Copy Markdown

hashicorp-vault-sonar-prod Bot commented Feb 25, 2026

JS-1379

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 25, 2026

Ruling Report

Code no longer flagged (21508 issues)

S105

Joust/ts/Entity.spec.ts:7

     5 | describe("Entity", () => {
     6 | 
>    7 | 	it("should save the entity id", () => {
     8 | 		let entity = new Entity(1, Immutable.Map<string, number>());
     9 | 		expect(entity.id).toBe(1);

Joust/ts/Player.spec.ts:6

     4 | describe("Player", () => {
     5 | 
>    6 | 	it("should have a well-formed constructor", () => {
     7 | 		let tags = Immutable.Map<string, number>();
     8 | 		let player = new Player(2, tags, 1, "BehEh", 3, 5, true);

Joust/ts/protocol/HSReplayDecoder.spec.ts:11

     9 | describe("HSReplayDecoder", () => {
    10 | 
>   11 | 	let decoder;
    12 | 
    13 | 	beforeEach(() => {

Joust/ts/state/GameState.spec.ts:5

     3 | describe("GameState", () => {
     4 | 
>    5 | 	it("should start at time null", () => {
     6 | 		let state = new GameState();
     7 | 		expect(state.time).toBeNull();

Joust/ts/state/GameStateHistory.spec.ts:9

     7 | describe("GameStateHistory", () => {
     8 | 
>    9 | 	let history;
    10 | 	let stateZero = new GameState(undefined, undefined, undefined, undefined, 0);
    11 | 	let stateOne = new GameState(undefined, undefined, undefined, undefined, 1);

Joust/ts/state/mutators/IncrementTimeMutator.spec.ts:6

     4 | describe("IncrementTimeMutator", () => {
     5 | 
>    6 | 	it("should have a default time increment of 1", () => {
     7 | 		let mutator = new IncrementTimeMutator();
     8 | 		expect(mutator.time).toEqual(1);

searchkit/packages/searchkit-elastic-ui/src/Facets/ComboBoxFacet/tests/index.test.tsx:107

   105 |     })
   106 |     // act(() => {
>  107 |     // 	fireEvent.click(screen.getByRole('option', { name: 'turkey' }))
   108 |     // })
   109 |   })

tailwindcss/tests/plugins/gradientColorStops.test.js:43

    41 |           --tw-text-opacity: 0.5
    42 |         }
>   43 | 				.from-primary {
    44 | 					--tw-gradient-from: rgb(31,31,31);
    45 | 					--tw-gradient-stops: var(--tw-gradient-from), var(--tw-gradient-to, rgba(31, 31, 31, 0))

S109

Joust/ts/Entity.spec.ts:14

    12 | 	it("should save an identical set of tags", () => {
    13 | 		let tags = Immutable.Map<string, number>();
>   14 | 		tags = tags.set("" + GameTag.HEALTH, 42);
    15 | 		let entity = new Entity(15, tags);
    16 | 		expect(entity.getTags()).toBe(tags);

Joust/ts/Entity.spec.ts:15

    13 | 		let tags = Immutable.Map<string, number>();
    14 | 		tags = tags.set("" + GameTag.HEALTH, 42);
>   15 | 		let entity = new Entity(15, tags);
    16 | 		expect(entity.getTags()).toBe(tags);
    17 | 	});

...and 21498 more

New issues flagged (97 issues)

S6767

ant-design/components/anchor/Anchor.tsx:70

    68 | 
    69 | export interface AnchorState {
>   70 |   activeLink: null | string;
    71 | }
    72 | 

eigen/src/app/Scenes/Artwork/Components/CommercialInformation.tsx:34

    32 |   me: CommercialInformation_me$data
    33 |   hasStarted?: boolean
>   34 |   tracking?: TrackingProp
    35 |   biddingEndAt?: string
    36 |   hasBeenExtended?: boolean

eigen/src/app/Scenes/Artwork/Components/CommercialInformation.tsx:37

    35 |   biddingEndAt?: string
    36 |   hasBeenExtended?: boolean
>   37 |   refetchArtwork: () => void
    38 |   setAuctionTimerState?: (auctionTimerState: string) => void
    39 | }

S5914

console/src/actions/databrowser/tests/data.ts:172

   170 |       })
   171 |       .catch(() => {
>  172 |         expect(false).toBe(true)
   173 |       })
   174 |   })

console/src/actions/databrowser/tests/data.ts:184

   182 |       .catch((e) => {
   183 |         console.error(e)
>  184 |         expect(false).toBe(true)
   185 |       })
   186 |   })

console/src/actions/databrowser/tests/data.ts:196

   194 |       .catch((e) => {
   195 |         console.error(e)
>  196 |         expect(false).toBe(true)
   197 |       })
   198 |   })

console/src/actions/databrowser/tests/data.ts:216

   214 |       })
   215 |       .catch((e) => {
>  216 |         expect(false).toBe(true)
   217 |       })
   218 |   })

console/src/actions/databrowser/tests/data.ts:232

   230 |       })
   231 |       .catch((e) => {
>  232 |         expect(false).toBe(true)
   233 |       })
   234 |   })

postgraphql/src/postgres/inventory/collection/tests/PgCollectionKey-test.js:185

   183 |   try {
   184 |     await collectionKey1.update(context, new Map([['person_id_1', 1], ['person_id_2', 2]]), new Map([['a', 1]]))
>  185 |     expect(true).toBe(false)
   186 |   }
   187 |   catch (error) {

postgraphql/src/postgres/inventory/collection/tests/PgCollectionKey-test.js:201

   199 |   try {
   200 |     await collectionKey2.update(context, new Map([['email', 'does.not.exist@email.com']]), new Map([['about', 'xxxx']]))
>  201 |     expect(true).toBe(false)
   202 |   }
   203 |   catch (error) {

...and 87 more

📋 View full report

Code no longer flagged (21508)

S105

S109

@vdiez vdiez marked this pull request as draft February 25, 2026 18:20
@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented Feb 25, 2026

Implementation Details — Build & CI Setup

Commits after initial implementation

The initial implementation commit required several follow-up fixes to get the build working end-to-end:

1. fix: tsgolint submodule setup and build fixes

Problem: The initial commit had Go server files committed directly under tsgolint/cmd/sonar-server/. But tsgolint is an upstream repo — we can't push to it.

Solution:

  • Added https://github.com/oxc-project/tsgolint.git as a git submodule at ./tsgolint
  • Moved our Go server overlay files to sonar-plugin/bridge/src/main/go/sonar-server/ (source of truth in our repo)
  • Maven copies them into the submodule at build time
  • Fixed Go module path: github.com/typescript-eslint/tsgolint (from actual go.mod)
  • Fixed protobuf-maven-plugin: added <clearOutputDirectory>false</clearOutputDirectory> to the 2nd execution — it was wiping the 1st execution's output (estree proto)
  • Added org.apache.tomcat:annotations-api dependency — gRPC generated code uses @javax.annotation.Generated
  • Added GOWORK=off to all Go build env vars — tsgolint's go.work references a local typescript-go directory we don't have
  • Excluded tsgolint/**/* from license header checks in root pom.xml
  • Fixed tsConfigPaths access — used context.getTsConfigPaths() instead of package-private field

2. chore: ignore dirty state in tsgolint submodule

Added ignore = dirty in .gitmodules for the tsgolint submodule. Build artifacts (generated proto stubs, modified go.mod/go.sum, overlay files) make the submodule working tree dirty — this is expected and not a source change.

3. fix: CI setup for tsgolint Go build

The local build required tools that CI doesn't have. Fixed by adding setup steps.


Build Pipeline — Step by Step

Prerequisites (CI installs these, local devs need them)

Tool Version Purpose
Go 1.26+ Cross-compile tsgolint sonar-server binary
protoc 28.3 Generate Go proto stubs from analyzer.proto
protoc-gen-go latest protoc plugin for Go message types
protoc-gen-go-grpc latest protoc plugin for Go gRPC service stubs

CI Workflow Changes (.github/workflows/build.yml)

The Build SonarJS on Linux job now has 3 extra steps before the Maven build:

# 1. Checkout WITH submodules (was: plain checkout)
- uses: actions/checkout@v6
  with:
    submodules: true   # clones tsgolint submodule

# 2. Go 1.26 via mise (separate from shared &mise anchor to avoid polluting other jobs)
- uses: jdx/mise-action@v3.6.1
  with:
    mise_toml: |
      [tools]
      go = "1.26"

# 3. Install protoc + Go proto generators
- run: |
    curl -sSLo /tmp/protoc.zip "https://github.com/protocolbuffers/protobuf/releases/download/v28.3/protoc-28.3-linux-x86_64.zip"
    sudo unzip -o /tmp/protoc.zip -d /usr/local bin/protoc
    go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
    go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest

Maven Build Steps (sonar-plugin/sonar-javascript-plugin/pom.xml)

During generate-resources phase, these exec-maven-plugin executions run in order:

# Step What it does
1 tsgolint-submodule-init git submodule update --init tsgolint (safety net for local builds)
2 tsgolint-copy-overlay Copies bridge/src/main/go/sonar-server/*.gotsgolint/cmd/sonar-server/
3 tsgolint-install-proto-gen go install protoc-gen-go@latest (idempotent)
4 tsgolint-install-proto-gen-grpc go install protoc-gen-go-grpc@latest (idempotent)
5 tsgolint-proto-gen-go protoc --go_out --go-grpc_out generates tsgolint/cmd/sonar-server/grpc/*.pb.go
6 tsgolint-go-get-grpc go get google.golang.org/grpc@latest google.golang.org/protobuf@latest
7-12 build-tsgolint-{platform} go build ./cmd/sonar-server for 6 platforms (linux-x64, linux-arm64, linux-x64-musl, win-x64, darwin-arm64, darwin-x64)
13-18 compress-tsgolint-{platform} xz -k -f compresses each binary

All Go commands use GOWORK=off and CGO_ENABLED=0 for static cross-compilation.

Proto Compilation for Java (sonar-plugin/bridge/pom.xml)

Two protobuf-maven-plugin executions:

# Execution Proto source Output
1 Generate Protobuf Java Sources packages/jsts/src/parsers/ (estree) target/generated-sources/ (protobuf package)
2 Generate gRPC Analyzer Java Sources bridge/src/main/proto/ (analyzer) target/generated-sources/ (grpc package)

Execution 2 has <clearOutputDirectory>false</clearOutputDirectory> to avoid wiping execution 1's output. It also uses protoc-gen-grpc-java plugin for gRPC service stubs.

Shade Plugin (sonar-plugin/sonar-javascript-plugin/pom.xml)

Each platform-specific shade execution (win-x64, linux-x64, etc.) includes an IncludeResourceTransformer for the tsgolint binary:

<resource>tsgolint/{platform}/tsgolint.xz</resource>
<file>${project.build.directory}/tsgolint/{platform}/tsgolint.xz</file>

The multi execution includes all 6 platforms.

@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented Feb 25, 2026

Note: typescript-go nested submodule is NOT needed

tsgolint has a typescript-go git submodule inside it, but we do not initialize it during the build. Here's why it works:

Dependency chain with GOWORK=off:

  1. tsgolint's go.mod requires github.com/microsoft/typescript-go/shim/* v0.0.0
  2. replace directives redirect those to ./shim/* — local directories already present in the tsgolint repo
  3. Each shim's own go.mod depends on github.com/microsoft/typescript-go v0.0.0-20260207160609-5597f4c8ecf4 — a published pseudo-version fetched from the Go module proxy
  4. Go downloads it automatically during go build, no local clone needed

What the submodule + go.work is for:

The typescript-go submodule exists for tsgolint developers who want to iterate on both repos simultaneously. The go.work file ties them together as a local workspace. We skip this entirely with GOWORK=off, so Go resolves everything through the standard module proxy.

# Proof that typescript-go resolves from proxy, not submodule:
$ GOWORK=off go list -m github.com/microsoft/typescript-go
github.com/microsoft/typescript-go v0.0.0-20260207160609-5597f4c8ecf4

@vdiez vdiez force-pushed the feat/tsgolint-grpc-poc branch from 0a80b64 to e3ce7b0 Compare March 3, 2026 13:26
@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented Mar 4, 2026

Session: Wire up tsgolint linter for ruling parity (2026-03-04)

What was done

Commit 7e44188: feat: wire up tsgolint linter with real rule execution

  1. POM build steps updated (sonar-plugin/sonar-javascript-plugin/pom.xml):

    • Removed GOWORK=off from all 9 Go build executions — workspace mode is required for the linter to compile (shim/internal type compatibility)
    • Added 3 new steps after overlay copy: tsgolint-init-typescript-go (submodule init), tsgolint-apply-patches (git am), tsgolint-copy-collections (copy non-test files from patched typescript-go)
    • Cleaned up empty <environmentVariables> blocks from proto-gen steps that only had GOWORK
  2. Go overlay files (already existed, now staged):

    • service.go — Real linter.RunLinter() integration
    • converter.go — Uses rule.RuleDiagnostic + scanner.GetECMALineAndCharacterOfPosition()
    • rules.go — New file with 7 rule imports
  3. tsgolint submodule updated to v0.16.0

  4. WebSensor.java — Send both JS and TS files to tsgolint (was TS-only, but S6671 fires on .js files too)

  5. RulingTest.java — Reduced to router project only for PoC validation

Gotchas discovered

  1. GOWORK=off breaks the build: The tsgolint linter imports from typescript-go/internal/collections via the Go workspace. Disabling workspace mode causes "use of internal package not allowed" errors. Always build with workspace mode enabled.

  2. Patches must be applied before collections copy: typescript-go/internal/collections/ordered_map.go imports github.com/microsoft/typescript-go/internal/json (an internal package). Patch 0006-fix-collections-ordered-map-public-json.patch replaces this with github.com/go-json-experiment/json. The build order must be: init submodule → apply patches → copy collections.

  3. git am glob in POM: The original "${0}"/*.patch glob pattern failed silently in Maven's exec-maven-plugin (the || true swallowed the error). Changed to find ... | sort -z | xargs -0 git am for robustness. The glob failure cause is unclear — may be related to how exec-maven-plugin passes arguments to bash on Windows.

  4. JS files must be sent to tsgolint: The runTsgolintAnalysis() method originally filtered to TypeScript files only. But rules like S6671 (prefer-promise-reject-errors) can fire on JavaScript files too. Changed the filter to include both JS and TS (still excluding CSS/HTML/YAML/Web).

  5. Scanner logs not visible in Maven output: The orchestrator runs SonarQube + scanner as subprocesses. Scanner sensor logs (where tsgolint output appears) are not forwarded to Maven stdout. CE logs are at D:/.sonar/orchestrator/workspace/1/sonarqube-*/logs/ but don't contain sensor output either. Need to investigate how to capture scanner-side logs for debugging.

Remaining issue: ruling test still shows 1 difference

The ruling test on router still fails with "Issues differences: 1" — S6671 expected on router:src/pipeline.js:71 but not produced. The build succeeds and tsgolint binary is in the JAR. However we couldn't verify whether tsgolint is actually starting and analyzing files because scanner logs aren't visible.

Next steps

  1. Debug tsgolint execution during ruling: Find a way to capture scanner-side logs to confirm tsgolint starts, receives the right rules, and processes JS files. Options:

    • Add sonar.verbose=true to the scanner build AND find where orchestrator routes scanner stdout
    • Run a manual SonarQube instance with the plugin and analyze the router project directly
    • Add file-based logging to TsgolintBundle/WebSensor that writes to a known path
  2. Investigate the S6671 gap: Once we can see logs, determine if:

    • tsgolint binary starts successfully
    • prefer-promise-reject-errors rule is in the request
    • pipeline.js is included in the file list
    • The linter produces the diagnostic but it's lost in conversion/reporting
  3. Full ruling suite: Once router passes, restore the full project list in RulingTest.java

@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented Mar 5, 2026

Implemented all requested fixes and validated through the real fast ITS path (WebSensor -> tsgolint binary over gRPC), without relying on the smoke test.

What was fixed

  1. Classloader/runtime crash when creating the gRPC channel
  • Switched Java gRPC client transport from Netty-shaded to OkHttp:
    • AnalyzerGrpcServerImpl: NettyChannelBuilder -> OkHttpChannelBuilder
    • bridge/pom.xml: grpc-netty-shaded -> grpc-okhttp (+ explicit grpc-core)
  • Why this fixes it:
    • The scanner runtime was failing on shaded Netty classes (io.grpc.netty.shaded...DefaultPromise$1) due to classloading conflicts.
    • OkHttp transport avoids Netty-shaded internals entirely while staying on gRPC/HTTP2.
  1. Missing load-balancer policy (pick_first) in scanner runtime
  • Added explicit registration before channel creation:
    • LoadBalancerRegistry.getDefaultRegistry().register(new PickFirstLoadBalancerProvider())
  • This addresses runtime environments where service loader registration for pick_first is incomplete/corrupted.
  1. Stale cached tsgolint binary causing inconsistent behavior
  • Fixed deployment strategy in TsgolintBundle:
    • deploy path now includes SHA-256 of embedded archive: ~/.sonar/js/tsgolint/<hash>/tsgolint[.exe]
    • binary path returned from the bundle points to that hash-specific executable
  • Why this matters:
    • avoids reusing old binaries from cache
    • avoids in-place overwrite of a running .exe on Windows (file lock issue)
    • guarantees scanner run uses the binary matching the plugin artifact under test
  1. WebSensor issue ingestion path for tsgolint
  • WebSensor now normalizes path keys and resolves input files robustly.
  • tsgolint issues are saved through AnalysisProcessor.saveIssue(context, checks, inputFile, issue) to ensure correct file/check context.
  • TsgolintIssueConverter maps rule names -> Sonar rule keys and infers issue language from file extension.
  1. ITS/profile/test data fixes to make assertions valid and deterministic
  • tsgolint-rules.xml: added <priority>INFO</priority> for each active rule entry.
  • no-unnecessary-type-assertion.ts: adjusted fixture to reliably trigger S4325 with current rule behavior.

Validation run

Used the faster profile because no Go code changed:

  • -Pskip-tsgolint (and local -Pskip-node where applicable)
  • -Dlicense.skip=true

Executed:

  1. mvn -pl sonar-plugin/sonar-javascript-plugin -am -Pskip-node,skip-tsgolint -Dlicense.skip=true -DskipTests package
  2. mvn -pl its/plugin/fast-tests -am -Pskip-node,skip-tsgolint -Dlicense.skip=true -DskipTests=false -Dtest=TsgolintIntegrationTest -Dsurefire.failIfNoSpecifiedTests=false test

Result: TsgolintIntegrationTest now passes end-to-end, with WebSensor communicating directly to the tsgolint binary over gRPC and producing the expected rule issues.

Pushed commit:

  • ed3e003eb4 (Fix tsgolint gRPC runtime/classloader path and stabilize ITS)

@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented Mar 5, 2026

Follow-up proposal: externalize tsgolint build orchestration from SonarJS Maven into tsgolint-owned tasks

Given that tsgolint already defines just init (submodule + patch apply + collections sync), the next cleanup should move the remaining orchestration responsibility out of sonar-javascript-plugin/pom.xml and into tsgolint entrypoints.

Suggested next steps

  1. Add dedicated Sonar entrypoints in tsgolint
  • Add stable tasks/scripts in tsgolint for Sonar usage, e.g.:
    • just sonar-init (idempotent init + patching + collections sync)
    • just sonar-build-matrix (build cmd/sonar-server binaries for Sonar target OS/arch)
    • optional just sonar-package (compress outputs and emit manifest)
  • Keep these tasks CI-safe (no global git config assumptions, deterministic paths).
  1. Make SonarJS call those entrypoints instead of re-implementing them
  • In sonar-javascript-plugin/pom.xml, replace granular exec blocks (tsgolint-init-typescript-go, tsgolint-apply-patches, tsgolint-copy-collections, cross-build commands) with one/few calls to tsgolint tasks.
  • Keep SonarJS responsible only for plugin-specific packaging/wiring into shaded artifacts.
  1. Eliminate source duplication for cmd/sonar-server
  • Today SonarJS copies overlay files into tsgolint/cmd/sonar-server.
  • Move toward a single canonical source (preferably in tsgolint/cmd/sonar-server) to avoid drift and copy-step maintenance.
  1. Define clear contract between repos
  • Inputs: proto path, target platform list, output directory.
  • Outputs: built binaries (+ optional checksums/manifest).
  • Versioning: pin tsgolint + patchset compatibility explicitly.
  1. Validation/rollout
  • Add a CI check in SonarJS that verifies generated artifacts via new tsgolint entrypoints.
  • Keep a temporary fallback path for one PR if needed, then remove old Maven-only flow.

Expected benefits

  • Less duplicated build logic in SonarJS.
  • Fewer CI breakages from patch/apply drift.
  • Clear ownership: tsgolint owns how tsgolint is prepared/built; SonarJS owns plugin integration.

@vdiez vdiez force-pushed the feat/tsgolint-grpc-poc branch 2 times, most recently from f3f3941 to dba86da Compare March 16, 2026 15:33
@sonarqube-next
Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
6 New issues
5.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE SonarQube for IDE

vdiez added a commit that referenced this pull request Apr 15, 2026
@vdiez vdiez force-pushed the feat/tsgolint-grpc-poc branch from dba86da to ef9768c Compare April 15, 2026 11:26
@github-actions
Copy link
Copy Markdown
Contributor

README Freshness Check

The rules README is out of date.

A fix PR has been created: #6824

Please review and merge it into your branch.

@sonarqube-next
Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
6 New issues
5.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE SonarQube for IDE

@vdiez vdiez force-pushed the feat/tsgolint-grpc-poc branch from ef9768c to ef4b6f2 Compare April 27, 2026 14:29
@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented Apr 29, 2026

Implemented and pushed in 5f30300.

What was done

  • Extended the Go-side decoration harness in sonar-plugin/bridge/src/main/go/sonar-server so decorated tsgolint rules can be handled in SonarJS-owned code, without committing anything in the tsgolint submodule.
  • Added support for per-run decorator binding and diagnostic transformation/relocation, plus focused Go tests for the harness.
  • Added Go-side decorators for the migrated rules that previously relied on JS-side decoration/filtering behavior:
    • S6544 / no-misused-promises
    • S6551 / no-base-to-string
    • S6557 / prefer-string-starts-ends-with
    • S6571 / no-redundant-type-constituents
    • S6582 / prefer-optional-chain
    • S6606 / prefer-nullish-coalescing
  • Added the Go companion rule no-async-promise-executor and mapped it back to S6544, so S6544 now stays a composite on the Go side.
  • Added requested_rules.go so rule options now flow from protobuf JsTsRule.configurations into the Go endpoint instead of always running with nil options.
  • Kept Sonar-side defaults where needed on the Go path, notably for S6544 and S6606.
  • Registered the migrated rules in JsTsChecks.java and updated the Maven overlay copy step so the full Go overlay and tests are copied into tsgolint/cmd/sonar-server during packaging.

Validation

  • mvn -pl sonar-plugin/sonar-javascript-plugin -am package -DskipTests passed.
  • go test ./cmd/sonar-server in tsgolint passed.
  • After validation, tsgolint and nested typescript-go were restored to clean state. The preserved stashes in both submodules were left untouched.

Final assessment

  • SonarJS currently has 39 Sonar rules whose analyzer implementation references typescript-eslint.
  • The updated tsgolint submodule currently exposes matching implementations for 14 of those 39.
  • This branch now wires 13 of those 14 to the tsgolint gRPC endpoint.
  • Including custom S2870, the Go endpoint now executes 14 Sonar rule keys.

Rules now offloaded to tsgolint

  • S2933 -> prefer-readonly
  • S4123 -> await-thenable
  • S4157 -> no-unnecessary-type-arguments
  • S4325 -> no-unnecessary-type-assertion
  • S6544 -> no-misused-promises (+ Go companion no-async-promise-executor)
  • S6551 -> no-base-to-string
  • S6557 -> prefer-string-starts-ends-with
  • S6565 -> prefer-return-this-type
  • S6571 -> no-redundant-type-constituents
  • S6582 -> prefer-optional-chain
  • S6583 -> no-mixed-enums
  • S6606 -> prefer-nullish-coalescing
  • S6671 -> prefer-promise-reject-errors
  • custom S2870 -> no-array-delete

What is still open

  • There is exactly one additional analyzer-relevant typescript-eslint rule now present in the updated tsgolint submodule but not yet migrated on this branch:
    • S131 -> switch-exhaustiveness-check
  • S131 is not a plain pass-through in SonarJS today. The current JS implementation combines local �missing default� logic with a decorated switch-exhaustiveness-check, forces considerDefaultExhaustiveForUnions: true, and relocates reports to the switch keyword. That needs a dedicated Go port/decorator similar to the rules migrated here.

Remaining SonarJS rules that still use typescript-eslint but are not currently available in tsgolint

  • S109, S1117, S1186, S1534, S1788, S2094, S2814, S2966, S3257, S4023
  • S4124, S4136, S4137, S4138, S4156, S4204, S4327, S6550
  • S6568, S6569, S6572, S6578, S6590, S6598, S905

So the practical status is:

  • 13/39 of SonarJS's current typescript-eslint-backed rules are offloaded today.
  • 13/14 of the analyzer-relevant rules already available in updated tsgolint are offloaded.
  • The remaining migration headroom unlocked by the current submodule update is S131.

@vdiez vdiez force-pushed the feat/tsgolint-grpc-poc branch 2 times, most recently from 7debf2f to 182f9d7 Compare May 6, 2026 07:46
vdiez and others added 7 commits May 12, 2026 10:23
Placeholder commit for tsgolint integration branch.
Offload 7 pure-external typescript-eslint rules to tsgolint (Go-based linter)
running alongside the existing Node.js bridge, orchestrated by Java via gRPC.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds the complete scaffolding to offload 7 pure-external
typescript-eslint rules (S4123, S2933, S4157, S4325, S6565,
S6583, S6671) to tsgolint, a Go-based TypeScript linter,
running alongside the existing Node.js bridge via gRPC.

Key changes:
- analyzer.proto: generic AnalyzerService with server-side streaming
- Java gRPC client: AnalyzerGrpcServerImpl manages process lifecycle,
  ManagedChannel, and streaming response handling
- TsgolintBundle: binary extraction following EmbeddedNode pattern
- TsgolintIssueConverter: proto Issue -> BridgeServer.Issue mapping
- JsTsChecks: rule partitioning (bridge vs tsgolint)
- WebSensor: starts tsgolint alongside bridge, runs parallel analysis
- Maven: Go cross-compilation (6 platforms), xz compression, shade bundling
- Go server scaffold: cmd/sonar-server with gRPC bootstrap
- Integration test with 7 TS files triggering each rule

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add tsgolint (oxc-project/tsgolint) as git submodule
- Move Go server overlay files to sonar-plugin/bridge/src/main/go/
  (copied into submodule at build time since we can't push to upstream)
- Fix proto go_package to match actual module path (typescript-eslint)
- Fix protobuf-maven-plugin clearOutputDirectory conflict
- Add javax.annotation (annotations-api) for gRPC generated code
- Add GOWORK=off to all Go build steps (tsgolint go.work needs local deps)
- Add git submodule init, proto gen, go get, overlay copy build steps
- Exclude tsgolint submodule from license header checks
- Fix tsConfigPaths access (use JsTsContext accessor)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Build process modifies submodule working tree (go get, proto gen,
overlay copy). These are build artifacts, not source changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Linux build: checkout with submodules so tsgolint is available
- Add Go 1.26 via mise (separate step, doesn't pollute shared anchor)
- Install protoc v28.3 from GitHub releases
- Install protoc-gen-go and protoc-gen-go-grpc in CI and POM
- POM go install steps ensure local builds also work

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add mkdir -p before copying Go overlay files into tsgolint submodule
  (the target directory doesn't exist in the upstream repo)
- Download protoc binary via maven-dependency-plugin instead of
  requiring it on PATH — removes curl/unzip from CI workflow
- Lift protobuf.version and os-maven-plugin to parent pom so both
  bridge and sonar-javascript-plugin share the same config
- Add Go setup to Windows build job
- Remove redundant protoc + go proto generator install from CI
  (Maven exec steps already handle this)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Maven dependency:copy doesn't preserve execute permissions.
Wrap protoc call in bash -c with chmod +x to fix permission
denied error on Linux CI.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vdiez vdiez temporarily deployed to sca-checking May 21, 2026 15:06 — with GitHub Actions Inactive
@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 12:58 — with GitHub Actions Inactive
@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented May 22, 2026

Correction to my 2026-05-20 status note: the parity-harness gap is now only \S4123\. \S4325\ no longer belongs on that list after commit \58bfdb0b6\ (\JS-1746 JS-1743 close S4325 parity gaps\). That change added violating-fixture coverage to the \S4325\ parity corpus, aligned the Go diagnostic shape with Node for the remaining mismatches, and both \
pm run jsts-go-parity:diff -- --project S4325-no-unnecessary-type-assertion\ and \
pm run jsts-go-parity:diff-all\ now pass. So the earlier bullet saying both \S4123\ and \S4325\ only exercised the clean path is stale.

Comment on lines +57 to +71
run := s.analyze(analysis.ctx, input)
if err := grpcCallCancelled(stream.Context()); err != nil {
return err
}

for _, filePath := range run.orderedFiles {
if err := stream.Send(&pb.AnalyzeProjectStreamResponse{
Message: &pb.AnalyzeProjectStreamResponse_FileResult{
FileResult: &pb.FileResultMessage{
FilePath: filePath,
Result: run.results[filePath],
},
},
}); err != nil {
return err
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Bug: AnalyzeProject streams results even when analysis was cancelled

In AnalyzeProject (streaming), after s.analyze() returns, the code unconditionally iterates run.orderedFiles and sends all file results via stream.Send (lines 62-73), and only afterwards checks run.cancelled (line 75) to send the cancellation marker. This means that even when analysis was cancelled partway through, all accumulated partial results are sent to the client before the cancellation signal. The client has no way to know which results are complete and which are partial/incomplete due to cancellation.

In contrast, AnalyzeProjectUnary (line 110-113) returns all results including partial ones without any cancellation indication in the response body, so the client silently receives incomplete data.

Consider checking run.cancelled before sending file results, or at minimum documenting that partial results are intentionally sent before the cancellation marker.

Skip sending file results when analysis was cancelled, only send the cancellation marker:

run := s.analyze(analysis.ctx, input)
if err := grpcCallCancelled(stream.Context()); err != nil {
	return err
}

if !run.cancelled {
	for _, filePath := range run.orderedFiles {
		if err := stream.Send(&pb.AnalyzeProjectStreamResponse{
			Message: &pb.AnalyzeProjectStreamResponse_FileResult{
				FileResult: &pb.FileResultMessage{
					FilePath: filePath,
					Result:   run.results[filePath],
				},
			},
		}); err != nil {
			return err
		}
	}
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

Comment on lines +379 to +383
void saveIssue(JsTsContext<?> context, JsTsChecks checks, InputFile inputFile, Issue issue) {
this.checks = checks;
this.file = inputFile;
saveIssue(context, issue);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: AnalysisProcessor.saveIssue mutates shared instance fields

The new saveIssue(JsTsContext, JsTsChecks, InputFile, Issue) overload at lines 379-383 sets the instance fields this.checks and this.file before delegating to the private saveIssue(context, issue). While the two analysis paths (bridge and jsts-go) currently execute sequentially in analyzeFiles() (line 360-362 in WebSensor.java), this pattern is fragile: it silently relies on call-site ordering for correctness, and any future parallelism would introduce a race condition on these shared mutable fields.

Consider passing checks and inputFile through to the methods that need them rather than storing them as instance state.

Was this helpful? React with 👍 / 👎

Comment on lines +26 to +27
- checkerPool = newCheckerPool(4, program, nil)
+ checkerPool = newCheckerPool(runtime.GOMAXPROCS(0), program, nil)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Edge Case: Checker pool sized to GOMAXPROCS may over-allocate on large machines

Patch 0001 changes the TypeScript checker pool size from a hardcoded 4 to runtime.GOMAXPROCS(0). On machines with many cores (e.g., 32-128 in CI or cloud), this creates one checker per logical CPU. Each checker holds a full copy of the type-checking state, so this can significantly increase memory usage for large TypeScript projects without proportional benefit — the linter itself is hardcoded to workers=1 (serial execution in run_linter_on_file.go:125), so only one checker is used at a time during linting.

Consider capping the pool size (e.g., min(runtime.GOMAXPROCS(0), 8)) or aligning it with the actual degree of parallelism in the analysis pipeline.

Cap checker pool at 8 to avoid excessive memory use on high-core machines:

-		checkerPool = newCheckerPool(runtime.GOMAXPROCS(0), program, nil)
+		n := runtime.GOMAXPROCS(0)
+		if n > 8 { n = 8 }
+		checkerPool = newCheckerPool(n, program, nil)
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 13:43 — with GitHub Actions Inactive
Comment on lines +135 to +138
var rulesRunnableWithoutProgram = map[string]struct{}{
"consistent-type-assertions": {},
"no-async-promise-executor": {},
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Bug: AST-only rules missing from rulesRunnableWithoutProgram map

16 of the newly added rules are purely AST-based (they never call ResolvesToGlobalValue, GetTypeAtLocation, or any other type-checker API), yet none of them are registered in rulesRunnableWithoutProgram. This means they will be skipped when no typed program is available (e.g. disableTypeChecking or orphan/AST-only fallback lanes), even though they could run perfectly fine.

Affected rules: no-debugger, no-ternary, no-object-as-default-parameter, adjacent-overload-signatures, no-confusing-non-null-assertion, no-duplicate-enum-values, no-misused-new, no-non-null-assertion, no-explicit-any, no-inferrable-types, no-unnecessary-type-constraint, prefer-as-const, no-negation-in-equality-check, no-this-assignment, no-typeof-undefined, no-instanceof-builtins.

Rules that correctly remain excluded (they use ResolvesToGlobalValue or type-checker): radix, no-setter-return, no-new-func, valid-typeof, no-array-method-this-argument.

Add all 16 AST-only rules to rulesRunnableWithoutProgram so they run even when type information is unavailable.:

var rulesRunnableWithoutProgram = map[string]struct{}{
	"consistent-type-assertions":        {},
	"no-async-promise-executor":          {},
	"no-debugger":                        {},
	"no-ternary":                         {},
	"no-object-as-default-parameter":     {},
	"adjacent-overload-signatures":       {},
	"no-confusing-non-null-assertion":    {},
	"no-duplicate-enum-values":           {},
	"no-misused-new":                     {},
	"no-non-null-assertion":              {},
	"no-explicit-any":                    {},
	"no-inferrable-types":                {},
	"no-unnecessary-type-constraint":     {},
	"prefer-as-const":                    {},
	"no-negation-in-equality-check":      {},
	"no-this-assignment":                 {},
	"no-typeof-undefined":                {},
	"no-instanceof-builtins":             {},
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

Comment on lines +21 to +35
func runDirectRuleOnCode(
t *testing.T,
availableRule rule.Rule,
options any,
fileName string,
code string,
tsconfigName string,
tsconfigContent string,
) []rule.RuleDiagnostic {
t.Helper()

rootDir := fixtures.GetRootDir()
filePath := tspath.ResolvePath(rootDir, fileName)
overlays := map[string]string{filePath: code}
if tsconfigContent != "" {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Duplicate test helper: runDirectRuleOnCode ≈ runRuleOnCode

direct_rule_test_helpers_test.go defines runDirectRuleOnCode which is functionally identical to runRuleOnCode in rule_test_helpers_test.go. Both create overlays, build an overlay VFS over an identically-initialised cached FS (cachedvfs.From(bundled.WrapFS(osvfs.FS()))), create a program, and run the linter with the same parameters. The only differences are variable names (availableRule vs ruleToRun, rule vs rulepkg) and a separate cached FS variable. Tests like no_setter_return_test.go use runRuleOnCode while adjacent_overload_signatures_test.go uses runDirectRuleOnCode, with no semantic distinction. Consider consolidating into a single helper to reduce maintenance burden.

Was this helpful? React with 👍 / 👎

@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 14:39 — with GitHub Actions Inactive
@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 16:33 — with GitHub Actions Inactive
@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 18:17 — with GitHub Actions Inactive
@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 19:21 — with GitHub Actions Inactive
@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 21:18 — with GitHub Actions Inactive
Comment on lines +463 to +469
case '\n':
return "", pos + 1
default:
if !lineBreaksError && source[pos] == '\n' {
return "\n", pos + 1
}
return source[pos : pos+1], pos + 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Dead code in decodeEscapeSequence default branch

In decodeEscapeSequence, the case ' ' at line 463 already handles the newline character and returns ("", pos + 1). The condition source[pos] == ' ' in the default branch (line 466) can therefore never be true, since the switch would have matched the explicit case first. This is dead code that adds confusion about the intended behavior for newline handling in template literals vs quoted strings.

Was this helpful? React with 👍 / 👎

Comment on lines +190 to +204
func SplitRegexLiteral(text string) (string, string, bool) {
if len(text) < 2 || text[0] != '/' {
return "", "", false
}

escaped := false
closingSlash := -1
for index := 1; index < len(text); index++ {
switch {
case escaped:
escaped = false
case text[index] == '\\':
escaped = true
case text[index] == '/':
closingSlash = index
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Duplicate regex parsing infrastructure across two packages

The codebase now has two independent regex parsing implementations: regex_batch_helpers (1080 lines, returns an AST with typed nodes) and regexbatch (731 lines, returns a flat parsed structure). Both implement pattern parsing, escape handling, character classes, quantifiers, and groups with slightly different APIs and completeness levels. For example, regex_batch_helpers.SplitRegexLiteral does not track character classes when finding the closing slash (works correctly because it finds the last unescaped slash, but is fragile), while regexbatch.splitRegexLiteral properly tracks inCharacterClass state. Having two parsers increases maintenance burden and risk of divergent behavior. Consider consolidating into a single regex parser with different output adapters.

Was this helpful? React with 👍 / 👎

@vdiez vdiez temporarily deployed to sca-checking May 22, 2026 22:48 — with GitHub Actions Inactive
@vdiez
Copy link
Copy Markdown
Contributor Author

vdiez commented May 22, 2026

All hard type-service rules are now routed to jsts-go. The last batch passed focused Go tests and the targeted parity corpus, but there are still a few known residuals worth keeping visible in the PR:

  • S6299 (no-vue-bypass-sanitization): the Go implementation currently covers the JSX / render-object cases only. Vue template-body coverage such as v-html is still missing.
  • S5852 (slow-regex): the Go port is still heuristic. It matches the focused parity corpus we added, but it is not yet a full port of the Node-side slow-regex machinery.
  • S4328 (no-implicit-dependencies): focused parity passes, but broader path-resolution edge cases around baseUrl, package layout, and mixed local-vs-package imports may still diverge from Node.
  • S6759 (prefer-read-only-props): the Go implementation is intentionally narrower than Node for React component detection. The focused corpus is green, but broader React patterns may still need follow-up.

So the blocking type-service routing work is in place, but these four rules still have known parity gaps outside the current acceptance corpus.

Comment on lines +141 to +142
case ast.KindForInStatement, ast.KindForOfStatement:
forOf := node.AsForInOrOfStatement()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Bug: for-in/for-of treated as never completing normally without break

In statementCanCompleteNormally, ForInStatement and ForOfStatement return hasBreakStatement(forOf.Statement). This means if the loop body always returns/throws and has no break, the function concludes the for-of cannot complete normally. However, a for-of/for-in over an empty iterable always completes normally regardless of body contents.

Example that would be missed:

function example(items: string[]): number {
  for (const item of items) {
    return item.length;
  }
  // implicit return when items is empty — should be flagged
}

With the current logic, hasImplicitReturn returns false (because the for-of is considered to never complete), so the inconsistency is not reported. This causes false negatives.

for-in/for-of loops can always complete normally (empty iterable), so unconditionally return true.:

case ast.KindForInStatement, ast.KindForOfStatement:
	return true
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 22, 2026

CI failed: The migration to the new Go-based JS/TS analyzer is causing critical runtime crashes (SIGSEGV) in the analysis engine, leading to widespread test failures and gRPC connection errors.

Overview

Multiple CI jobs failed across integration test modules due to segmentation faults in the jsts-go analyzer. These crashes cause the gRPC connection to drop, resulting in UNAVAILABLE errors and subsequent NoSuchFileException failures in the test suite.

Failures

jsts-go Runtime Segmentation Fault (confidence: high)

  • Type: build
  • Affected jobs: javascript-it-plugin-fast-tests, javascript-it-ruling
  • Related to change: yes
  • Root cause: The jsts-go analyzer is encountering SIGSEGV (nil pointer dereference) during execution, likely due to improper handling of specific JS/TS code patterns now routed to the Go service.
  • Suggested fix: Debug the jsts-go binary using the provided stack trace. The crash occurs in main.expressionStatementCall. Check for null pointers when processing AST nodes in the Go migration logic.

gRPC Connection Loss (confidence: high)

  • Type: dependency
  • Affected jobs: javascript-it-plugin-fast-tests, javascript-it-ruling
  • Related to change: yes
  • Root cause: The underlying analyzer crash terminates the gRPC server process. The Java-based integration tests then fail to communicate with the service, reporting io.grpc.StatusRuntimeException: UNAVAILABLE.
  • Suggested fix: Ensure the gRPC server implementation in Go provides better error handling or process monitoring so that crashes are reported more clearly instead of leaving the Java client hanging.

Git SCM Branch Detection Failure (confidence: low)

  • Type: configuration
  • Affected jobs: All
  • Related to change: no
  • Root cause: git symbolic-ref HEAD fails because the environment is using a detached HEAD state.
  • Suggested fix: This is a non-blocking infrastructure warning. If required for SCM features, ensure fetch-depth: 0 is set in the checkout step.

Summary

  • Change-related failures: 2 (Critical analyzer crashes and resulting gRPC failures).
  • Infrastructure/flaky failures: 1 (Git branch detection warning).
  • Recommended action: Immediate focus is required on fixing the null pointer dereference in the Go-based analyzer (jsts-go). The test failures are a direct symptom of the service crashing during rule execution.
Code Review ⚠️ Changes requested 0 resolved / 8 findings

Integrates the Go-based JS/TS analyzer for 15 additional type-service rules, but requires fixes for streaming response leaks, excessive checker pool allocation, and misidentified AST-only rules. Additionally, the analysis processor currently suffers from shared state mutation during issue saving.

⚠️ Bug: AnalyzeProject streams results even when analysis was cancelled

📄 server-go/sonar-server/service.go:57-71

In AnalyzeProject (streaming), after s.analyze() returns, the code unconditionally iterates run.orderedFiles and sends all file results via stream.Send (lines 62-73), and only afterwards checks run.cancelled (line 75) to send the cancellation marker. This means that even when analysis was cancelled partway through, all accumulated partial results are sent to the client before the cancellation signal. The client has no way to know which results are complete and which are partial/incomplete due to cancellation.

In contrast, AnalyzeProjectUnary (line 110-113) returns all results including partial ones without any cancellation indication in the response body, so the client silently receives incomplete data.

Consider checking run.cancelled before sending file results, or at minimum documenting that partial results are intentionally sent before the cancellation marker.

Skip sending file results when analysis was cancelled, only send the cancellation marker
run := s.analyze(analysis.ctx, input)
if err := grpcCallCancelled(stream.Context()); err != nil {
	return err
}

if !run.cancelled {
	for _, filePath := range run.orderedFiles {
		if err := stream.Send(&pb.AnalyzeProjectStreamResponse{
			Message: &pb.AnalyzeProjectStreamResponse_FileResult{
				FileResult: &pb.FileResultMessage{
					FilePath: filePath,
					Result:   run.results[filePath],
				},
			},
		}); err != nil {
			return err
		}
	}
}
⚠️ Edge Case: Checker pool sized to GOMAXPROCS may over-allocate on large machines

📄 server-go/patches/typescript-go/0001-Adapt-project-service-for-single-run-mode.patch:26-27

Patch 0001 changes the TypeScript checker pool size from a hardcoded 4 to runtime.GOMAXPROCS(0). On machines with many cores (e.g., 32-128 in CI or cloud), this creates one checker per logical CPU. Each checker holds a full copy of the type-checking state, so this can significantly increase memory usage for large TypeScript projects without proportional benefit — the linter itself is hardcoded to workers=1 (serial execution in run_linter_on_file.go:125), so only one checker is used at a time during linting.

Consider capping the pool size (e.g., min(runtime.GOMAXPROCS(0), 8)) or aligning it with the actual degree of parallelism in the analysis pipeline.

Cap checker pool at 8 to avoid excessive memory use on high-core machines
-		checkerPool = newCheckerPool(runtime.GOMAXPROCS(0), program, nil)
+		n := runtime.GOMAXPROCS(0)
+		if n > 8 { n = 8 }
+		checkerPool = newCheckerPool(n, program, nil)
⚠️ Bug: AST-only rules missing from rulesRunnableWithoutProgram map

📄 server-go/sonar-server/rules.go:135-138

16 of the newly added rules are purely AST-based (they never call ResolvesToGlobalValue, GetTypeAtLocation, or any other type-checker API), yet none of them are registered in rulesRunnableWithoutProgram. This means they will be skipped when no typed program is available (e.g. disableTypeChecking or orphan/AST-only fallback lanes), even though they could run perfectly fine.

Affected rules: no-debugger, no-ternary, no-object-as-default-parameter, adjacent-overload-signatures, no-confusing-non-null-assertion, no-duplicate-enum-values, no-misused-new, no-non-null-assertion, no-explicit-any, no-inferrable-types, no-unnecessary-type-constraint, prefer-as-const, no-negation-in-equality-check, no-this-assignment, no-typeof-undefined, no-instanceof-builtins.

Rules that correctly remain excluded (they use ResolvesToGlobalValue or type-checker): radix, no-setter-return, no-new-func, valid-typeof, no-array-method-this-argument.

Add all 16 AST-only rules to rulesRunnableWithoutProgram so they run even when type information is unavailable.
var rulesRunnableWithoutProgram = map[string]struct{}{
	"consistent-type-assertions":        {},
	"no-async-promise-executor":          {},
	"no-debugger":                        {},
	"no-ternary":                         {},
	"no-object-as-default-parameter":     {},
	"adjacent-overload-signatures":       {},
	"no-confusing-non-null-assertion":    {},
	"no-duplicate-enum-values":           {},
	"no-misused-new":                     {},
	"no-non-null-assertion":              {},
	"no-explicit-any":                    {},
	"no-inferrable-types":                {},
	"no-unnecessary-type-constraint":     {},
	"prefer-as-const":                    {},
	"no-negation-in-equality-check":      {},
	"no-this-assignment":                 {},
	"no-typeof-undefined":                {},
	"no-instanceof-builtins":             {},
}
⚠️ Bug: for-in/for-of treated as never completing normally without break

📄 server-go/sonar-server/internal/rules/s3801_inconsistent_return/s3801_inconsistent_return.go:141-142

In statementCanCompleteNormally, ForInStatement and ForOfStatement return hasBreakStatement(forOf.Statement). This means if the loop body always returns/throws and has no break, the function concludes the for-of cannot complete normally. However, a for-of/for-in over an empty iterable always completes normally regardless of body contents.

Example that would be missed:

function example(items: string[]): number {
  for (const item of items) {
    return item.length;
  }
  // implicit return when items is empty — should be flagged
}

With the current logic, hasImplicitReturn returns false (because the for-of is considered to never complete), so the inconsistency is not reported. This causes false negatives.

for-in/for-of loops can always complete normally (empty iterable), so unconditionally return true.
case ast.KindForInStatement, ast.KindForOfStatement:
	return true
💡 Quality: AnalysisProcessor.saveIssue mutates shared instance fields

📄 sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/analysis/AnalysisProcessor.java:379-383

The new saveIssue(JsTsContext, JsTsChecks, InputFile, Issue) overload at lines 379-383 sets the instance fields this.checks and this.file before delegating to the private saveIssue(context, issue). While the two analysis paths (bridge and jsts-go) currently execute sequentially in analyzeFiles() (line 360-362 in WebSensor.java), this pattern is fragile: it silently relies on call-site ordering for correctness, and any future parallelism would introduce a race condition on these shared mutable fields.

Consider passing checks and inputFile through to the methods that need them rather than storing them as instance state.

💡 Quality: Duplicate test helper: runDirectRuleOnCode ≈ runRuleOnCode

📄 server-go/sonar-server/direct_rule_test_helpers_test.go:21-35 📄 server-go/sonar-server/rule_test_helpers_test.go:15-29

direct_rule_test_helpers_test.go defines runDirectRuleOnCode which is functionally identical to runRuleOnCode in rule_test_helpers_test.go. Both create overlays, build an overlay VFS over an identically-initialised cached FS (cachedvfs.From(bundled.WrapFS(osvfs.FS()))), create a program, and run the linter with the same parameters. The only differences are variable names (availableRule vs ruleToRun, rule vs rulepkg) and a separate cached FS variable. Tests like no_setter_return_test.go use runRuleOnCode while adjacent_overload_signatures_test.go uses runDirectRuleOnCode, with no semantic distinction. Consider consolidating into a single helper to reduce maintenance burden.

💡 Quality: Dead code in decodeEscapeSequence default branch

📄 server-go/sonar-server/internal/rules/regexbatch/source.go:463-469

In decodeEscapeSequence, the case ' ' at line 463 already handles the newline character and returns ("", pos + 1). The condition source[pos] == ' ' in the default branch (line 466) can therefore never be true, since the switch would have matched the explicit case first. This is dead code that adds confusion about the intended behavior for newline handling in template literals vs quoted strings.

💡 Quality: Duplicate regex parsing infrastructure across two packages

📄 server-go/sonar-server/internal/rules/regex_batch_helpers/regex_batch_helpers.go:190-204 📄 server-go/sonar-server/internal/rules/regexbatch/source.go:473-487

The codebase now has two independent regex parsing implementations: regex_batch_helpers (1080 lines, returns an AST with typed nodes) and regexbatch (731 lines, returns a flat parsed structure). Both implement pattern parsing, escape handling, character classes, quantifiers, and groups with slightly different APIs and completeness levels. For example, regex_batch_helpers.SplitRegexLiteral does not track character classes when finding the closing slash (works correctly because it finds the last unescaped slash, but is fragile), while regexbatch.splitRegexLiteral properly tracks inCharacterClass state. Having two parsers increases maintenance burden and risk of divergent behavior. Consider consolidating into a single regex parser with different output adapters.

🤖 Prompt for agents
Code Review: Integrates the Go-based JS/TS analyzer for 15 additional type-service rules, but requires fixes for streaming response leaks, excessive checker pool allocation, and misidentified AST-only rules. Additionally, the analysis processor currently suffers from shared state mutation during issue saving.

1. ⚠️ Bug: AnalyzeProject streams results even when analysis was cancelled
   Files: server-go/sonar-server/service.go:57-71

   In `AnalyzeProject` (streaming), after `s.analyze()` returns, the code unconditionally iterates `run.orderedFiles` and sends all file results via `stream.Send` (lines 62-73), and only afterwards checks `run.cancelled` (line 75) to send the cancellation marker. This means that even when analysis was cancelled partway through, all accumulated partial results are sent to the client before the cancellation signal. The client has no way to know which results are complete and which are partial/incomplete due to cancellation.
   
   In contrast, `AnalyzeProjectUnary` (line 110-113) returns all results including partial ones without any cancellation indication in the response body, so the client silently receives incomplete data.
   
   Consider checking `run.cancelled` before sending file results, or at minimum documenting that partial results are intentionally sent before the cancellation marker.

   Fix (Skip sending file results when analysis was cancelled, only send the cancellation marker):
   run := s.analyze(analysis.ctx, input)
   if err := grpcCallCancelled(stream.Context()); err != nil {
   	return err
   }
   
   if !run.cancelled {
   	for _, filePath := range run.orderedFiles {
   		if err := stream.Send(&pb.AnalyzeProjectStreamResponse{
   			Message: &pb.AnalyzeProjectStreamResponse_FileResult{
   				FileResult: &pb.FileResultMessage{
   					FilePath: filePath,
   					Result:   run.results[filePath],
   				},
   			},
   		}); err != nil {
   			return err
   		}
   	}
   }

2. 💡 Quality: AnalysisProcessor.saveIssue mutates shared instance fields
   Files: sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/analysis/AnalysisProcessor.java:379-383

   The new `saveIssue(JsTsContext, JsTsChecks, InputFile, Issue)` overload at lines 379-383 sets the instance fields `this.checks` and `this.file` before delegating to the private `saveIssue(context, issue)`. While the two analysis paths (bridge and jsts-go) currently execute sequentially in `analyzeFiles()` (line 360-362 in WebSensor.java), this pattern is fragile: it silently relies on call-site ordering for correctness, and any future parallelism would introduce a race condition on these shared mutable fields.
   
   Consider passing `checks` and `inputFile` through to the methods that need them rather than storing them as instance state.

3. ⚠️ Edge Case: Checker pool sized to GOMAXPROCS may over-allocate on large machines
   Files: server-go/patches/typescript-go/0001-Adapt-project-service-for-single-run-mode.patch:26-27

   Patch 0001 changes the TypeScript checker pool size from a hardcoded `4` to `runtime.GOMAXPROCS(0)`. On machines with many cores (e.g., 32-128 in CI or cloud), this creates one checker per logical CPU. Each checker holds a full copy of the type-checking state, so this can significantly increase memory usage for large TypeScript projects without proportional benefit — the linter itself is hardcoded to `workers=1` (serial execution in `run_linter_on_file.go:125`), so only one checker is used at a time during linting.
   
   Consider capping the pool size (e.g., `min(runtime.GOMAXPROCS(0), 8)`) or aligning it with the actual degree of parallelism in the analysis pipeline.

   Fix (Cap checker pool at 8 to avoid excessive memory use on high-core machines):
   -		checkerPool = newCheckerPool(runtime.GOMAXPROCS(0), program, nil)
   +		n := runtime.GOMAXPROCS(0)
   +		if n > 8 { n = 8 }
   +		checkerPool = newCheckerPool(n, program, nil)

4. ⚠️ Bug: AST-only rules missing from rulesRunnableWithoutProgram map
   Files: server-go/sonar-server/rules.go:135-138

   16 of the newly added rules are purely AST-based (they never call `ResolvesToGlobalValue`, `GetTypeAtLocation`, or any other type-checker API), yet none of them are registered in `rulesRunnableWithoutProgram`. This means they will be skipped when no typed program is available (e.g. `disableTypeChecking` or orphan/AST-only fallback lanes), even though they could run perfectly fine.
   
   Affected rules: `no-debugger`, `no-ternary`, `no-object-as-default-parameter`, `adjacent-overload-signatures`, `no-confusing-non-null-assertion`, `no-duplicate-enum-values`, `no-misused-new`, `no-non-null-assertion`, `no-explicit-any`, `no-inferrable-types`, `no-unnecessary-type-constraint`, `prefer-as-const`, `no-negation-in-equality-check`, `no-this-assignment`, `no-typeof-undefined`, `no-instanceof-builtins`.
   
   Rules that correctly remain excluded (they use `ResolvesToGlobalValue` or type-checker): `radix`, `no-setter-return`, `no-new-func`, `valid-typeof`, `no-array-method-this-argument`.

   Fix (Add all 16 AST-only rules to rulesRunnableWithoutProgram so they run even when type information is unavailable.):
   var rulesRunnableWithoutProgram = map[string]struct{}{
   	"consistent-type-assertions":        {},
   	"no-async-promise-executor":          {},
   	"no-debugger":                        {},
   	"no-ternary":                         {},
   	"no-object-as-default-parameter":     {},
   	"adjacent-overload-signatures":       {},
   	"no-confusing-non-null-assertion":    {},
   	"no-duplicate-enum-values":           {},
   	"no-misused-new":                     {},
   	"no-non-null-assertion":              {},
   	"no-explicit-any":                    {},
   	"no-inferrable-types":                {},
   	"no-unnecessary-type-constraint":     {},
   	"prefer-as-const":                    {},
   	"no-negation-in-equality-check":      {},
   	"no-this-assignment":                 {},
   	"no-typeof-undefined":                {},
   	"no-instanceof-builtins":             {},
   }

5. 💡 Quality: Duplicate test helper: runDirectRuleOnCode ≈ runRuleOnCode
   Files: server-go/sonar-server/direct_rule_test_helpers_test.go:21-35, server-go/sonar-server/rule_test_helpers_test.go:15-29

   `direct_rule_test_helpers_test.go` defines `runDirectRuleOnCode` which is functionally identical to `runRuleOnCode` in `rule_test_helpers_test.go`. Both create overlays, build an overlay VFS over an identically-initialised cached FS (`cachedvfs.From(bundled.WrapFS(osvfs.FS()))`), create a program, and run the linter with the same parameters. The only differences are variable names (`availableRule` vs `ruleToRun`, `rule` vs `rulepkg`) and a separate cached FS variable. Tests like `no_setter_return_test.go` use `runRuleOnCode` while `adjacent_overload_signatures_test.go` uses `runDirectRuleOnCode`, with no semantic distinction. Consider consolidating into a single helper to reduce maintenance burden.

6. 💡 Quality: Dead code in decodeEscapeSequence default branch
   Files: server-go/sonar-server/internal/rules/regexbatch/source.go:463-469

   In `decodeEscapeSequence`, the `case '
   '` at line 463 already handles the newline character and returns `("", pos + 1)`. The condition `source[pos] == '
   '` in the `default` branch (line 466) can therefore never be true, since the switch would have matched the explicit case first. This is dead code that adds confusion about the intended behavior for newline handling in template literals vs quoted strings.

7. 💡 Quality: Duplicate regex parsing infrastructure across two packages
   Files: server-go/sonar-server/internal/rules/regex_batch_helpers/regex_batch_helpers.go:190-204, server-go/sonar-server/internal/rules/regexbatch/source.go:473-487

   The codebase now has two independent regex parsing implementations: `regex_batch_helpers` (1080 lines, returns an AST with typed nodes) and `regexbatch` (731 lines, returns a flat parsed structure). Both implement pattern parsing, escape handling, character classes, quantifiers, and groups with slightly different APIs and completeness levels. For example, `regex_batch_helpers.SplitRegexLiteral` does not track character classes when finding the closing slash (works correctly because it finds the *last* unescaped slash, but is fragile), while `regexbatch.splitRegexLiteral` properly tracks `inCharacterClass` state. Having two parsers increases maintenance burden and risk of divergent behavior. Consider consolidating into a single regex parser with different output adapters.

8. ⚠️ Bug: for-in/for-of treated as never completing normally without break
   Files: server-go/sonar-server/internal/rules/s3801_inconsistent_return/s3801_inconsistent_return.go:141-142

   In `statementCanCompleteNormally`, `ForInStatement` and `ForOfStatement` return `hasBreakStatement(forOf.Statement)`. This means if the loop body always returns/throws and has no break, the function concludes the for-of cannot complete normally. However, a for-of/for-in over an empty iterable always completes normally regardless of body contents.
   
   Example that would be missed:
   ```typescript
   function example(items: string[]): number {
     for (const item of items) {
       return item.length;
     }
     // implicit return when items is empty — should be flagged
   }
   ```
   
   With the current logic, `hasImplicitReturn` returns false (because the for-of is considered to never complete), so the inconsistency is not reported. This causes false negatives.

   Fix (for-in/for-of loops can always complete normally (empty iterable), so unconditionally return true.):
   case ast.KindForInStatement, ast.KindForOfStatement:
   	return true

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change:

Auto-apply Compact Unblock
gitar auto-apply:on         
gitar display:verbose         
gitar unblock         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next
Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
447 New issues
12 Security Hotspots
2.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE SonarQube for IDE

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.

1 participant