From b2bbc792d9f7e5acf4ac84f678d5ad7da53ab173 Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Fri, 6 Sep 2024 10:54:28 -0300 Subject: [PATCH] Adds diagnostics for bad types in doc comment tags --- src/DiagnosticMessages.ts | 8 +++ .../validation/BrsFileValidator.spec.ts | 12 +++- .../validation/ScopeValidator.spec.ts | 42 ++++++++++++ src/bscPlugin/validation/ScopeValidator.ts | 28 ++++++++ src/index.ts | 1 + src/parser/BrightScriptDocParser.ts | 53 +++++++++------ src/util.ts | 65 ++++++++++--------- 7 files changed, 157 insertions(+), 52 deletions(-) diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts index 5d481f59e..43d089dd2 100644 --- a/src/DiagnosticMessages.ts +++ b/src/DiagnosticMessages.ts @@ -809,6 +809,14 @@ export let DiagnosticMessages = { message: `Unsafe unmatched terminator '${terminator}' in conditional compilation block`, code: 1153, severity: DiagnosticSeverity.Error + }), + cannotFindTypeInCommentDoc: (name: string) => ({ + message: `Cannot find type '${name}' in doc comment`, + code: 1154, + data: { + name: name + }, + severity: DiagnosticSeverity.Error }) }; export const defaultMaximumTruncationLength = 160; diff --git a/src/bscPlugin/validation/BrsFileValidator.spec.ts b/src/bscPlugin/validation/BrsFileValidator.spec.ts index 09d7a04be..4a5dc5f96 100644 --- a/src/bscPlugin/validation/BrsFileValidator.spec.ts +++ b/src/bscPlugin/validation/BrsFileValidator.spec.ts @@ -993,7 +993,9 @@ describe('BrsFileValidator', () => { end function `); program.validate(); - expectZeroDiagnostics(program); + expectDiagnostics(program, [ + DiagnosticMessages.cannotFindTypeInCommentDoc('TypeNotThere').message + ]); const data = {} as ExtraSymbolData; expectTypeToBe( file.ast.findChild(isFunctionParameterExpression).getType({ @@ -1035,7 +1037,9 @@ describe('BrsFileValidator', () => { end function `); program.validate(); - expectZeroDiagnostics(program); + expectDiagnostics(program, [ + DiagnosticMessages.cannotFindTypeInCommentDoc('TypeNotThere').message + ]); const data = {} as ExtraSymbolData; const funcStmt = file.ast.findChild(isFunctionStatement); const funcType = funcStmt.getType({ flags: SymbolTypeFlag.runtime, data: data }); @@ -1140,7 +1144,9 @@ describe('BrsFileValidator', () => { end function `); program.validate(); - expectZeroDiagnostics(program); + expectDiagnostics(program, [ + DiagnosticMessages.cannotFindTypeInCommentDoc('unknown').message + ]); const data = {} as ExtraSymbolData; const funcStmt = file.ast.findChild(isFunctionStatement); const funcType = funcStmt.getType({ flags: SymbolTypeFlag.runtime, data: data }); diff --git a/src/bscPlugin/validation/ScopeValidator.spec.ts b/src/bscPlugin/validation/ScopeValidator.spec.ts index 5c5126e7b..3609c3d32 100644 --- a/src/bscPlugin/validation/ScopeValidator.spec.ts +++ b/src/bscPlugin/validation/ScopeValidator.spec.ts @@ -2972,6 +2972,48 @@ describe('ScopeValidator', () => { }); + describe('cannotFindTypeInDocComment', () => { + it('validates types it cannot find in @param', () => { + program.setFile('source/main.brs', ` + ' @param {TypeNotThere} info + function sayHello(info) + print "Hello " + info.prop + end function + `); + program.validate(); + expectDiagnostics(program, [ + DiagnosticMessages.cannotFindTypeInCommentDoc('TypeNotThere').message + ]); + }); + + it('validates types it cannot find in @return', () => { + program.setFile('source/main.brs', ` + ' @return {TypeNotThere} info + function sayHello(info) + return {data: info.prop} + end function + `); + program.validate(); + expectDiagnostics(program, [ + DiagnosticMessages.cannotFindTypeInCommentDoc('TypeNotThere').message + ]); + }); + + it('validates types it cannot find in @type', () => { + program.setFile('source/main.brs', ` + function sayHello(info) + ' @type {TypeNotThere} + value = info.prop + end function + `); + program.validate(); + expectDiagnostics(program, [ + DiagnosticMessages.cannotFindTypeInCommentDoc('TypeNotThere').message + ]); + }); + + }); + describe('revalidation', () => { it('revalidates when a enum defined in a different namespace changes', () => { diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts index 0feecda4b..9a4ed0bb4 100644 --- a/src/bscPlugin/validation/ScopeValidator.ts +++ b/src/bscPlugin/validation/ScopeValidator.ts @@ -28,6 +28,8 @@ import type { XmlFile } from '../../files/XmlFile'; import { SGFieldTypes } from '../../parser/SGTypes'; import { DynamicType } from '../../types'; import { BscTypeKind } from '../../types/BscTypeKind'; +import type { BrsDocWithType } from '../../parser/BrightScriptDocParser'; +import brsDocParser from '../../parser/BrightScriptDocParser'; /** * The lower-case names of all platform-included scenegraph nodes @@ -179,6 +181,15 @@ export class ScopeValidator { type: this.getNodeTypeWrapper(file, funcParam, { flags: SymbolTypeFlag.runtime }), nameRange: funcParam.tokens.name.location?.range }); + }, + AstNode: (node) => { + //check for doc comments + if (!node.leadingTrivia || node.leadingTrivia.filter(triviaToken => triviaToken.kind === TokenKind.Comment).length === 0) { + return; + } + + this.validateDocComments(node); + } }); // validate only what's needed in the file @@ -1157,6 +1168,23 @@ export class ScopeValidator { } } + private validateDocComments(node: AstNode) { + const doc = brsDocParser.parseNode(node); + for (const docTag of doc.tags) { + const docTypeTag = docTag as BrsDocWithType; + if (!docTypeTag.type || !docTypeTag.location) { + continue; + } + const foundType = doc.getTypeFromContext(docTypeTag.type, node, { flags: SymbolTypeFlag.typetime }); + if (!foundType?.isResolvable()) { + this.addMultiScopeDiagnostic({ + ...DiagnosticMessages.cannotFindTypeInCommentDoc(docTypeTag.type), + location: docTypeTag.location + }); + } + } + } + /** * Detect when a child has imported a script that an ancestor also imported */ diff --git a/src/index.ts b/src/index.ts index 5202ac6c4..d1664134f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -15,6 +15,7 @@ export * from './parser/Parser'; export * from './parser/AstNode'; export * from './parser/Expression'; export * from './parser/Statement'; +export * from './parser/BrightScriptDocParser'; export * from './BsConfig'; export * from './deferred'; // convenience re-export from vscode diff --git a/src/parser/BrightScriptDocParser.ts b/src/parser/BrightScriptDocParser.ts index 27242f60c..2c2de1cb0 100644 --- a/src/parser/BrightScriptDocParser.ts +++ b/src/parser/BrightScriptDocParser.ts @@ -2,7 +2,7 @@ import type { GetSymbolTypeOptions } from '../SymbolTable'; import { SymbolTypeFlag } from '../SymbolTypeFlag'; import util from '../util'; import type { AstNode } from './AstNode'; - +import type { Location } from 'vscode-languageserver'; const tagRegex = /@(\w+)(?:\s+(.*))?/; const paramRegex = /(?:{([^}]*)}\s+)?(?:(\[?\w+\]?))\s*(.*)/; @@ -21,33 +21,48 @@ export enum BrsDocTagKind { export class BrightScriptDocParser { public parseNode(node: AstNode) { - return this.parse(util.getNodeDocumentation(node, false)); + const matchingLocations: Location[] = []; + return this.parse( + util.getNodeDocumentation(node, { + prettyPrint: false, + matchingLocations: matchingLocations + }), + matchingLocations); } - public parse(documentation: string) { + public parse(documentation: string, matchingLocations: Location[] = []) { const brsDoc = new BrightScriptDoc(documentation); if (!documentation) { return brsDoc; } const lines = documentation.split('\n'); - const blockLines = [] as string[]; - const descriptionLines = [] as string[]; + const blockLines = [] as { line: string; location?: Location }[]; + const descriptionLines = [] as { line: string; location?: Location }[]; let lastTag: BrsDocTag; + let haveMatchingLocations = false; + if (lines.length === matchingLocations.length) { + // We locations for each line, so we can add Locations + haveMatchingLocations = true; + } function augmentLastTagWithBlockLines() { if (blockLines.length > 0 && lastTag) { // add to the description or details to previous tag if (typeof (lastTag as BrsDocWithDescription).description !== 'undefined') { - (lastTag as BrsDocWithDescription).description += '\n' + blockLines.join('\n'); + (lastTag as BrsDocWithDescription).description += '\n' + blockLines.map(obj => obj.line).join('\n'); (lastTag as BrsDocWithDescription).description = (lastTag as BrsDocWithDescription).description.trim(); } if (typeof lastTag.detail !== 'undefined') { - lastTag.detail += '\n' + blockLines.join('\n'); + lastTag.detail += '\n' + blockLines.map(obj => obj.line).join('\n'); lastTag.detail = lastTag.detail.trim(); } + if (haveMatchingLocations) { + lastTag.location = util.createBoundingLocation(lastTag.location, blockLines[blockLines.length - 1].location); + } } blockLines.length = 0; } for (let line of lines) { + let location = haveMatchingLocations ? matchingLocations.shift() : undefined; line = line.trim(); while (line.startsWith('\'')) { // remove leading apostrophes @@ -56,14 +71,14 @@ export class BrightScriptDocParser { if (!line.startsWith('@')) { if (lastTag) { - blockLines.push(line); + blockLines.push({ line: line, location: location }); } else if (descriptionLines.length > 0 || line) { // add a line to the list if it's not empty - descriptionLines.push(line); + descriptionLines.push({ line: line, location: location }); } } else { augmentLastTagWithBlockLines(); - const newTag = this.parseLine(line); + const newTag = this.parseLine(line, location); lastTag = newTag; if (newTag) { brsDoc.tags.push(newTag); @@ -71,11 +86,11 @@ export class BrightScriptDocParser { } } augmentLastTagWithBlockLines(); - brsDoc.description = descriptionLines.join('\n').trim(); + brsDoc.description = descriptionLines.map(obj => obj.line).join('\n').trim(); return brsDoc; } - private parseLine(line: string) { + private parseLine(line: string, location?: Location) { line = line.trim(); const match = tagRegex.exec(line); if (!match) { @@ -86,18 +101,19 @@ export class BrightScriptDocParser { switch (tagName) { case BrsDocTagKind.Param: - return this.parseParam(detail); + return { ...this.parseParam(detail), location: location }; case BrsDocTagKind.Return: case 'returns': - return this.parseReturn(detail); + return { ...this.parseReturn(detail), location: location }; case BrsDocTagKind.Type: - return this.parseType(detail); + return { ...this.parseType(detail), location: location }; case BrsDocTagKind.Var: - return { ...this.parseParam(detail), tagName: BrsDocTagKind.Var }; + return { ...this.parseParam(detail), tagName: BrsDocTagKind.Var, location: location }; } return { tagName: tagName, - detail: detail + detail: detail, + location: location }; } @@ -229,13 +245,11 @@ export class BrightScriptDoc { getParamBscType(name: string, nodeContext: AstNode, options: GetSymbolTypeOptions) { const param = this.getParam(name); - return this.getTypeFromContext(param?.type, nodeContext, options); } getVarBscType(name: string, nodeContext: AstNode, options: GetSymbolTypeOptions) { const param = this.getVar(name); - return this.getTypeFromContext(param?.type, nodeContext, options); } @@ -276,6 +290,7 @@ export class BrightScriptDoc { export interface BrsDocTag { tagName: string; detail?: string; + location?: Location; } export interface BrsDocWithType extends BrsDocTag { type?: string; diff --git a/src/util.ts b/src/util.ts index cc1d4f683..2ef00a5a2 100644 --- a/src/util.ts +++ b/src/util.ts @@ -645,12 +645,14 @@ export class Util { /** * Combine all the documentation for a node - uses the AstNode's leadingTrivia property * @param node the node to get the documentation for - * @param prettyPrint if true, will format the comment text for markdown + * @param options prettyPrint- if true, will format the comment text for markdown, matchingLocations: out Array of locations that match the comment lines */ - public getNodeDocumentation(node: AstNode, prettyPrint = true) { + public getNodeDocumentation(node: AstNode, options: { prettyPrint?: boolean; matchingLocations?: Location[] } = { prettyPrint: true }) { if (!node) { return ''; } + options = options ?? { prettyPrint: true }; + options.matchingLocations = options.matchingLocations ?? []; const nodeTrivia = node.leadingTrivia ?? []; const leadingTrivia = isStatement(node) ? [...(node.annotations?.map(anno => anno.leadingTrivia ?? []).flat() ?? []), ...nodeTrivia] @@ -680,36 +682,39 @@ export class Util { } const jsDocCommentBlockLine = /(\/\*{2,}|\*{1,}\/)/i; let usesjsDocCommentBlock = false; - if (comments.length > 0) { - return comments.reverse() - .map(x => x.text.replace(/^('|rem)/i, '').trim()) - .filter(line => { - if (jsDocCommentBlockLine.exec(line)) { - usesjsDocCommentBlock = true; - return false; - } - return true; - }).map(line => { - if (usesjsDocCommentBlock) { - if (line.startsWith('*')) { - //remove jsDoc leading '*' - line = line.slice(1).trim(); - } + if (comments.length === 0) { + return ''; + } + return comments.reverse() + .map(x => ({ line: x.text.replace(/^('|rem)/i, '').trim(), location: x.location })) + .filter(({ line }) => { + if (jsDocCommentBlockLine.exec(line)) { + usesjsDocCommentBlock = true; + return false; + } + return true; + }).map(({ line, location }) => { + if (usesjsDocCommentBlock) { + if (line.startsWith('*')) { + //remove jsDoc leading '*' + line = line.slice(1).trim(); } - if (prettyPrint && line.startsWith('@')) { - // Handle jsdoc/brightscriptdoc tags specially - // make sure they are on their own markdown line, and add italics - const firstSpaceIndex = line.indexOf(' '); - if (firstSpaceIndex === -1) { - return `\n_${line}_`; - } - const firstWord = line.substring(0, firstSpaceIndex); - return `\n_${firstWord}_ ${line.substring(firstSpaceIndex + 1)}`; + } + if (options.prettyPrint && line.startsWith('@')) { + // Handle jsdoc/brightscriptdoc tags specially + // make sure they are on their own markdown line, and add italics + const firstSpaceIndex = line.indexOf(' '); + if (firstSpaceIndex === -1) { + return `\n_${line}_`; } - return line; - }).join('\n'); - } - return ''; + const firstWord = line.substring(0, firstSpaceIndex); + return `\n_${firstWord}_ ${line.substring(firstSpaceIndex + 1)}`; + } + if (options.matchingLocations) { + options.matchingLocations.push(location); + } + return line; + }).join('\n'); } /**