quick-lint-js

1. Diagnostic type and metadata

Diagnostic types are listed in src/quick-lint-js/diag/diagnostic-types.h. They look like this:

QLJS_DIAG_TYPE(                                                  \
    diag_missing_body_for_try_statement, "E0120",                \
    diagnostic_severity::error,                                  \
    { source_code_span try_token; },                             \
    MESSAGE(QLJS_TRANSLATABLE("missing body for try statement"), \
            try_token))                                          \

Each diagnostic type is created using the QLJS_DIAG_TYPE macro. Note the trailing backslashes (\) which serve as line continuations.

The macro generates a class using the first argument and the fourth argument:

struct diag_missing_body_for_try_statement {
    source_code_span try_token;
};

Let's pick a name for our diagnostic as the first argument. The name needs to start with diag_ and be a legal C++ class name. We will use this name later to report the diagnostic and to test for the diagnostic:

QLJS_DIAG_TYPE(                        \
    diag_comparison_with_empty_string, \
    /* TODO */,                        \
    /* TODO */,                        \
    /* TODO */,                        \
    /* TODO */)                        \

Each diagnostic in quick-lint-js has a unique diagnostic code as the second argument. A diagnostic code is the letter E followed by four decimal digits. Let's be lazy and reuse an existing diagnostic code for now:

QLJS_DIAG_TYPE(                        \
    diag_comparison_with_empty_string, \
    "E0001",                           \
    /* TODO */,                        \
    /* TODO */,                        \
    /* TODO */)                        \

We also need to pick a diagnostic severity, error or warning, as the third argument. Our new diagnostic is for a possible issue, so let's pick warning:

QLJS_DIAG_TYPE(                        \
    diag_comparison_with_empty_string, \
    "E0001",                           \
    diagnostic_severity::warning,      \
    /* TODO */,                        \
    /* TODO */)                        \

Each diagnostic is actually a class. The class needs to store at least one source_code_span or identifier variable so the editor knows where to put the squigglies. We should think about where the squigglies should go and name our variable appropriately to make the reporting and testing code easier to read. Since our diagnostic is about string comparisons, let's name the variable comparison_operator. We write the class' body in the fourth argument:

QLJS_DIAG_TYPE(                                \
    diag_comparison_with_empty_string,         \
    "E0001",                                   \
    diagnostic_severity::warning,              \
    { source_code_span comparison_operator; }, \
    /* TODO */)                                \

Important: The curly brackets around the fourth argument are significant. Don't forget them!

Each diagnostic needs a message in the fifth argument. Most diagnostics are simple and just have a simple string. Other diagnostics might have formatting or multiple strings. Our diagnostic is simple, so let's just write a single string with no formatting. Don't forget to mention the source_code_span variable we defined inside our diag_ class:

QLJS_DIAG_TYPE(                                    \
    diag_comparison_with_empty_string,             \
    "E0001",                                       \
    diagnostic_severity::warning,                  \
    { source_code_span comparison_operator; },     \
    MESSAGE(QLJS_TRANSLATABLE(                     \
      "comparing against empty strings is silly"), \
      comparison_operator))                        \

After added the diagnostic type to diagnostic-types.h, build quick-lint-js. You should get a build error like one of the following:

src/quick-lint-js/diag/diagnostic.cpp:128:5: error: constexpr variable 'all_diagnostic_infos' must be initialized by a constant expression
    all_diagnostic_infos[] = {
    ^                        ~
src/quick-lint-js/i18n/translation-table-generated.h:444:3: note: subexpression not valid in a constant expression
  QLJS_CONSTEXPR_ASSERT(false);
  ^
src/quick-lint-js/assert.h:92:7: note: expanded from macro 'QLJS_CONSTEXPR_ASSERT'
      asm("");                     \
      ^
[snip]
src/quick-lint-js/diag/diagnostic.cpp:130:3: note: expanded from macro 'QLJS_DIAG_TYPE'
  info_for_diagnostic<name>::get(),
  ^
1 error generated.

In file included from src/quick-lint-js/diag/diagnostic.cpp:7:
[snip]
src/quick-lint-js/assert.h:92:7: error: inline assembly is not a constant expression
   92 |       asm("");                     \
      |       ^~~
[snip]
src/quick-lint-js/diag/diagnostic.cpp: At global scope:
src/quick-lint-js/diag/diagnostic.cpp:131:9:   in ‘constexpr’ expansion of ‘quick_lint_js::{anonymous}::info_for_diagnostic­<quick_lint_js::diag_comparison_with_empty_string>::get()’
src/quick-lint-js/diag/diagnostic.cpp:133:1: error: ‘constexpr’ call flows off the end of the function
  133 | };
      | ^

Don't panic! These errors are expected. The problem is that our new diagnostic's message is not in the translation table. This is easy to fix: From a command line, run the update-translator-sources script in the quick-lint-js repository:

