From fa6a65c9633dc999f672820a175a333dc93fd5fc Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Tue, 30 Jul 2024 14:02:42 -0300 Subject: [PATCH] Adds Name Shadowing lint rule (#124) * Adds Name Shadowing lint rule * Refined messaging for shadowing a global function --- src/index.ts | 2 + src/plugins/codeStyle/diagnosticMessages.ts | 9 +- src/plugins/codeStyle/index.spec.ts | 55 +++++++++ src/plugins/codeStyle/index.ts | 112 +++++++++++++++++- src/util.ts | 6 +- .../source/name-shadowing-functions.bs | 8 ++ test/project1/source/name-shadowing-import.bs | 12 ++ test/project1/source/name-shadowing.bs | 40 +++++++ 8 files changed, 240 insertions(+), 4 deletions(-) create mode 100644 test/project1/source/name-shadowing-functions.bs create mode 100644 test/project1/source/name-shadowing-import.bs create mode 100644 test/project1/source/name-shadowing.bs diff --git a/src/index.ts b/src/index.ts index 86af465..b2767b7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -51,6 +51,7 @@ export type BsLintConfig = Pick ({ + message: `${ST} ${thisThingKind} has same name as ${thatThingKind ? thatThingKind + ' ' : ''}'${thatThingName}'`, + code: CodeStyleError.NameShadowing, + severity: severity, + source: 'bslint' }) }; diff --git a/src/plugins/codeStyle/index.spec.ts b/src/plugins/codeStyle/index.spec.ts index 760ab0d..88ee181 100644 --- a/src/plugins/codeStyle/index.spec.ts +++ b/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 = [ diff --git a/src/plugins/codeStyle/index.ts b/src/plugins/codeStyle/index.ts index 5f1d416..f71a4b9 100644 --- a/src/plugins/codeStyle/index.ts +++ b/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)[] = []; + 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)[] = []; + 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)[]) { 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)[]) { + 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 + }); + } } /** diff --git a/src/util.ts b/src/util.ts index 5895248..4969825 100644 --- a/src/util.ts +++ b/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']) }; } diff --git a/test/project1/source/name-shadowing-functions.bs b/test/project1/source/name-shadowing-functions.bs new file mode 100644 index 0000000..b441af1 --- /dev/null +++ b/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 diff --git a/test/project1/source/name-shadowing-import.bs b/test/project1/source/name-shadowing-import.bs new file mode 100644 index 0000000..bfc87da --- /dev/null +++ b/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 diff --git a/test/project1/source/name-shadowing.bs b/test/project1/source/name-shadowing.bs new file mode 100644 index 0000000..3be3425 --- /dev/null +++ b/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