diff --git a/src/DiagnosticManager.spec.ts b/src/DiagnosticManager.spec.ts index 61e32cbd3..e80da8db6 100644 --- a/src/DiagnosticManager.spec.ts +++ b/src/DiagnosticManager.spec.ts @@ -3,8 +3,11 @@ 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'; +import { createToken } from './astUtils/creators'; import { DiagnosticMessages } from './DiagnosticMessages'; @@ -383,6 +386,242 @@ 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({ 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({ startPosition: subToken.startPosition, endPosition: subToken.endPosition, sourceInfo: subToken.sourceInfo }); + }); + + 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('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({ startPosition: printToken.startPosition, endPosition: printToken.endPosition, sourceInfo: printToken.sourceInfo }); + 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); + 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); + 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) + }]); + }); + + it('keeps the diagnostic source string and does not mistake it for a locatable', () => { + 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 location = util.createLocation(0, 0, 0, 3, printToken.sourceInfo.uri); + program.diagnostics.register([ + { + message: 'location', + source: 'bslint', + location: location, + relatedInformation: [{ message: 'related', location: location }] + }, + { + message: 'locatable', + source: 'bslint', + location: printToken, + relatedInformation: [{ message: 'related', location: subToken }] + } + ]); + + const diagnostics = program.getDiagnostics(); + const locationDiagnostic = diagnostics.find(x => x.message === 'location'); + expect(locationDiagnostic.source).to.eql('bslint'); + expect(locationDiagnostic.location).to.eql(location); + expect(locationDiagnostic.relatedInformation).to.eql([{ message: 'related', location: location }]); + + const locatableDiagnostic = diagnostics.find(x => x.message === 'locatable'); + expect(locatableDiagnostic.source).to.eql('bslint'); + expect(locatableDiagnostic.location).to.eql(util.getLocation(printToken)); + expect(locatableDiagnostic.relatedInformation).to.eql([{ message: 'related', location: util.getLocation(subToken) }]); + + //both should be cleared by their file + program.diagnostics.clearForFile(file.srcPath); + expectZeroDiagnostics(program.getDiagnostics()); + }); + }); + 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 e6c8671d1..2aac417bc 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 { isBlock, isBrsFile, isBsDiagnostic, isXmlScope } from './astUtils/reflection'; -import type { DiagnosticRelatedInformation, Location, Range } from 'vscode-languageserver-protocol'; +import type { Location, Position, Range } 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. @@ -72,12 +93,14 @@ 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) { this.registerDiagnostics(diagnosticArg, context, false); } @@ -86,24 +109,28 @@ export class DiagnosticManager { * Unlike other diagnostics, these are dropped while the code they describe is in an inactive `#if` / `#else if` / `#else` branch, * because the device never compiles that code. The branches follow the file's most recent validation. */ - public registerParseDiagnostics(diagnostics: BsDiagnostic[]) { + public registerParseDiagnostics(diagnostics: BsDiagnosticInput[]) { this.registerDiagnostics(diagnostics, undefined, true); } - private registerDiagnostics(diagnosticArg: BsDiagnostic | Array, context: DiagnosticContext | undefined, isParseDiagnostic: boolean) { + private registerDiagnostics(diagnosticArg: BsDiagnosticInput | Array, context: DiagnosticContext | undefined, isParseDiagnostic: boolean) { 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; @@ -112,8 +139,8 @@ export class DiagnosticManager { } else { this.parseDiagnosticKeys.delete(key); } - if (!fromCache && diagnostic.relatedInformation) { - this.mergeRelatedInformation(cachedDiagnostic.relatedInformation, diagnostic.relatedInformation); + if (fromCache) { + this.mergeRelatedInformation(cachedDiagnostic.relatedInformation, relatedInformation); } const contexts = cacheData.contexts; if (diagContext) { @@ -123,8 +150,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()); @@ -184,11 +211,29 @@ export class DiagnosticManager { const results = [] as Array; const isInInactiveBranch = this.createInactiveBranchDetector(); for (const cachedDiagnostic of this.diagnosticsCache.values()) { - if (isInInactiveBranch(cachedDiagnostic.diagnostic)) { + const storedDiagnostic = cachedDiagnostic.diagnostic; + if (isInInactiveBranch(storedDiagnostic)) { + continue; + } + 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 diagnostic = { ...cachedDiagnostic.diagnostic }; - const relatedInformation = [...cachedDiagnostic.diagnostic.relatedInformation]; + + 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) { @@ -207,25 +252,19 @@ 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; } /** @@ -234,10 +273,13 @@ export class DiagnosticManager { */ private createInactiveBranchDetector() { const inactiveRangesByUri = new Map(); - return (diagnostic: BsDiagnosticWithKey) => { - const uri = diagnostic.location?.uri; - const start = diagnostic.location?.range?.start; - if (!uri || !start || !this.parseDiagnosticKeys.has(diagnostic.key)) { + return (diagnostic: StoredDiagnostic) => { + if (!this.parseDiagnosticKeys.has(diagnostic.key)) { + return false; + } + const uri = this.getUri(diagnostic.location); + const start = this.getStart(diagnostic.location); + if (!uri || !start) { return false; } const uriLower = uri.toLowerCase(); @@ -266,17 +308,27 @@ export class DiagnosticManager { * 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.getStart(location); + 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 @@ -466,7 +518,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; @@ -484,7 +536,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) { @@ -530,25 +582,25 @@ 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.parseDiagnosticKeys.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)); @@ -560,6 +612,49 @@ export class DiagnosticManager { } } + private isDetachedLocatable(location: StoredLocation | DiagnosticLocationInput): location is DetachedLocatable { + //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)); + } + + /** + * 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 = { 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.sourceInfo ? location.endPosition - util.getContentEnd(location) : 0; + if (newlineLength > 0) { + detached.text = newlineLength === 2 ? '\r\n' : '\n'; + } + return detached; + } + + private getUri(location: StoredLocation | undefined): string | undefined { + return this.isDetachedLocatable(location) ? location.sourceInfo?.uri : location?.uri; + } + + private getStart(location: StoredLocation | undefined): Position | undefined { + return this.isDetachedLocatable(location) ? util.getStartPosition(location) : location?.range?.start; + } + + private getLocationKey(location: StoredLocation | undefined) { + if (this.isDetachedLocatable(location)) { + return `${location.sourceInfo?.uri ?? 'No uri'} @${location.startPosition}-${location.endPosition}`; + } + 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; diff --git a/src/util.spec.ts b/src/util.spec.ts index 575e7ba6a..70537791a 100644 --- a/src/util.spec.ts +++ b/src/util.spec.ts @@ -1184,6 +1184,20 @@ describe('util', () => { }); }); + describe('firstLocatable', () => { + 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 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].sourceInfo; diff --git a/src/util.ts b/src/util.ts index a1b9f9685..06b962f6b 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1423,6 +1423,19 @@ export class Util { return locatable.endPosition; } + /** + * 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?.sourceInfo) { + return item; + } + } + return undefined; + } + /** * Build a line/character `Location` for the locatable. Returns `undefined` for synthetic items (those without a `sourceInfo`) */