./tools/update-translator-sources
updating: po/messages.pot
... done.
updating: po/de.po
... done.
updating: po/en_US@snarky.po
... done.
updating: po/fr_FR.po
... done.
updating: po/pt_BR.po
... done.
updating: po/sv_SE.po
... done.

The update-translator-sources script modified a few files:

git status
git status
On branch dev3
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   po/de.po
        modified:   po/en_US@snarky.po
        modified:   po/fr_FR.po
        modified:   po/messages.pot
        modified:   po/pt_BR.po
        modified:   po/sv_SE.po
        modified:   src/quick-lint-js/diag/diagnostic-types.h
        modified:   src/quick-lint-js/i18n/translation-table-generated.cpp
        modified:   src/quick-lint-js/i18n/translation-table-generated.h
        modified:   test/test-translation-table-generated.cpp

no changes added to commit (use "git add" and/or "git commit -a")

Now let's build again and run the tests:

ninja -C build quick-lint-js-test
ninja: Entering directory `build'
[103/103] Linking CXX executable test/quick-lint-js-test

./build/test/quick-lint-js-test --gtest_brief=1
Running main() from gmock_main.cc
test/test-diag-code.cpp:50: Failure
Failed
diag code E0001 used for multiple diags:
diag_comparison_with_empty_string, diag_assignment_before_variable_declaration
try this unused diag code: E0331
[  FAILED  ] test_diag_code.diag_codes_are_unique (1 ms)
main thread ID: 372035
[==========] 1980 tests from 151 test suites ran. (772 ms total)
[  PASSED  ] 1979 tests.

A test is telling us that the error code we chose (E0001) is already in use. Let's change our QLJS_DIAG_TYPE call in diagnostic-types.h to use the unused code suggested by the test:

QLJS_DIAG_TYPE(                                    \
    diag_comparison_with_empty_string,             \
    "E0331",                                       \
    diagnostic_severity::warning,                  \
    { source_code_span comparison_operator; },     \
    MESSAGE(QLJS_TRANSLATABLE(                     \
      "comparing against empty strings is silly"), \
      comparison_operator))                        \

Build and re-run the tests. We should get no failures, which means we didn't break anything.

Now that we have created the diagnostic type, let's move on to writing a test.

2. Test for the diagnostic

All diagnostics must be tested with an automated test. To create a test, copy-paste an existing test in a test/test-parse-*.cpp file and tweak it. Let's put our test in test/test-parse-warning.cpp:

TEST_F(test_parse_warning, warn_on_empty_string_literal_comparison) {
  {
    test_parser p(u8"a === ''"_sv, capture_diags);
    p.parse_and_visit_expression();
    EXPECT_THAT(
        p.errors,
        ElementsAreArray({
            DIAG_TYPE_OFFSETS(p.code, diag_comparison_with_empty_string,
                              comparison_operator, strlen(u8"a "), u8"==="_sv),
        }));
  }
}

There are a few pieces of this test worth mentioning:

u8"a == ''"_sv

The input source code we want to test. The u8 prefix is required so the code is parsed as UTF-8. The _sv suffix is required so that code containing null bytes is handled correctly.

capture_diags

By default, test_parser fails the test if any diagnostics are reported. Because we want to check for diagnostics ourselves, we must specify the capture_diags tag.

p.parse_and_visit_expression();

quick-lint-js' parser can parse several things, including statements, expressions, and TypeScript types. Our diagnostic is specific to JavaScript expressions, so we call parse_and_visit_expression.

diag_comparison_with_empty_string

We need to tell DIAG_TYPE_OFFSETS which kind of diagnostic we expect. We do this by writing the diagnostic class' name as it appears in QLJS_DIAG_TYPE.

comparison_operator

We need to tell DIAG_TYPE_OFFSETS which variable in the diagnostic class we want to check. Most diagnostic classes (such as ours) have only one variable, but we still need to write it explicitly.

strlen(u8"a "), u8"==="_sv

We need to tell DIAG_TYPE_OFFSETS where in the source code the diagnostic should be reported. This is represented using two parameters: a beginning offset and a span of characters. strlen(u8"a ") is 2, so the diagnostic should start at the character at offset 2. u8"==="_sv has 3 characters, so the diagnostic should cover offsets 2, 3, and 4:

a === ''
  ^~~
  offsets 2, 3, and 4

Build and run the test to make sure it fails. The failure says that we expected a diagnostic, but didn't get any. This makes sense because we haven't written the code to report the diagnostic yet:

ninja -C build quick-lint-js-test
ninja: Entering directory `build'
[2/2] Linking CXX executable test/quick-lint-js-test

./build/test/quick-lint-js-test --gtest_brief=1
Running main() from gmock_main.cc
/home/strager/Projects/quick-lint-js-sl/test/test-parse-warning.cpp:561: Failure
Value of: p.errors
Expected: has 1 element that has type diag_comparison_with_empty_string
  Actual: {}
[  FAILED  ] test_parse_warning.warn_on_empty_string_literal_comparison (0 ms)
main thread ID: 373982
[==========] 1981 tests from 151 test suites ran. (755 ms total)
[  PASSED  ] 1980 tests.

