Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create object fixes #568

Merged
merged 4 commits into from
Apr 14, 2022
+195 −97
Merged

Create object fixes #568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e733c2d
Move CreateObject validation into plugin
TwitchBronBron Apr 14, 2022
6b9e89e
Only add single CreateObject diagnostics for all scopes
TwitchBronBron Apr 14, 2022
a09fe6d
Don't flag component library CreateObject calls
TwitchBronBron Apr 14, 2022
d7f190a
Fix lint issues
TwitchBronBron Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions src/Scope.spec.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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