Skip to content

Commit

Permalink
Create object fixes (#568)
Browse files Browse the repository at this point in the history 
* Move CreateObject validation into plugin

* Only add single CreateObject diagnostics for all scopes

* Don't flag component library CreateObject calls

* Fix lint issues
  • Loading branch information
@TwitchBronBron
TwitchBronBron authored Apr 14, 2022
1 parent 39e19e2 commit f3c1dbd
Show file tree
Hide file tree
Showing 5 changed files with 195 additions and 97 deletions.
50 changes: 50 additions & 0 deletions src/Scope.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,56 @@ describe('Scope', () => {
]);
});

it('only adds a single diagnostic when the file is used in multiple scopes', () => {
program.setFile('components/Comp1.xml', trim`
<?xml version="1.0" encoding="utf-8" ?>
<component name="Comp1" extends="Scene">
<script type="text/brightscript" uri="lib.brs" />
</component>
`);
program.setFile('components/Comp2.xml', trim`
<?xml version="1.0" encoding="utf-8" ?>
<component name="Comp2" extends="Scene">
<script type="text/brightscript" uri="lib.brs" />
</component>
`);
program.setFile('components/lib.brs', `
sub init()
'unknown BrightScript component
createObject("roDateTime_FAKE")
'Wrong number of params
createObject("roDateTime", "this param should not be here")
'unknown roSGNode
createObject("roSGNode", "Rectangle_FAKE")
'deprecated
fontMetrics = CreateObject("roFontMetrics", "someFontName")
end sub
`);
program.validate();
expectDiagnostics(program, [
DiagnosticMessages.unknownBrightScriptComponent('roDateTime_FAKE'),
DiagnosticMessages.mismatchCreateObjectArgumentCount('roDateTime', [1, 1], 2),
DiagnosticMessages.unknownRoSGNode('Rectangle_FAKE'),
DiagnosticMessages.deprecatedBrightScriptComponent('roFontMetrics').code
]);
});

it('disregards component library components', () => {
program.setFile(`source/file.brs`, `
sub main()
scene = CreateObject("roSGNode", "Complib1:MainScene")
button = CreateObject("roSGNode", "buttonlib:Button")
list = CreateObject("roSGNode", "listlib:List")
end sub
`);
program.validate();
expectZeroDiagnostics(program);
});