3. Detection and reporting of the diagnostic

Now for the hard part: writing the production code. Most likely we will report our diagnostic in one of these files:

• src/quick-lint-js/fe/lex.cpp
• src/quick-lint-js/fe/parse.cpp or parse-*.cpp
• src/quick-lint-js/fe/variable-analyzer.cpp

But these files contain thousands of lines of code. How do we know where to put our new code?

One technique is to step through the code in a debugger:

$ gdb --args ./build/test/quick-lint-js-test --gtest_filter=test_parse_warning.warn_on_empty_string_literal_comparison
(gdb) b parse_and_visit_expression
Breakpoint 1 at 0x754ae0: parse_and_visit_expression. (3 locations)
(gdb) run
Starting program: build/test/quick-lint-js-test --gtest_filter=test_parse_warning.warn_on_empty_string_literal_comparison
Running main() from gmock_main.cc
Note: Google Test filter = test_parse_warning.warn_on_empty_string_literal_comparison
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from test_parse_warning
[ RUN      ] test_parse_warning.warn_on_empty_string_literal_comparison

Breakpoint 1, quick_lint_js::test_parser::parse_and_visit_expression at test/./quick-lint-js/parse-support.h:97
97          this->parser_.parse_and_visit_expression(this->errors_);
(gdb) c
Continuing.

Breakpoint 1, quick_lint_js::parser::parse_and_visit_expression at src/./quick-lint-js/fe/parse.h:174
174         this->parse_and_visit_expression(v, precedence{});
(gdb) c
Continuing.

Breakpoint 1, quick_lint_js::parser::parse_and_visit_expression at src/./quick-lint-js/fe/parse.h:550
550         monotonic_allocator &alloc = *this->expressions_.allocator();
(gdb) n
551         auto rewind_guard = alloc.make_rewind_guard();
(gdb) n
553         expression *ast = this->parse_expression(v, prec);
(gdb) n
555           auto disable_guard = alloc.disable();
(gdb) n
556           this->visit_expression(ast, v, variable_context::rhs);
(gdb) step
quick_lint_js::parser::visit_expression at src/quick-lint-js/fe/parse-expression.cpp:36
36        auto visit_children = [&] {
(gdb) n
41        switch (ast->kind()) {
(gdb) n
67          visit_children();
(gdb) n
69          static_cast<expression::binary_operator*>(ast));
(gdb) n
68          this->error_on_pointless_compare_against_literal(

error_on_pointless_compare_against_literal looks like a good place to put our code.

Detecting when to report the diagnostic is up to you. But once you have the information you need, reporting a diagnostics is easy:

source_code_span op_span = /* usually this->peek().span */;
this->diag_reporter_->report(diag_comparison_with_empty_string{
    .comparison_operator = op_span,
});

Build and test to prove that our code worked:

ninja -C build quick-lint-js-test
ninja: Entering directory `build'
[3/3] Linking CXX executable test/quick-lint-js-test

./build/test/quick-lint-js-test --gtest_brief=1
Running main() from gmock_main.cc
main thread ID: 375845
[==========] 1981 tests from 151 test suites ran. (785 ms total)
[  PASSED  ] 1981 tests.

Huzzah! 🥳

But we're not done yet... We still have to write 💀 documentation 💀

4. Documentation for the website

Each diagnostic has associated documentation stored separately from the code. The docs are stored in docs/errors/ with one file per diagnostic. Let's write our documentation:

# E0331: comparing against empty strings is silly

Empty strings are error-prone, and comparing against empty strings is extra
error-prone:

```javascript
let x = prompt();
if (x === '') {
    alert('You were supposed to type something!');
}
```

To fix this mistake, check the string's `length` property instead:

```javascript
let x = prompt();
if (x.length === 0) {
    alert('You were supposed to type something!');
}
```

Alternatively, treat the string as a boolean:

```javascript
let x = prompt();
if (!x) {
    alert('You were supposed to type something!');
}
```

(This is an example diagnostic for contributor documentation purposes. Comparing
against empty strings is totally fine, and the fact that quick-lint-js reports a
diagnostic in this case is sad.)

Some important parts of diagnostic documentation:

title

The title of the document should include the diagnostic's code. The diagnostic's message should follow the code. The title should include the message mentioned in diagnostic-types.h, but it doesn't have to match exactly. Interpolation markers such as {1:headlinese} should be omitted.

first example

The first code snippet should be fenced with ```javascript or ```typescript (or another other support language). This code snippet demonstrates broken code and must cause quick-lint-js to report a diagnostic. A broken code snippet is required.

second example

The second code snippet should also be fenced. This code snippet demonstrates working code and must not cause quick-lint-js to report any diagnostic. A working code snippet is required.

extra examples

You can include more code snippets after the second. Each of these extra code snippet must cause no diagnostics. Usually these code snippets show alternate ways of addressing the original issue. These extra examples are optional.