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:
- Diagnostic type and metadata
- Test for the diagnostic
- Detection and reporting of the diagnostic
- Documentation for the website
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 thecapture_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 inQLJS_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.