From 81d4173e77ad49a8f524437c8a4db664dd1d83e8 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Tue, 14 Jan 2025 17:07:38 +0100 Subject: [PATCH 1/2] feat: Add global detection for XML Templating JIRA: CPOUI5FOUNDATION-917 --- src/linter/binding/BindingLinter.ts | 33 +- src/linter/xmlTemplate/Parser.ts | 67 +- .../generator/AbstractGenerator.ts | 4 + .../controller/TemplatingMain.controller.js | 7 + .../webapp/view/TemplatingMain.view.xml | 26 + .../webapp/view/TemplatingTable.fragment.xml | 56 ++ .../rules/NoGlobals/XMLTemplatingV2.view.xml | 40 ++ .../rules/NoGlobals/XMLTemplatingV4.view.xml | 50 ++ .../rules/snapshots/NoDeprecatedApi.ts.md | 66 +- .../rules/snapshots/NoDeprecatedApi.ts.snap | Bin 19978 -> 19687 bytes .../linter/rules/snapshots/NoGlobals.ts.md | 72 ++ .../linter/rules/snapshots/NoGlobals.ts.snap | Bin 4076 -> 4439 bytes test/lib/linter/snapshots/linter.ts.md | 346 ++++++++++ test/lib/linter/snapshots/linter.ts.snap | Bin 23638 -> 26000 bytes .../xmlTemplate/snapshots/transpiler.ts.md | 8 + .../xmlTemplate/snapshots/transpiler.ts.snap | Bin 9340 -> 9417 bytes test/lib/snapshots/index.ts.md | 624 ++++++++++++++++++ test/lib/snapshots/index.ts.snap | Bin 16158 -> 19082 bytes 18 files changed, 1318 insertions(+), 81 deletions(-) create mode 100644 test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/controller/TemplatingMain.controller.js create mode 100644 test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingMain.view.xml create mode 100644 test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingTable.fragment.xml create mode 100644 test/fixtures/linter/rules/NoGlobals/XMLTemplatingV2.view.xml create mode 100644 test/fixtures/linter/rules/NoGlobals/XMLTemplatingV4.view.xml diff --git a/src/linter/binding/BindingLinter.ts b/src/linter/binding/BindingLinter.ts index f068fb82d..b8665fd54 100644 --- a/src/linter/binding/BindingLinter.ts +++ b/src/linter/binding/BindingLinter.ts @@ -29,10 +29,21 @@ export default class BindingLinter { #analyzeCommonBindingParts( bindingInfo: BindingInfo, requireDeclarations: RequireDeclaration[], position: PositionInfo) { - const {events} = bindingInfo; + const {events, path} = bindingInfo; if (events && typeof events === "object") { for (const eventHandler of Object.values(events)) { - this.#checkForGlobalReference(eventHandler, requireDeclarations, position); + this.checkForGlobalReference(eventHandler, requireDeclarations, position); + } + } + if (path) { + // Check for computed annotations (@@ syntax) + const atAtIndex = path.indexOf("@@"); + if (atAtIndex >= 0) { + const openingBracketIndex = path.indexOf("(", atAtIndex); // opening bracket is optional + const computationFunction = path.slice( + atAtIndex + 2, openingBracketIndex !== -1 ? openingBracketIndex : undefined + ); + this.checkForGlobalReference(computationFunction, requireDeclarations, position); } } } @@ -44,14 +55,14 @@ export default class BindingLinter { if (formatter) { if (Array.isArray(formatter)) { formatter.forEach((formatterItem) => { - this.#checkForGlobalReference(formatterItem, requireDeclarations, position); + this.checkForGlobalReference(formatterItem, requireDeclarations, position); }); } else { - this.#checkForGlobalReference(formatter, requireDeclarations, position); + this.checkForGlobalReference(formatter, requireDeclarations, position); } } if (type) { - this.#checkForGlobalReference(type, requireDeclarations, position); + this.checkForGlobalReference(type, requireDeclarations, position); } } @@ -60,10 +71,10 @@ export default class BindingLinter { this.#analyzeCommonBindingParts(bindingInfo, requireDeclarations, position); const {factory, groupHeaderFactory, filters, sorter} = bindingInfo; if (factory) { - this.#checkForGlobalReference(factory, requireDeclarations, position); + this.checkForGlobalReference(factory, requireDeclarations, position); } if (groupHeaderFactory) { - this.#checkForGlobalReference(groupHeaderFactory, requireDeclarations, position); + this.checkForGlobalReference(groupHeaderFactory, requireDeclarations, position); } if (filters) { this.#analyzeFilters(filters, requireDeclarations, position); @@ -83,7 +94,7 @@ export default class BindingLinter { } const {test, filters: nestedFilters, condition} = filters; if (test) { - this.#checkForGlobalReference(test, requireDeclarations, position); + this.checkForGlobalReference(test, requireDeclarations, position); } if (nestedFilters) { this.#analyzeFilters(nestedFilters, requireDeclarations, position); @@ -103,14 +114,14 @@ export default class BindingLinter { } const {group, comparator} = sorter; if (group && typeof group !== "boolean") { - this.#checkForGlobalReference(group, requireDeclarations, position); + this.checkForGlobalReference(group, requireDeclarations, position); } if (comparator) { - this.#checkForGlobalReference(comparator, requireDeclarations, position); + this.checkForGlobalReference(comparator, requireDeclarations, position); } } - #checkForGlobalReference(ref: string, requireDeclarations: RequireDeclaration[], position: PositionInfo) { + checkForGlobalReference(ref: string, requireDeclarations: RequireDeclaration[], position: PositionInfo) { if (ref.startsWith(".")) { // Ignore empty reference or reference to the controller (as indicated by the leading dot) return false; diff --git a/src/linter/xmlTemplate/Parser.ts b/src/linter/xmlTemplate/Parser.ts index ad0acf0f3..5f6292cf8 100644 --- a/src/linter/xmlTemplate/Parser.ts +++ b/src/linter/xmlTemplate/Parser.ts @@ -3,7 +3,7 @@ import he from "he"; import ViewGenerator from "./generator/ViewGenerator.js"; import FragmentGenerator from "./generator/FragmentGenerator.js"; import JSTokenizer from "./lib/JSTokenizer.js"; -import LinterContext from "../LinterContext.js"; +import LinterContext, {PositionInfo} from "../LinterContext.js"; import {TranspileResult} from "../LinterContext.js"; import AbstractGenerator from "./generator/AbstractGenerator.js"; import {getLogger} from "@ui5/logger"; @@ -82,7 +82,7 @@ export interface RequireExpression extends AttributeDeclaration { } export interface RequireDeclaration { - moduleName: string; + moduleName?: string; variableName: string; } @@ -285,7 +285,7 @@ export default class Parser { [tagNamespace, tagName] = tagName.split(":"); } - const attributes = new Set(); + const attributes = new Map(); tag.attributes.forEach((attr) => { const attrName = attr.name.value; const attrValue = he.decode(attr.value.value); @@ -305,7 +305,7 @@ export default class Parser { } else if (attrName.includes(":")) { // Namespaced attribute const [attrNamespace, attrLocalName] = attrName.split(":"); - attributes.add({ + attributes.set(attrName, { name: attrLocalName, value: attrValue, localNamespace: attrNamespace, @@ -316,7 +316,7 @@ export default class Parser { }), }); } else { - attributes.add({ + attributes.set(attrName, { name: attrName, value: attrValue, start: toPosition(attr.name.start), @@ -368,13 +368,7 @@ export default class Parser { end: toPosition(tag.openEnd), }; } else if (namespace === TEMPLATING_NAMESPACE) { - return { - kind: NodeKind.Template, - name: tagName, - namespace, - start: toPosition(tag.openStart), - end: toPosition(tag.openEnd), - }; + return this._handleTemplatingNamespace(tagName, namespace, attributes, tag); } else if (PATTERN_LIBRARY_NAMESPACES.test(namespace)) { const lastIdx = tagName.lastIndexOf("."); if (lastIdx !== -1) { @@ -396,7 +390,7 @@ export default class Parser { } _handleUi5LibraryNamespace( - moduleName: string, namespace: Namespace, attributes: Set, + moduleName: string, namespace: Namespace, attributes: Map, tag: SaxTag ): ControlDeclaration | AggregationDeclaration | FragmentDefinitionDeclaration { const controlProperties = new Set(); @@ -606,4 +600,51 @@ export default class Parser { return node; } } + + _handleTemplatingNamespace( + tagName: string, namespace: Namespace, attributes: Map, tag: SaxTag + ): NodeDeclaration { + let globalReferenceCheckAttribute: AttributeDeclaration | undefined; + if (tagName === "alias") { + const aliasName = attributes.get("name"); + if (aliasName) { + // Add alias to list of local names so that the global check takes them into account + this.#requireDeclarations.push({ + variableName: aliasName.value, + }); + } + globalReferenceCheckAttribute = attributes.get("value"); + } else if (tagName === "with") { + globalReferenceCheckAttribute = attributes.get("helper"); + } else if (tagName === "if" || tagName === "elseif") { + const testAttribute = attributes.get("test"); + if (testAttribute) { + // template:if/elseif test attribute is handled like a property binding in XMLPreprocessor + this.#bindingLinter.lintPropertyBinding(testAttribute.value, this.#requireDeclarations, { + line: testAttribute.start.line + 1, // Add one to align with IDEs + column: testAttribute.start.column + 1, + }); + } + } + + if (globalReferenceCheckAttribute) { + this._checkGlobalReference(globalReferenceCheckAttribute.value, globalReferenceCheckAttribute.start); + } + + return { + kind: NodeKind.Template, + name: tagName, + namespace, + start: toPosition(tag.openStart), + end: toPosition(tag.openEnd), + }; + } + + _checkGlobalReference(value: string, {line, column}: PositionInfo) { + this.#bindingLinter.checkForGlobalReference(value, this.#requireDeclarations, { + // Add one to align with IDEs + line: line + 1, + column: column + 1, + }); + } } diff --git a/src/linter/xmlTemplate/generator/AbstractGenerator.ts b/src/linter/xmlTemplate/generator/AbstractGenerator.ts index baac71098..f12c7c4be 100644 --- a/src/linter/xmlTemplate/generator/AbstractGenerator.ts +++ b/src/linter/xmlTemplate/generator/AbstractGenerator.ts @@ -85,6 +85,10 @@ export default abstract class AbstractGenerator { writeRequire(requireExpression: RequireExpression) { requireExpression.declarations.forEach((declaration) => { + if (!declaration.moduleName) { + // Module name might be missing, e.g. when a variable is declared via template:alias + return; + } if (!declaration.variableName) { // Side effect require? declaration.variableName = "_"; diff --git a/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/controller/TemplatingMain.controller.js b/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/controller/TemplatingMain.controller.js new file mode 100644 index 000000000..03b7dc0d5 --- /dev/null +++ b/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/controller/TemplatingMain.controller.js @@ -0,0 +1,7 @@ +sap.ui.define(["./BaseController"], function (BaseController) { + "use strict"; + + const TemplatingMainController = BaseController.extend("com.ui5.troublesome.app.controller.TemplatingMain", { + }); + return TemplatingMainController; +}); diff --git a/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingMain.view.xml b/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingMain.view.xml new file mode 100644 index 000000000..c5fcdc320 --- /dev/null +++ b/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingMain.view.xml @@ -0,0 +1,26 @@ + + + + + + + + + + + +
+ + + + + + + diff --git a/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingTable.fragment.xml b/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingTable.fragment.xml new file mode 100644 index 000000000..db2281cf5 --- /dev/null +++ b/test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/TemplatingTable.fragment.xml @@ -0,0 +1,56 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +