Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Detect inline XML fragments & views #519

Merged
merged 58 commits into from
Feb 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
975862a
refactor: Consider ManagedObject as base class for bind* & applySetti…
d3xter666 Feb 14, 2025
c8958ec
fix: Prevent false-positive "prefer-test-starter" in testsuite (follo…
matz3 Feb 14, 2025
c19bc93
test: Add test cases
d3xter666 Feb 3, 2025
8069210
test: Initial snapshots
d3xter666 Feb 3, 2025
8c22589
test: Update lint tests
d3xter666 Feb 4, 2025
b066384
refactor: Extract jsToXML extractor
d3xter666 Feb 5, 2025
548e919
test: Update tests and how snapshots are made
d3xter666 Feb 5, 2025
1c0c914
test: Update snapshots
d3xter666 Feb 5, 2025
d8ae4a1
fix: Correct mappings back to the root
d3xter666 Feb 6, 2025
ed14add
refactor: Adjust types
d3xter666 Feb 6, 2025
ee825ac
docs: Add comments to the code
d3xter666 Feb 6, 2025
94b33ac
test: Stabilize tests
d3xter666 Feb 6, 2025
ab36422
test: Fix OS slashes
d3xter666 Feb 6, 2025
d2ed744
refactor: Detect precisely the imports (in progress)
d3xter666 Feb 7, 2025
5a0b940
feat: Reuse program
d3xter666 Feb 7, 2025
c8f9901
fix: Eslint findings
d3xter666 Feb 7, 2025
f19262e
refactor: Migrate to the new LanguageService
d3xter666 Feb 7, 2025
c87b425
fix: Tests
d3xter666 Feb 7, 2025
27d4d52
refactor: Find original position
d3xter666 Feb 10, 2025
c9e6e0e
fix: Node + sourceMaps positional info
d3xter666 Feb 10, 2025
dffb735
fix: Merge conflicts
d3xter666 Feb 10, 2025
83eb9cc
refactor: Filter duplicate runs of XML linter
d3xter666 Feb 10, 2025
fcc982c
refactor: Tests
d3xter666 Feb 10, 2025
517f674
test: Update snapshots
d3xter666 Feb 10, 2025
bf6777c
refactor: Cleanups
d3xter666 Feb 10, 2025
dd0e673
refactor: Do correct rename
d3xter666 Feb 10, 2025
d653cd9
test: Update snapshots
d3xter666 Feb 10, 2025
07ee83f
feat: Add precise detection of XML from JS creation calls
d3xter666 Feb 10, 2025
2fd7c14
test: Sort messages as XML findings could be random
d3xter666 Feb 10, 2025
ed91d1d
fix: Add Fragment creation check
d3xter666 Feb 10, 2025
7f7de45
refactor: Address GH comments
d3xter666 Feb 11, 2025
efc3996
refactor: Renames
d3xter666 Feb 11, 2025
f17a046
refactor: Rename folder
d3xter666 Feb 11, 2025
2f1765c
refactor: Renames
d3xter666 Feb 11, 2025
576ed81
refactor: Improve perf by skipping iterations if no XML snippets are …
d3xter666 Feb 11, 2025
b77ba78
fix: Consider correct extension of the XML extract
d3xter666 Feb 11, 2025
f294ba4
refactor: Cleanup of invalid patterns
d3xter666 Feb 11, 2025
ca2d816
test: Add typescriptcase
d3xter666 Feb 11, 2025
978b675
refactor: Add strict checks for XML view type
d3xter666 Feb 11, 2025
984f904
fix: Delete snapshots for rename
d3xter666 Feb 11, 2025
6c59c66
test: Update snapshots
d3xter666 Feb 11, 2025
ae7aa41
fix: Col shifting for inline xml definitions
d3xter666 Feb 11, 2025
f6f19fb
test: Update snapshots
d3xter666 Feb 11, 2025
82dee17
refactor: Provide deterministic results for the lint output
d3xter666 Feb 12, 2025
bf57bf1
refactor: Allow for alternative Glob patterns within linters
d3xter666 Feb 12, 2025
1c42e39
refactor: Reuse CallExpression to extract Xml fragments
d3xter666 Feb 12, 2025
82bb718
fix: Provide dot namespace for xml extraction
d3xter666 Feb 12, 2025
444435b
fix: Do not extract from XML compiled resources
d3xter666 Feb 12, 2025
73c219e
refactor: Restore parts of the xml linter
d3xter666 Feb 12, 2025
58f27ca
refactor: Allow alternative pattern only for lintXml
d3xter666 Feb 13, 2025
b3a38f7
refactor: Reset message sorting
d3xter666 Feb 13, 2025
ec54413
fix: Improve type checks
d3xter666 Feb 13, 2025
06b8d86
test: Update snapshots
d3xter666 Feb 13, 2025
a3dfe15
fix: Merge conflicts
d3xter666 Feb 14, 2025
b6d0b40
fix: Merge conflicts
d3xter666 Feb 14, 2025
18d5dba
refactor: Always sort all messages to provide deterministic results
d3xter666 Feb 12, 2025
8437bbb
refactor: Always sort messages
d3xter666 Feb 13, 2025
3cd78b5
fix: Update snapshots
d3xter666 Feb 14, 2025
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
15 changes: 9 additions & 6 deletions src/linter/LinterContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {resolveLinks} from "../formatter/lib/resolveLinks.js";
import {LintMessageSeverity, MESSAGE, MESSAGE_INFO} from "./messages.js";
import {MessageArgs} from "./MessageArgs.js";
import {Directive} from "./ui5Types/directives.js";
import ts from "typescript";

export type FilePattern = string; // glob patterns
export type FilePath = string; // Platform-dependent path
Expand Down Expand Up @@ -97,6 +98,7 @@ export interface LintMetadata {
directives: Set<Directive>;
transformedImports: Map<string, Set<string>>;
xmlCompiledResource: string;
jsToXmlPosMapping: {pos: ts.LineAndCharacter; originalPath: string};
}

export default class LinterContext {
Expand Down Expand Up @@ -237,13 +239,18 @@ export default class LinterContext {
}

#getFilteredMessages(resourcePath: ResourcePath): RawLintMessage[] {
const sortFn = (a: RawLintMessage, b: RawLintMessage) => {
const aPos = a.position ?? {line: 0, column: 0};
const bPos = b.position ?? {line: 0, column: 0};
return aPos.line === bPos.line ? aPos.column - bPos.column : aPos.line - bPos.line;
};
const rawMessages = this.#rawMessages.get(resourcePath);
if (!rawMessages) {
return [];
}
const metadata = this.#metadata.get(resourcePath);
if (!metadata?.directives?.size) {
return rawMessages;
return rawMessages.sort(sortFn);
}

const filteredMessages: RawLintMessage[] = [];
Expand All @@ -255,11 +262,7 @@ export default class LinterContext {
return false;
}
return true;
}).sort((a, b) => {
const aPos = a.position!;
const bPos = b.position!;
return aPos.line === bPos.line ? aPos.column - bPos.column : aPos.line - bPos.line;
});
}).sort(sortFn);

