Adds Name Shadowing lint rule (#124)

* Adds Name Shadowing lint rule

* Refined messaging for shadowing a global function

src/index.ts

@@ -51,6 +51,7 @@ export type BsLintConfig = Pick<BsConfig, 'project' | 'rootDir' | 'files' | 'cwd
         'color-cert'?: RuleColorCertCompliant;
         'no-assocarray-component-field-type'?: RuleSeverity;
         'no-array-component-field-type'?: RuleSeverity;
+        'name-shadowing'?: RuleSeverity;
     };
     globals?: string[];
     ignores?: string[];
@@ -86,6 +87,7 @@ export interface BsLintRules {
     colorCertCompliant: RuleColorCertCompliant;
     noAssocarrayComponentFieldType: BsLintSeverity;
     noArrayComponentFieldType: BsLintSeverity;
+    nameShadowing: BsLintSeverity;
 }
 
 export { Linter };

src/plugins/codeStyle/diagnosticMessages.ts

@@ -25,7 +25,8 @@ export enum CodeStyleError {
     ColorAlphaDefaults = 'LINT3022',
     ColorCertCompliant = 'LINT3023',
     NoAssocarrayFieldType = 'LINT3024',
-    NoArrayFieldType = 'LINT3025'
+    NoArrayFieldType = 'LINT3025',
+    NameShadowing = 'LINT3026'
 }
 
 const CS = 'Code style:';
@@ -215,5 +216,11 @@ export const messages = {
         severity: severity,
         source: 'bslint',
         range
+    }),
+    nameShadowing: (thisThingKind: string, thatThingKind: string, thatThingName: string, severity: DiagnosticSeverity) => ({
+        message: `${ST} ${thisThingKind} has same name as ${thatThingKind ? thatThingKind + ' ' : ''}'${thatThingName}'`,
+        code: CodeStyleError.NameShadowing,
+        severity: severity,
+        source: 'bslint'
     })
 };

src/plugins/codeStyle/index.spec.ts

@@ -943,6 +943,61 @@ describe('codeStyle', () => {
         });
     });
 
