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

Jump to bottom

support on-demand parse for typedef-shadowed files #237

Merged
merged 1 commit into from
Nov 1, 2020
Merged

support on-demand parse for typedef-shadowed files #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 46 additions & 5 deletions src/files/BrsFile.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2134,18 +2134,19 @@ describe('BrsFile', () => {

describe('type definitions', () => {
it('only exposes defined functions even if source has more', async () => {
await program.addOrReplaceFile('source/main.d.bs', `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapped the order of the file loading, so we can ensure the file was fully parsed (since it didn't know about the .d.bs file)

//parse the .brs file first so it doesn't know about the typedef
await program.addOrReplaceFile<BrsFile>('source/main.brs', `
sub main()
end sub
sub speak()
end sub
`);

const file = await program.addOrReplaceFile<BrsFile>('source/main.brs', `
await program.addOrReplaceFile('source/main.d.bs', `
sub main()
end sub
sub speak()
end sub
`);
expect(file.parser.references.functionStatements).to.be.empty;

const sourceScope = program.getScopeByName('source');
const functionNames = sourceScope.getAllCallables().map(x => x.callable.name);
expect(functionNames).to.include('main');
Expand Down Expand Up @@ -2429,6 +2430,46 @@ describe('BrsFile', () => {
});
});

describe('parser getter', () => {
it('recreates the parser when missing', async () => {
const file = await program.addOrReplaceFile<BrsFile>('source/main.brs', `
sub main()
end sub
`);
const parser = file['_parser'];
//clear the private _parser instance
file['_parser'] = undefined;

//force the file to get a new instance of parser
const newParser = file.parser;

expect(newParser).to.exist.and.to.not.equal(parser);

//reference shouldn't change in subsequent accesses
expect(file.parser).to.equal(newParser);
});

it('call parse when previously skipped', async () => {
await program.addOrReplaceFile<BrsFile>('source/main.d.bs', `
sub main()
end sub
`);
const file = await program.addOrReplaceFile<BrsFile>('source/main.brs', `
sub main()
end sub
`);
//no functions should be found since the parser was skipped
expect(file['_parser']).to.not.exist;

const stub = sinon.stub(file, 'parse').callThrough();

//`file.parser` is a getter, so that should force the parse to occur
expect(file.parser.references.functionStatements).to.be.lengthOf(1);
expect(stub.called).to.be.true;
//parse should have been called
});
});

describe('Plugins', () => {
it('can load a plugin which transforms the AST', async () => {
program.plugins = new PluginInterface(
Expand Down
56 changes: 30 additions & 26 deletions src/files/BrsFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,23 @@ export class BrsFile {
}
}

public parser = new Parser();
public get parser() {
if (!this._parser) {
//remove the typedef file (if it exists)
this.hasTypedef = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these need to be removed before calling parse, because parse will stop parsing if it detects a typedef file. It's a bit of a hack, but since it's all internal logic, it works fine.

this.typedefFile = undefined;

//reset the deferred
this.parseDeferred = new Deferred();
//parse the file (it should parse fully since there's no linked typedef
this.parse(this.fileContents);

//re-link the typedef (if it exists...which it should)
this.resolveTypdef();
}
return this._parser;
}
private _parser: Parser;

public fileContents: string;

Expand Down Expand Up @@ -188,13 +204,6 @@ export class BrsFile {
//event that fires anytime a dependency changes
this.unsubscribeFromDependencyGraph = this.program.dependencyGraph.onchange(this.dependencyGraphKey, () => {
this.resolveTypdef();

//if there is no typedef file, and this file hasn't been parsed yet, parse it now
//(solves issue when typedef gets deleted and this file had skipped parsing)
if (!this.hasTypedef && this.wasParseSkipped) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer necessary, since the parser is a getter, it will parse the file on-demand the first time it's needed.

this.parseDeferred = new Deferred();
this.parse(this.fileContents);
}
});

const dependencies = this.ownScriptImports.filter(x => !!x.pkgPath).map(x => x.pkgPath.toLowerCase());
Expand All @@ -208,11 +217,6 @@ export class BrsFile {
dependencyGraph.addOrReplace(this.dependencyGraphKey, dependencies);
}

/**
* Was parsing skipped because the file has a typedef?
*/
private wasParseSkipped = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary now that parser is a getter.


/**
* Calculate the AST for this file
* @param fileContents
Expand All @@ -226,7 +230,6 @@ export class BrsFile {

//if we have a typedef file, skip parsing this file
if (this.hasTypedef) {
this.wasParseSkipped = true;
this.parseDeferred.resolve();
return;
}
Expand Down Expand Up @@ -260,7 +263,7 @@ export class BrsFile {
let tokens = preprocessor.processedTokens.length > 0 ? preprocessor.processedTokens : lexer.tokens;

this.program.logger.time(LogLevel.debug, ['parser.parse', chalk.green(this.pathAbsolute)], () => {
this.parser.parse(tokens, {
this._parser = Parser.parse(tokens, {
mode: this.parseMode,
logger: this.program.logger
});
Expand All @@ -270,7 +273,7 @@ export class BrsFile {
this.diagnostics.push(
...lexer.diagnostics as BsDiagnostic[],
...preprocessor.diagnostics as BsDiagnostic[],
...this.parser.diagnostics as BsDiagnostic[]
...this._parser.diagnostics as BsDiagnostic[]
);

//notify AST ready
Expand All @@ -289,14 +292,13 @@ export class BrsFile {
diagnostic.file = this;
}
} catch (e) {
this.parser = new Parser();
this._parser = new Parser();
this.diagnostics.push({
file: this,
range: util.createRange(0, 0, 0, Number.MAX_VALUE),
...DiagnosticMessages.genericParserMessage('Critical error parsing file: ' + JSON.stringify(serializeError(e)))
});
}
this.wasParseSkipped = false;
this.parseDeferred.resolve();
}

Expand All @@ -318,8 +320,8 @@ export class BrsFile {
}

let statements = [
...this.parser.references.libraryStatements,
...this.parser.references.importStatements
...this._parser.references.libraryStatements,
...this._parser.references.importStatements
];
for (let result of statements) {
//register import statements
Expand Down Expand Up @@ -652,7 +654,7 @@ export class BrsFile {
private findFunctionCalls() {
this.functionCalls = [];
//for every function in the file
for (let func of this.parser.references.functionExpressions) {
for (let func of this._parser.references.functionExpressions) {
//for all function calls in this function
for (let expression of func.callExpressions) {

Expand Down Expand Up @@ -891,8 +893,9 @@ export class BrsFile {

//consume tokens backwards until we find someting other than a dot or an identifier
let tokens = [];
for (let i = this.parser.tokens.indexOf(currentToken); i >= 0; i--) {
currentToken = this.parser.tokens[i];
const parser = this.parser;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lift the parser out of the loop so we only pay the getter cost once.

for (let i = parser.tokens.indexOf(currentToken); i >= 0; i--) {
currentToken = parser.tokens[i];
if (identifierAndDotKinds.includes(currentToken.kind)) {
tokens.unshift(currentToken.text);
} else {
Expand Down Expand Up @@ -932,8 +935,9 @@ export class BrsFile {
}

public getPreviousToken(token: Token) {
let idx = this.parser.tokens.indexOf(token);
return this.parser.tokens[idx - 1];
const parser = this.parser;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lift the parser out of the loop so we only pay the getter cost once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use _parser no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's not safe. This is a public method, which provides no guarantees about who will be calling it. Could be a plug-in, in which case _parser might be undefined.

let idx = parser.tokens.indexOf(token);
return parser.tokens[idx - 1];
}

/**
Expand Down Expand Up @@ -1093,7 +1097,7 @@ export class BrsFile {
}

public dispose() {
this.parser?.dispose();
this._parser?.dispose();
}
}

Expand Down
4 changes: 1 addition & 3 deletions src/globalCallables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,10 @@ import { InterfaceType } from './types/InterfaceType';
import { ObjectType } from './types/ObjectType';
import { StringType } from './types/StringType';
import { VoidType } from './types/VoidType';
import { Parser } from './parser';
import util from './util';

export let globalFile = new BrsFile('global', 'global', null);
globalFile.parser = new Parser();
globalFile.parser.parse([]);
globalFile.parse('');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplify the global file a bit


let mathFunctions = [{
name: 'Abs',
Expand Down