Creating a new diagnostic in quick-lint-js

1. Diagnostic type and metadata

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

struct Diag_Missing_Body_For_Try_Statement {
  [[qljs::diag("E0120", Diagnostic_Severity::error)]] //
  [[qljs::message("missing body for try statement",
                  ARG(try_token))]]  //
  Source_Code_Span try_token;
};

Each diagnostic type is a C++ struct with some custom C++ [[attributes]].

The C++ compiler ignores the attributes and just sees a class:

struct Diag_Missing_Body_For_Try_Statement {
    Source_Code_Span try_token;
};

The custom attributes are processed by a separate tool as part of quick-lint-js's build system. We will discuss the custom attributes shortly.

Let's pick a name for our diagnostic. 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:

struct Diag_Comparison_With_Empty_String {
  [[qljs::diag(/* TODO */, /* TODO */)]]  //
  [[qljs::message(/* TODO */)]]           //
  /* TODO */
};

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

struct Diag_Comparison_With_Empty_String {
  [[qljs::diag("E0001", /* TODO */)]]  //
  [[qljs::message(/* TODO */)]]        //
  /* TODO */
};

We also need to pick a diagnostic severity, either Diagnostic_Severity::error or Diagnostic_Severity::warning, as the second argument to [[qljs::diag]]. Our new diagnostic is for a possible issue, so let's pick warning:

struct Diag_Comparison_With_Empty_String {
  [[qljs::diag("E0001", Diagnostic_Severity::warning)]]  //
  [[qljs::message(/* TODO */)]]                          //
  /* TODO */
};

Each diagnostic is a class (struct). The class needs to store at least one Source_Code_Span member variable so the editor knows where to put the squigglies. We should think about where the squigglies should go and name our member variable appropriately to make the reporting and testing code easier to read. Since our diagnostic is about string comparisons, let's name the member variable comparison_operator. We write the member variables after the C++ attributes:

struct Diag_Comparison_With_Empty_String {
  [[qljs::diag("E0001", Diagnostic_Severity::warning)]]  //
  [[qljs::message(/* TODO */)]]                          //
  Source_Code_Span comparison_operator;
};

Each diagnostic needs a message specified by one or more [[qljs::message]] attributes. 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 member variable we defined inside our Diag_ class:

struct Diag_Comparison_With_Empty_String {
  [[qljs::diag("E0001", Diagnostic_Severity::warning)]]  //
  [[qljs::message("comparing against empty strings is silly",
                  ARG(comparison_operator))]]  //
  Source_Code_Span comparison_operator;
};

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

diagnostic-types-2.h:655:20: error: diag code "E0001" already in use; try this unused diag code: "E0331"
diagnostic-types-2.h:107:16: note: Diag_Assignment_Before_Variable_Declaration used code "E0001" here

A build check is telling us that the error code we chose (E0001) is already in use. Let's change our Diag_ class in diagnostic-types-2.h to use the unused diagnostic code suggested by the check:

struct Diag_Comparison_With_Empty_String {
  [[qljs::diag("E0331", Diagnostic_Severity::warning)]]  //
  [[qljs::message("comparing against empty strings is silly",
                  ARG(comparison_operator))]]  //
  Source_Code_Span comparison_operator;
};

Now let's build quick-lint-js again and run the tests. We should get no failures, which means we didn't break anything:

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
[==========] 1980 tests from 151 test suites ran. (772 ms total)
[  PASSED  ] 1980 tests.

The build scripts modified a few files for us. Make sure you include these files in your commit:

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/messages.pot
        modified:   src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
        modified:   src/quick-lint-js/diag/diagnostic-types-2.h
        modified:   src/quick-lint-js/i18n/translation-table-generated.cpp
        modified:   src/quick-lint-js/i18n/translation-table-generated.h
        modified:   src/quick-lint-js/i18n/translation-table-test-generated.h

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

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_parse_and_visit_expression(
      u8"a === ''"_sv,  //
      u8"  ^^^ Diag_Comparison_With_Empty_String"_diag);
}

There are a few pieces of this test worth mentioning:

test_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 test_parse_and_visit_expression.

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.

  ^^^

We need to tell test_parse_and_visit_expression where in the source code the diagnostic should be reported. This is represented using two parts inside a string: alignment (spaces) and a span (one or more ^s, or one `). In our example, there are two leading spaces, so the diagnostic should start at the third character (byte offset 2). The span is three characters wide (^^^), so the diagnostic covers three characters (up until byte offset 5). The alignment and span specify that the diagnostic should cover offsets 2, 3, and 4:

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

Diag_Comparison_With_Empty_String

We need to tell test_parse_and_visit_expression which kind of diagnostic we expect. We do this by writing the diagnostic class's name after the span.

If a diagnostic has multiple members (such as if the diagnostic has multiple messages), the member name must appear after the diagnostic class's name. See the documentation for _diag and NOTE[_diag-syntax] for details.

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-2.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.