+    describe('name-shadowing', () => {
+        it('detects name shadowings', async () => {
+            const diagnostics = await linter.run({
+                ...project1,
+                files: ['source/name-shadowing.bs'],
+                rules: {
+                    'name-shadowing': 'error'
+                }
+            });
+            expectDiagnosticsFmt(diagnostics, [
+                '15:LINT3026:Strictness: Class has same name as Class \'TestClass\'',
+                '18:LINT3026:Strictness: Enum has same name as Enum \'TestEnum\'',
+                '21:LINT3026:Strictness: Interface has same name as Interface \'TestInterface\'',
+                '24:LINT3026:Strictness: Const has same name as Const \'TestConst\'',
+                '26:LINT3026:Strictness: Const has same name as Namespace \'TestNamespace\''
+            ]);
+        });
+
+        it('detects name shadowings across scope', async () => {
+            const diagnostics = await linter.run({
+                ...project1,
+                files: ['source/name-shadowing.bs', 'source/name-shadowing-import.bs'],
+                rules: {
+                    'name-shadowing': 'error'
+                }
+            });
+            expectDiagnosticsFmt(diagnostics, [
+                '02:LINT3026:Strictness: Class has same name as Class \'TestImportClass\'',
+                '05:LINT3026:Strictness: Enum has same name as Enum \'TestImportEnum\'',
+                '08:LINT3026:Strictness: Interface has same name as Interface \'TestImportInterface\'',
+                '11:LINT3026:Strictness: Const has same name as Const \'TestImportConst\'',
+                '15:LINT3026:Strictness: Class has same name as Class \'TestClass\'',
+                '18:LINT3026:Strictness: Enum has same name as Enum \'TestEnum\'',
+                '21:LINT3026:Strictness: Interface has same name as Interface \'TestInterface\'',
+                '24:LINT3026:Strictness: Const has same name as Const \'TestConst\'',
+                '26:LINT3026:Strictness: Const has same name as Namespace \'TestNamespace\''
+
+            ]);
+        });
+
+        it('detects name shadowing function', async () => {
+            const diagnostics = await linter.run({
+                ...project1,
+                files: ['source/name-shadowing-functions.bs'],
+                rules: {
+                    'name-shadowing': 'error'
+                }
+            });
+            expectDiagnosticsFmt(diagnostics, [
+                '02:LINT3026:Strictness: Const has same name as Function \'TestFunction\'',
+                '03:LINT3026:Strictness: Const has same name as Global Function \'Lcase\''
+            ]);
+        });
+    });
+
     describe('fix', () => {
         // Filenames (without the extension) that we want to copy with a "-temp" suffix
         const tmpFileNames = [

src/plugins/codeStyle/index.ts

@@ -18,7 +18,16 @@ import {
     isVoidType,
     Statement,
     SymbolTypeFlag,
-    XmlFile
+    XmlFile,
+    AstNode,
+    Token,
+    isNamespaceStatement,
+    util,
+    isAnyReferenceType,
+    ExtraSymbolData,
+    OnScopeValidateEvent,
+    InternalWalkMode,
+    isCallableType
 } from 'brighterscript';
 import { RuleAAComma } from '../..';
 import { addFixesToEvent } from '../../textEdit';
@@ -27,6 +36,7 @@ import { createColorValidator } from '../../createColorValidator';
 import { messages } from './diagnosticMessages';
 import { extractFixes } from './styleFixes';
 import { BsLintDiagnosticContext } from '../../Linter';
+import { Location } from 'vscode-languageserver-types';
 
 export default class CodeStyle implements CompilerPlugin {
 
@@ -218,6 +228,33 @@ export default class CodeStyle implements CompilerPlugin {
         return diagnostics;
     }
 
+    validateBrsFileInScope(file: BrsFile) {
+        const diagnostics: (Omit<BsDiagnostic, 'file'>)[] = [];
+        const { severity } = this.lintContext;
+        const { nameShadowing } = severity;
+
+        file.ast.walk(createVisitor({
+            NamespaceStatement: (nsStmt) => {
+                this.validateNameShadowing(file, nsStmt, nsStmt.getNameParts()?.[0], nameShadowing, diagnostics);
+            },
+            ClassStatement: (classStmt) => {
+                this.validateNameShadowing(file, classStmt, classStmt.tokens.name, nameShadowing, diagnostics);
+            },
+            InterfaceStatement: (ifaceStmt) => {
+                this.validateNameShadowing(file, ifaceStmt, ifaceStmt.tokens.name, nameShadowing, diagnostics);
+            },
+            ConstStatement: (constStmt) => {
+                this.validateNameShadowing(file, constStmt, constStmt.tokens.name, nameShadowing, diagnostics);
+            },
+            EnumStatement: (enumStmt) => {
+                this.validateNameShadowing(file, enumStmt, enumStmt.tokens.name, nameShadowing, diagnostics);
+            }
+            // eslint-disable-next-line no-bitwise
+        }), { walkMode: WalkMode.visitStatementsRecursive | InternalWalkMode.visitFalseConditionalCompilationBlocks });
+
+        return diagnostics;
+    }
+
     afterFileValidate(event: AfterFileValidateEvent) {
         const { file } = event;
         if (this.lintContext.ignores(file)) {
@@ -248,6 +285,35 @@ export default class CodeStyle implements CompilerPlugin {
         event.program.diagnostics.register(bsDiagnostics, BsLintDiagnosticContext);
     }
 
+    onScopeValidate(event: OnScopeValidateEvent) {
+        for (const file of event.scope.getOwnFiles()) {
+            if (this.lintContext.ignores(file)) {
+                return;
+            }
+
+            const diagnostics: (Omit<BsDiagnostic, 'file'>)[] = [];
+            if (isBrsFile(file)) {
+                diagnostics.push(...this.validateBrsFileInScope(file));
+            }
+
+            // add file reference
+            let bsDiagnostics: BsDiagnostic[] = diagnostics.map(diagnostic => ({
+                ...diagnostic,
+                file
+            }));
+
+            const { fix } = this.lintContext;
+
+            // apply fix
+            if (fix) {
+                bsDiagnostics = extractFixes(this.lintContext.addFixes, bsDiagnostics);
+            }
+
+            // append diagnostics
+            event.program.diagnostics.register(bsDiagnostics, BsLintDiagnosticContext);
+        }
+    }
+
     validateAAStyle(aa: AALiteralExpression, aaCommaStyle: RuleAAComma, diagnostics: (Omit<BsDiagnostic, 'file'>)[]) {
         const indexes = collectWrappingAAMembersIndexes(aa);
         const last = indexes.length - 1;
@@ -337,6 +403,50 @@ export default class CodeStyle implements CompilerPlugin {
         }
         return hasReturnedValue;
     }
+
+    validateNameShadowing(file: BrsFile, node: AstNode, nameIdentifier: Token, severity: DiagnosticSeverity, diagnostics: (Omit<BsDiagnostic, 'file'>)[]) {
+        const name = nameIdentifier?.text;
+        if (!name || !node) {
+            return;
+        }
+        const nameLocation = nameIdentifier.location;
+
+        const astTable = file.ast.getSymbolTable();
+        const data = {} as ExtraSymbolData;
+        const typeChain = [];
+        // eslint-disable-next-line no-bitwise
+        const existingType = astTable.getSymbolType(name, { flags: SymbolTypeFlag.runtime | SymbolTypeFlag.typetime, data: data, typeChain: typeChain });
+
+        if (!existingType || isAnyReferenceType(existingType)) {
+            return;
+        }
+        if ((data.definingNode === node) || (isNamespaceStatement(data.definingNode) && isNamespaceStatement(node))) {
+            return;
+        }
+        const otherNode = data.definingNode as unknown as { tokens: { name: Token }; location: Location };
+        const thisNodeKindName = util.getAstNodeFriendlyName(node);
+        let thatNodeKindName = util.getAstNodeFriendlyName(data.definingNode) ?? '';
+        if (!thatNodeKindName && isCallableType(existingType)) {
+            thatNodeKindName = 'Global Function';
+        }
+
+        let thatNameLocation = otherNode?.tokens?.name?.location ?? otherNode?.location;
+
+        if (isNamespaceStatement(data.definingNode)) {
+            thatNameLocation = data.definingNode.getNameParts()?.[0]?.location;
+        }
+
+        const relatedInformation = thatNameLocation ? [{
+            message: `${thatNodeKindName} declared here`,
+            location: thatNameLocation
+        }] : undefined;
+
+        diagnostics.push({
+            ...messages.nameShadowing(thisNodeKindName, thatNodeKindName, name, severity),
+            range: nameLocation.range,
+            relatedInformation: relatedInformation
+        });
+    }
 }
 
 /**

src/util.ts

@@ -28,7 +28,8 @@ export function getDefaultRules(): BsLintConfig['rules'] {
         'type-annotations': 'off',
         'no-print': 'off',
         'no-assocarray-component-field-type': 'off',
-        'no-array-component-field-type': 'off'
+        'no-array-component-field-type': 'off',
+        'name-shadowing': 'off'
     };
 }
 
@@ -166,7 +167,8 @@ function rulesToSeverity(rules: BsLintConfig['rules']) {
         colorAlphaDefaults: rules['color-alpha-defaults'],
         colorCertCompliant: rules['color-cert'],
         noAssocarrayComponentFieldType: ruleToSeverity(rules['no-assocarray-component-field-type']),
-        noArrayComponentFieldType: ruleToSeverity(rules['no-array-component-field-type'])
+        noArrayComponentFieldType: ruleToSeverity(rules['no-array-component-field-type']),
+        nameShadowing: ruleToSeverity(rules['name-shadowing'])
     };
 }
 

test/project1/source/name-shadowing-functions.bs

@@ -0,0 +1,8 @@
+namespace TestNamespace
+    const TestFunction = 3
+    const Lcase = 4
+end namespace
+
+function TestFunction()
+    return 2
+end function

test/project1/source/name-shadowing-import.bs

@@ -0,0 +1,12 @@
+namespace TestNamespace
+    class TestImportClass
+    end class
+
+    enum TestImportEnum
+    end enum
+
+    interface TestImportInterface
+    end interface
+
+    const TestImportConst = 1
+end namespace

test/project1/source/name-shadowing.bs

@@ -0,0 +1,40 @@
+class TestClass
+end class
+
+enum TestEnum
+end enum
+
+interface TestInterface
+end interface
+
+const TestConst = 1
+
+
+namespace TestNamespace
+
+    class TestClass
+    end class
+
+    enum TestEnum
+    end enum
+
+    interface TestInterface
+    end interface
+
+    const TestConst = 1
+
+    const TestNamespace = 2
+
+end namespace
+
+
+class TestImportClass
+end class
+
+enum TestImportEnum
+end enum
+
+interface TestImportInterface
+end interface
+
+const TestImportConst = 1