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.

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

Move parse and validate events out to program level #494

Merged
merged 2 commits into from
Jan 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 36 additions & 0 deletions src/Program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,10 +395,27 @@ export class Program {
this.logger.time(LogLevel.debug, ['parse', chalk.green(srcPath)], () => {
brsFile.parse(sourceObj.source);
});

//notify plugins that this file has finished parsing
this.plugins.emit('afterFileParse', brsFile);

file = brsFile;

brsFile.attachDependencyGraph(this.dependencyGraph);

this.plugins.emit('beforeFileValidate', {
program: this,
file: file
});

file.validate();

//emit an event to allow plugins to contribute to the file validation process
this.plugins.emit('onFileValidate', {
program: this,
file: file
});

this.plugins.emit('afterFileValidate', brsFile);
} else if (
//is xml file
Expand All @@ -415,10 +432,14 @@ export class Program {
source: fileContents
};
this.plugins.emit('beforeFileParse', sourceObj);

this.logger.time(LogLevel.debug, ['parse', chalk.green(srcPath)], () => {
xmlFile.parse(sourceObj.source);
});

//notify plugins that this file has finished parsing
this.plugins.emit('afterFileParse', xmlFile);

file = xmlFile;

//create a new scope for this xml file
Expand All @@ -428,6 +449,21 @@ export class Program {
//register this compoent now that we have parsed it and know its component name
this.registerComponent(xmlFile, scope);


//emit an event before starting to validate this file
this.plugins.emit('beforeFileValidate', {
file: file,
program: this
});

xmlFile.validate();

//emit an event to allow plugins to contribute to the file validation process
this.plugins.emit('onFileValidate', {
file: xmlFile,
program: this
});

this.plugins.emit('afterFileValidate', xmlFile);
} else {
//TODO do we actually need to implement this? Figure out how to handle img paths
Expand Down
20 changes: 9 additions & 11 deletions src/astUtils/visitors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CancellationTokenSource, Range } from 'vscode-languageserver';
import { expect } from 'chai';
import * as sinon from 'sinon';
import { Program } from '../Program';
import { BrsFile } from '../files/BrsFile';
import type { BrsFile } from '../files/BrsFile';
import type { Statement } from '../parser/Statement';
import { PrintStatement, Block, ReturnStatement } from '../parser/Statement';
import type { Expression } from '../parser/Expression';
Expand All @@ -17,7 +17,6 @@ import { createStackedVisitor } from './stackedVisitor';
describe('astUtils visitors', () => {
const rootDir = process.cwd();
let program: Program;
let file: BrsFile;

const PRINTS_SRC = `
sub Main()
Expand Down Expand Up @@ -76,7 +75,6 @@ describe('astUtils visitors', () => {

beforeEach(() => {
program = new Program({ rootDir: rootDir });
file = new BrsFile('abs.bs', 'rel.bs', program);
});
afterEach(() => {
program.dispose();
Expand All @@ -102,9 +100,9 @@ describe('astUtils visitors', () => {
const walker = functionsWalker(visitor);
program.plugins.add({
name: 'walker',
afterFileParse: () => walker(file)
afterFileParse: file => walker(file as BrsFile)
});
file.parse(PRINTS_SRC);
program.addOrReplaceFile('source/main.brs', PRINTS_SRC);
expect(actual).to.deep.equal([
'Block:0', // Main sub body
'PrintStatement:1', // print 1
Expand Down Expand Up @@ -139,9 +137,9 @@ describe('astUtils visitors', () => {
const walker = functionsWalker(s => actual.push(s.constructor.name), cancel.token);
program.plugins.add({
name: 'walker',
afterFileParse: () => walker(file)
afterFileParse: file => walker(file as BrsFile)
});
file.parse(PRINTS_SRC);
program.addOrReplaceFile('source/main.brs', PRINTS_SRC);
expect(actual).to.deep.equal([
'Block', // Main sub body
'PrintStatement', // print 1
Expand Down Expand Up @@ -184,9 +182,9 @@ describe('astUtils visitors', () => {
}, cancel.token);
program.plugins.add({
name: 'walker',
afterFileParse: () => walker(file)
afterFileParse: file => walker(file as BrsFile)
});
file.parse(PRINTS_SRC);
program.addOrReplaceFile('source/main.brs', PRINTS_SRC);
expect(actual).to.deep.equal([
'Block', // Main sub body
'PrintStatement', // print 1
Expand Down Expand Up @@ -263,10 +261,10 @@ describe('astUtils visitors', () => {
});
program.plugins.add({
name: 'walker',
afterFileParse: () => walker(file)
afterFileParse: (file) => walker(file as BrsFile)
});

file.parse(EXPRESSIONS_SRC);
program.addOrReplaceFile('source/main.brs', EXPRESSIONS_SRC);
expect(actual).to.deep.equal([
//The comment statement is weird because it can't be both a statement and expression, but is treated that way. Just ignore it for now until we refactor comments.
//'CommentStatement:1:CommentStatement', // '<comment>
Expand Down
5 changes: 2 additions & 3 deletions src/files/BrsFile.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2293,13 +2293,12 @@ describe('BrsFile', () => {
name: 'transform callback',
afterFileParse: onParsed
});
const file = new BrsFile(`absolute_path/file${ext}`, `relative_path/file${ext}`, program);
expect(file.extension).to.equal(ext);
file.parse(`
file = program.addOrReplaceFile({ src: `absolute_path/file${ext}`, dest: `relative_path/file${ext}` }, `
sub Sum()
print "hello world"
end sub
`);
expect(file.extension).to.equal(ext);
return file;
}

Expand Down
52 changes: 24 additions & 28 deletions src/files/BrsFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,28 +268,14 @@ export class BrsFile {
...this._parser.diagnostics as BsDiagnostic[]
);

//notify AST ready
this.program.plugins.emit('afterFileParse', this);

//extract all callables from this file
this.findCallables();

//find all places where a sub/function is being called
this.findFunctionCalls();

//emit an event before starting to validate this file
this.program.plugins.emit('beforeFileValidate', {
file: this,
program: this.program
});

//emit an event to allow plugins to contribute to the file validation process
this.program.plugins.emit('onFileValidate', {
file: this,
program: this.program
});

this.findAndValidateImportAndImportStatements();
//register all import statements for use in the rest of the program
this.registerImports();

//attach this file to every diagnostic
for (let diagnostic of this.diagnostics) {
Expand All @@ -305,9 +291,29 @@ export class BrsFile {
}
}

public findAndValidateImportAndImportStatements() {
let topOfFileIncludeStatements = [] as Array<LibraryStatement | ImportStatement>;
private registerImports() {
for (const statement of this.parser?.references?.importStatements ?? []) {
//register import statements
if (isImportStatement(statement) && statement.filePathToken) {
this.ownScriptImports.push({
filePathRange: statement.filePathToken.range,
pkgPath: util.getPkgPathFromTarget(this.pkgPath, statement.filePath),
sourceFile: this,
text: statement.filePathToken?.text
});
}
}
}

public validate() {
//only validate the file if it was actually parsed (skip files containing typedefs)
if (!this.hasTypedef) {
this.validateImportStatements();
}
}

private validateImportStatements() {
let topOfFileIncludeStatements = [] as Array<LibraryStatement | ImportStatement>;
for (let stmt of this.ast.statements) {
//skip comments
if (isCommentStatement(stmt)) {
Expand All @@ -327,16 +333,6 @@ export class BrsFile {
...this._parser.references.importStatements
];
for (let result of statements) {
//register import statements
if (isImportStatement(result) && result.filePathToken) {
this.ownScriptImports.push({
filePathRange: result.filePathToken.range,
pkgPath: util.getPkgPathFromTarget(this.pkgPath, result.filePath),
sourceFile: this,
text: result.filePathToken?.text
});
}

//if this statement is not one of the top-of-file statements,
//then add a diagnostic explaining that it is invalid
if (!topOfFileIncludeStatements.includes(result)) {
Expand Down
47 changes: 24 additions & 23 deletions src/files/XmlFile.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { standardizePath as s } from '../util';
import { expectDiagnostics, expectZeroDiagnostics, getTestTranspile, trim, trimMap } from '../testHelpers.spec';
import { ProgramBuilder } from '../ProgramBuilder';
import { LogLevel } from '../Logger';
import { isXmlFile } from '../astUtils/reflection';

describe('XmlFile', () => {
const tempDir = s`${process.cwd()}/.tmp`;
Expand Down Expand Up @@ -40,17 +41,17 @@ describe('XmlFile', () => {
describe('parse', () => {
it('allows modifying the parsed XML model', () => {
const expected = 'OtherName';
file = new XmlFile('abs', 'rel', program);
program.plugins.add({
name: 'allows modifying the parsed XML model',
afterFileParse: () => {
file.parser.ast.root.attributes[0].value.text = expected;
afterFileParse: (file) => {
if (isXmlFile(file) && file.parser.ast.root?.attributes?.[0]?.value) {
file.parser.ast.root.attributes[0].value.text = expected;
}
}
});
file.parse(trim`
file = program.addOrReplaceFile('components/ChildScene.xml', trim`
<?xml version="1.0" encoding="utf-8" ?>
<component name="ChildScene" extends="Scene">
<script type="text/brightscript" uri="ChildScene1.brs" /> <script type="text/brightscript" uri="ChildScene2.brs" /> <script type="text/brightscript" uri="ChildScene3.brs" />
</component>
`);
expect(file.componentName.text).to.equal(expected);
Expand Down Expand Up @@ -191,26 +192,24 @@ describe('XmlFile', () => {
});

it('Adds error when no component is declared in xml', () => {
file = new XmlFile('abs', 'rel', program);
file.parse('<script type="text/brightscript" uri="ChildScene.brs" />');
expect(file.diagnostics).to.be.lengthOf(2);
expect(file.diagnostics[0]).to.deep.include({
...DiagnosticMessages.xmlUnexpectedTag('script'),
range: Range.create(0, 1, 0, 7)
});
expect(file.diagnostics[1]).to.deep.include(
file = program.addOrReplaceFile('components/comp.xml', '<script type="text/brightscript" uri="ChildScene.brs" />');
expectDiagnostics(program, [
{
...DiagnosticMessages.xmlUnexpectedTag('script'),
range: Range.create(0, 1, 0, 7)
},
DiagnosticMessages.xmlComponentMissingComponentDeclaration()
);
]);
});

it('adds error when component does not declare a name', () => {
file = new XmlFile('abs', 'rel', program);
file.parse(trim`
file = program.addOrReplaceFile('components/comp.xml', trim`
<?xml version="1.0" encoding="utf-8" ?>
<component extends="ParentScene">
<script type="text/brightscript" uri="ChildScene.brs" />
</component>
`);
program.validate();
expect(file.diagnostics).to.be.lengthOf(1);
expect(file.diagnostics[0]).to.deep.include(<BsDiagnostic>{
message: DiagnosticMessages.xmlComponentMissingNameAttribute().message,
Expand All @@ -219,12 +218,12 @@ describe('XmlFile', () => {
});

it('catches xml parse errors', () => {
file = new XmlFile('abs', 'rel', program);
file.parse(trim`
file = program.addOrReplaceFile('components/comp.xml', trim`
<?xml version="1.0" encoding="utf-8" ?>
<component 1extends="ParentScene">
</component>
`);
program.validate();
expect(file.diagnostics).to.be.lengthOf(2);
expect(file.diagnostics[0].code).to.equal(DiagnosticMessages.xmlGenericParseError('').code); //unexpected character '1'
expect(file.diagnostics[1]).to.deep.include(<BsDiagnostic>{
Expand Down Expand Up @@ -916,24 +915,26 @@ describe('XmlFile', () => {
const program = new Program({
rootDir: rootDir
});
file = new XmlFile('abs', 'rel', program);
program.plugins.add({
name: 'Transform plugins',
afterFileParse: () => validateXml(file)
afterFileParse: file => validateXml(file as XmlFile)
});
file.parse(trim`
file = program.addOrReplaceFile<XmlFile>('components/component.xml', trim`
<?xml version="1.0" encoding="utf-8" ?>
<component name="Cmp1" extends="Scene">
</component>
`);
program.validate();
return file;
}

it('Calls XML file validation plugins', () => {
const validateXml = sinon.spy();
const file = parseFileWithPlugins(validateXml);
expect(validateXml.callCount).to.equal(1);
expect(validateXml.calledWith(file)).to.be.true;
expect(validateXml.callCount).to.be.greaterThan(0);
expect(
validateXml.getCalls().flatMap(x => x.args)
).to.include(file);
});
});

Expand Down
25 changes: 6 additions & 19 deletions src/files/XmlFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,24 +193,15 @@ export class XmlFile {

this.getCommentFlags(this.parser.tokens as any[]);

//notify AST ready
this.program.plugins.emit('afterFileParse', this);
//needsTranspiled should be true if an import is brighterscript
this.needsTranspiled = this.needsTranspiled || this.ast.component?.scripts?.some(
script => script.type?.indexOf('brighterscript') > 0 || script.uri?.endsWith('.bs')
);
}

//emit an event before starting to validate this file
this.program.plugins.emit('beforeFileValidate', {
file: this,
program: this.program
});

//emit an event to allow plugins to contribute to the file validation process
this.program.plugins.emit('onFileValidate', {
file: this,
program: this.program
});

public validate() {
if (this.parser.ast.root) {

//initial validation
this.validateComponent(this.parser.ast);
} else {
//skip empty XML
Expand Down Expand Up @@ -265,10 +256,6 @@ export class XmlFile {
});
}

//needsTranspiled should be true if an import is brighterscript
this.needsTranspiled = this.needsTranspiled || component.scripts.some(
script => script.type?.indexOf('brighterscript') > 0 || script.uri?.endsWith('.bs')
);

//catch script imports with same path as the auto-imported codebehind file
const scriptTagImports = this.parser.references.scriptTagImports;
Expand Down