# Creating a new diagnostic in quick-lint-js

A common task when contributing to quick-lint-js is to create a new diagnostic. In quick-lint-js, diagnostic is a warning or error reported while parsing the user's JavaScript code.

Creating a diagnostic involves four pieces of code:

1. Diagnostic type and metadata
2. Test for the diagnostic
3. Detection and reporting of the diagnostic
4. Documentation for the website

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

Build error after creating a diagnostic type

    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.