Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions src/linter/binding/BindingLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand All @@ -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);
}
}

Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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;
Expand Down
11 changes: 11 additions & 0 deletions src/linter/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export enum MESSAGE {
NO_EXPORTED_VALUES_BY_LIB,
NO_GLOBALS,
NO_ICON_POOL_RENDERER,
NO_LEGACY_TEMPLATE_REQUIRE_SYNTAX,
NO_ODATA_GLOBALS,
NOT_STATIC_CONTROL_RENDERER,
PARSING_ERROR,
Expand Down Expand Up @@ -368,6 +369,16 @@ export const MESSAGE_INFO = {
details: () => `{@link sap.ui.core.RenderManager#methods/icon RenderManager}`,
},

[MESSAGE.NO_LEGACY_TEMPLATE_REQUIRE_SYNTAX]: {
severity: LintMessageSeverity.Error,
ruleId: RULES["no-deprecated-api"],

message: ({moduleNames}: {moduleNames: string}) =>
`Usage of space-separated list '${moduleNames}' in template:require`,
details: () => `Use the object notation of template:require instead ` +
`{@link topic:263f6e5a915f430894ee290040e7e220}`,
Comment on lines +376 to +379
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KlattG please review this new message

},

[MESSAGE.NOT_STATIC_CONTROL_RENDERER]: {
severity: LintMessageSeverity.Warning,
ruleId: RULES["ui5-class-declaration"],
Expand Down
74 changes: 61 additions & 13 deletions src/linter/xmlTemplate/Parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -82,7 +82,7 @@ export interface RequireExpression extends AttributeDeclaration {
}

export interface RequireDeclaration {
moduleName: string;
moduleName?: string;
variableName: string;
}

Expand Down Expand Up @@ -285,7 +285,7 @@ export default class Parser {
[tagNamespace, tagName] = tagName.split(":");
}

const attributes = new Set<AttributeDeclaration>();
const attributes = new Map<string, AttributeDeclaration>();
tag.attributes.forEach((attr) => {
const attrName = attr.name.value;
const attrValue = he.decode(attr.value.value);
Expand All @@ -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,
Expand All @@ -316,7 +316,7 @@ export default class Parser {
}),
});
} else {
attributes.add({
attributes.set(attrName, {
name: attrName,
value: attrValue,
start: toPosition(attr.name.start),
Expand Down Expand Up @@ -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) {
Expand All @@ -396,7 +390,7 @@ export default class Parser {
}

_handleUi5LibraryNamespace(
moduleName: string, namespace: Namespace, attributes: Set<AttributeDeclaration>,
moduleName: string, namespace: Namespace, attributes: Map<string, AttributeDeclaration>,
tag: SaxTag
): ControlDeclaration | AggregationDeclaration | FragmentDefinitionDeclaration {
const controlProperties = new Set<PropertyDeclaration>();
Expand Down Expand Up @@ -432,6 +426,13 @@ export default class Parser {
variableName,
});
});
if (requireDeclarations.length) {
// Usage of space separated list is not recommended, as it only allows for global access
this.#context.addLintingMessage(this.#resourcePath,
MESSAGE.NO_LEGACY_TEMPLATE_REQUIRE_SYNTAX,
{moduleNames: attr.value}, attr.start
);
}
} else {
// Most common case: JSON-like representation
// e.g. core:require="{Helper: 'sap/ui/demo/todo/util/Helper'}"
Expand Down Expand Up @@ -606,4 +607,51 @@ export default class Parser {
return node;
}
}

_handleTemplatingNamespace(
tagName: string, namespace: Namespace, attributes: Map<string, AttributeDeclaration>, 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,
});
}
}
4 changes: 4 additions & 0 deletions src/linter/xmlTemplate/generator/AbstractGenerator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "_";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
sap.ui.define(["./BaseController"], function (BaseController) {
"use strict";

const TemplatingMainController = BaseController.extend("com.ui5.troublesome.app.controller.TemplatingMain", {
});
return TemplatingMainController;
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<mvc:View
controllerName="com.ui5.troublesome.app.controller.TemplatingMain"
xmlns="sap.m"
xmlns:core="sap.ui.core"
xmlns:layout="sap.ui.layout"
xmlns:mvc="sap.ui.core.mvc"
xmlns:template="http://schemas.sap.com/sapui5/extension/sap.ui.core.template/1">
<layout:VerticalLayout class="sapUiSmallMarginBottom">
<template:alias name=".formatParts" value="com.ui5.troublesome.app.helpers.Helper.formatParts">
<template:alias name=".id" value="com.ui5.troublesome.app.helpers.Helper.id">
<template:with path="meta>/" var="fieldGroup">
<template:with path="meta>entityType" helper="sap.ui.model.odata.AnnotationHelper.gotoEntityType" var="entityType">
<template:if test="{entityType>com.sap.vocabularies.UI.v1.LineItem}">
<Table headerText="Items" includeItemInSelection="true" mode="SingleSelectMaster"
selectionChange="onSelectionChange" items="{:= '{path:\'/' + ${meta>name} + '\', length: 5}' }">
<template:with path="entityType>com.sap.vocabularies.UI.v1.LineItem" var="target">
<core:Fragment fragmentName="com.ui5.troublesome.app.view.TemplatingTable" type="XML"/>
</template:with>
</Table>
</template:if>
</template:with>
</template:with>
</template:alias>
</template:alias>
</layout:VerticalLayout>
</mvc:View>
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<core:FragmentDefinition
xmlns="sap.m"
xmlns:core="sap.ui.core"
xmlns:template="http://schemas.sap.com/sapui5/extension/sap.ui.core.template/1">
<columns>
<template:repeat list="{target>}" var="field">
<template:if test="{field>Value}">
<Column>
<Text text="{path: 'field>Label', formatter: 'sap.ui.model.odata.AnnotationHelper.format'}"/>
</Column>
</template:if>
<template:if test="{field>Action}">
<Column demandPopin="true" minScreenWidth="1800px" popinDisplay="WithoutHeader">
<Text text="{path: 'field>Label', formatter: 'sap.ui.model.odata.AnnotationHelper.format'}"/>
</Column>
</template:if>
</template:repeat>
</columns>
<ColumnListItem>
<template:repeat list="{target>}" var="field">
<template:if test="{field>Value}">
<template:if test="{field>Target}">
<template:then>
<template:with path="field>Target" helper="sap.ui.model.odata.AnnotationHelper.resolvePath" var="entityType">
<Link text="{path: 'field>Value', formatter: 'sap.ui.model.odata.AnnotationHelper.format'}" press="onDetailsPressed">
<dependents>
<template:with path="entityType>com.sap.vocabularies.UI.v1.HeaderInfo" var="headerInfo">
<Popover binding="{path: 'field>Target', formatter: 'sap.ui.model.odata.AnnotationHelper.getNavigationPath'}"
title="{path: 'headerInfo>Title/Value', formatter: 'sap.ui.model.odata.AnnotationHelper.format'}">
</Popover>
</template:with>
</dependents>
</Link>
</template:with>
</template:then>
<template:else>
<Text text="{path: 'field>Value', formatter: 'sap.ui.model.odata.AnnotationHelper.format'}" />
</template:else>
</template:if>
</template:if>
<template:if test="{field>Action}">
<template:with path="field>Action" helper="sap.ui.model.odata.AnnotationHelper.gotoFunctionImport" var="function">
<template:if test="{function>com.sap.vocabularies.Common.v1.IsActionCritical}">
<template:then>
<Button text="{path: 'field>Label', formatter: 'sap.ui.model.odata.AnnotationHelper.format'}"
icon="{= ${function>com.sap.vocabularies.Common.v1.IsActionCritical/Bool} !== 'false' ? 'sap-icon://notification' : ''}" />
</template:then>
<template:else>
<Button text="{path: 'field>Label', formatter: 'sap.ui.model.odata.AnnotationHelper.format'}" />
</template:else>
</template:if>
</template:with>
</template:if>
</template:repeat>
</ColumnListItem>
</core:FragmentDefinition>
40 changes: 40 additions & 0 deletions test/fixtures/linter/rules/NoGlobals/XMLTemplatingV2.view.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<mvc:View
controllerName="sap.ui.core.sample.ViewTemplate.scenario.Detail"
template:require="{AH: 'sap/ui/model/odata/AnnotationHelper',
Helper: 'sap/ui/core/sample/ViewTemplate/scenario/Helper'}"
xmlns="sap.m"
xmlns:core="sap.ui.core"
xmlns:layout="sap.ui.layout"
xmlns:mvc="sap.ui.core.mvc"
xmlns:template="http://schemas.sap.com/sapui5/extension/sap.ui.core.template/1">

<!-- Global access of value function -->
<template:alias name="gotoEntityType" value="Helper.gotoEntityType">

<!-- Global access of value function -->
<template:alias name=".formatParts" value="sap.ui.core.sample.ViewTemplate.scenario.Helper.formatParts">

<!-- Negative test: Access via local name -->
<template:alias name=".id" value="Helper.id">

<!-- Global access of helper function -->
<template:with path="meta>entityType" helper="sap.ui.model.odata.AnnotationHelper.gotoEntityType" var="entityType">

<!-- Negative test: Access via local alias name -->
<template:with path="meta>entityType" helper="gotoEntityType" var="entityType2">

<template:if test="{entityType>com.sap.vocabularies.UI.v1.LineItem}">
<Table headerText="Items" includeItemInSelection="true" mode="SingleSelectMaster"
selectionChange="onSelectionChange" items="{:= '{path:\'/' + ${meta>name} + '\', length: 5}' }">
<template:with path="entityType>com.sap.vocabularies.UI.v1.LineItem" var="target">
<core:Fragment fragmentName="sap.ui.core.sample.ViewTemplate.scenario.Table" type="XML"/>
</template:with>
</Table>
</template:if>

</template:with>
</template:with>
</template:alias>
</template:alias>
</template:alias>
</mvc:View>
Loading