From 38f93918ac50b9000f97a9fc3cdf0a24beb43811 Mon Sep 17 00:00:00 2001 From: Bronley Date: Wed, 17 Feb 2021 21:47:55 -0500 Subject: [PATCH 1/2] Catch circular class extend references --- src/DiagnosticMessages.ts | 5 ++++ src/files/BrsFile.Class.spec.ts | 40 ++++++++++++++++++++++++++++++++ src/validators/ClassValidator.ts | 33 ++++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts index 2ffb9a14d..95a9f9bfc 100644 --- a/src/DiagnosticMessages.ts +++ b/src/DiagnosticMessages.ts @@ -620,6 +620,11 @@ export let DiagnosticMessages = { message: `Missing expression(s) in 'dim' statement`, code: 1121, severity: DiagnosticSeverity.Error + }), + circularReferenceDetected: (items: string[]) => ({ + message: `Circular reference detected between ${Array.isArray(items) ? items.join(' -> ') : ''}`, + code: 1122, + severity: DiagnosticSeverity.Error }) }; diff --git a/src/files/BrsFile.Class.spec.ts b/src/files/BrsFile.Class.spec.ts index 5dd9e8d49..cc6c3a9e7 100644 --- a/src/files/BrsFile.Class.spec.ts +++ b/src/files/BrsFile.Class.spec.ts @@ -741,6 +741,46 @@ describe('BrsFile BrighterScript classes', () => { ); }); + it('detects direct circular extends', () => { + //direct + program.addOrReplaceFile('source/Direct.bs', ` + class Parent extends Child + end class + + class Child extends Parent + end class + `); + program.validate(); + expect( + program.getDiagnostics().map(x => x.message).sort() + ).to.eql([ + DiagnosticMessages.circularReferenceDetected(['Child', 'Parent', 'Child']).message, + DiagnosticMessages.circularReferenceDetected(['Parent', 'Child', 'Parent']).message + ]); + }); + + it('detects indirect circular extends', () => { + //direct + program.addOrReplaceFile('source/Direct.bs', ` + class Parent extends Grandchild + end class + + class Child extends Parent + end class + + class Grandchild extends Child + end class + `); + program.validate(); + expect( + program.getDiagnostics().map(x => x.message).sort() + ).to.eql([ + DiagnosticMessages.circularReferenceDetected(['Child', 'Parent', 'Grandchild', 'Child']).message, + DiagnosticMessages.circularReferenceDetected(['Grandchild', 'Child', 'Parent', 'Grandchild']).message, + DiagnosticMessages.circularReferenceDetected(['Parent', 'Grandchild', 'Child', 'Parent']).message + ]); + }); + it('detects duplicate member names', () => { program.addOrReplaceFile({ src: `${rootDir}/source/main.bs`, dest: 'source/main.bs' }, ` class Animal diff --git a/src/validators/ClassValidator.ts b/src/validators/ClassValidator.ts index f2955c6e3..4521cfbcc 100644 --- a/src/validators/ClassValidator.ts +++ b/src/validators/ClassValidator.ts @@ -14,6 +14,9 @@ import type { BrsFile } from '../files/BrsFile'; export class BsClassValidator { private scope: Scope; public diagnostics: BsDiagnostic[]; + /** + * The key is the namespace-prefixed class name. (i.e. `NameA.NameB.SomeClass` or `CoolClass` + */ private classes: Record; public validate(scope: Scope) { @@ -24,6 +27,7 @@ export class BsClassValidator { this.findClasses(); this.findNamespaceNonNamespaceCollisions(); this.linkClassesWithParents(); + this.detectCircularReferences(); this.validateMemberCollisions(); this.verifyChildConstructor(); this.verifyNewExpressions(); @@ -156,6 +160,31 @@ export class BsClassValidator { } } + private detectCircularReferences() { + for (let key in this.classes) { + let cls = this.classes[key]; + const names = new Map(); + do { + const className = cls.getName(ParseMode.BrighterScript); + const lowerClassName = className.toLowerCase(); + //if we've already seen this class name before, then we have a circular dependency + if (names.has(lowerClassName)) { + this.diagnostics.push({ + ...DiagnosticMessages.circularReferenceDetected([ + ...names.values(), + className + ]), + file: cls.file, + range: cls.name.range + }); + break; + } + names.set(lowerClassName, className); + cls = cls.parentClass; + } while (cls); + } + } + private validateMemberCollisions() { for (let key in this.classes) { let classStatement = this.classes[key]; @@ -270,7 +299,7 @@ export class BsClassValidator { /** * Get the closest member with the specified name (case-insensitive) */ - private getAncestorMember(classStatement: AugmentedClassStatement, memberName: string) { + getAncestorMember(classStatement, memberName) { let lowerMemberName = memberName.toLowerCase(); let ancestor = classStatement.parentClass; while (ancestor) { @@ -281,7 +310,7 @@ export class BsClassValidator { classStatement: ancestor }; } - ancestor = ancestor.parentClass; + ancestor = ancestor.parentClass !== ancestor ? ancestor.parentClass : null; } } From 328c5a70c335297962acc57624b4dad20b987a3b Mon Sep 17 00:00:00 2001 From: Bronley Date: Wed, 17 Feb 2021 21:51:08 -0500 Subject: [PATCH 2/2] Add scope name to diagnostic message --- src/DiagnosticMessages.ts | 4 ++-- src/files/BrsFile.Class.spec.ts | 14 +++++++++----- src/validators/ClassValidator.ts | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts index 95a9f9bfc..16c4e8e1c 100644 --- a/src/DiagnosticMessages.ts +++ b/src/DiagnosticMessages.ts @@ -621,8 +621,8 @@ export let DiagnosticMessages = { code: 1121, severity: DiagnosticSeverity.Error }), - circularReferenceDetected: (items: string[]) => ({ - message: `Circular reference detected between ${Array.isArray(items) ? items.join(' -> ') : ''}`, + circularReferenceDetected: (items: string[], scopeName: string) => ({ + message: `Circular reference detected between ${Array.isArray(items) ? items.join(' -> ') : ''} in scope '${scopeName}'`, code: 1122, severity: DiagnosticSeverity.Error }) diff --git a/src/files/BrsFile.Class.spec.ts b/src/files/BrsFile.Class.spec.ts index cc6c3a9e7..35eaacccb 100644 --- a/src/files/BrsFile.Class.spec.ts +++ b/src/files/BrsFile.Class.spec.ts @@ -754,8 +754,8 @@ describe('BrsFile BrighterScript classes', () => { expect( program.getDiagnostics().map(x => x.message).sort() ).to.eql([ - DiagnosticMessages.circularReferenceDetected(['Child', 'Parent', 'Child']).message, - DiagnosticMessages.circularReferenceDetected(['Parent', 'Child', 'Parent']).message + DiagnosticMessages.circularReferenceDetected(['Child', 'Parent', 'Child'], 'source').message, + DiagnosticMessages.circularReferenceDetected(['Parent', 'Child', 'Parent'], 'source').message ]); }); @@ -775,12 +775,16 @@ describe('BrsFile BrighterScript classes', () => { expect( program.getDiagnostics().map(x => x.message).sort() ).to.eql([ - DiagnosticMessages.circularReferenceDetected(['Child', 'Parent', 'Grandchild', 'Child']).message, - DiagnosticMessages.circularReferenceDetected(['Grandchild', 'Child', 'Parent', 'Grandchild']).message, - DiagnosticMessages.circularReferenceDetected(['Parent', 'Grandchild', 'Child', 'Parent']).message + DiagnosticMessages.circularReferenceDetected(['Child', 'Parent', 'Grandchild', 'Child'], 'source').message, + DiagnosticMessages.circularReferenceDetected(['Grandchild', 'Child', 'Parent', 'Grandchild'], 'source').message, + DiagnosticMessages.circularReferenceDetected(['Parent', 'Grandchild', 'Child', 'Parent'], 'source').message ]); }); + it('does not cause infinite loop', () => { + + }); + it('detects duplicate member names', () => { program.addOrReplaceFile({ src: `${rootDir}/source/main.bs`, dest: 'source/main.bs' }, ` class Animal diff --git a/src/validators/ClassValidator.ts b/src/validators/ClassValidator.ts index 4521cfbcc..a1cbf7e65 100644 --- a/src/validators/ClassValidator.ts +++ b/src/validators/ClassValidator.ts @@ -173,7 +173,7 @@ export class BsClassValidator { ...DiagnosticMessages.circularReferenceDetected([ ...names.values(), className - ]), + ], this.scope.name), file: cls.file, range: cls.name.range });