diff --git a/src/linter/LinterContext.ts b/src/linter/LinterContext.ts index 05840a79b..d3f1fb65f 100644 --- a/src/linter/LinterContext.ts +++ b/src/linter/LinterContext.ts @@ -3,6 +3,7 @@ import {createReader} from "@ui5/fs/resourceFactory"; 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"; export type FilePattern = string; // glob patterns export type FilePath = string; // Platform-dependent path @@ -94,7 +95,7 @@ export interface LintMetadata { // TODO: Use this to store information shared across linters, // such as the async flag state in manifest.json which might be relevant // when parsing the Component.js - _todo: string; + directives: Set; } export default class LinterContext { diff --git a/src/linter/ui5Types/SourceFileLinter.ts b/src/linter/ui5Types/SourceFileLinter.ts index 6a5c084f2..3e66e4b7a 100644 --- a/src/linter/ui5Types/SourceFileLinter.ts +++ b/src/linter/ui5Types/SourceFileLinter.ts @@ -10,6 +10,7 @@ import {getPropertyName} from "./utils.js"; import {taskStart} from "../../utils/perf.js"; import {getPositionsForNode} from "../../utils/nodePosition.js"; import {TraceMap} from "@jridgewell/trace-mapping"; +import {findDirectives} from "./directives.js"; const log = getLogger("linter:ui5Types:SourceFileLinter"); @@ -81,6 +82,12 @@ export default class SourceFileLinter { // eslint-disable-next-line @typescript-eslint/require-await async lint() { try { + const metadata = this.#context.getMetadata(this.#resourcePath); + if (!metadata.directives) { + // Directives might have already been extracted by the amd transpiler + // This is done since the transpile process might loose comments + findDirectives(this.#sourceFile, metadata); + } this.visitNode(this.#sourceFile); this.#reporter.deduplicateMessages(); } catch (err) { diff --git a/src/linter/ui5Types/TypeLinter.ts b/src/linter/ui5Types/TypeLinter.ts index c292f6a74..9974e70db 100644 --- a/src/linter/ui5Types/TypeLinter.ts +++ b/src/linter/ui5Types/TypeLinter.ts @@ -92,7 +92,7 @@ export default class TypeChecker { } } - const host = await createVirtualCompilerHost(this.#compilerOptions, files, sourceMaps); + const host = await createVirtualCompilerHost(this.#compilerOptions, files, sourceMaps, this.#context); const createProgramDone = taskStart("ts.createProgram", undefined, true); const program = ts.createProgram(pathsToLint, this.#compilerOptions, host); diff --git a/src/linter/ui5Types/amdTranspiler/transpiler.ts b/src/linter/ui5Types/amdTranspiler/transpiler.ts index aead940cb..0331161e7 100644 --- a/src/linter/ui5Types/amdTranspiler/transpiler.ts +++ b/src/linter/ui5Types/amdTranspiler/transpiler.ts @@ -1,7 +1,8 @@ import ts from "typescript"; +import path from "node:path/posix"; import {getLogger} from "@ui5/logger"; import {taskStart} from "../../../utils/perf.js"; -import {TranspileResult} from "../../LinterContext.js"; +import LinterContext, {TranspileResult} from "../../LinterContext.js"; import {createTransformer} from "./tsTransformer.js"; import {UnsupportedModuleError} from "./util.js"; @@ -49,9 +50,11 @@ function createProgram(inputFileNames: string[], host: ts.CompilerHost): ts.Prog return ts.createProgram(inputFileNames, compilerOptions, host); } -export default function transpileAmdToEsm(fileName: string, content: string, strict?: boolean): TranspileResult { +export default function transpileAmdToEsm( + resourcePath: string, content: string, context: LinterContext, strict?: boolean +): TranspileResult { // This is heavily inspired by the TypesScript "transpileModule" API - + const fileName = path.basename(resourcePath); const taskDone = taskStart("Transpiling AMD to ESM", fileName, true); const sourceFile = ts.createSourceFile( fileName, @@ -71,7 +74,7 @@ export default function transpileAmdToEsm(fileName: string, content: string, str const program = createProgram([fileName], compilerHost); const transformers: ts.CustomTransformers = { - before: [createTransformer(program)], + before: [createTransformer(program, resourcePath, context)], }; try { diff --git a/src/linter/ui5Types/amdTranspiler/tsTransformer.ts b/src/linter/ui5Types/amdTranspiler/tsTransformer.ts index 19dab9ed4..6473d1bc6 100644 --- a/src/linter/ui5Types/amdTranspiler/tsTransformer.ts +++ b/src/linter/ui5Types/amdTranspiler/tsTransformer.ts @@ -9,6 +9,8 @@ import replaceNodeInParent, {NodeReplacement} from "./replaceNodeInParent.js"; import {toPosStr, UnsupportedModuleError} from "./util.js"; import rewriteExtendCall, {UnsupportedExtendCall} from "./rewriteExtendCall.js"; import insertNodesInParent from "./insertNodesInParent.js"; +import LinterContext from "../../LinterContext.js"; +import {findDirectives} from "../directives.js"; const log = getLogger("linter:ui5Types:amdTranspiler:TsTransformer"); @@ -44,21 +46,23 @@ function isBlockLike(node: ts.Node): node is ts.BlockLike { * error is thrown. In that case, the rest of the module is still processed. However it's possible that the result * will be equal to the input. */ -export function createTransformer(program: ts.Program): ts.TransformerFactory { - return function transformer(context: ts.TransformationContext) { +export function createTransformer( + program: ts.Program, resourcePath: string, context: LinterContext +): ts.TransformerFactory { + return function transformer(tContext: ts.TransformationContext) { return (sourceFile: ts.SourceFile): ts.SourceFile => { - return transform(program, sourceFile, context); + return transform(program, sourceFile, tContext, resourcePath, context); }; }; } function transform( - program: ts.Program, sourceFile: ts.SourceFile, context: ts.TransformationContext + program: ts.Program, sourceFile: ts.SourceFile, tContext: ts.TransformationContext, resourcePath: string, + context: LinterContext ): ts.SourceFile { - const resourcePath = sourceFile.fileName; log.verbose(`Transforming ${resourcePath}`); const checker = program.getTypeChecker(); - const {factory: nodeFactory} = context; + const {factory: nodeFactory} = tContext; const moduleDefinitions: ModuleDefinition[] = []; // TODO: Filter duplicate imports, maybe group by module definition const requireImports: ts.ImportDeclaration[] = []; @@ -96,9 +100,12 @@ function transform( insertions.push(nodeToBeInserted); } + const metadata = context.getMetadata(resourcePath); + findDirectives(sourceFile, metadata); + // Visit the AST depth-first and collect module definitions function visit(nodeIn: ts.Node): ts.VisitResult { - const node = ts.visitEachChild(nodeIn, visit, context); + const node = ts.visitEachChild(nodeIn, visit, tContext); if (ts.isCallExpression(node) && ts.isPropertyAccessExpression(node.expression)) { if (matchPropertyAccessExpression(node.expression, "sap.ui.define")) { @@ -346,7 +353,7 @@ function transform( node = replaceNodeInParent(node, replacement, nodeFactory); } } - return ts.visitEachChild(node, applyModifications, context); + return ts.visitEachChild(node, applyModifications, tContext); } processedSourceFile = ts.visitNode(processedSourceFile, applyModifications) as ts.SourceFile; diff --git a/src/linter/ui5Types/directives.ts b/src/linter/ui5Types/directives.ts new file mode 100644 index 000000000..bf7825dda --- /dev/null +++ b/src/linter/ui5Types/directives.ts @@ -0,0 +1,98 @@ +import ts from "typescript"; +import {LintMetadata} from "../LinterContext.js"; + +export function findDirectives(sourceFile: ts.SourceFile, metadata: LintMetadata) { + metadata.directives = new Set(); + + const possibleDirectives = collectPossibleDirectives(sourceFile); + if (possibleDirectives.size === 0) { + return; + } + const sourceText = sourceFile.getFullText(); + + traverseAndFindDirectives(sourceFile, sourceText, possibleDirectives, metadata.directives); +} + +function traverseAndFindDirectives( + node: ts.Node, sourceText: string, possibleDirectives: Set, confirmedDirectives: Set +) { + findDirectivesAroundNode(node, sourceText, possibleDirectives, confirmedDirectives); + node.getChildren().forEach((child) => { + traverseAndFindDirectives(child, sourceText, possibleDirectives, confirmedDirectives); + }); +} + +export function findDirectivesAroundNode( + node: ts.Node, sourceText: string, possibleDirectives: Set, confirmedDirectives: Set +) { + /* + // This is a comment + // ui5-lint-disable + myCallExpression() + // ui5-lint-enable + // This is a comment + */ + for (const directive of possibleDirectives) { + if (directive.pos >= node.getFullStart() && directive.pos + directive.length < node.getStart()) { + const leadingComments = ts.getLeadingCommentRanges(sourceText, node.getFullStart()); + if (leadingComments?.length) { + leadingComments.some((comment) => { + if (comment.pos === directive.pos) { + possibleDirectives.delete(directive); + confirmedDirectives.add(directive); + return true; + } + return false; + }); + break; + } + } else if (directive.pos > node.getEnd() && directive.pos + directive.length <= node.getEnd()) { + const trailingComments = ts.getTrailingCommentRanges(sourceText, node.getEnd()); + if (trailingComments?.length) { + trailingComments.some((comment) => { + if (comment.pos === directive.pos) { + possibleDirectives.delete(directive); + confirmedDirectives.add(directive); + return true; + } + return false; + }); + break; + } + } + } +} + +/* Match things like: + ui5-lint-disable-next-line no-deprecated-api no-global + ui5-lint-disable + no-deprecated-api + no-global + ui5-lint-enable-next-line +*/ +const disableCommentRegex = /\/[/*]\s*ui5lint-(enable|disable)((?:-next)?-line)?((?:\s+[\w-]+,)*(?:\s+[\w-]+))?\s*(?:\*\/|$)/mg; + +export interface Directive { + action: string; + isLine: boolean; + isNextLine: boolean; + ruleNames: string[]; + pos: number; + length: number; +} + +export function collectPossibleDirectives(sourceFile: ts.SourceFile) { + const text = sourceFile.getFullText(); + let match; + const comments = new Set(); + while ((match = disableCommentRegex.exec(text)) !== null) { + const [, action, nextLineOrLine, rules] = match; + const pos = match.index; + const length = match[0].length; + const ruleNames = rules?.trim().split(",") ?? []; + const isLine = nextLineOrLine === "-line"; + const isNextLine = nextLineOrLine === "-next-line"; + comments.add({action, isLine, isNextLine, ruleNames, pos, length}); + } + return comments; +} diff --git a/src/linter/ui5Types/host.ts b/src/linter/ui5Types/host.ts index 96cff16a9..ffeffeb75 100644 --- a/src/linter/ui5Types/host.ts +++ b/src/linter/ui5Types/host.ts @@ -4,7 +4,7 @@ import posixPath from "node:path/posix"; import fs from "node:fs/promises"; import {createRequire} from "node:module"; import transpileAmdToEsm from "./amdTranspiler/transpiler.js"; -import {ResourcePath} from "../LinterContext.js"; +import LinterContext, {ResourcePath} from "../LinterContext.js"; import {getLogger} from "@ui5/logger"; const log = getLogger("linter:ui5Types:host"); const require = createRequire(import.meta.url); @@ -65,7 +65,8 @@ export type FileContents = Map string)>; export async function createVirtualCompilerHost( options: ts.CompilerOptions, - files: FileContents, sourceMaps: FileContents + files: FileContents, sourceMaps: FileContents, + context: LinterContext ): Promise { const silly = log.isLevelEnabled("silly"); @@ -113,25 +114,25 @@ export async function createVirtualCompilerHost( } } - function getFile(fileName: string): string | undefined { + function getFile(resourcePath: string): string | undefined { // NOTE: This function should be kept in sync with "fileExists" - if (files.has(fileName)) { - let fileContent = files.get(fileName); + if (files.has(resourcePath)) { + let fileContent = files.get(resourcePath); if (typeof fileContent === "function") { fileContent = fileContent(); } - if (fileContent && fileName.endsWith(".js") && !sourceMaps.get(fileName)) { + if (fileContent && resourcePath.endsWith(".js") && !sourceMaps.get(resourcePath)) { // No source map indicates no transpilation was done yet - const res = transpileAmdToEsm(path.basename(fileName), fileContent); - files.set(fileName, res.source); - sourceMaps.set(fileName, res.map); + const res = transpileAmdToEsm(resourcePath, fileContent, context); + files.set(resourcePath, res.source); + sourceMaps.set(resourcePath, res.map); fileContent = res.source; } return fileContent; } - if (fileName.startsWith("/types/")) { - const fsPath = mapToTypePath(fileName); + if (resourcePath.startsWith("/types/")) { + const fsPath = mapToTypePath(resourcePath); if (fsPath) { return ts.sys.readFile(fsPath); } diff --git a/test/fixtures/linter/rules/Directives/Directives.js b/test/fixtures/linter/rules/Directives/Directives.js index 7e01c3794..fba174a86 100644 --- a/test/fixtures/linter/rules/Directives/Directives.js +++ b/test/fixtures/linter/rules/Directives/Directives.js @@ -94,4 +94,11 @@ sap.ui.define([ blocked: true, // REPORT: Property "blocked" is deprecated tap: () => console.log("Tapped") // REPORT: Event "tap" is deprecated }); + return; + // ui5lint-disable +}); + +new sap.m.Button({ // IGNORE: Global variable "sap" + blocked: true, // IGNORE: Property "blocked" is deprecated + tap: () => console.log("Tapped") // IGNORE: Event "tap" is deprecated }); diff --git a/test/lib/linter/rules/snapshots/Directives.ts.md b/test/lib/linter/rules/snapshots/Directives.ts.md index bc14e3bee..ab79a35af 100644 --- a/test/lib/linter/rules/snapshots/Directives.ts.md +++ b/test/lib/linter/rules/snapshots/Directives.ts.md @@ -140,7 +140,7 @@ Generated by [AVA](https://avajs.dev). line: 51, message: 'Use of deprecated property \'webview\'', messageDetails: 'Deprecated test message', - ruleId: 'no-deprecated-property', + ruleId: 'no-deprecated-api', severity: 2, }, { @@ -148,7 +148,7 @@ Generated by [AVA](https://avajs.dev). line: 53, message: 'Use of deprecated property \'webview\'', messageDetails: 'Deprecated test message', - ruleId: 'no-deprecated-property', + ruleId: 'no-deprecated-api', severity: 2, }, { @@ -156,7 +156,7 @@ Generated by [AVA](https://avajs.dev). line: 55, message: 'Use of deprecated property \'webview\'', messageDetails: 'Deprecated test message', - ruleId: 'no-deprecated-property', + ruleId: 'no-deprecated-api', severity: 2, }, { @@ -164,7 +164,7 @@ Generated by [AVA](https://avajs.dev). line: 58, message: 'Use of deprecated property \'AnimationMode\'', messageDetails: 'Deprecated test message', - ruleId: 'no-deprecated-property', + ruleId: 'no-deprecated-api', severity: 2, }, { @@ -172,7 +172,7 @@ Generated by [AVA](https://avajs.dev). line: 61, message: 'Use of deprecated property \'MessageType\'', messageDetails: 'Deprecated test message', - ruleId: 'no-deprecated-property', + ruleId: 'no-deprecated-api', severity: 2, }, { @@ -180,7 +180,7 @@ Generated by [AVA](https://avajs.dev). line: 64, message: 'Use of deprecated property \'MessageType\'', messageDetails: 'Deprecated test message', - ruleId: 'no-deprecated-property', + ruleId: 'no-deprecated-api', severity: 2, }, { @@ -220,7 +220,7 @@ Generated by [AVA](https://avajs.dev). line: 79, message: 'Use of deprecated property \'Date\'', messageDetails: 'Deprecated test message', - ruleId: 'no-deprecated-property', + ruleId: 'no-deprecated-api', severity: 2, }, { @@ -235,6 +235,7 @@ Generated by [AVA](https://avajs.dev). column: 6, line: 87, message: 'Access of global variable \'sap\' (sap.m.Button)', + messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)', ruleId: 'no-globals', severity: 2, }, @@ -242,6 +243,7 @@ Generated by [AVA](https://avajs.dev). column: 6, line: 93, message: 'Access of global variable \'sap\' (sap.m.Button)', + messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)', ruleId: 'no-globals', severity: 2, }, diff --git a/test/lib/linter/rules/snapshots/Directives.ts.snap b/test/lib/linter/rules/snapshots/Directives.ts.snap index b56b50101..efb6e78d6 100644 Binary files a/test/lib/linter/rules/snapshots/Directives.ts.snap and b/test/lib/linter/rules/snapshots/Directives.ts.snap differ