Skip to content

Commit e97969a

Browse files
TwitchBronBronTwitchBronBron
authored
Flag usage of undefined variables (#631)
* stupid. it's too slow. dangit. * Fix performance issue, repair broken test * remove unused WeakMapCache * Better flagging for unknown namespace items. (need to dedupe from enum checking yet) * Better scope validations, grouping functions * Fix namespace-relative import validations * Fix missing enum validations * remove dead comments * Better relatedInformation handling * More tests * Add comments about symbol table linking
1 parent 9e6e03b commit e97969a

27 files changed

+1182
-349
lines changed

‎src/DiagnosticMessages.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,11 @@ export let DiagnosticMessages = {
1313
code: 1000,
1414
severity: DiagnosticSeverity.Error
1515
}),
16-
callToUnknownFunction: (name: string, scopeName: string) => ({
17-
message: `Cannot find function with name '${name}' when this file is included in scope '${scopeName}'`,
16+
cannotFindName: (name: string) => ({
17+
message: `Cannot find name '${name}'`,
1818
code: 1001,
1919
data: {
20-
functionName: name
20+
name: name
2121
},
2222
severity: DiagnosticSeverity.Error
2323
}),

‎src/LanguageServer.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ export class LanguageServer {
694694
//clone the diagnostics for each code action, since certain diagnostics can have circular reference properties that kill the language server if serialized
695695
for (const codeAction of codeActions) {
696696
if (codeAction.diagnostics) {
697-
codeAction.diagnostics = codeAction.diagnostics.map(x => util.toDiagnostic(x));
697+
codeAction.diagnostics = codeAction.diagnostics.map(x => util.toDiagnostic(x, params.textDocument.uri));
698698
}
699699
}
700700
return codeActions;
@@ -1213,10 +1213,11 @@ export class LanguageServer {
12131213
const patch = this.diagnosticCollection.getPatch(this.projects);
12141214

12151215
for (let filePath in patch) {
1216-
const diagnostics = patch[filePath].map(d => util.toDiagnostic(d));
1216+
const uri = URI.file(filePath).toString();
1217+
const diagnostics = patch[filePath].map(d => util.toDiagnostic(d, uri));
12171218

12181219
this.connection.sendDiagnostics({
1219-
uri: URI.file(filePath).toString(),
1220+
uri: uri,
12201221
diagnostics: diagnostics
12211222
});
12221223
}

‎src/Program.spec.ts‎

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ describe('Program', () => {
494494

495495
program.validate();
496496
expectDiagnostics(program, [
497-
DiagnosticMessages.callToUnknownFunction('DoSomething', 'source')
497+
DiagnosticMessages.cannotFindName('DoSomething')
498498
]);
499499
});
500500

@@ -1360,7 +1360,7 @@ describe('Program', () => {
13601360

13611361
//there should be an error when calling DoParentThing, since it doesn't exist on child or parent
13621362
expectDiagnostics(program, [
1363-
DiagnosticMessages.callToUnknownFunction('DoParentThing', '').code
1363+
DiagnosticMessages.cannotFindName('DoParentThing')
13641364
]);
13651365

13661366
//add the script into the parent
@@ -1519,7 +1519,7 @@ describe('Program', () => {
15191519
];
15201520

15211521
expectDiagnostics(program, [
1522-
DiagnosticMessages.callToUnknownFunction('C', 'source')
1522+
DiagnosticMessages.cannotFindName('C')
15231523
]);
15241524
});
15251525
});
@@ -2186,7 +2186,8 @@ describe('Program', () => {
21862186
it('gets signature help for callfunc method', () => {
21872187
program.setFile('source/main.bs', `
21882188
function main()
2189-
myNode@.sayHello(arg1)
2189+
myNode = createObject("roSGNode", "Component1")
2190+
myNode@.sayHello(1)
21902191
end function
21912192
`);
21922193
program.setFile('components/MyNode.bs', `
@@ -2203,15 +2204,16 @@ describe('Program', () => {
22032204
</component>`);
22042205
program.validate();
22052206

2206-
let signatureHelp = (program.getSignatureHelp(`${rootDir}/source/main.bs`, Position.create(2, 36)));
2207+
let signatureHelp = (program.getSignatureHelp(`${rootDir}/source/main.bs`, Position.create(3, 36)));
22072208
expectZeroDiagnostics(program);
22082209
expect(signatureHelp[0].signature.label).to.equal('function sayHello(text, text2)');
22092210
});
22102211

22112212
it('does not get signature help for callfunc method, referenced by dot', () => {
22122213
program.setFile('source/main.bs', `
22132214
function main()
2214-
myNode.sayHello(arg1)
2215+
myNode = createObject("roSGNode", "Component1")
2216+
myNode.sayHello(1, 2)
22152217
end function
22162218
`);
22172219
program.setFile('components/MyNode.bs', `
@@ -2228,7 +2230,7 @@ describe('Program', () => {
22282230
</component>`);
22292231
program.validate();
22302232

2231-
let signatureHelp = (program.getSignatureHelp(`${rootDir}/source/main.bs`, Position.create(2, 36)));
2233+
let signatureHelp = (program.getSignatureHelp(`${rootDir}/source/main.bs`, Position.create(3, 36)));
22322234
expectZeroDiagnostics(program);
22332235
//note - callfunc completions and signatures are not yet correctly identifying methods that are exposed in an interace - waiting on the new xml branch for that
22342236
expect(signatureHelp).to.be.empty;
@@ -2532,4 +2534,26 @@ describe('Program', () => {
25322534
expect(plugin.afterFileValidate.callCount).to.equal(1);
25332535
});
25342536
});
2537+
2538+
describe('getScopesForFile', () => {
2539+
it('returns empty array when no scopes were found', () => {
2540+
expect(program.getScopesForFile('does/not/exist')).to.eql([]);
2541+
});
2542+
});
2543+
2544+
describe('findFilesForEnum', () => {
2545+
it('finds files', () => {
2546+
const file = program.setFile('source/main.bs', `
2547+
enum Direction
2548+
up
2549+
down
2550+
end enum
2551+
`);
2552+
expect(
2553+
program.findFilesForEnum('Direction').map(x => x.srcPath)
2554+
).to.eql([
2555+
file.srcPath
2556+
]);
2557+
});
2558+
});
25352559
});

‎src/Program.ts‎

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -736,13 +736,18 @@ export class Program {
736736
* Get a list of all scopes the file is loaded into
737737
* @param file
738738
*/
739-
public getScopesForFile(file: XmlFile | BrsFile) {
739+
public getScopesForFile(file: XmlFile | BrsFile | string) {
740+
if (typeof file === 'string') {
741+
file = this.getFile(file);
742+
}
740743
let result = [] as Scope[];
741-
for (let key in this.scopes) {
742-
let scope = this.scopes[key];
744+
if (file) {
745+
for (let key in this.scopes) {
746+
let scope = this.scopes[key];
743747

744-
if (scope.hasFile(file)) {
745-
result.push(scope);
748+
if (scope.hasFile(file)) {
749+
result.push(scope);
750+
}
746751
}
747752
}
748753
return result;
@@ -1484,6 +1489,34 @@ export class Program {
14841489
return files;
14851490
}
14861491

1492+
public findFilesForNamespace(name: string) {
1493+
const files = [] as BscFile[];
1494+
const lowerName = name.toLowerCase();
1495+
//find every file with this class defined
1496+
for (const file of Object.values(this.files)) {
1497+
if (isBrsFile(file)) {
1498+
if (file.parser.references.namespaceStatements.find(x => x.name.toLowerCase() === lowerName)) {
1499+
files.push(file);
1500+
}
1501+
}
1502+
}
1503+
return files;
1504+
}
1505+
1506+
public findFilesForEnum(name: string) {
1507+
const files = [] as BscFile[];
1508+
const lowerName = name.toLowerCase();
1509+
//find every file with this class defined
1510+
for (const file of Object.values(this.files)) {
1511+
if (isBrsFile(file)) {
1512+
if (file.parser.references.enumStatementLookup.get(lowerName)) {
1513+
files.push(file);
1514+
}
1515+
}
1516+
}
1517+
return files;
1518+
}
1519+
14871520
/**
14881521
* Get a map of the manifest information
14891522
*/

‎src/Scope.spec.ts‎

Lines changed: 145 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { expect } from 'chai';
22
import * as sinonImport from 'sinon';
33
import { Position, Range } from 'vscode-languageserver';
4-
import { standardizePath as s } from './util';
4+
import util, { standardizePath as s } from './util';
55
import { DiagnosticMessages } from './DiagnosticMessages';
66
import { Program } from './Program';
77
import { ParseMode } from './parser/Parser';
@@ -42,6 +42,44 @@ describe('Scope', () => {
4242
expectZeroDiagnostics(program);
4343
});
4444

45+
it('builds symbol table with namespace-relative entries', () => {
46+
const file = program.setFile<BrsFile>('source/alpha.bs', `
47+
namespace alpha
48+
class Beta
49+
end class
50+
end namespace
51+
namespace alpha
52+
class Charlie extends Beta
53+
end class
54+
function createBeta()
55+
return new Beta()
56+
end function
57+
end namespace
58+
`);
59+
program.setFile('source/main.bs', `
60+
function main()
61+
alpha.createBeta()
62+
thing = new alpha.Beta()
63+
end function
64+
`);
65+
program.validate();
66+
const scope = program.getScopesForFile('source/alpha.bs')[0];
67+
scope.linkSymbolTable();
68+
const symbolTable = file.parser.references.namespaceStatements[1].symbolTable;
69+
//the symbol table should contain the relative names for all items in this namespace across files
70+
expect(
71+
symbolTable.hasSymbol('Beta')
72+
).to.be.true;
73+
expect(
74+
symbolTable.hasSymbol('Charlie')
75+
).to.be.true;
76+
expect(
77+
symbolTable.hasSymbol('createBeta')
78+
).to.be.true;
79+
80+
expectZeroDiagnostics(program);
81+
});
82+
4583
it('handles variables with javascript prototype names', () => {
4684
program.setFile('source/main.brs', `
4785
sub main()
@@ -160,6 +198,109 @@ describe('Scope', () => {
160198
});
161199

162200
describe('validate', () => {
201+
it('detects unknown namespace names', () => {
202+
program.setFile('source/main.bs', `
203+
sub main()
204+
Name1.thing()
205+
Name2.thing()
206+
end sub
207+
namespace Name1
208+
sub thing()
209+
end sub
210+
end namespace
211+
`);
212+
program.validate();
213+
expectDiagnostics(program, [
214+
DiagnosticMessages.cannotFindName('Name2')
215+
]);
216+
});
217+
218+
it('detects unknown namespace sub-names', () => {
219+
program.setFile('source/main.bs', `
220+
sub main()
221+
Name1.subname.thing()
222+
end sub
223+
namespace Name1
224+
sub thing()
225+
end sub
226+
end namespace
227+
`);
228+
program.validate();
229+
expectDiagnostics(program, [{
230+
...DiagnosticMessages.cannotFindName('subname'),
231+
range: util.createRange(2, 26, 2, 33)
232+
}]);
233+
});
234+
235+
it('detects unknown enum names', () => {
236+
program.setFile('source/main.bs', `
237+
sub main()
238+
print Direction.up
239+
print up.Direction
240+
end sub
241+
enum Direction
242+
up
243+
end enum
244+
`);
245+
program.validate();
246+
expectDiagnostics(program, [
247+
DiagnosticMessages.cannotFindName('up')
248+
]);
249+
});
250+
251+
it('detects unknown function names', () => {
252+
program.setFile('source/main.bs', `
253+
sub main()
254+
print go.toStr()
255+
print go2.toStr()
256+
end sub
257+
258+
function go()
259+
end function
260+
`);
261+
program.validate();
262+
expectDiagnostics(program, [
263+
DiagnosticMessages.cannotFindName('go2')
264+
]);
265+
});
266+
267+
it('detects unknown local var names', () => {
268+
program.setFile('source/lib.bs', `
269+
sub libFunc(param1)
270+
print param1
271+
print param2
272+
name1 = "bob"
273+
print name1
274+
print name2
275+
for each item1 in param1
276+
print item1
277+
print item2
278+
end for
279+
for idx1 = 0 to 10
280+
print idx1
281+
print idx2
282+
end for
283+
try
284+
print 1
285+
catch ex1
286+
print ex1
287+
print ex2
288+
end try
289+
end sub
290+
291+
function go()
292+
end function
293+
`);
294+
program.validate();
295+
expectDiagnostics(program, [
296+
DiagnosticMessages.cannotFindName('param2'),
297+
DiagnosticMessages.cannotFindName('name2'),
298+
DiagnosticMessages.cannotFindName('item2'),
299+
DiagnosticMessages.cannotFindName('idx2'),
300+
DiagnosticMessages.cannotFindName('ex2')
301+
]);
302+
});
303+
163304
describe('createObject', () => {
164305
it('recognizes various scenegraph nodes', () => {
165306
program.setFile(`source/file.brs`, `
@@ -463,7 +604,7 @@ describe('Scope', () => {
463604
});
464605

465606
it('flags scope function with same name (but different case) as built-in function', () => {
466-
program.setFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
607+
program.setFile('source/main.brs', `
467608
sub main()
468609
print str(12345) ' prints 12345 (i.e. our str() function below is ignored)
469610
end sub
@@ -509,7 +650,7 @@ describe('Scope', () => {
509650
//validate the scope
510651
program.validate();
511652
expectDiagnostics(program, [
512-
DiagnosticMessages.callToUnknownFunction('DoB', 'source')
653+
DiagnosticMessages.cannotFindName('DoB')
513654
]);
514655
});
515656

@@ -525,7 +666,7 @@ describe('Scope', () => {
525666
//validate the scope
526667
program.validate();
527668
expectDiagnostics(program, [
528-
DiagnosticMessages.callToUnknownFunction('DoC', 'source')
669+
DiagnosticMessages.cannotFindName('DoC')
529670
]);
530671
});
531672

0 commit comments

Comments
 (0)