Skip to content

Commit

Permalink
feat: Add ui5lint-disable directives (#327)
Browse files Browse the repository at this point in the history
Sources may contain the following comments to suppress reporting of
findings. Both, line and block comments are supported. The comment
should contain nothing but the directive, optionally followed by a
description (separated using "--").

This should behave similar to ESLint's disable comments.

General syntax:
`// ui5lint-disable(-next)(-line) rule-1, rule2 -- Description`

Examples:
```js
foo(); // ui5lint-disable-line

// ui5lint-disable-next-line
foo();

// ui5lint-disable no-deprecated-api, no-globals
sap.foo();
// ui5lint-enable no-deprecated-api, no-globals

/* ui5lint-disable-next-line no-deprecated-api --
  To be discussed
*/
foo();
```


JIRA: CPOUI5FOUNDATION-911

Resolves #268

---------

Co-authored-by: Matthias Oßwald <mat.osswald@sap.com>
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
  • Loading branch information
3 people authored Nov 13, 2024
1 parent a024d44 commit 3c29e52
Show file tree
Hide file tree
Showing 26 changed files with 2,595 additions and 47 deletions.
45 changes: 34 additions & 11 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,19 +54,19 @@ Run the `ui5lint [files...]` command in your project root folder
UI5 linter report:

/application/webapp/controller/App.controller.js
10:4 error Call to deprecated function 'attachTap' of class 'Button'
10:4 error Call to deprecated function 'attachTap' of class 'Button' no-deprecated-api

/application/webapp/manifest.json
81:17 error Use of deprecated model type 'sap.ui5/models/odata/type="sap.ui.model.odata.ODataModel"'
81:17 error Use of deprecated model type 'sap.ui5/models/odata/type="sap.ui.model.odata.ODataModel"' no-deprecated-api

/application/webapp/test/unit/unitTests.qunit.js
6:1 error Call to deprecated function 'attachInit' of class 'Core'
6:1 error Call to deprecated function 'getCore' (sap.ui.getCore)
6:1 error Access of global variable 'sap' (sap.ui.getCore)
6:1 error Call to deprecated function 'attachInit' of class 'Core' no-deprecated-api
6:1 error Call to deprecated function 'getCore' (sap.ui.getCore) no-deprecated-api
6:1 error Access of global variable 'sap' (sap.ui.getCore) no-globals

/application/webapp/view/Main.view.xml
16:39 error Import of deprecated module 'sap/m/MessagePage'
22:5 error Use of deprecated property 'blocked' of class 'Button'
16:39 error Import of deprecated module 'sap/m/MessagePage' no-deprecated-api
22:5 error Use of deprecated property 'blocked' of class 'Button' no-deprecated-api

7 problems (7 errors, 0 warnings)

Expand All @@ -85,16 +85,14 @@ You can provide multiple glob patterns as arguments after the `ui5lint` command
UI5 linter report:

/application/webapp/view/Main.view.xml
16:39 error Import of deprecated module 'sap/m/MessagePage'
22:5 error Use of deprecated property 'blocked' of class 'Button'
16:39 error Import of deprecated module 'sap/m/MessagePage' no-deprecated-api
22:5 error Use of deprecated property 'blocked' of class 'Button' no-deprecated-api

2 problems (2 errors, 0 warnings)

Note: Use "ui5lint --details" to show more information about the findings
```



### Options

#### `--details`
Expand Down Expand Up @@ -208,6 +206,31 @@ module.exports = {
];
```

## Directives

UI5 linter supports directives similar to ESLint's configuration comments, allowing you to control linting rules in specific sections of your code.

* **ui5lint-disable**: Disables all linting rules from the position of the comment
* **ui5lint-enable**: Re-enables linting rules that were disabled by ui5lint-disable
* **ui5lint-disable-line**: Disables all linting rules for the current line
* **ui5lint-disable-next-line**: Disables all linting rules for the next line

### Specifying Rules

You can disable specific rules by listing them after the directive. Rules must be separated by commas if several are given:

* `/* ui5lint-disable no-deprecated-api */`
* `/* ui5lint-disable no-deprecated-api, no-deprecated-library */`
* `// ui5lint-disable-line no-deprecated-api`

An explanation why a rule is disabled can be added after the rule name; it must be separated from the preceding text by two dashes:

* `// ui5lint-disable-next-line no-deprecated-api -- explanation`

### Scope

Directives are currently supported in JavaScript and TypeScript files only; they are **not** supported in XML, YAML, HTML, or any other type of file.

## Internals

UI5 linter makes use of the [TypeScript compiler](https://github.com/microsoft/TypeScript/) to parse and analyze the source code (both JavaScript and TypesScript) of a UI5 project. This allows for a decent level of accuracy and performance.
Expand Down
7 changes: 4 additions & 3 deletions src/cli/base.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {Argv, ArgumentsCamelCase, CommandModule, MiddlewareFunction} from "yargs";
import path from "node:path";
import {lintProject} from "../linter/linter.js";
import {Text} from "../formatter/text.js";
import {Json} from "../formatter/json.js";
Expand Down Expand Up @@ -148,10 +147,12 @@ async function handleLint(argv: ArgumentsCamelCase<LinterArg>) {
await profile.start();
}

const rootDir = process.cwd();

const reportCoverage = !!(process.env.UI5LINT_COVERAGE_REPORT ?? coverage);

const res = await lintProject({
rootDir: path.join(process.cwd()),
rootDir,
ignorePatterns,
filePatterns,
coverage: reportCoverage,
Expand All @@ -174,7 +175,7 @@ async function handleLint(argv: ArgumentsCamelCase<LinterArg>) {
process.stdout.write(markdownFormatter.format(res, details));
process.stdout.write("\n");
} else if (format === "" || format === "stylish") {
const textFormatter = new Text();
const textFormatter = new Text(rootDir);
process.stderr.write(textFormatter.format(res, details));
}
// Stop profiling after CLI finished execution
Expand Down
12 changes: 10 additions & 2 deletions src/formatter/text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,16 @@ function formatMessageDetails(msg: LintMessage, showDetails: boolean) {
return `. ${detailsHeader} ${chalk.italic(msg.messageDetails.replace(/\s\s+|\n/g, " "))}`;
}

function formatRule(ruleId: string) {
return chalk.dim(` ${ruleId}`);
}

export class Text {
#buffer = "";

constructor(private readonly cwd: string) {
}

format(lintResults: LintResult[], showDetails: boolean) {
this.#writeln(`UI5 linter report:`);
this.#writeln("");
Expand All @@ -51,7 +58,7 @@ export class Text {
totalWarningCount += warningCount;
totalFatalErrorCount += fatalErrorCount;

this.#writeln(chalk.inverse(path.resolve(process.cwd(), filePath)));
this.#writeln(chalk.inverse(path.resolve(this.cwd, filePath)));

// Determine maximum line and column for position formatting
let maxLine = 0;
Expand Down Expand Up @@ -80,7 +87,8 @@ export class Text {
`${formatSeverity(msg.severity)} ` +
`${msg.fatal ? "Fatal error: " : ""}` +
`${msg.message}` +
`${formatMessageDetails(msg, showDetails)}`);
`${formatMessageDetails(msg, showDetails)}` +
`${formatRule(msg.ruleId)}`);
});

this.#writeln("");
Expand Down
96 changes: 91 additions & 5 deletions src/linter/LinterContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -92,10 +93,8 @@ export interface PositionRange {
}

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;
// The metadata holds information to be shared across linters
directives: Set<Directive>;
}

export default class LinterContext {
Expand Down Expand Up @@ -235,13 +234,100 @@ export default class LinterContext {
return message;
}

#getFilteredMessages(resourcePath: ResourcePath): RawLintMessage[] {
const rawMessages = this.#rawMessages.get(resourcePath);
if (!rawMessages) {
return [];
}
const metadata = this.#metadata.get(resourcePath);
if (!metadata?.directives?.size) {
return rawMessages;
}

const filteredMessages: RawLintMessage[] = [];
const directives = new Set(metadata.directives);
// Sort messages by position
const sortedMessages = rawMessages.filter((rawMessage) => {
if (!rawMessage.position) {
filteredMessages.push(rawMessage);
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;
});

// Filter messages based on directives
let directiveStack: Directive[] = [];
for (const rawMessage of sortedMessages) {
const {position} = rawMessage;
const {line, column} = position!; // Undefined positions are already filtered out above

directiveStack = directiveStack.filter((dir) => {
// Filter out line-based directives that are no longer relevant
if (dir.scope === "line" && dir.line !== line) {
return false;
}
if (dir.scope === "next-line" && dir.line !== line - 1) {
return false;
}
return true;
});

for (const dir of directives) {
if (dir.line > line) {
continue;
}
if (dir.scope !== "line" && dir.line === line && dir.column > column) {
continue;
}
directives.delete(dir);
if (dir.scope === "line" && dir.line !== line) {
continue;
}
if (dir.scope === "next-line" && dir.line !== line - 1) {
continue;
}
directiveStack.push(dir);
}

if (!directiveStack.length) {
filteredMessages.push(rawMessage);
continue;
}

const messageInfo = MESSAGE_INFO[rawMessage.id];
if (!messageInfo) {
throw new Error(`Invalid message id '${rawMessage.id}'`);
}

let disabled = false;
for (const dir of directiveStack) {
if (dir.action === "disable" &&
(!dir.ruleNames.length || dir.ruleNames.includes(messageInfo.ruleId))) {
disabled = true;
} else if (dir.action === "enable" &&
(!dir.ruleNames.length || dir.ruleNames.includes(messageInfo.ruleId))) {
disabled = false;
}
}
if (!disabled) {
filteredMessages.push(rawMessage);
}
}
return filteredMessages;
}

generateLintResult(resourcePath: ResourcePath): LintResult {
const rawMessages = this.#rawMessages.get(resourcePath) ?? [];
const coverageInfo = this.#coverageInfo.get(resourcePath) ?? [];
let errorCount = 0;
let warningCount = 0;
let fatalErrorCount = 0;

const rawMessages = this.#getFilteredMessages(resourcePath);

const messages: LintMessage[] = rawMessages.map((rawMessage) => {
const message = this.#getMessageFromRawMessage(rawMessage);
if (message.severity === LintMessageSeverity.Error) {
Expand Down
7 changes: 7 additions & 0 deletions src/linter/ui5Types/SourceFileLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {taskStart} from "../../utils/perf.js";
import {getPositionsForNode} from "../../utils/nodePosition.js";
import {TraceMap} from "@jridgewell/trace-mapping";
import type {ApiExtract} from "../../utils/ApiExtract.js";
import {findDirectives} from "./directives.js";

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

Expand Down Expand Up @@ -80,6 +81,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) {
Expand Down
2 changes: 1 addition & 1 deletion src/linter/ui5Types/TypeLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
11 changes: 7 additions & 4 deletions src/linter/ui5Types/amdTranspiler/transpiler.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
23 changes: 15 additions & 8 deletions src/linter/ui5Types/amdTranspiler/tsTransformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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<ts.SourceFile> {
return function transformer(context: ts.TransformationContext) {
export function createTransformer(
program: ts.Program, resourcePath: string, context: LinterContext
): ts.TransformerFactory<ts.SourceFile> {
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[] = [];
Expand Down Expand Up @@ -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<ts.Node> {
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")) {
Expand Down Expand Up @@ -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;

Expand Down
Loading

0 comments on commit 3c29e52

Please sign in to comment.