Skip to content

Commit

Permalink
Adds diagnostics for bad types in doc comment tags
Browse files Browse the repository at this point in the history
  • Loading branch information
markwpearce committed Sep 6, 2024
1 parent 3d66f6a commit b2bbc79
Show file tree
Hide file tree
Showing 7 changed files with 157 additions and 52 deletions.
8 changes: 8 additions & 0 deletions src/DiagnosticMessages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
12 changes: 9 additions & 3 deletions src/bscPlugin/validation/BrsFileValidator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 });
Expand Down
42 changes: 42 additions & 0 deletions src/bscPlugin/validation/ScopeValidator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2972,6 +2972,48 @@ describe('ScopeValidator', () => {

});

describe('cannotFindTypeInDocComment', () => {
it('validates types it cannot find in @param', () => {
program.setFile<BrsFile>('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<BrsFile>('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<BrsFile>('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', () => {
Expand Down
28 changes: 28 additions & 0 deletions src/bscPlugin/validation/ScopeValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
*/
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
53 changes: 34 additions & 19 deletions src/parser/BrightScriptDocParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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*(.*)/;
Expand All @@ -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
Expand All @@ -56,26 +71,26 @@ 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);
}
}
}
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) {
Expand All @@ -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
};
}

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -276,6 +290,7 @@ export class BrightScriptDoc {
export interface BrsDocTag {
tagName: string;
detail?: string;
location?: Location;
}
export interface BrsDocWithType extends BrsDocTag {
type?: string;
Expand Down
65 changes: 35 additions & 30 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / create-package

Missing @param "options.prettyPrint"

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / create-package

Missing @param "options.matchingLocations"

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / ci (macos-latest)

Missing @param "options.prettyPrint"

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / ci (macos-latest)

Missing @param "options.matchingLocations"

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest)

Missing @param "options.prettyPrint"

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest)

Missing @param "options.matchingLocations"

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / ci (windows-latest)

Missing @param "options.prettyPrint"

Check warning on line 647 in src/util.ts

View workflow job for this annotation

GitHub Actions / ci (windows-latest)

Missing @param "options.matchingLocations"
* @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]
Expand Down Expand Up @@ -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');
}

/**
Expand Down

0 comments on commit b2bbc79

Please sign in to comment.