feat: Add type support for "byId" in controllers (#423)

This feature adds type definitions for `byId` within controllers so that
the usage of deprecated functionality can be detected when controls or
elements from the view are accessed by their ID.
Previously, only the base element class was known, which is still the
case as a fallback.

JIRA: CPOUI5FOUNDATION-843

src/linter/ui5Types/TypeLinter.ts

@@ -8,6 +8,7 @@ import path from "node:path/posix";
 import {AbstractAdapter} from "@ui5/fs";
 import {createAdapter, createResource} from "@ui5/fs/resourceFactory";
 import {loadApiExtract} from "../../utils/ApiExtract.js";
+import {CONTROLLER_BY_ID_DTS_PATH} from "../xmlTemplate/linter.js";
 
 const log = getLogger("linter:ui5Types:TypeLinter");
 
@@ -151,29 +152,38 @@ export default class TypeChecker {
 						resourcePath, fileContent, sourceMap);
 				}
 			}
+			// Although not being a typical transformed source, write out the byId dts file for debugging purposes
+			let byIdDts = files.get(CONTROLLER_BY_ID_DTS_PATH);
+			if (byIdDts) {
+				if (typeof byIdDts === "function") {
+					byIdDts = byIdDts();
+				}
+				await writeTransformedSources(process.env.UI5LINT_WRITE_TRANSFORMED_SOURCES,
+					CONTROLLER_BY_ID_DTS_PATH, byIdDts);
+			}
 		}
 	}
 }
 
 async function writeTransformedSources(fsBasePath: string,
 	originalResourcePath: string,
-	source: string, map: string | undefined) {
+	source: string, map?: string) {
 	const transformedWriter = createAdapter({
 		fsBasePath,
 		virBasePath: "/",
 	});
 
 	await transformedWriter.write(
 		createResource({
-			path: originalResourcePath + ".ui5lint.transformed.js",
+			path: originalResourcePath,
 			string: source,
 		})
 	);
 
 	if (map) {
 		await transformedWriter.write(
 			createResource({
-				path: originalResourcePath + ".ui5lint.transformed.js.map",
+				path: originalResourcePath + ".map",
 				string: JSON.stringify(JSON.parse(map), null, "\t"),
 			})
 		);

src/linter/xmlTemplate/ControllerByIdInfo.ts

@@ -0,0 +1,38 @@
+// Multiple views/fragments can use the same ID and link to the same controller,
+// so we need to store a set of module names for each ID.
+export type IdModulesMap = Map<string, Set<string>>;
+
+type ControllerElementsMap = Map<string, IdModulesMap>;
+
+export default class ControllerByIdInfo {
+	private map: ControllerElementsMap = new Map();
+
+	private getControllerMapping(controllerName: string) {
+		let controllerMap = this.map.get(controllerName);
+		if (!controllerMap) {
+			controllerMap = new Map();
+			this.map.set(controllerName, controllerMap);
+		}
+		return controllerMap;
+	}
+
+	public addMappings(controllerName: string, idModuleMap: Map<string, string>) {
+		// Do not add empty mappings
+		if (!idModuleMap.size) {
+			return;
+		}
+		const controllerMapping = this.getControllerMapping(controllerName);
+		for (const [id, module] of idModuleMap) {
+			let existingModules = controllerMapping.get(id);
+			if (!existingModules) {
+				existingModules = new Set();
+				controllerMapping.set(id, existingModules);
+			}
+			existingModules.add(module);
+		}
+	}
+
+	public getMappings() {
+		return this.map;
+	}
+}

src/linter/xmlTemplate/Parser.ts

@@ -9,6 +9,7 @@ import AbstractGenerator from "./generator/AbstractGenerator.js";
 import {getLogger} from "@ui5/logger";
 import {MESSAGE} from "../messages.js";
 import {ApiExtract} from "../../utils/ApiExtract.js";
+import ControllerByIdInfo from "./ControllerByIdInfo.js";
 const log = getLogger("linter:xmlTemplate:Parser");
 
 export type Namespace = string;
@@ -133,16 +134,18 @@ export default class Parser {
 	#generator: AbstractGenerator;
 	#apiExtract: ApiExtract;
 
-	constructor(resourceName: string, apiExtract: ApiExtract, context: LinterContext) {
+	constructor(
+		resourceName: string, apiExtract: ApiExtract, context: LinterContext, controllerByIdInfo: ControllerByIdInfo
+	) {
 		const xmlDocumentKind = determineDocumentKind(resourceName);
 		if (xmlDocumentKind === null) {
 			throw new Error(`Unknown document type for resource ${resourceName}`);
 		}
 		this.#resourceName = resourceName;
 		this.#xmlDocumentKind = xmlDocumentKind;
 		this.#generator = xmlDocumentKind === DocumentKind.View ?
-			new ViewGenerator(resourceName) :
-			new FragmentGenerator(resourceName);
+			new ViewGenerator(resourceName, controllerByIdInfo) :
+			new FragmentGenerator(resourceName, controllerByIdInfo);
 
 		this.#apiExtract = apiExtract;
 		this.#context = context;

src/linter/xmlTemplate/generator/AbstractGenerator.ts

@@ -1,3 +1,4 @@
+import ControllerByIdInfo from "../ControllerByIdInfo.js";
 import {
 	ControlDeclaration, RequireExpression, Position,
 } from "../Parser.js";
@@ -17,10 +18,13 @@ export default abstract class AbstractGenerator {
 	_imports = new Set<ImportStatement>();
 	_variableNames = new Set<string>();
 	_body: Writer;
+	_idToModule = new Map<string, string>();
+	_controllerByIdInfo: ControllerByIdInfo;
 
-	constructor(filePath: string) {
+	constructor(filePath: string, controllerByIdInfo: ControllerByIdInfo) {
 		const fileName = path.basename(filePath, ".xml");
 		this._body = new Writer(fileName + ".js", fileName + ".xml");
+		this._controllerByIdInfo = controllerByIdInfo;
 	}
 
 	// eslint-disable-next-line @typescript-eslint/no-unused-vars
@@ -31,9 +35,11 @@ export default abstract class AbstractGenerator {
 	writeControl(controlDeclaration: ControlDeclaration) {
 		const importVariableName = this._getUniqueVariableName(controlDeclaration.name);
 
+		const moduleName = controlDeclaration.namespace.replaceAll(".", "/") + `/${controlDeclaration.name}`;
+
 		// Add import
 		this._imports.add({
-			moduleName: controlDeclaration.namespace.replaceAll(".", "/") + `/${controlDeclaration.name}`,
+			moduleName,
 			variableName: importVariableName,
 			start: controlDeclaration.start,
 			end: controlDeclaration.end,
@@ -46,6 +52,13 @@ export default abstract class AbstractGenerator {
 		this._body.writeln(`new ${importVariableName}({`, controlDeclaration.start, controlDeclaration.end);
 		// Write properties
 		controlDeclaration.properties.forEach((attribute) => {
+			// Add mapping of id to module name so that specific byId lookup for controllers can be generated.
+			// This information can only be added to the ControllerByIdInfo once the controllerName is known,
+			// which happens last as outer nodes are visited last.
+			if (attribute.name === "id") {
+				this._idToModule.set(attribute.value, moduleName);
+			}
+
 			// TODO: Determine attribute type based on metadata and parse/write value accordingly
 			this._body.write(`    `);
 			this._body.writeln(

src/linter/xmlTemplate/generator/ControllerByIdDtsGenerator.ts

@@ -0,0 +1,70 @@
+import ControllerByIdInfo, {IdModulesMap} from "../ControllerByIdInfo.js";
+
+export class ControllerByIdDtsGenerator {
+	// Maps module names to local names
+	private imports = new Map<string, string>();
+
+	generate(controllerByIdInfo: ControllerByIdInfo) {
+		const mappings = controllerByIdInfo.getMappings();
+		if (!mappings.size) {
+			return null;
+		}
+		this.imports = new Map<string, string>();
+		this.addImport("sap/ui/core/mvc/View"); // View is needed for interface ControllerView
+		this.addImport("sap/ui/core/Element"); // Element is needed for byId fallback signature
+		let out = "";
+		mappings.forEach((idToModules, controllerName) => {
+			out += this.generateModuleDeclaration(controllerName, idToModules);
+		});
+		return this.generateCollectedImports() + out;
+	}
+
+	private generateCollectedImports() {
+		return Array.from(this.imports.entries())
+			.map(([moduleName, localName]) => `import ${localName} from "${moduleName}";`)
+			.join("\n") + "\n";
+	}
+
+	private generateByIdMapping(idToModules: IdModulesMap) {
+		let out = "interface ByIdMapping {\n";
+		idToModules.forEach((modules, id) => {
+			const localNames = Array.from(modules).map((moduleName: string) => this.addImport(moduleName));
+			out += `\t\t"${id}": ${localNames.join(" | ")};\n`;
+		});
+		out += "\t}";
+		return out;
+	}
+
+	private generateModuleDeclaration(controllerName: string, idToModules: IdModulesMap) {
+		const moduleName = controllerName.replace(/\./g, "/") + ".controller";
+
+		return `
+declare module "${moduleName}" {
+	${this.generateByIdMapping(idToModules)}
+	type ByIdFunction = {
+		<T extends keyof ByIdMapping>(sId: T): ByIdMapping[T];
+		(sId: string): ${this.imports.get("sap/ui/core/Element")};
+	}
+	interface ControllerView extends ${this.imports.get("sap/ui/core/mvc/View")} {
+		byId: ByIdFunction;
+	}
+	export default interface Controller {
+		byId: ByIdFunction;
+		getView(): ControllerView;
+	}
+}
+`;
+	}
+
+	private addImport(moduleName: string) {
+		const localName = this.getLocalModuleName(moduleName);
+		if (!this.imports.has(moduleName)) {
+			this.imports.set(moduleName, localName);
+		}
+		return localName;
+	}
+
+	private getLocalModuleName(moduleName: string) {
+		return moduleName.replace(/[/.]/g, "_");
+	}
+}

src/linter/xmlTemplate/generator/ViewGenerator.ts

@@ -3,6 +3,13 @@ import {ControlDeclaration} from "../Parser.js";
 
 export default class ViewGenerator extends AbstractGenerator {
 	writeRootControl(controlInfo: ControlDeclaration) {
+		controlInfo.properties.forEach((attribute) => {
+			if (attribute.name === "controllerName") {
+				// Outer nodes are visited last, so the controllerName is only known after
+				// all controls have been visited. Only then the collected mappings can be added.
+				this._controllerByIdInfo.addMappings(attribute.value, this._idToModule);
+			}
+		});
 		this._body.write(`export default `);
 		this.writeControl(controlInfo);
 	}

src/linter/xmlTemplate/linter.ts

@@ -2,12 +2,19 @@ import {Resource} from "@ui5/fs";
 import {createResource} from "@ui5/fs/resourceFactory";
 import transpileXml from "./transpiler.js";
 import {LinterParameters} from "../LinterContext.js";
+import ControllerByIdInfo from "./ControllerByIdInfo.js";
+import {ControllerByIdDtsGenerator} from "./generator/ControllerByIdDtsGenerator.js";
+
+// For usage in TypeLinter to write the file as part of the internal WRITE_TRANSFORMED_SOURCES debug mode
+export const CONTROLLER_BY_ID_DTS_PATH = "/types/@ui5/linter/virtual/ControllerById.d.ts";
 
 export default async function lintXml({filePathsWorkspace, workspace, context}: LinterParameters) {
 	const xmlResources = await filePathsWorkspace.byGlob("**/{*.view.xml,*.fragment.xml}");
 
+	const controllerByIdInfo = new ControllerByIdInfo();
+
 	await Promise.all(xmlResources.map(async (resource: Resource) => {
-		const res = await transpileXml(resource.getPath(), resource.getStream(), context);
+		const res = await transpileXml(resource.getPath(), resource.getStream(), context, controllerByIdInfo);
 		if (!res) {
 			return;
 		}
@@ -31,4 +38,15 @@ export default async function lintXml({filePathsWorkspace, workspace, context}:
 		await filePathsWorkspace.write(transpiledResourceSourceMap);
 		await workspace.write(transpiledResourceSourceMap);
 	}));
+
+	// Generate dts file with specific byId signatures for controllers based on view IDs
+	const controllerByIdDtsGenerator = new ControllerByIdDtsGenerator();
+	const controllerByIdDts = controllerByIdDtsGenerator.generate(controllerByIdInfo);
+	if (controllerByIdDts) {
+		const dtsResource = createResource({
+			path: CONTROLLER_BY_ID_DTS_PATH,
+			string: controllerByIdDts,
+		});
+		await workspace.write(dtsResource);
+	}
 }

src/linter/xmlTemplate/transpiler.ts

@@ -9,6 +9,7 @@ import {getLogger} from "@ui5/logger";
 import {createRequire} from "node:module";
 import {MESSAGE} from "../messages.js";
 import {loadApiExtract, ApiExtract} from "../../utils/ApiExtract.js";
+import ControllerByIdInfo from "./ControllerByIdInfo.js";
 const require = createRequire(import.meta.url);
 
 const log = getLogger("linter:xmlTemplate:transpiler");
@@ -17,12 +18,12 @@ let saxWasmBuffer: Buffer;
 let apiExtract: ApiExtract;
 
 export default async function transpileXml(
-	resourcePath: string, contentStream: ReadStream, context: LinterContext
+	resourcePath: string, contentStream: ReadStream, context: LinterContext, controllerByIdInfo: ControllerByIdInfo
 ): Promise<TranspileResult | undefined> {
 	await init();
 	try {
 		const taskEnd = taskStart("Transpile XML", resourcePath, true);
-		const res = await transpileXmlToJs(resourcePath, contentStream, context);
+		const res = await transpileXmlToJs(resourcePath, contentStream, context, controllerByIdInfo);
 		taskEnd();
 		if (!res.source) {
 			log.verbose(`XML transpiler returned no result for ${resourcePath}`);
@@ -56,9 +57,9 @@ async function init() {
 }
 
 async function transpileXmlToJs(
-	resourcePath: string, contentStream: ReadStream, context: LinterContext
+	resourcePath: string, contentStream: ReadStream, context: LinterContext, controllerByIdInfo: ControllerByIdInfo
 ): Promise<TranspileResult> {
-	const parser = new Parser(resourcePath, apiExtract, context);
+	const parser = new Parser(resourcePath, apiExtract, context, controllerByIdInfo);
 
 	// Initialize parser
 	const options = {highWaterMark: 32 * 1024}; // 32k chunks

test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/controller/Main.controller.js

@@ -4,6 +4,32 @@ sap.ui.define(["./BaseController", "sap/m/MessageBox"], function (BaseController
 	return BaseController.extend("com.ui5.troublesome.app.controller.Main", {
 		sayHello: function () {
 			MessageBox.show("Hello World!");
+		},
+
+		registerButtonEventHandlers() {
+			// this.byId and this.getView().byId should report the same issues
+			this.byId("helloButton").attachTap(function() {
+				console.log("Tapped");
+			});
+			this.getView().byId("helloButton").attachTap(function() {
+				console.log("Tapped");
+			});
+
+			// testButton exists in two views and could be a sap.m.Button or a sap.ui.commons.Button.
+			// The detection of deprecated button API depends requires TypeScript compliant probing (e.g. using "attachTap" in testButton).
+			// In any case, the detection of UI5 Base Control API should still work as both inherit from it.
+			const testButton = this.byId("testButton");
+			testButton.getBlocked(); // This should be reported
+			if ("attachTap" in testButton) {
+				// When probing for the existence of the method, the type can be determined and the issue should be reported
+				testButton.attachTap(function() {
+					console.log("Tapped");
+				});
+			}
+
+			// UI5 Element API should still be checked for unknown IDs
+			this.byId("unknown").prop("foo", "bar");
+			this.getView().byId("unknown").prop("foo", "bar");
 		}
 	});
 });

test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/DesktopMain.view.xml

@@ -0,0 +1,17 @@
+<!--
+	This file is a partial copy of Main.view.xml but uses other controls.
+	Its purpose is to cover the case where multiple views refer to
+	the same controller (controllerName) and contain controls with
+	the same ID but a different type. The resulting type for the
+	controller byId() call should be the union of the types.
+-->
+<mvc:View
+	controllerName="com.ui5.troublesome.app.controller.Main"
+	displayBlock="true"
+	xmlns="sap.ui.commons"
+	xmlns:mvc="sap.ui.core.mvc"
+	xmlns:core="sap.ui.core">
+
+	<Button id="testButton" />
+
+</mvc:View>

test/fixtures/linter/projects/com.ui5.troublesome.app/webapp/view/Main.view.xml

@@ -23,4 +23,6 @@
 		</buttons>
 	</MessagePage>
 
+	<Button id="testButton" />
+
 </mvc:View>