it('disregards non-literal args', () => {
program.setFile(`source/file.brs`, `
sub main()
Expand Down
82 changes: 2 additions & 80 deletions src/Scope.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,7 @@ import { URI } from 'vscode-uri';
import { LogLevel } from './Logger';
import type { BrsFile } from './files/BrsFile';
import type { DependencyGraph, DependencyChangedEvent } from './DependencyGraph';
import { isBrsFile, isClassMethodStatement, isClassStatement, isCustomType, isEnumStatement, isFunctionStatement, isFunctionType, isLiteralExpression, isXmlFile } from './astUtils/reflection';
import { nodes, components } from './roku-types';
import type { BRSComponentData } from './roku-types';
import type { Token } from './lexer/Token';

/**
* The lower-case names of all platform-included scenegraph nodes
*/
const platformNodeNames = new Set(Object.values(nodes).map(x => x.name.toLowerCase()));
const platformComponentNames = new Set(Object.values(components).map(x => x.name.toLowerCase()));
import { isBrsFile, isClassMethodStatement, isClassStatement, isCustomType, isEnumStatement, isFunctionStatement, isFunctionType, isXmlFile } from './astUtils/reflection';

/**
* A class to keep track of all declarations within a given scope (like source scope, component scope)
Expand Down Expand Up @@ -240,7 +231,7 @@ export class Scope {
* Get the list of files referenced by this scope that are actually loaded in the program.
* Includes files from this scope and all ancestor scopes
*/
public getAllFiles() {
public getAllFiles(): BscFile[] {
return this.cache.getOrAdd('getAllFiles', () => {
let result = [] as BscFile[];
let dependencies = this.dependencyGraph.getAllDependencies(this.dependencyGraphKey);
Expand Down Expand Up @@ -503,78 +494,9 @@ export class Scope {
this.diagnosticDetectFunctionCollisions(file);
this.detectVariableNamespaceCollisions(file);
this.diagnosticDetectInvalidFunctionExpressionTypes(file);
this.validateCreateObjectCalls(file);
});
}

/**
* Validate every function call to `CreateObject`.
* Ideally we would create better type checking/handling for this, but in the mean time, we know exactly
* what these calls are supposed to look like, and this is a very common thing for brs devs to do, so just
* do this manually for now.
*/
protected validateCreateObjectCalls(file: BrsFile) {
for (const call of file.functionCalls) {
if (call.name?.toLowerCase() === 'createobject' && isLiteralExpression(call?.args[0]?.expression)) {
const firstParamToken = (call?.args[0]?.expression as any)?.token;
const firstParamStringValue = firstParamToken?.text?.replace(/"/g, '');
//if this is a `createObject('roSGNode'` call, only support known sg node types
if (firstParamStringValue?.toLowerCase() === 'rosgnode' && isLiteralExpression(call?.args[1]?.expression)) {
const componentName: Token = (call?.args[1]?.expression as any)?.token;
//add diagnostic for unknown components
const unquotedComponentName = componentName?.text?.replace(/"/g, '');
if (unquotedComponentName && !platformNodeNames.has(unquotedComponentName.toLowerCase()) && !this.program.getComponent(unquotedComponentName)) {
this.diagnostics.push({
file: file as BscFile,
...DiagnosticMessages.unknownRoSGNode(unquotedComponentName),
range: componentName.range
});
} else if (call?.args.length !== 2) {
// roSgNode should only ever have 2 args in `createObject`
this.diagnostics.push({
file: file as BscFile,
...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, [2], call?.args.length),
range: call.range
});
}
} else if (!platformComponentNames.has(firstParamStringValue.toLowerCase())) {
this.diagnostics.push({
file: file as BscFile,
...DiagnosticMessages.unknownBrightScriptComponent(firstParamStringValue),
range: firstParamToken.range
});
} else {
// This is valid brightscript component
// Test for invalid arg counts
const brightScriptComponent: BRSComponentData = components[firstParamStringValue.toLowerCase()];
// Valid arg counts for createObject are 1+ number of args for constructor
let validArgCounts = brightScriptComponent.constructors.map(cnstr => cnstr.params.length + 1);
if (validArgCounts.length === 0) {
// no constructors for this component, so createObject only takes 1 arg
validArgCounts = [1];
}
if (!validArgCounts.includes(call?.args.length)) {
// Incorrect number of arguments included in `createObject()`
this.diagnostics.push({
file: file as BscFile,
...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, validArgCounts, call?.args.length),
range: call.range
});
}

// Test for deprecation
if (brightScriptComponent.isDeprecated) {
this.diagnostics.push({
file: file as BscFile,
...DiagnosticMessages.deprecatedBrightScriptComponent(firstParamStringValue, brightScriptComponent.deprecatedDescription),
range: call.range
});
}
}
}
}
}

/**
* Mark this scope as invalid, which means its `validate()` function needs to be called again before use.
*/
Expand Down
10 changes: 9 additions & 1 deletion src/bscPlugin/BscPlugin.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { isBrsFile } from '../astUtils/reflection';
import type { BrsFile } from '../files/BrsFile';
import type { BeforeFileTranspileEvent, CompilerPlugin, OnFileValidateEvent, OnGetCodeActionsEvent, OnGetSemanticTokensEvent, OnScopeValidateEvent } from '../interfaces';
import type { Program } from '../Program';
import { CodeActionsProcessor } from './codeActions/CodeActionsProcessor';
import { BrsFileSemanticTokensProcessor } from './semanticTokens/BrsFileSemanticTokensProcessor';
import { BrsFilePreTranspileProcessor } from './transpile/BrsFilePreTranspileProcessor';
Expand All @@ -26,8 +27,15 @@ export class BscPlugin implements CompilerPlugin {
}
}

private scopeValidator = new ScopeValidator();

public onScopeValidate(event: OnScopeValidateEvent) {
return new ScopeValidator(event).process();
this.scopeValidator.processEvent(event);
}

public afterProgramValidate(program: Program) {
//release memory once the validation cycle has finished
this.scopeValidator.reset();
}

public beforeFileTranspile(event: BeforeFileTranspileEvent) {
Expand Down
143 changes: 127 additions & 16 deletions src/bscPlugin/validation/ScopeValidator.ts
View file
Original file line number Diff line number Diff line change
@@ -1,42 +1,74 @@
import { Location } from 'vscode-languageserver';
import { URI } from 'vscode-uri';
import { isBrsFile } from '../../astUtils/reflection';
import { isBrsFile, isLiteralExpression } from '../../astUtils/reflection';
import { Cache } from '../../Cache';
import { DiagnosticMessages } from '../../DiagnosticMessages';
import type { BrsFile } from '../../files/BrsFile';
import type { BsDiagnostic, OnScopeValidateEvent } from '../../interfaces';
import type { BscFile, BsDiagnostic, OnScopeValidateEvent } from '../../interfaces';
import type { DottedGetExpression } from '../../parser/Expression';
import type { EnumStatement } from '../../parser/Statement';
import util from '../../util';
import { nodes, components } from '../../roku-types';
import type { BRSComponentData } from '../../roku-types';
import type { Token } from '../../lexer/Token';

/**
* The lower-case names of all platform-included scenegraph nodes
*/
const platformNodeNames = new Set(Object.values(nodes).map(x => x.name.toLowerCase()));
const platformComponentNames = new Set(Object.values(components).map(x => x.name.toLowerCase()));

/**
* A validator that handles all scope validations for a program validation cycle.
* You should create ONE of these to handle all scope events between beforeProgramValidate and afterProgramValidate,
* and call reset() before using it again in the next cycle
*/
export class ScopeValidator {
constructor(
private event: OnScopeValidateEvent
) {

private events: OnScopeValidateEvent[] = [];

public processEvent(event: OnScopeValidateEvent) {
this.events.push(event);
this.validateEnumUsage(event);
this.detectDuplicateEnums(event);
this.validateCreateObjectCalls(event);
}

public reset() {
this.cache.clear();
this.events = [];
}

public process() {
this.validateEnumUsage();
this.detectDuplicateEnums();
/**
* Adds a diagnostic to the first scope for this key. Prevents duplicate diagnostics
* for diagnostics where scope isn't important. (i.e. CreateObject validations)
*/
private addDiagnosticOnce(event: OnScopeValidateEvent, diagnostic: BsDiagnostic) {
const key = `${diagnostic.code}-${diagnostic.message}-${util.rangeToString(diagnostic.range)}`;
if (!this.cache.has(key)) {
this.cache.set(key, true);
event.scope.addDiagnostics([diagnostic]);
}
}

private cache = new Map<string, boolean>();

/**
* Find all expressions and validate the ones that look like enums
*/
public validateEnumUsage() {
public validateEnumUsage(event: OnScopeValidateEvent) {
const diagnostics = [] as BsDiagnostic[];

const membersByEnum = new Cache<string, Map<string, string>>();
//if there are any enums defined in this scope
const enumLookup = this.event.scope.getEnumMap();
const enumLookup = event.scope.getEnumMap();

//skip enum validation if there are no enums defined in this scope
if (enumLookup.size === 0) {
return;
}

this.event.scope.enumerateOwnFiles((file) => {
event.scope.enumerateOwnFiles((file) => {
//skip non-brs files
if (!isBrsFile(file)) {
return;
Expand Down Expand Up @@ -74,13 +106,13 @@ export class ScopeValidator {
}
}
});
this.event.scope.addDiagnostics(diagnostics);
event.scope.addDiagnostics(diagnostics);
}

private detectDuplicateEnums() {
private detectDuplicateEnums(event: OnScopeValidateEvent) {
const diagnostics: BsDiagnostic[] = [];
const enumLocationsByName = new Cache<string, Array<{ file: BrsFile; statement: EnumStatement }>>();
this.event.scope.enumerateBrsFiles((file) => {
event.scope.enumerateBrsFiles((file) => {
for (const enumStatement of file.parser.references.enumStatements) {
const fullName = enumStatement.fullName;
const nameLower = fullName?.toLowerCase();
Expand All @@ -101,7 +133,7 @@ export class ScopeValidator {
const fullName = primaryEnum.statement.fullName;
for (const duplicateEnumInfo of enumLocations) {
diagnostics.push({
...DiagnosticMessages.duplicateEnumDeclaration(this.event.scope.name, fullName),
...DiagnosticMessages.duplicateEnumDeclaration(event.scope.name, fullName),
file: duplicateEnumInfo.file,
range: duplicateEnumInfo.statement.tokens.name.range,
relatedInformation: [{
Expand All @@ -114,6 +146,85 @@ export class ScopeValidator {
});
}
}
this.event.scope.addDiagnostics(diagnostics);
event.scope.addDiagnostics(diagnostics);
}

/**
* Validate every function call to `CreateObject`.
* Ideally we would create better type checking/handling for this, but in the mean time, we know exactly
* what these calls are supposed to look like, and this is a very common thing for brs devs to do, so just
* do this manually for now.
*/
protected validateCreateObjectCalls(event: OnScopeValidateEvent) {
const diagnostics: BsDiagnostic[] = [];

event.scope.enumerateBrsFiles((file) => {
for (const call of file.functionCalls) {
//skip non CreateObject function calls
if (call.name?.toLowerCase() !== 'createobject' || !isLiteralExpression(call?.args[0]?.expression)) {
continue;
}
const firstParamToken = (call?.args[0]?.expression as any)?.token;
const firstParamStringValue = firstParamToken?.text?.replace(/"/g, '');
//if this is a `createObject('roSGNode'` call, only support known sg node types
if (firstParamStringValue?.toLowerCase() === 'rosgnode' && isLiteralExpression(call?.args[1]?.expression)) {
const componentName: Token = (call?.args[1]?.expression as any)?.token;
//don't validate any components with a colon in their name (probably component libraries, but regular components can have them too).
if (componentName?.text?.includes(':')) {
continue;
}
//add diagnostic for unknown components
const unquotedComponentName = componentName?.text?.replace(/"/g, '');
if (unquotedComponentName && !platformNodeNames.has(unquotedComponentName.toLowerCase()) && !event.program.getComponent(unquotedComponentName)) {
this.addDiagnosticOnce(event, {
file: file as BscFile,
...DiagnosticMessages.unknownRoSGNode(unquotedComponentName),
range: componentName.range
});
} else if (call?.args.length !== 2) {
// roSgNode should only ever have 2 args in `createObject`
this.addDiagnosticOnce(event, {
file: file as BscFile,
...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, [2], call?.args.length),
range: call.range
});
}
} else if (!platformComponentNames.has(firstParamStringValue.toLowerCase())) {
this.addDiagnosticOnce(event, {
file: file as BscFile,
...DiagnosticMessages.unknownBrightScriptComponent(firstParamStringValue),
range: firstParamToken.range
});
} else {
// This is valid brightscript component
// Test for invalid arg counts
const brightScriptComponent: BRSComponentData = components[firstParamStringValue.toLowerCase()];
// Valid arg counts for createObject are 1+ number of args for constructor
let validArgCounts = brightScriptComponent.constructors.map(cnstr => cnstr.params.length + 1);
if (validArgCounts.length === 0) {
// no constructors for this component, so createObject only takes 1 arg
validArgCounts = [1];
}
if (!validArgCounts.includes(call?.args.length)) {
// Incorrect number of arguments included in `createObject()`
this.addDiagnosticOnce(event, {
file: file as BscFile,
...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, validArgCounts, call?.args.length),
range: call.range
});
}

// Test for deprecation
if (brightScriptComponent.isDeprecated) {
this.addDiagnosticOnce(event, {
file: file as BscFile,
...DiagnosticMessages.deprecatedBrightScriptComponent(firstParamStringValue, brightScriptComponent.deprecatedDescription),
range: call.range
});
}
}
}
});
event.scope.addDiagnostics(diagnostics);
}
}
Loading

0 comments on commit f3c1dbd

Please sign in to comment.