From e776bfea114273d1352a40431a1d8191b88df55d Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Mon, 5 Oct 2026 20:20:02 -0300 Subject: [PATCH 1/5] Let DiagnosticManager take Locatables instead of Locations register() now accepts a Locatable (node/token) anywhere it took a Location, including relatedInformation. Only pos/end/source get kept, so the AST isn't retained, and nothing gets turned into a Location until getDiagnostics(). Suppressed diagnostics never get resolved at all. Also fixes register() never merging relatedInformation when the same diagnostic was registered again. Co-Authored-By: Claude Opus 5.5 --- src/DiagnosticManager.spec.ts | 171 +++++++++++++++++++++++++++++- src/DiagnosticManager.ts | 189 ++++++++++++++++++++++++---------- src/interfaces.ts | 22 +++- 3 files changed, 327 insertions(+), 55 deletions(-) diff --git a/src/DiagnosticManager.spec.ts b/src/DiagnosticManager.spec.ts index e9985c3a4..0e5acee17 100644 --- a/src/DiagnosticManager.spec.ts +++ b/src/DiagnosticManager.spec.ts @@ -3,8 +3,10 @@ import type { BrsFile } from './files/BrsFile'; import type { BsDiagnostic } from './interfaces'; import { Program } from './Program'; import { expectDiagnostics, expectZeroDiagnostics } from './testHelpers.spec'; -import util from './util'; -import { expect } from 'chai'; +import util, { standardizePath as s } from './util'; +import { expect } from './chai-config.spec'; +import { createSandbox } from 'sinon'; +import { TokenKind } from './lexer/TokenKind'; describe('DiagnosticManager', () => { @@ -324,6 +326,171 @@ describe('DiagnosticManager', () => { }); }); + describe('register', () => { + it('merges relatedInformation when the same diagnostic is registered more than once', () => { + const location = { uri: 'source/main.brs', range: util.createRange(1, 2, 3, 4) }; + const related1 = { message: 'related1', location: { uri: 'source/a.brs', range: util.createRange(1, 1, 1, 1) } }; + const related2 = { message: 'related2', location: { uri: 'source/b.brs', range: util.createRange(2, 2, 2, 2) } }; + program.diagnostics.register({ message: 'test', location: location, relatedInformation: [related1] }); + program.diagnostics.register({ message: 'test', location: location, relatedInformation: [related1, related2] }); + + const diagnostics = program.getDiagnostics(); + expect(diagnostics).to.be.lengthOf(1); + expect(diagnostics[0].relatedInformation).to.eql([related1, related2]); + }); + }); + + describe('locatables', () => { + const sinon = createSandbox(); + afterEach(() => { + sinon.restore(); + }); + + function getTokens(code: string) { + const file = program.setFile('source/main.brs', code) as BrsFile; + return { file: file, tokens: file.parser.tokens }; + } + + it('resolves a locatable location in getDiagnostics', () => { + const { tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + program.diagnostics.register({ message: 'test', location: printToken }); + expectDiagnostics(program.getDiagnostics(), [ + { message: 'test', location: util.getLocation(printToken) } + ]); + }); + + it('resolves locatable related information in getDiagnostics', () => { + const { tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + const subToken = tokens.find(x => x.kind === TokenKind.Sub); + program.diagnostics.register({ + message: 'test', + location: printToken, + relatedInformation: [{ message: 'related', location: subToken }] + }); + const diagnostics = program.getDiagnostics(); + expect(diagnostics[0].relatedInformation).to.eql([ + { message: 'related', location: util.getLocation(subToken) } + ]); + }); + + it('resolves a token ending in a newline to the same location as util.getLocation', () => { + const { tokens } = getTokens('sub main()\r\n print "hello"\nend sub\n'); + const newlineTokens = tokens.filter(x => x.kind === TokenKind.Newline); + program.diagnostics.register(newlineTokens.map((token, i) => ({ message: `test${i}`, location: token }))); + expectDiagnostics(program.getDiagnostics(), newlineTokens.map((token, i) => ({ + message: `test${i}`, + location: util.getLocation(token) + }))); + }); + + it('does not retain the locatable itself', () => { + const { tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + const subToken = tokens.find(x => x.kind === TokenKind.Sub); + program.diagnostics.register({ + message: 'test', + location: printToken, + relatedInformation: [{ message: 'related', location: subToken }] + }); + const cached = [...program.diagnostics['diagnosticsCache'].values()][0].diagnostic; + expect(cached.location).not.to.equal(printToken); + expect(cached.location).to.eql({ pos: printToken.pos, end: printToken.end, source: printToken.source }); + expect(cached.relatedInformation[0].location).not.to.equal(subToken); + expect(cached.relatedInformation[0].location).to.eql({ pos: subToken.pos, end: subToken.end, source: subToken.source }); + }); + + it('only computes Locations during getDiagnostics', () => { + const { file, tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + const subToken = tokens.find(x => x.kind === TokenKind.Sub); + const scope = new Scope('scope1', program); + const getLocationSpy = sinon.spy(util, 'getLocation'); + + program.diagnostics.register({ + message: 'test', + location: printToken, + relatedInformation: [{ message: 'related', location: subToken }] + }, { scope: scope, tags: ['tag1'] }); + program.diagnostics.register({ message: 'test2', location: subToken }, { tags: ['tag1'] }); + program.diagnostics.clearByFilter({ tag: 'tag2' }); + program.diagnostics.clearForFile('source/other.brs'); + expect(getLocationSpy.called).to.be.false; + + expectDiagnostics(program.getDiagnostics(), [{ message: 'test' }, { message: 'test2' }]); + expect(getLocationSpy.called).to.be.true; + + program.diagnostics.clearForFile(file.srcPath); + expectZeroDiagnostics(program.getDiagnostics()); + }); + + it('clears by file for both locatable and Location diagnostics', () => { + const { file, tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + const otherUri = util.pathToUri(s`${file.srcPath}/../other.brs`); + program.diagnostics.register([ + { message: 'locatable', location: printToken }, + { message: 'location', location: util.getLocation(printToken) }, + { message: 'other', location: { uri: otherUri, range: util.createRange(1, 2, 3, 4) } } + ]); + + program.diagnostics.clearForFile(file.srcPath); + expectDiagnostics(program.getDiagnostics(), [{ message: 'other' }]); + }); + + it('clears by fileUri filter for locatable diagnostics', () => { + const { file, tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + program.diagnostics.register({ message: 'test', location: printToken }, { tags: ['tag1'] }); + + program.diagnostics.clearByFilter({ fileUri: util.pathToUri(file.srcPath), tag: 'tag1' }); + expectZeroDiagnostics(program.getDiagnostics()); + }); + + it('dedupes the same locatable diagnostic from multiple scopes', () => { + const { tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + const subToken = tokens.find(x => x.kind === TokenKind.Sub); + const scope1 = new Scope('scope1', program); + const scope2 = new Scope('scope2', program); + const getDiagnostic = () => ({ + message: 'test', + location: printToken, + relatedInformation: [{ message: 'related', location: subToken }] + }); + program.diagnostics.register([ + { diagnostic: getDiagnostic(), context: { scope: scope1 } }, + { diagnostic: getDiagnostic(), context: { scope: scope2 } } + ]); + + const diagnostics = program.getDiagnostics(); + expect(diagnostics).to.be.lengthOf(1); + expect(diagnostics[0].relatedInformation).to.eql([ + { message: 'related', location: util.getLocation(subToken) }, + { message: `In scope 'scope1'`, location: util.getLocation(printToken) }, + { message: `In scope 'scope2'`, location: util.getLocation(printToken) } + ]); + }); + + it('suppresses locatable diagnostics with comment flags', () => { + const { tokens } = getTokens('sub main()\n \'bs:disable-next-line\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + program.diagnostics.register({ message: 'test', code: 1234, location: printToken }); + expectZeroDiagnostics(program.getDiagnostics()); + }); + + it('uses the locationResolver for synthetic locatables', () => { + const { file } = getTokens('sub main()\nend sub\n'); + program.diagnostics.locationResolver = () => util.createLocation(0, 0, 0, 100, file.srcPath); + program.diagnostics.register({ message: 'test', location: { pos: 0, end: 0, source: undefined } }); + expectDiagnostics(program.getDiagnostics(), [{ + message: 'test (location unknown, added here for visibility)', + location: util.createLocation(0, 0, 0, 100, file.srcPath) + }]); + }); + }); + describe('canSkipScopeValidationForFile', () => { it('returns true if the DiagnosticFilterer says to filter the file', () => { const file = program.setFile('source/main.brs', '') as BrsFile; diff --git a/src/DiagnosticManager.ts b/src/DiagnosticManager.ts index 2024cf268..81e1f4831 100644 --- a/src/DiagnosticManager.ts +++ b/src/DiagnosticManager.ts @@ -1,10 +1,11 @@ -import type { DiagnosticContext, BsDiagnostic, DiagnosticContextPair } from './interfaces'; +import type { DiagnosticContext, BsDiagnostic, DiagnosticContextPair, BsDiagnosticInput, DiagnosticLocationInput } from './interfaces'; +import type { Locatable } from './lexer/Token'; import type { AstNode } from './parser/AstNode'; import type { Scope } from './Scope'; import { util } from './util'; import { Cache } from './Cache'; import { isBsDiagnostic, isXmlScope } from './astUtils/reflection'; -import type { DiagnosticRelatedInformation, Location } from 'vscode-languageserver-protocol'; +import type { Location, Position } from 'vscode-languageserver-protocol'; import { DiagnosticFilterer } from './DiagnosticFilterer'; import { DiagnosticSeverityAdjuster } from './DiagnosticSeverityAdjuster'; import type { FinalizedBsConfig } from './BsConfig'; @@ -17,20 +18,40 @@ import { DiagnosticCodeMap, DiagnosticMessages } from './DiagnosticMessages'; import * as path from 'path'; interface DiagnosticWithContexts { - diagnostic: BsDiagnosticWithKey; + diagnostic: StoredDiagnostic; contexts: Set; } -interface BsDiagnosticWithKey extends BsDiagnostic { +/** + * The position details of a `Locatable`, detached from the locatable itself so the node/token (and its AST) is not retained. + * `text` is only set (to a newline constant) when the locatable ended in a newline, so `util.getLocation()` resolves it the same way + */ +interface DetachedLocatable extends Locatable { + text?: string; +} + +/** + * Either an already-resolved `Location`, or a detached locatable that is only resolved in `getDiagnostics()` + */ +export type StoredLocation = Location | DetachedLocatable; + +export interface StoredRelatedInformation { + message: string; + location: StoredLocation; +} + +export interface StoredDiagnostic extends Omit { key: string; + location: StoredLocation; + relatedInformation: StoredRelatedInformation[]; } export interface LocationResolverArgs { - diagnostic: BsDiagnosticWithKey; + diagnostic: StoredDiagnostic; contexts: Set; } -type LocationResolver = (options: LocationResolverArgs) => Location | undefined; +type LocationResolver = (options: LocationResolverArgs) => DiagnosticLocationInput | undefined; /** * Manages all diagnostics for a program. @@ -67,30 +88,36 @@ export class DiagnosticManager { /** * Registers a diagnostic (or multiple diagnostics) for a program. - * Diagnostics can optionally be associated with a context + * Diagnostics can optionally be associated with a context. + * Locations (including in `relatedInformation`) may be `Locatable`s. Only their position details are kept, + * and they are not resolved into a `Location` until `getDiagnostics()` */ - public register(diagnostic: BsDiagnostic, context?: DiagnosticContext); - public register(diagnostics: Array, context?: DiagnosticContext); + public register(diagnostic: BsDiagnosticInput, context?: DiagnosticContext); + public register(diagnostics: Array, context?: DiagnosticContext); public register(diagnostics: Array); - public register(diagnosticArg: BsDiagnostic | Array, context?: DiagnosticContext) { + public register(diagnosticArg: BsDiagnosticInput | Array, context?: DiagnosticContext) { const diagnostics = Array.isArray(diagnosticArg) ? diagnosticArg : [{ diagnostic: diagnosticArg, context: context }]; for (const diagnosticData of diagnostics) { - const diagnostic = isBsDiagnostic(diagnosticData) ? diagnosticData : diagnosticData.diagnostic; + const diagnostic = isBsDiagnostic(diagnosticData) ? diagnosticData as BsDiagnosticInput : (diagnosticData as DiagnosticContextPair).diagnostic; const diagContext = (diagnosticData as DiagnosticContextPair)?.context ?? context; - const key = this.getDiagnosticKey(diagnostic); + const location = this.storeLocation(diagnostic.location); + const relatedInformation = (diagnostic.relatedInformation ?? []).map(x => ({ + message: x.message, + location: this.storeLocation(x.location) + })); + const key = this.getDiagnosticKey(diagnostic, location); let fromCache = true; const cacheData = this.diagnosticsCache.getOrAdd(key, () => { - - if (!diagnostic.relatedInformation) { - diagnostic.relatedInformation = []; - } fromCache = false; - return { diagnostic: { key: key, ...diagnostic }, contexts: new Set() }; + return { + diagnostic: { ...diagnostic, key: key, location: location, relatedInformation: relatedInformation }, + contexts: new Set() + }; }); const cachedDiagnostic = cacheData.diagnostic; - if (!fromCache && diagnostic.relatedInformation) { - this.mergeRelatedInformation(cachedDiagnostic.relatedInformation, diagnostic.relatedInformation); + if (fromCache) { + this.mergeRelatedInformation(cachedDiagnostic.relatedInformation, relatedInformation); } const contexts = cacheData.contexts; if (diagContext) { @@ -100,8 +127,8 @@ export class DiagnosticManager { } } - private addToMaps(diagnostic: BsDiagnosticWithKey, context?: DiagnosticContext) { - const uriLower = util.pathToUri(diagnostic.location?.uri?.toLowerCase()); + private addToMaps(diagnostic: StoredDiagnostic, context?: DiagnosticContext) { + const uriLower = util.pathToUri(this.getUri(diagnostic.location)?.toLowerCase()); if (uriLower) { if (!this.fileUriMap.has(uriLower)) { this.fileUriMap.set(uriLower, new Set()); @@ -160,8 +187,26 @@ export class DiagnosticManager { private getNonSuppressedDiagnostics() { const results = [] as Array; for (const cachedDiagnostic of this.diagnosticsCache.values()) { - const diagnostic = { ...cachedDiagnostic.diagnostic }; - const relatedInformation = [...cachedDiagnostic.diagnostic.relatedInformation]; + const storedDiagnostic = cachedDiagnostic.diagnostic; + let location = storedDiagnostic.location; + let message = storedDiagnostic.message; + if (!this.getUri(location)) { + location = this.storeLocation(this.locationResolver?.(cachedDiagnostic)); + if (location) { + //if we found a location, tweak the message a bit to let devs know this was not the original location + message = `${message} (location unknown, added here for visibility)`; + } + } + //check suppression before resolving any locations, so suppressed diagnostics never pay for it + if (this.isSuppressed(storedDiagnostic.code, storedDiagnostic.legacyCode, location)) { + continue; + } + + const originalLocation = this.resolveLocation(storedDiagnostic.location); + const relatedInformation = storedDiagnostic.relatedInformation.map(x => ({ + message: x.message, + location: this.resolveLocation(x.location) + })); const affectedScopes = new Set(); for (const context of cachedDiagnostic.contexts.values()) { if (context.scope) { @@ -180,42 +225,46 @@ export class DiagnosticManager { } else { relatedInformation.push({ message: `In scope '${scope.name}'`, - location: diagnostic.location + location: originalLocation }); } } - diagnostic.relatedInformation = relatedInformation; - if (!diagnostic.location?.uri) { - diagnostic.location = this.locationResolver?.(cachedDiagnostic); - if (diagnostic.location) { - //if we found a location, tweak the message a bit to let devs know this was not the original location - diagnostic.message = `${diagnostic.message} (location unknown, added here for visibility)`; - } - } - results.push(diagnostic); + results.push({ + ...storedDiagnostic, + message: message, + location: location === storedDiagnostic.location ? originalLocation : this.resolveLocation(location), + relatedInformation: relatedInformation + }); } - const filteredResults = results.filter((x) => { - return !this.isDiagnosticSuppressed(x); - }); - return filteredResults; + return results; } /** * Determine whether this diagnostic should be supressed or not, based on brs comment-flags */ public isDiagnosticSuppressed(diagnostic: BsDiagnostic) { - const diagnosticCode = typeof diagnostic.code === 'string' ? diagnostic.code.toLowerCase() : diagnostic.code?.toString() ?? undefined; - const diagnosticLegacyCode = typeof diagnostic.legacyCode === 'string' ? diagnostic.legacyCode.toLowerCase() : diagnostic.legacyCode; - const file = this.program?.getFile(diagnostic.location?.uri); + return this.isSuppressed(diagnostic.code, diagnostic.legacyCode, diagnostic.location); + } + + private isSuppressed(code: number | string | undefined, legacyCode: number | string | undefined, location: StoredLocation) { + const diagnosticCode = typeof code === 'string' ? code.toLowerCase() : code?.toString() ?? undefined; + const diagnosticLegacyCode = typeof legacyCode === 'string' ? legacyCode.toLowerCase() : legacyCode; + const file = this.program?.getFile(this.getUri(location)); if (diagnosticCode === DiagnosticCodeMap.unknownDiagnosticCode) { return false; } + //only computed if the file actually has comment flags + let start: Position | undefined; + let isStartComputed = false; for (let flag of file?.commentFlags ?? []) { - - if (!diagnostic.location?.range || !util.rangeContains(flag.affectedRange, diagnostic.location.range.start)) { + if (!isStartComputed) { + start = this.isDetachedLocatable(location) ? util.getStartPosition(location) : location?.range?.start; + isStartComputed = true; + } + if (!start || !util.rangeContains(flag.affectedRange, start)) { continue; } //if this flag explicitly re-enables the code, it's not suppressed here, keep looking @@ -404,7 +453,7 @@ export class DiagnosticManager { isMatch = context.scope?.name === filter.scope.name; } if (isMatch && needToMatch.fileUri) { - isMatch = diagnostic.location?.uri === filter.fileUri; + isMatch = this.getUri(diagnostic.location) === filter.fileUri; } if (isMatch && needToMatch.segment) { isMatch = context.segment === filter.segment; @@ -422,7 +471,7 @@ export class DiagnosticManager { } } - private deleteContextsFromDiagnostic(diagnostic: BsDiagnosticWithKey, contexts: DiagnosticContext[]) { + private deleteContextsFromDiagnostic(diagnostic: StoredDiagnostic, contexts: DiagnosticContext[]) { const key = diagnostic.key; const cachedData = this.diagnosticsCache.get(key); for (const context of contexts) { @@ -468,24 +517,24 @@ export class DiagnosticManager { } } - private removeDiagnosticIfNoContexts(diagnostic: BsDiagnosticWithKey) { + private removeDiagnosticIfNoContexts(diagnostic: StoredDiagnostic) { const key = diagnostic.key; const cachedData = this.diagnosticsCache.get(key); if (cachedData.contexts.size === 0) { this.diagnosticsCache.delete(key); - this.fileUriMap.get(diagnostic.location?.uri?.toLowerCase())?.delete(key); + this.fileUriMap.get(this.getUri(diagnostic.location)?.toLowerCase())?.delete(key); } } - private getDiagnosticKey(diagnostic: BsDiagnostic) { - return `${diagnostic.location?.uri ?? 'No uri'} ${util.rangeToString(diagnostic.location?.range)} - ${diagnostic.code} - ${diagnostic.message}`; + private getDiagnosticKey(diagnostic: BsDiagnosticInput, location: StoredLocation) { + return `${this.getLocationKey(location)} - ${diagnostic.code} - ${diagnostic.message}`; } - private mergeRelatedInformation(target: DiagnosticRelatedInformation[], source: DiagnosticRelatedInformation[]) { - function getRiKey(relatedInfo: DiagnosticRelatedInformation) { - return `${relatedInfo.message} - ${relatedInfo.location?.uri} - ${util.rangeToString(relatedInfo.location?.range)}`.toLowerCase(); - } + private mergeRelatedInformation(target: StoredRelatedInformation[], source: StoredRelatedInformation[]) { + const getRiKey = (relatedInfo: StoredRelatedInformation) => { + return `${relatedInfo.message} - ${this.getLocationKey(relatedInfo.location)}`.toLowerCase(); + }; const existingKeys = target.map(ri => getRiKey(ri)); @@ -497,6 +546,42 @@ export class DiagnosticManager { } } + private isDetachedLocatable(location: StoredLocation | DiagnosticLocationInput): location is DetachedLocatable { + return typeof (location as Locatable)?.pos === 'number'; + } + + /** + * Convert an incoming location into the form we keep in the cache. Locatables are copied down to just their position details + * so the node/token they came from (and its whole AST) is not retained by the diagnostic + */ + private storeLocation(location: DiagnosticLocationInput | undefined): StoredLocation | undefined { + if (!this.isDetachedLocatable(location)) { + return location; + } + const detached: DetachedLocatable = { pos: location.pos, end: location.end, source: location.source }; + //`util.getLocation()` keeps a newline-terminated token on its own line by looking at its text, so keep just enough text for that + const newlineLength = location.end - util.getContentEnd(location); + if (newlineLength > 0) { + detached.text = newlineLength === 2 ? '\r\n' : '\n'; + } + return detached; + } + + private getUri(location: StoredLocation | undefined): string | undefined { + return this.isDetachedLocatable(location) ? location.source?.uri : location?.uri; + } + + private getLocationKey(location: StoredLocation | undefined) { + if (this.isDetachedLocatable(location)) { + return `${location.source?.uri ?? 'No uri'} @${location.pos}-${location.end}`; + } + return `${location?.uri ?? 'No uri'} ${util.rangeToString(location?.range)}`; + } + + private resolveLocation(location: StoredLocation | undefined): Location | undefined { + return this.isDetachedLocatable(location) ? util.getLocation(location) : location; + } + /** * Are the diagnostics for this file completely filtered? * If so, we can skip any Scope-based validation on this file at all, which can save a lot of time for large files diff --git a/src/interfaces.ts b/src/interfaces.ts index 04f8d35fa..b6586d65e 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -89,10 +89,30 @@ export interface DiagnosticContext { } export interface DiagnosticContextPair { - diagnostic: BsDiagnostic; + diagnostic: BsDiagnosticInput; context?: DiagnosticContext; } +/** + * A location as accepted by the `DiagnosticManager`: either an already-resolved `Location`, or a `Locatable` that is resolved + * into a `Location` only when diagnostics are retrieved + */ +export type DiagnosticLocationInput = Location | Locatable; + +export interface BsDiagnosticRelatedInformationInput { + message: string; + location: DiagnosticLocationInput; +} + +/** + * A diagnostic as accepted by `DiagnosticManager.register()`. Locations may be `Locatable`s (such as a node or token), + * which are not retained: only their position details are kept until `getDiagnostics()` resolves them + */ +export type BsDiagnosticInput = Omit & { + location: DiagnosticLocationInput; + relatedInformation?: BsDiagnosticRelatedInformationInput[]; +}; + export interface Callable { file: BscFile; name: string; From 237a5ec9056a94dfb75ee8459a62f7e6bd17fd7e Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Tue, 6 Oct 2026 08:22:01 -0300 Subject: [PATCH 2/5] Add util.firstLocatable() Locatable version of `util.getLocation(a) ?? util.getLocation(b)`, for diagnostics that fall back to another node when the first one is synthetic. Co-Authored-By: Claude Opus 5.5 --- src/util.spec.ts | 14 ++++++++++++++ src/util.ts | 13 +++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/util.spec.ts b/src/util.spec.ts index f6b6a7169..fe01533cf 100644 --- a/src/util.spec.ts +++ b/src/util.spec.ts @@ -1177,6 +1177,20 @@ describe('util', () => { }); }); + describe('firstLocatable', () => { + it('returns the first item that has a source', () => { + const [a, b] = Lexer.scan('a b').tokens; + const synthetic = createToken(TokenKind.Identifier, 'c'); + expect(util.firstLocatable(undefined, synthetic, a, b)).to.equal(a); + expect(util.firstLocatable(b, a)).to.equal(b); + }); + + it('returns undefined when nothing has a source', () => { + expect(util.firstLocatable(undefined, createToken(TokenKind.Identifier, 'c'))).to.be.undefined; + expect(util.firstLocatable()).to.be.undefined; + }); + }); + describe('getOffset', () => { const source = Lexer.scan('ab\ncd\nef').tokens[0].source; diff --git a/src/util.ts b/src/util.ts index 0c2b6ba51..ac286c09d 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1446,6 +1446,19 @@ export class Util { return locatable.end; } + /** + * Get the first item that has a real location (a `source`), skipping `undefined` and synthetic items. + * The locatable equivalent of `util.getLocation(a) ?? util.getLocation(b)` + */ + public firstLocatable(...items: Array): T | undefined { + for (const item of items) { + if (item?.source) { + return item; + } + } + return undefined; + } + /** * Build a line/character `Location` for the locatable. Returns `undefined` for synthetic items (those without a `source`) */ From 5a3c22b1f7a0726468e80a8b5901e4e626659361 Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Tue, 6 Oct 2026 08:35:40 -0300 Subject: [PATCH 3/5] Treat synthetic tokens/nodes as locatables in DiagnosticManager They have no pos, so they were being stored (and returned) as if they were a Location. Check for a Location's uri/range instead. Co-Authored-By: Claude Opus 5.5 --- src/DiagnosticManager.spec.ts | 15 +++++++++++++++ src/DiagnosticManager.ts | 5 +++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/DiagnosticManager.spec.ts b/src/DiagnosticManager.spec.ts index 0e5acee17..b2f69453e 100644 --- a/src/DiagnosticManager.spec.ts +++ b/src/DiagnosticManager.spec.ts @@ -7,6 +7,7 @@ import util, { standardizePath as s } from './util'; import { expect } from './chai-config.spec'; import { createSandbox } from 'sinon'; import { TokenKind } from './lexer/TokenKind'; +import { createToken } from './astUtils/creators'; describe('DiagnosticManager', () => { @@ -480,6 +481,20 @@ describe('DiagnosticManager', () => { expectZeroDiagnostics(program.getDiagnostics()); }); + it('does not return or retain a synthetic token that has no position', () => { + const { file } = getTokens('sub main()\nend sub\n'); + program.diagnostics.locationResolver = () => util.createLocation(0, 0, 0, 100, file.srcPath); + const token = createToken(TokenKind.Identifier, 'synthetic'); + program.diagnostics.register({ message: 'test', location: token }); + + const cached = [...program.diagnostics['diagnosticsCache'].values()][0].diagnostic; + expect(cached.location).not.to.equal(token); + expectDiagnostics(program.getDiagnostics(), [{ + message: 'test (location unknown, added here for visibility)', + location: util.createLocation(0, 0, 0, 100, file.srcPath) + }]); + }); + it('uses the locationResolver for synthetic locatables', () => { const { file } = getTokens('sub main()\nend sub\n'); program.diagnostics.locationResolver = () => util.createLocation(0, 0, 0, 100, file.srcPath); diff --git a/src/DiagnosticManager.ts b/src/DiagnosticManager.ts index 81e1f4831..5e7045126 100644 --- a/src/DiagnosticManager.ts +++ b/src/DiagnosticManager.ts @@ -547,7 +547,8 @@ export class DiagnosticManager { } private isDetachedLocatable(location: StoredLocation | DiagnosticLocationInput): location is DetachedLocatable { - return typeof (location as Locatable)?.pos === 'number'; + //a `Location` always has `uri` and `range`, tokens and nodes never do (and synthetic ones don't even have a `pos`) + return !!location && !('range' in location) && !('uri' in location); } /** @@ -560,7 +561,7 @@ export class DiagnosticManager { } const detached: DetachedLocatable = { pos: location.pos, end: location.end, source: location.source }; //`util.getLocation()` keeps a newline-terminated token on its own line by looking at its text, so keep just enough text for that - const newlineLength = location.end - util.getContentEnd(location); + const newlineLength = location.source ? location.end - util.getContentEnd(location) : 0; if (newlineLength > 0) { detached.text = newlineLength === 2 ? '\r\n' : '\n'; } From 97fde8ea33f65a179cbc8973aec7b766da4f1d3a Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Tue, 6 Oct 2026 09:00:26 -0300 Subject: [PATCH 4/5] Detect locatables by pos, not by a missing range Tokens and nodes can have deprecated range/location getters (#1876), which made them look like a Location. Co-Authored-By: Claude Opus 5.5 --- src/DiagnosticManager.spec.ts | 21 +++++++++++++++++++++ src/DiagnosticManager.ts | 5 +++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/DiagnosticManager.spec.ts b/src/DiagnosticManager.spec.ts index b2f69453e..3f5139adf 100644 --- a/src/DiagnosticManager.spec.ts +++ b/src/DiagnosticManager.spec.ts @@ -481,6 +481,27 @@ describe('DiagnosticManager', () => { expectZeroDiagnostics(program.getDiagnostics()); }); + it('treats tokens and nodes with deprecated range/location getters as locatables', () => { + const { tokens } = getTokens('sub main()\n print "hello"\nend sub\n'); + const printToken = tokens.find(x => x.kind === TokenKind.Print); + //mimic the deprecated `range`/`location` getters that tokens and nodes can have + const tokenWithGetters = Object.create({ + get range() { + return util.getLocation(printToken).range; + }, + get location() { + return util.getLocation(printToken); + } + }, Object.getOwnPropertyDescriptors(printToken)); + program.diagnostics.register({ message: 'test', location: tokenWithGetters }); + + const cached = [...program.diagnostics['diagnosticsCache'].values()][0].diagnostic; + expect(cached.location).to.eql({ pos: printToken.pos, end: printToken.end, source: printToken.source }); + expectDiagnostics(program.getDiagnostics(), [ + { message: 'test', location: util.getLocation(printToken) } + ]); + }); + it('does not return or retain a synthetic token that has no position', () => { const { file } = getTokens('sub main()\nend sub\n'); program.diagnostics.locationResolver = () => util.createLocation(0, 0, 0, 100, file.srcPath); diff --git a/src/DiagnosticManager.ts b/src/DiagnosticManager.ts index 5e7045126..707e97d7d 100644 --- a/src/DiagnosticManager.ts +++ b/src/DiagnosticManager.ts @@ -547,8 +547,9 @@ export class DiagnosticManager { } private isDetachedLocatable(location: StoredLocation | DiagnosticLocationInput): location is DetachedLocatable { - //a `Location` always has `uri` and `range`, tokens and nodes never do (and synthetic ones don't even have a `pos`) - return !!location && !('range' in location) && !('uri' in location); + //a `Location` always has a `uri` and never a `pos`. Tokens and nodes always have a `pos` key (even synthetic ones, where it's undefined), + //and may also have deprecated `range`/`location` getters, so those can't be used to tell them apart + return !!location && ('pos' in location || !('uri' in location)); } /** From c873d2961ad7c2d5e31be2fe65883f119533f110 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Thu, 15 Oct 2026 16:30:34 -0400 Subject: [PATCH 5/5] Rename Locatable props to match flatten-locations Co-Authored-By: Claude Opus 5.5 --- src/DiagnosticManager.spec.ts | 8 ++--- src/DiagnosticManager.ts | 15 ++++---- src/util.spec.ts | 24 ++++++------- src/util.ts | 66 +++++++++++++++++------------------ 4 files changed, 57 insertions(+), 56 deletions(-) diff --git a/src/DiagnosticManager.spec.ts b/src/DiagnosticManager.spec.ts index 3f5139adf..243e557e2 100644 --- a/src/DiagnosticManager.spec.ts +++ b/src/DiagnosticManager.spec.ts @@ -397,9 +397,9 @@ describe('DiagnosticManager', () => { }); const cached = [...program.diagnostics['diagnosticsCache'].values()][0].diagnostic; expect(cached.location).not.to.equal(printToken); - expect(cached.location).to.eql({ pos: printToken.pos, end: printToken.end, source: printToken.source }); + expect(cached.location).to.eql({ startPosition: printToken.startPosition, endPosition: printToken.endPosition, sourceInfo: printToken.sourceInfo }); expect(cached.relatedInformation[0].location).not.to.equal(subToken); - expect(cached.relatedInformation[0].location).to.eql({ pos: subToken.pos, end: subToken.end, source: subToken.source }); + expect(cached.relatedInformation[0].location).to.eql({ startPosition: subToken.startPosition, endPosition: subToken.endPosition, sourceInfo: subToken.sourceInfo }); }); it('only computes Locations during getDiagnostics', () => { @@ -496,7 +496,7 @@ describe('DiagnosticManager', () => { program.diagnostics.register({ message: 'test', location: tokenWithGetters }); const cached = [...program.diagnostics['diagnosticsCache'].values()][0].diagnostic; - expect(cached.location).to.eql({ pos: printToken.pos, end: printToken.end, source: printToken.source }); + expect(cached.location).to.eql({ startPosition: printToken.startPosition, endPosition: printToken.endPosition, sourceInfo: printToken.sourceInfo }); expectDiagnostics(program.getDiagnostics(), [ { message: 'test', location: util.getLocation(printToken) } ]); @@ -519,7 +519,7 @@ describe('DiagnosticManager', () => { it('uses the locationResolver for synthetic locatables', () => { const { file } = getTokens('sub main()\nend sub\n'); program.diagnostics.locationResolver = () => util.createLocation(0, 0, 0, 100, file.srcPath); - program.diagnostics.register({ message: 'test', location: { pos: 0, end: 0, source: undefined } }); + program.diagnostics.register({ message: 'test', location: { startPosition: 0, endPosition: 0, sourceInfo: undefined } }); expectDiagnostics(program.getDiagnostics(), [{ message: 'test (location unknown, added here for visibility)', location: util.createLocation(0, 0, 0, 100, file.srcPath) diff --git a/src/DiagnosticManager.ts b/src/DiagnosticManager.ts index 707e97d7d..df7f28bc8 100644 --- a/src/DiagnosticManager.ts +++ b/src/DiagnosticManager.ts @@ -547,9 +547,10 @@ export class DiagnosticManager { } private isDetachedLocatable(location: StoredLocation | DiagnosticLocationInput): location is DetachedLocatable { - //a `Location` always has a `uri` and never a `pos`. Tokens and nodes always have a `pos` key (even synthetic ones, where it's undefined), - //and may also have deprecated `range`/`location` getters, so those can't be used to tell them apart - return !!location && ('pos' in location || !('uri' in location)); + //a `Location` always has a `uri` and never a `startPosition`. Tokens and nodes always have a `startPosition` key (even synthetic ones, where it's undefined), + //and may also have deprecated `range`/`location` getters, so those can't be used to tell them apart. + //never check for `source` here: that's the diagnostic's own `source` string (i.e. 'brs' or 'bslint'), not a locatable field + return !!location && ('startPosition' in location || !('uri' in location)); } /** @@ -560,9 +561,9 @@ export class DiagnosticManager { if (!this.isDetachedLocatable(location)) { return location; } - const detached: DetachedLocatable = { pos: location.pos, end: location.end, source: location.source }; + const detached: DetachedLocatable = { startPosition: location.startPosition, endPosition: location.endPosition, sourceInfo: location.sourceInfo }; //`util.getLocation()` keeps a newline-terminated token on its own line by looking at its text, so keep just enough text for that - const newlineLength = location.source ? location.end - util.getContentEnd(location) : 0; + const newlineLength = location.sourceInfo ? location.endPosition - util.getContentEnd(location) : 0; if (newlineLength > 0) { detached.text = newlineLength === 2 ? '\r\n' : '\n'; } @@ -570,12 +571,12 @@ export class DiagnosticManager { } private getUri(location: StoredLocation | undefined): string | undefined { - return this.isDetachedLocatable(location) ? location.source?.uri : location?.uri; + return this.isDetachedLocatable(location) ? location.sourceInfo?.uri : location?.uri; } private getLocationKey(location: StoredLocation | undefined) { if (this.isDetachedLocatable(location)) { - return `${location.source?.uri ?? 'No uri'} @${location.pos}-${location.end}`; + return `${location.sourceInfo?.uri ?? 'No uri'} @${location.startPosition}-${location.endPosition}`; } return `${location?.uri ?? 'No uri'} ${util.rangeToString(location?.range)}`; } diff --git a/src/util.spec.ts b/src/util.spec.ts index 226aa74d9..bf0805f2b 100644 --- a/src/util.spec.ts +++ b/src/util.spec.ts @@ -1135,7 +1135,7 @@ describe('util', () => { }); it('clears the bounds when nothing has a source', () => { - const result = util.setBounds({ pos: 1, end: 2, source: testLocatable(0, 0, 0, 0).source }, createToken(TokenKind.Identifier, 'a')); + const result = util.setBounds({ pos: 1, end: 2, source: testLocatable(0, 0, 0, 0).sourceInfo }, createToken(TokenKind.Identifier, 'a')); expect(result).to.eql({ pos: undefined, end: undefined, source: undefined }); }); @@ -1185,21 +1185,21 @@ describe('util', () => { }); describe('firstLocatable', () => { - it('returns the first item that has a source', () => { + it('returns the first item that has a sourceInfo', () => { const [a, b] = Lexer.scan('a b').tokens; const synthetic = createToken(TokenKind.Identifier, 'c'); expect(util.firstLocatable(undefined, synthetic, a, b)).to.equal(a); expect(util.firstLocatable(b, a)).to.equal(b); }); - it('returns undefined when nothing has a source', () => { + it('returns undefined when nothing has a sourceInfo', () => { expect(util.firstLocatable(undefined, createToken(TokenKind.Identifier, 'c'))).to.be.undefined; expect(util.firstLocatable()).to.be.undefined; }); }); describe('getOffset', () => { - const source = Lexer.scan('ab\ncd\nef').tokens[0].source; + const source = Lexer.scan('ab\ncd\nef').tokens[0].sourceInfo; it('converts positions to offsets', () => { expect(util.getOffset(source, util.createPosition(0, 0))).to.eql(0); @@ -1223,37 +1223,37 @@ describe('util', () => { it('maps the location into the source, so it combines with that source', () => { const tokens = Lexer.scan('a = 1\nbb = 2', { srcPath: srcPath }).tokens; - const sourceInfo = tokens[0].source; + const sourceInfo = tokens[0].sourceInfo; const location = util.createLocation(1, 0, 1, 2, sourceInfo.uri); const result = util.setLocation({} as Locatable, location, sourceInfo); - expect(result.source).to.equal(sourceInfo); - expect(result.pos).to.eql(6); - expect(result.end).to.eql(8); + expect(result.sourceInfo).to.equal(sourceInfo); + expect(result.startPosition).to.eql(6); + expect(result.endPosition).to.eql(8); expect(util.getLocation(result)).to.eql(location); expect(util.getLocation(util.setBounds({} as Locatable, tokens[0], result)).range).to.eql(util.createRange(0, 0, 1, 2)); }); it('clears the position when given no location or source', () => { - const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].source; + const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].sourceInfo; expect(util.setLocation(testLocatable(0, 0, 0, 1), undefined, sourceInfo)).to.eql({ pos: undefined, end: undefined, source: undefined }); expect(util.setLocation(testLocatable(0, 0, 0, 1), util.createLocation(0, 0, 0, 1), undefined)).to.eql({ pos: undefined, end: undefined, source: undefined }); }); it('clears the position for a location in a different file', () => { - const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].source; + const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].sourceInfo; const result = util.setLocation(testLocatable(0, 0, 0, 1), util.createLocation(0, 0, 0, 1, 'file:///other.brs'), sourceInfo); expect(util.getLocation(result)).to.be.undefined; }); it('clears the position for lines that are not in the source', () => { - const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].source; + const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].sourceInfo; //bsc builds ranges like this for comment flags that run to the end of the file const result = util.setLocation({} as Locatable, util.createLocation(0, 0, Number.MAX_SAFE_INTEGER, 0, sourceInfo.uri), sourceInfo); expect(util.getLocation(result)).to.be.undefined; }); it('keeps a character past the end of a line on that line', () => { - const sourceInfo = Lexer.scan('a = 1\nb = 2', { srcPath: srcPath }).tokens[0].source; + const sourceInfo = Lexer.scan('a = 1\nb = 2', { srcPath: srcPath }).tokens[0].sourceInfo; //bsc builds ranges like this for whole-file diagnostics const result = util.setLocation({} as Locatable, util.createLocation(0, 0, 0, Number.MAX_VALUE, sourceInfo.uri), sourceInfo); expect(util.getLocation(result).range.end.line).to.eql(0); diff --git a/src/util.ts b/src/util.ts index 20c4ef5a2..fa125f5f7 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1268,9 +1268,9 @@ export class Util { token.kind, token.text, token.isReserved, - token.pos, - token.end, - token.source, + token.startPosition, + token.endPosition, + token.sourceInfo, token.leadingTrivia ? token.leadingTrivia.map(x => this.cloneToken(x)) : undefined ) as Token; //handle those tokens that have charCode @@ -1364,20 +1364,20 @@ export class Util { let end: number; let source: SourceInfo; for (const locatable of locatables) { - if (!locatable?.source) { + if (!locatable?.sourceInfo) { continue; - } else if (!source || (!source.uri && locatable.source.uri)) { - source = locatable.source; - pos = locatable.pos; + } else if (!source || (!source.uri && locatable.sourceInfo.uri)) { + source = locatable.sourceInfo; + pos = locatable.startPosition; end = this.getContentEnd(locatable); - } else if (locatable.source === source) { - pos = Math.min(pos, locatable.pos); + } else if (locatable.sourceInfo === source) { + pos = Math.min(pos, locatable.startPosition); end = Math.max(end, this.getContentEnd(locatable)); } } - target.pos = pos; - target.end = end; - target.source = source; + target.startPosition = pos; + target.endPosition = end; + target.sourceInfo = source; return target; } @@ -1394,13 +1394,13 @@ export class Util { const pos = this.getOffset(sourceInfo, range?.start); const end = this.getOffset(sourceInfo, range?.end); if (pos === undefined || end === undefined || (location.uri && location.uri !== sourceInfo.uri)) { - target.source = undefined; - target.pos = undefined; - target.end = undefined; + target.sourceInfo = undefined; + target.startPosition = undefined; + target.endPosition = undefined; } else { - target.source = sourceInfo; - target.pos = pos; - target.end = end; + target.sourceInfo = sourceInfo; + target.startPosition = pos; + target.endPosition = end; } return target; } @@ -1414,22 +1414,22 @@ export class Util { const lastCharCode = text.charCodeAt(text.length - 1); // \n (and \r\n) if (lastCharCode === 10) { - return locatable.end - (text.length > 1 && text.charCodeAt(text.length - 2) === 13 ? 2 : 1); + return locatable.endPosition - (text.length > 1 && text.charCodeAt(text.length - 2) === 13 ? 2 : 1); // \r } else if (lastCharCode === 13) { - return locatable.end - 1; + return locatable.endPosition - 1; } } - return locatable.end; + return locatable.endPosition; } /** - * Get the first item that has a real location (a `source`), skipping `undefined` and synthetic items. + * Get the first item that has a real location (a `sourceInfo`), skipping `undefined` and synthetic items. * The locatable equivalent of `util.getLocation(a) ?? util.getLocation(b)` */ public firstLocatable(...items: Array): T | undefined { for (const item of items) { - if (item?.source) { + if (item?.sourceInfo) { return item; } } @@ -1440,20 +1440,20 @@ export class Util { * Build a line/character `Location` for the locatable. Returns `undefined` for synthetic items (those without a `source`) */ public getLocation(locatable: Locatable): Location | undefined { - const source = locatable?.source; + const source = locatable?.sourceInfo; if (!source) { return undefined; } - const lineStarts = source.lineStarts; - const startLine = this.getLineIndex(lineStarts, locatable.pos, 0); - let endLine = this.getLineIndex(lineStarts, locatable.end, startLine); + const lineStarts = source.linePositions; + const startLine = this.getLineIndex(lineStarts, locatable.startPosition, 0); + let endLine = this.getLineIndex(lineStarts, locatable.endPosition, startLine); //a token that ends with a newline ends on its own line rather than at the start of the next one - if (endLine > startLine && lineStarts[endLine] === locatable.end && this.getContentEnd(locatable) !== locatable.end) { + if (endLine > startLine && lineStarts[endLine] === locatable.endPosition && this.getContentEnd(locatable) !== locatable.endPosition) { endLine--; } return { uri: source.uri, - range: this.createRange(startLine, locatable.pos - lineStarts[startLine], endLine, locatable.end - lineStarts[endLine]) + range: this.createRange(startLine, locatable.startPosition - lineStarts[startLine], endLine, locatable.endPosition - lineStarts[endLine]) }; } @@ -1462,12 +1462,12 @@ export class Util { * Returns `undefined` for synthetic items (those without a `source`) */ public getStartPosition(locatable: Locatable): Position | undefined { - const lineStarts = locatable?.source?.lineStarts; + const lineStarts = locatable?.sourceInfo?.linePositions; if (!lineStarts) { return undefined; } - const line = this.getLineIndex(lineStarts, locatable.pos, 0); - return this.createPosition(line, locatable.pos - lineStarts[line]); + const line = this.getLineIndex(lineStarts, locatable.startPosition, 0); + return this.createPosition(line, locatable.startPosition - lineStarts[line]); } /** @@ -1476,7 +1476,7 @@ export class Util { * Returns `undefined` if the source or the position's line is missing */ public getOffset(source: SourceInfo | undefined, position: Position | undefined): number | undefined { - const lineStarts = source?.lineStarts; + const lineStarts = source?.linePositions; const lineStart = lineStarts?.[position?.line]; if (lineStart === undefined) { return undefined;