diff --git a/CHANGELOG.md b/CHANGELOG.md index 00d7e52..b20a37e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,18 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog][keepachangelog], and this project adheres to [Semantic Versioning][semver]. +## [Unreleased] + +### Added + +- Support of `!#else` preprocessor directive [#185]. + + ## [2.0.5] - 2023-09-07 ### Changed -- Updated `@adguard/agtree` to `v1.1.5`. +- Updated [@adguard/agtree] to `v1.1.5`. ## [2.0.4] - 2023-08-30 @@ -17,7 +24,7 @@ The format is based on [Keep a Changelog][keepachangelog], and this project adhe ### Changed - Override `config.extends`'s presets by the user config. -- Updated `@adguard/agtree` to `v1.1.4`. +- Updated [@adguard/agtree] to `v1.1.4`. ## [2.0.3] - 2023-08-29 @@ -30,7 +37,7 @@ The format is based on [Keep a Changelog][keepachangelog], and this project adhe ### Changed -- Updated `@adguard/agtree` to `v1.1.3`. +- Updated [@adguard/agtree] to `v1.1.3`. ## [2.0.1] - 2023-08-14 @@ -38,7 +45,7 @@ The format is based on [Keep a Changelog][keepachangelog], and this project adhe ### Changed - Make `syntax` property in the config required. -- Updated `@adguard/agtree` to `v1.1.2`. +- Updated [@adguard/agtree] to `v1.1.2`. ## [2.0.0] - 2023-08-11 @@ -50,7 +57,7 @@ The format is based on [Keep a Changelog][keepachangelog], and this project adhe ### Changed -- Updated `@adguard/agtree` to `v1.1.1`. +- Updated [@adguard/agtree] to `v1.1.1`. ## [1.0.11] - 2023-04-21 @@ -139,6 +146,7 @@ The format is based on [Keep a Changelog][keepachangelog], and this project adhe - Initial version of the linter and CLI. - Initial version of the adblock rule parser. +[Unreleased]: https://github.com/AdguardTeam/AGLint/compare/v2.0.5...HEAD [2.0.5]: https://github.com/AdguardTeam/AGLint/compare/v2.0.4...v2.0.5 [2.0.4]: https://github.com/AdguardTeam/AGLint/compare/v2.0.3...v2.0.4 [2.0.3]: https://github.com/AdguardTeam/AGLint/compare/v2.0.1...v2.0.3 @@ -155,4 +163,7 @@ The format is based on [Keep a Changelog][keepachangelog], and this project adhe [keepachangelog]: https://keepachangelog.com/en/1.0.0/ [semver]: https://semver.org/spec/v2.0.0.html +[@adguard/agtree]: https://github.com/AdguardTeam/tsurlfilter/blob/master/packages/agtree/CHANGELOG.md + [#10]: https://github.com/AdguardTeam/AGLint/issues/10 +[#185]: https://github.com/AdguardTeam/AGLint/issues/185 diff --git a/README.md b/README.md index c8f2812..9e00ba4 100644 --- a/README.md +++ b/README.md @@ -125,13 +125,16 @@ it as a CLI tool with the default configuration. That's all! :hugs: The linter will check all filter lists in your project and print the results to the console. -> **Note**: You can also install AGLint globally, so you can use it without `npx` or `yarn`, but we recommend to install +> [!NOTE] +> You can also install AGLint globally, so you can use it without `npx` or `yarn`, but we recommend to install > it locally to your project. -> **Note**: If you want to lint just some specific files, you can pass them as arguments: +> [!NOTE] +> If you want to lint just some specific files, you can pass them as arguments: > `aglint path/to/file.txt path/to/another/file.txt` -> **Note**: To see all available options, run `aglint --help`. +> [!NOTE] +> To see all available options, run `aglint --help`. *To customize the default configuration, see [Configuration](#configuration) for more info. If you want to use AGLint programmatically, see [Use programmatically](#use-programmatically).* @@ -264,7 +267,8 @@ project.** This command will create a `.aglintrc.yaml` file in the current direc You can also create a configuration file manually, please check the section below for more info. -> **Note**: We are planning to add a configuration wizard in the future, so you will be able to create a configuration +> [!NOTE] +> We are planning to add a configuration wizard in the future, so you will be able to create a configuration > file by answering a few questions. ### Configuration file name and format @@ -282,13 +286,16 @@ We also plan to support `.aglintrc.js` (JavaScript) in the future. We recommend using `.aglintrc.yaml` or `.aglintrc.yml` because YAML is more compact and easier to read, and it supports comments. -> **Warning**: If you have multiple configuration files in the same directory, the CLI will throw an error and ask you +> [!WARNING] +> If you have multiple configuration files in the same directory, the CLI will throw an error and ask you > to fix it. -> **Warning**: If your configuration file is syntactically invalid or contains unknown / invalid options, the CLI will +> [!WARNING] +> If your configuration file is syntactically invalid or contains unknown / invalid options, the CLI will > throw an error and ask you to fix it. -> **Warning**: If your configuration file is not named in one of the ways listed above, the CLI will ignore it (since it +> [!WARNING] +> If your configuration file is not named in one of the ways listed above, the CLI will ignore it (since it > cannot recognize it as a configuration file). ### Configuration file structure @@ -389,13 +396,16 @@ Currently, there are two built-in presets available (click on the name to see th - [`aglint:all`][aglint-all] — a set of **all** rules that are available in the linter. This option maybe too strict for most projects. -> **Note**: Presets contain `syntax` and `rules` which shall be overridden if they are specified in the config. +> [!NOTE] +> Presets contain `syntax` and `rules` which shall be overridden if they are specified in the config. -> **Note**: All presets have `syntax` property set to `Common` a default value. +> [!NOTE] +> All presets have `syntax` property set to `Common` a default value. > You may need to specify it in your [configuration file](#configuration-file-structure) > for better linting, e.g. modifiers validation. -> **Note**: We are planning to add more presets in the future, +> [!NOTE] +> We are planning to add more presets in the future, > and also allow users to create their own presets but currently it is not possible. ### Default configuration file @@ -428,7 +438,8 @@ It simply extends the `aglint:recommended` preset and specifies the `root` optio } ``` -> **Note**: JavaScript configuration files aren't supported at the moment +> [!NOTE] +> JavaScript configuration files aren't supported at the moment > but we plan to add support for them in the future (CJS and ESM syntaxes). ### Configuration cascading and hierarchy @@ -522,7 +533,9 @@ Currently, the following linter rules are available (we will add more rules in t ### `if-closed` -Checks if the `if` statement is closed and no unclosed `endif` statements are present. +Checks if the `if` statement is closed and no unclosed `endif` or unopened `else` statements are present. +It also checks whether `else` and `endif` statements are used correctly +since they can only be used alone without other parameters or statements. - **Severity:** `error` (2) - **Options:** none @@ -535,11 +548,14 @@ Checks if the `if` statement is closed and no unclosed `endif` statements are pr !#endif !#if (adguard_ext_firefox) example.org##.something + !#else if (adguard_ext_opera) + example.org##.operaBanner ``` will be reported as error: ```txt 1:0 error Using an "endif" directive without an opening "if" directive 5:0 error Unclosed "if" directive + 7:0 error Invalid usage of preprocessor directive: "else" ``` since the first `endif` are unnecessary, and the last `if` statement is not closed. @@ -591,6 +607,9 @@ Checks if the same modifier is used multiple times in a single network rule. Checks if the used preprocessor directives are known. +> [!IMPORTANT] +> Preprocessor directives are case-sensitive, so `!#IF` is to be considered as invalid. + - **Severity:** `error` (2) - **Options:** none - **Fixable:** no @@ -606,6 +625,7 @@ Checks if the used preprocessor directives are known. - **Additional information:** - Currently, the following preprocessor directives are supported: - `if`: [reference][if-kb] + - `else`: [reference][if-kb] - `endif`: [reference][if-kb] - `include`: [reference][include-kb] - `safari_cb_affinity`: [reference][safari-cb-kb] diff --git a/docs/repo-integration.md b/docs/repo-integration.md index ad8183a..2a29fc6 100644 --- a/docs/repo-integration.md +++ b/docs/repo-integration.md @@ -208,7 +208,8 @@ AGLint by adding `--no-verify` to your commit command. Next time you clone the repository, you'll only need to run `npm install` and it will install Husky automatically, because it automatically runs `prepare` script after installing dependencies. -> **Note**: To make the hook work in Unix-like operating systems you may need to run +> [!NOTE] +> To make the hook work in Unix-like operating systems you may need to run > `chmod ug+x .husky/pre-commit`. ### Add post-merge hook @@ -217,7 +218,8 @@ If AGLint is updated on the remote repository (in practice, `package.json` or `p you need to update AGLint version in your local repository. If you just clone the changes, that does not sync your dependencies in `node_modules` directory, so you need to run `npm install` to sync your dependencies manually. -> :warning: **Warning**: If you do not sync your installed dependencies in your `node_modules` folder +> [!WARNING] +> If you do not sync your installed dependencies in your `node_modules` folder > after the `package.json` or `package-lock.json` is changed, > you may get errors when running AGLint or your CI may report different results than your local machine. @@ -258,7 +260,8 @@ fi This script will check if `package.json` or `package-lock.json` is changed between the old HEAD and the new HEAD. If one of them is changed, it will run `npm install` to sync your dependencies, so you don't need to do that manually. -> **Note**: To make the hook work in Unix-like operating systems you may need to run +> [!NOTE] +> To make the hook work in Unix-like operating systems you may need to run > `chmod ug+x .husky/post-merge`. diff --git a/src/common/constants.ts b/src/common/constants.ts index 1111057..5999485 100644 --- a/src/common/constants.ts +++ b/src/common/constants.ts @@ -204,6 +204,16 @@ export const PREPROCESSOR_MARKER = '!#'; export const PREPROCESSOR_MARKER_LEN = PREPROCESSOR_MARKER.length; export const PREPROCESSOR_SEPARATOR = ' '; -export const SAFARI_CB_AFFINITY = 'safari_cb_affinity'; -export const IF = 'if'; -export const INCLUDE = 'include'; +export const IF_DIRECTIVE = 'if'; +export const ELSE_DIRECTIVE = 'else'; +export const ENDIF_DIRECTIVE = 'endif'; +export const INCLUDE_DIRECTIVE = 'include'; +export const SAFARI_CB_AFFINITY_DIRECTIVE = 'safari_cb_affinity'; + +export const SUPPORTED_PREPROCESSOR_DIRECTIVES = new Set([ + ELSE_DIRECTIVE, + ENDIF_DIRECTIVE, + IF_DIRECTIVE, + INCLUDE_DIRECTIVE, + SAFARI_CB_AFFINITY_DIRECTIVE, +]); diff --git a/src/linter/rules/README.md b/src/linter/rules/README.md index 27eb132..f1768c4 100644 --- a/src/linter/rules/README.md +++ b/src/linter/rules/README.md @@ -167,7 +167,8 @@ export const ExampleRule: LinterRule, RuleConfig> = { }; ``` -> **Note**: You can use both the rule storage and the parameters at the same time. +> [!NOTE] +> You can use both the rule storage and the parameters at the same time. ### Report problems @@ -275,7 +276,8 @@ export const RuleName: LinterRule = { }; ``` -> **Note**: If multiple fixes are suggested for the same problem, then the linter will ignore all of them in order to +> [!NOTE] +> If multiple fixes are suggested for the same problem, then the linter will ignore all of them in order to > avoid conflicts. [main-readme-linter-rules]: ../../../README.md#linter-rules diff --git a/src/linter/rules/if-closed.ts b/src/linter/rules/if-closed.ts index 72f3cab..a13c6aa 100644 --- a/src/linter/rules/if-closed.ts +++ b/src/linter/rules/if-closed.ts @@ -2,9 +2,7 @@ import { CommentRuleType, type PreProcessorCommentRule, RuleCategory } from '@ad import { type LinterRule } from '../common'; import { SEVERITY } from '../severity'; - -const IF_DIRECTIVE = 'if'; -const ENDIF_DIRECTIVE = 'endif'; +import { ELSE_DIRECTIVE, ENDIF_DIRECTIVE, IF_DIRECTIVE } from '../../common/constants'; /** * Concreting the storage type definition (the linter only provides a general @@ -35,12 +33,43 @@ export const IfClosed: LinterRule = { const rule = context.getActualAdblockRuleAst(); // Check adblock rule category and type - if (rule.category === RuleCategory.Comment && rule.type === CommentRuleType.PreProcessorCommentRule) { - // Check for "if" and "endif" directives - if (rule.name.value === IF_DIRECTIVE) { + if (rule.category !== RuleCategory.Comment + || rule.type !== CommentRuleType.PreProcessorCommentRule) { + return; + } + + const directive = rule.name.value; + switch (directive) { + case IF_DIRECTIVE: // Collect open "if" context.storage.openIfs.push(rule); - } else if (rule.name.value === ENDIF_DIRECTIVE) { + break; + case ELSE_DIRECTIVE: + // '!#else' can only be used alone without any parameters + if (rule.params) { + context.report({ + message: `Invalid usage of preprocessor directive: "${ELSE_DIRECTIVE}"`, + node: rule, + }); + } + // Check if there is an open "!#if" before "!#else" + if (context.storage.openIfs.length === 0) { + context.report({ + // eslint-disable-next-line max-len + message: `Using an "${ELSE_DIRECTIVE}" directive without an opening "${IF_DIRECTIVE}" directive`, + node: rule, + }); + } + // otherwise do nothing + break; + case ENDIF_DIRECTIVE: + // '!#endif' can only be used alone without any parameters + if (rule.params) { + context.report({ + message: `Invalid usage of preprocessor directive: "${ENDIF_DIRECTIVE}"`, + node: rule, + }); + } if (context.storage.openIfs.length === 0) { context.report({ // eslint-disable-next-line max-len @@ -51,7 +80,11 @@ export const IfClosed: LinterRule = { // Mark "if" as closed (simply delete it from collection) context.storage.openIfs.pop(); } - } + break; + default: + // unsupported directives should be reported by another rule - 'unknown-preprocessor-directives' + // so do nothing + break; } }, onEndFilterList: (context): void => { diff --git a/src/linter/rules/unknown-preprocessor-directives.ts b/src/linter/rules/unknown-preprocessor-directives.ts index 0a905bc..99f169c 100644 --- a/src/linter/rules/unknown-preprocessor-directives.ts +++ b/src/linter/rules/unknown-preprocessor-directives.ts @@ -2,13 +2,7 @@ import { CommentRuleType, RuleCategory } from '@adguard/agtree'; import { SEVERITY } from '../severity'; import { type LinterRule } from '../common'; - -const COMMON_PREPROCESSOR_DIRECTIVES = new Set([ - 'if', - 'endif', - 'include', - 'safari_cb_affinity', -]); +import { SUPPORTED_PREPROCESSOR_DIRECTIVES } from '../../common/constants'; /** * Checks if a preprocessor directive is known @@ -17,11 +11,13 @@ const COMMON_PREPROCESSOR_DIRECTIVES = new Set([ * @returns `true` if the preprocessor directive is known, `false` otherwise */ function isKnownPreProcessorDirective(name: string): boolean { - return COMMON_PREPROCESSOR_DIRECTIVES.has(name); + return SUPPORTED_PREPROCESSOR_DIRECTIVES.has(name); } /** - * Rule that checks if a preprocessor directive is known + * Rule that checks if a preprocessor directive is known. + * + * Directives are case-sensitive, so `!#IF` is to be considered as invalid. */ export const UnknownPreProcessorDirectives: LinterRule = { meta: { diff --git a/test/linter/linter.test.ts b/test/linter/linter.test.ts index 61ba1b2..f97a5f4 100644 --- a/test/linter/linter.test.ts +++ b/test/linter/linter.test.ts @@ -13,6 +13,8 @@ import { SEVERITY, type SeverityValue, type SeverityName } from '../../src/linte import { EMPTY, NEWLINE } from '../../src/common/constants'; import { type LinterConfig, type LinterRule } from '../../src/linter/common'; import { InvalidModifiers } from '../../src/linter/rules/invalid-modifiers'; +import { IfClosed } from '../../src/linter/rules/if-closed'; +import { UnknownPreProcessorDirectives } from '../../src/linter/rules/unknown-preprocessor-directives'; const demoRule: LinterRule = { meta: { @@ -1125,6 +1127,91 @@ describe('Linter', () => { }); }); + describe('lint detects pre-processor directives', () => { + it('unknown ifelse directive', () => { + const linter = new Linter(false); + linter.addRule('if-closed', IfClosed); + linter.addRule('unknown-preprocessor-directives', UnknownPreProcessorDirectives); + + expect( + linter.lint( + [ + 'rule0', + '!#if (condition1)', + 'rule1', + '!#ifelse (condition2)', + 'rule2', + '!#endif', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [ + { + rule: 'unknown-preprocessor-directives', + severity: 2, + message: 'Unknown preprocessor directive "ifelse"', + position: { + startLine: 4, + startColumn: 2, + endLine: 4, + endColumn: 8, + }, + }, + ], + warningCount: 0, + errorCount: 1, + fatalErrorCount: 0, + }); + }); + + it('invalid includes and unclosed if', () => { + const linter = new Linter(false); + linter.addRule('if-closed', IfClosed); + linter.addRule('unknown-preprocessor-directives', UnknownPreProcessorDirectives); + + expect( + linter.lint( + [ + 'rule0', + '!#if (condition1)', + '!#includes https://raw.example.com/file1.txt', + 'rule1', + '!#else', + 'rule2', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [ + { + rule: 'unknown-preprocessor-directives', + severity: 2, + message: 'Unknown preprocessor directive "includes"', + position: { + startLine: 3, + startColumn: 2, + endLine: 3, + endColumn: 10, + }, + }, + { + rule: 'if-closed', + severity: 2, + message: 'Unclosed "if" directive', + position: { + startLine: 2, + startColumn: 0, + endLine: 2, + endColumn: 17, + }, + }, + ], + warningCount: 0, + errorCount: 2, + fatalErrorCount: 0, + }); + }); + }); + test("lint ignores inline config comments if they aren't allowed in the linter config", () => { const linter = new Linter(false, { allowInlineConfig: false, diff --git a/test/linter/rules/if-closed.test.ts b/test/linter/rules/if-closed.test.ts index a3fc1ef..6639d54 100644 --- a/test/linter/rules/if-closed.test.ts +++ b/test/linter/rules/if-closed.test.ts @@ -47,6 +47,37 @@ describe('if-closed', () => { errorCount: 0, fatalErrorCount: 0, }); + + // 'if' block with 'else' branch + expect( + linter.lint( + [ + 'rule0', + '!#if (condition1)', + 'rule1', + '!#else', + 'rule2', + '!#endif', + 'rule3', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [], + }); + + // 'include' directive inside 'if' block + expect( + linter.lint( + [ + 'rule', + '!#if (condition1)', + '!#include https://raw.example.com/file1.txt', + '!#endif', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [], + }); }); test('should detect unclosed if-s', () => { @@ -80,6 +111,109 @@ describe('if-closed', () => { errorCount: 1, fatalErrorCount: 0, }); + + // unclosed 'if' block with 'else' branch + expect( + linter.lint( + [ + 'rule0', + '!#if (condition1)', + 'rule1', + '!#endif', + '!#if (condition2)', + 'rule2', + '!#else', + 'rule3', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [ + { + rule: 'if-closed', + severity: 2, + message: 'Unclosed "if" directive', + position: { + startLine: 5, + startColumn: 0, + endLine: 5, + endColumn: 17, + }, + }, + ], + warningCount: 0, + errorCount: 1, + fatalErrorCount: 0, + }); + }); + + test('should detect unopened else directive', () => { + expect( + linter.lint( + [ + 'rule1', + '!#else', + 'rule2', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [ + { + rule: 'if-closed', + severity: 2, + message: 'Using an "else" directive without an opening "if" directive', + position: { + startLine: 2, + startColumn: 0, + endLine: 2, + endColumn: 6, + }, + }, + ], + warningCount: 0, + errorCount: 1, + fatalErrorCount: 0, + }); + + expect( + linter.lint( + [ + '!#if (condition1)', + 'rule1', + '!#endif', + '!#else', + 'rule2', + '!#endif', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [ + { + rule: 'if-closed', + severity: 2, + message: 'Using an "else" directive without an opening "if" directive', + position: { + startLine: 4, + startColumn: 0, + endLine: 4, + endColumn: 6, + }, + }, + { + rule: 'if-closed', + severity: 2, + message: 'Using an "endif" directive without an opening "if" directive', + position: { + startLine: 6, + startColumn: 0, + endLine: 6, + endColumn: 7, + }, + }, + ], + warningCount: 0, + errorCount: 2, + fatalErrorCount: 0, + }); }); test('should detect unopened endif-s', () => { @@ -114,4 +248,47 @@ describe('if-closed', () => { fatalErrorCount: 0, }); }); + + it('no value for else and endif directives', () => { + expect( + linter.lint( + [ + 'rule0', + '!#if (condition1)', + 'rule1', + '!#else (condition2)', + 'rule2', + '!#endif (condition1)', + ].join(NEWLINE), + ), + ).toMatchObject({ + problems: [ + { + rule: 'if-closed', + severity: 2, + message: 'Invalid usage of preprocessor directive: "else"', + position: { + startLine: 4, + startColumn: 0, + endLine: 4, + endColumn: 19, + }, + }, + { + rule: 'if-closed', + severity: 2, + message: 'Invalid usage of preprocessor directive: "endif"', + position: { + startLine: 6, + startColumn: 0, + endLine: 6, + endColumn: 20, + }, + }, + ], + warningCount: 0, + errorCount: 2, + fatalErrorCount: 0, + }); + }); }); diff --git a/test/linter/rules/unknown-hints-and-platforms.test.ts b/test/linter/rules/unknown-hints-and-platforms.test.ts index d0bfaa6..d6f0ebe 100644 --- a/test/linter/rules/unknown-hints-and-platforms.test.ts +++ b/test/linter/rules/unknown-hints-and-platforms.test.ts @@ -3,7 +3,7 @@ import { UnknownHintsAndPlatforms } from '../../../src/linter/rules/unknown-hint let linter: Linter; -describe('unknown-preprocessor-directives', () => { +describe('unknown-hints-and-platforms', () => { beforeAll(() => { // Configure linter with the rule linter = new Linter(false); diff --git a/test/linter/rules/unknown-preprocessor-directives.test.ts b/test/linter/rules/unknown-preprocessor-directives.test.ts index a296914..16b9e19 100644 --- a/test/linter/rules/unknown-preprocessor-directives.test.ts +++ b/test/linter/rules/unknown-preprocessor-directives.test.ts @@ -14,7 +14,7 @@ describe('unknown-preprocessor-directives', () => { expect(linter.lint('!#include https://example.org/path/includedfile.txt')).toMatchObject({ problems: [] }); expect(linter.lint('!#if (conditions)')).toMatchObject({ problems: [] }); expect(linter.lint('!#if (conditions_2)')).toMatchObject({ problems: [] }); - expect(linter.lint('!#endif')).toMatchObject({ problems: [] }); + expect(linter.lint('!#else')).toMatchObject({ problems: [] }); expect(linter.lint('!#endif')).toMatchObject({ problems: [] }); expect(linter.lint('!#safari_cb_affinity')).toMatchObject({ problems: [] }); expect(linter.lint('!#safari_cb_affinity()')).toMatchObject({ problems: [] });