// Filter messages based on directives
let directiveStack: Directive[] = [];
Expand Down
82 changes: 77 additions & 5 deletions src/linter/ui5Types/SourceFileLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,22 @@ import {
} from "./utils.js";
import {taskStart} from "../../utils/perf.js";
import {getPositionsForNode} from "../../utils/nodePosition.js";
import {TraceMap} from "@jridgewell/trace-mapping";
import {SourceMapInput, TraceMap, originalPositionFor} from "@jridgewell/trace-mapping";
import type {ApiExtract} from "../../utils/ApiExtract.js";
import {findDirectives} from "./directives.js";
import BindingLinter from "../binding/BindingLinter.js";
import {RequireDeclaration} from "../xmlTemplate/Parser.js";
import BindingParser, {PropertyBindingInfo} from "../binding/lib/BindingParser.js";
import {createResource} from "@ui5/fs/resourceFactory";
import {AbstractAdapter} from "@ui5/fs";

const log = getLogger("linter:ui5Types:SourceFileLinter");

const QUNIT_FILE_EXTENSION = /\.qunit\.(js|ts)$/;

// This is the same check as in the framework and prevents false-positives
// https://github.com/SAP/openui5/blob/32c21c33d9dc29a32bf7ee7f41d7bae23dcf086b/src/sap.ui.core/src/sap/ui/test/starter/_utils.js#L287
const VALID_TESTSUITE = /^\/testsuite(?:\.[a-z][a-z0-9-]*)*\.qunit\.(?:js|ts)$/;
const VALID_TESTSUITE = /\/testsuite(?:\.[a-z][a-z0-9-]*)*\.qunit\.(?:js|ts)$/;

const DEPRECATED_VIEW_TYPES = ["JS", "JSON", "HTML", "Template"];

Expand Down Expand Up @@ -67,6 +69,7 @@ export default class SourceFileLinter {
#isComponent: boolean;
#hasTestStarterFindings: boolean;
#metadata: LintMetadata;
#xmlContents: {xml: string; pos: ts.LineAndCharacter; viewType: "fragment" | "view"}[];

constructor(
private context: LinterContext,
Expand All @@ -77,6 +80,8 @@ export default class SourceFileLinter {
private reportCoverage = false,
private messageDetails = false,
private apiExtract: ApiExtract,
private filePathsWorkspace: AbstractAdapter,
private workspace: AbstractAdapter,
private manifestContent?: string
) {
this.#reporter = new SourceFileReporter(context, resourcePath,
Expand All @@ -86,6 +91,7 @@ export default class SourceFileLinter {
this.#isComponent = this.#fileName === "Component.js" || this.#fileName === "Component.ts";
this.#hasTestStarterFindings = false;
this.#metadata = this.context.getMetadata(this.resourcePath);
this.#xmlContents = [];
}

// eslint-disable-next-line @typescript-eslint/require-await
Expand All @@ -103,6 +109,21 @@ export default class SourceFileLinter {
this.#reportTestStarter(this.sourceFile);
}

let i = 0;
for (const xmlContent of this.#xmlContents) {
const fileName = `${this.sourceFile.fileName.replace(/(\.js|\.ts)$/, "")}.inline-${++i}.${xmlContent.viewType}.xml`;
const metadata = this.context.getMetadata(fileName);
const newResource = createResource({path: fileName, string: xmlContent.xml});
metadata.jsToXmlPosMapping = {
pos: xmlContent.pos,
originalPath: this.sourceFile.fileName,
};
await Promise.all([
await this.filePathsWorkspace.write(newResource),
await this.workspace.write(newResource),
]);
}

this.#reporter.deduplicateMessages();
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
Expand Down Expand Up @@ -736,7 +757,7 @@ export default class SourceFileLinter {
});
}

extractNamespace(node: ts.PropertyAccessExpression | ts.ElementAccessExpression): string {
extractNamespace(node: ts.PropertyAccessExpression | ts.ElementAccessExpression | ts.CallExpression): string {
const propAccessChain: string[] = [];
propAccessChain.push(node.expression.getText());

Expand Down Expand Up @@ -842,15 +863,15 @@ export default class SourceFileLinter {
this.#reportTestStarter(node);
} else if (symbolName === "applySettings" &&
nodeType.symbol?.declarations?.some((declaration) =>
this.isUi5ClassDeclaration(declaration, "sap/ui/core/Control"))) {
this.isUi5ClassDeclaration(declaration, "sap/ui/base/ManagedObject"))) {
this.#analyzeNewAndApplySettings(node);
} else if (["bindProperty", "bindAggregation"].includes(symbolName) &&
moduleName === "sap/ui/base/ManagedObject" &&
node.arguments[1] && ts.isObjectLiteralExpression(node.arguments[1])) {
this.#analyzePropertyBindings(node.arguments[1], ["type", "formatter"]);
} else if (symbolName.startsWith("bind") &&
nodeType.symbol?.declarations?.some((declaration) =>
this.isUi5ClassDeclaration(declaration, "sap/ui/core/Control")) &&
this.isUi5ClassDeclaration(declaration, "sap/ui/base/ManagedObject")) &&
node.arguments[0] && ts.isObjectLiteralExpression(node.arguments[0])) {
// Setting names in UI5 are case sensitive. So, we're not sure of the exact name of the property.
// Check decapitalized version of the property name as well.
Expand All @@ -860,6 +881,20 @@ export default class SourceFileLinter {
if (this.#isPropertyBinding(node, [propName, alternativePropName])) {
this.#analyzePropertyBindings(node.arguments[0], ["type", "formatter"]);
}
} else if (["view", "xmlview", "fragment", "xmlfragment"].includes(symbolName)) {
const namespace = this.extractNamespace(node)
.replace(/\[("|'|`)*/g, ".").replace(/("|'|`)*\]/g, "");
const typeProperty = node?.arguments?.[0] && ts.isObjectLiteralExpression(node.arguments[0]) &&
getPropertyAssignmentInObjectLiteralExpression("type", node.arguments[0]);
if (namespace === `sap.ui.${symbolName}` &&
(!typeProperty ||
(ts.isStringLiteralLike(typeProperty.initializer) && typeProperty.initializer.text === "XML"))
) {
const viewType = ["fragment", "xmlfragment"].includes(symbolName) ? "fragment" : "view";
this.#extractXmlFromJs(node, viewType);
}
} else if (symbolName === "create" && moduleName === "sap/ui/core/mvc/XMLView") {
this.#extractXmlFromJs(node, "view");
}
}

Expand Down Expand Up @@ -1003,6 +1038,38 @@ export default class SourceFileLinter {
}
}

#extractXmlFromJs(node: ts.CallExpression, viewType: "view" | "fragment" = "view") {
if (!node.arguments.length || !ts.isObjectLiteralExpression(node.arguments[0]) ||
// xmlCompiledResource is an XML file, so we don't need to do extraction here
this.#metadata?.xmlCompiledResource) {
return;
}

node.arguments[0].properties.forEach((prop) => {
if (ts.isPropertyAssignment(prop) &&
(ts.isIdentifier(prop.name) || ts.isStringLiteralLike(prop.name)) &&
ts.isStringLiteralLike(prop.initializer)) {
const sourceMapJson = this.sourceMaps?.get(this.sourceFile.fileName);
const traceMap = sourceMapJson ?
new TraceMap(JSON.parse(sourceMapJson) as SourceMapInput) :
undefined;
const pos = this.sourceFile.getLineAndCharacterOfPosition(prop.initializer.pos);
const posInSource = traceMap ?
originalPositionFor(traceMap, {line: pos.line, column: pos.character}) :
null;
const originalPos = posInSource ?
{line: posInSource.line ?? 0, character: posInSource.column ?? 0} :
pos;

this.#xmlContents.push({
xml: prop.initializer.text,
pos: originalPos,
viewType,
});
}
});
}

