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: Add ui5lint-disable directives #327

Merged
merged 14 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from 13 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
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, separated by commas:
RandomByte marked this conversation as resolved.
Show resolved Hide resolved

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

A description on why a rule is disabled can be added after the rule name, separated by two dashes:
RandomByte marked this conversation as resolved.
Show resolved Hide resolved

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

### Scope

Directives are currently supported only in JavaScript and TypeScript files, **not** in XML, YAML, HTML, or other file types.
RandomByte marked this conversation as resolved.
Show resolved Hide resolved

## 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