#analyzeParametersGetCall(node: ts.CallExpression) {
if (node.arguments.length && ts.isObjectLiteralExpression(node.arguments[0])) {
// Non-deprecated usage
Expand Down Expand Up @@ -1143,6 +1210,9 @@ export default class SourceFileLinter {
typeValue,
}, node);
}
if (typeValue === "XML") {
this.#extractXmlFromJs(node, "view");
}
}
}

Expand Down Expand Up @@ -1170,6 +1240,8 @@ export default class SourceFileLinter {
typeValue,
}, node);
}
} else if (typeValue === "XML" && symbolName === "load") {
this.#extractXmlFromJs(node, "fragment");
}
}
}
Expand Down
47 changes: 43 additions & 4 deletions src/linter/ui5Types/TypeLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +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";
import lintXml, {CONTROLLER_BY_ID_DTS_PATH} from "../xmlTemplate/linter.js";
import type SharedLanguageService from "./SharedLanguageService.js";

const log = getLogger("linter:ui5Types:TypeLinter");
Expand Down Expand Up @@ -113,11 +113,11 @@ export default class TypeChecker {
this.#sharedLanguageService.acquire(host);

const createProgramDone = taskStart("ts.createProgram", undefined, true);
const program = this.#sharedLanguageService.getProgram();
let program = this.#sharedLanguageService.getProgram();
createProgramDone();

const getTypeCheckerDone = taskStart("program.getTypeChecker", undefined, true);
const checker = program.getTypeChecker();
let checker = program.getTypeChecker();
getTypeCheckerDone();

const apiExtract = await loadApiExtract();
Expand Down Expand Up @@ -146,12 +146,51 @@ export default class TypeChecker {
this.#context, sourceFile.fileName,
sourceFile, sourceMaps,
checker, reportCoverage, messageDetails,
apiExtract, manifestContent
apiExtract, this.#filePathsWorkspace, this.#workspace, manifestContent
);
await linter.lint();
linterDone();
}
}

// Will eventually produce new JS files for XML views and fragments
await lintXml({
filePathsWorkspace: this.#filePathsWorkspace,
workspace: this.#workspace,
context: this.#context,
altGlob: "**/*.inline-*.{view,fragment}.xml",
});
const virtualXMLResources =
await this.#filePathsWorkspace.byGlob("/**/*.inline-*.{view,fragment}{.js,.ts,.js.map}");
if (virtualXMLResources.length > 0) {
for (const resource of virtualXMLResources) {
const resourcePath = resource.getPath();
if (resourcePath.endsWith(".js.map")) {
sourceMaps.set(
// Remove ".map" from path to have it reflect the associated source path
resourcePath.slice(0, -4),
await resource.getString()
);
} else {
files.set(resourcePath, await resource.getString());
}
}
program = this.#sharedLanguageService.getProgram();
checker = program.getTypeChecker();
for (const sourceFile of program.getSourceFiles()) {
if (!sourceFile.isDeclarationFile && /\.inline-[0-9]+\.(view|fragment)\.js/.exec(sourceFile.fileName)) {
const linterDone = taskStart("Type-check resource", sourceFile.fileName, true);
const linter = new SourceFileLinter(
this.#context, sourceFile.fileName,
sourceFile, sourceMaps,
checker, reportCoverage, messageDetails,
apiExtract, this.#filePathsWorkspace, this.#workspace
);
await linter.lint();
linterDone();
}
}
}
typeCheckDone();

this.#sharedLanguageService.release();
Expand Down
40 changes: 37 additions & 3 deletions src/linter/xmlTemplate/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ import transpileXml from "./transpiler.js";
import {LinterParameters} from "../LinterContext.js";
import ControllerByIdInfo from "./ControllerByIdInfo.js";
import {ControllerByIdDtsGenerator} from "./generator/ControllerByIdDtsGenerator.js";
import {decodedMap, DecodedSourceMap, SourceMapInput, TraceMap} from "@jridgewell/trace-mapping";

// 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}");
export default async function lintXml({filePathsWorkspace, workspace, context, altGlob}:
LinterParameters & {altGlob?: string}) {
altGlob = altGlob ?? "**/{*.view.xml,*.fragment.xml}";
const xmlResources = await filePathsWorkspace.byGlob(altGlob);

const controllerByIdInfo = new ControllerByIdInfo();

Expand All @@ -21,6 +24,20 @@ export default async function lintXml({filePathsWorkspace, workspace, context}:
const {source, map} = res;
const resourcePath = resource.getPath();

let xmlFromJsResourceMap;
const metadata = context.getMetadata(resourcePath);
if (metadata.jsToXmlPosMapping) {
xmlFromJsResourceMap = JSON.parse(map) as DecodedSourceMap;
const {pos} = metadata.jsToXmlPosMapping;
// If it's an XML snippet extracted from a JS file, adjust the source map positions
// as these positions are relative to the extracted string, not to the real position in the JS file.
// Add that missing line shift from the original JS file to the source map.
xmlFromJsResourceMap = fixSourceMapIndices(xmlFromJsResourceMap, pos.line, pos.character);
// Replace the name of the source file in the source map with the original JS file name,
// so that reporter will lead to the original source file.
xmlFromJsResourceMap.sources.splice(0, 1, metadata.jsToXmlPosMapping.originalPath.split("/").pop() ?? null);
}

// Write transpiled resource to workspace
// TODO: suffix name to prevent clashes with existing files?
const jsPath = resourcePath.replace(/\.xml$/, ".js");
Expand All @@ -30,7 +47,7 @@ export default async function lintXml({filePathsWorkspace, workspace, context}:
});
const transpiledResourceSourceMap = createResource({
path: jsPath + ".map",
string: map,
string: xmlFromJsResourceMap ? JSON.stringify(xmlFromJsResourceMap) : map,
});

await filePathsWorkspace.write(transpiledResource);
Expand All @@ -54,3 +71,20 @@ export default async function lintXml({filePathsWorkspace, workspace, context}:
await workspace.write(dtsResource);
}
}

function fixSourceMapIndices(map: SourceMapInput, lineShift = 0, columnShift = 0): DecodedSourceMap {
const decodedTraceMap = decodedMap(new TraceMap(map));
const {mappings} = decodedTraceMap;

const newMappings = mappings.map((segment) =>
segment.map(([genCol, srcIndex, origLine, origCol, nameIndex]) => {
const calculatedColShift = (origLine === 0 ? columnShift : 0);
return (srcIndex !== undefined ?
[genCol + calculatedColShift, srcIndex, origLine! + lineShift,
origCol! + calculatedColShift, nameIndex ?? 0] :
[genCol + calculatedColShift]);
})
);

return {...decodedTraceMap, mappings: newMappings as DecodedSourceMap["mappings"]};
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
sap.ui.define(function () {
sap.ui.define(["sap/ui/core/Core"], function (Core) {

function waitForCore(callback) {
// Usage of Core.ready / Core.attachInit in a test starter testsuite
// should not cause a false-positive for "prefer-test-starter"
if (Core.ready) {
Core.ready().then(callback);
} else {
Core.attachInit(callback);
}
}

"use strict";
return {
name: "QUnit test suite for sap.f",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
sap.ui.define(["sap/ui/core/Core"], function(Core) {
"use strict";

async function waitForCore(callback) {
function waitForCore(callback) {
// Usage of Core.ready / Core.attachInit in a test starter testsuite
// should not cause a false-positive for "prefer-test-starter"
if (Core.ready) {
Expand Down
Loading