Skip to content

Commit

Permalink
fix(fe): don't report React-specific JSX errors in non-React code
Browse files Browse the repository at this point in the history
Preact uses JSX but has different rules for attributes. Disable our
React-specific rules if a React import is not detected.
  • Loading branch information
strager committed Jan 5, 2024
1 parent a985342 commit a518996
Show file tree
Hide file tree
Showing 6 changed files with 167 additions and 20 deletions.
5 changes: 5 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ Semantic Versioning.

* quick-lint-js's tracing no longer crashes with an assertion failure when
setting its thread name on FreeBSD.
* React-specific JSX diagnostics, such as [E0193][] ("misspelled React
attribute; write 'className' instead"), are now only reported when 'react' is
imported. This fixes false warnings in Preact code. ([#1152][])

## 3.0.0 (2024-01-01)

Expand Down Expand Up @@ -1376,6 +1379,8 @@ Beta release.
[toastin0]: https://github.com/toastin0
[wagner riffel]: https://github.com/wgrr

[#1152]: https://github.com/quick-lint/quick-lint-js/issues/1152

[E0001]: https://quick-lint-js.com/errors/E0001/
[E0003]: https://quick-lint-js.com/errors/E0003/
[E0013]: https://quick-lint-js.com/errors/E0013/
Expand Down
2 changes: 1 addition & 1 deletion src/quick-lint-js/fe/parse-expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3851,7 +3851,7 @@ Expression* Parser::parse_jsx_element_or_fragment(Parse_Visitor_Base& v,
this->lexer_.skip_in_jsx();
}
if (is_intrinsic && !has_namespace && !tag_namespace) {
this->check_jsx_attribute(attribute);
this->jsx_intrinsic_attributes_.emplace_back(attribute);
}
if (this->peek().type == Token_Type::equal) {
this->lexer_.skip_in_jsx();
Expand Down
16 changes: 16 additions & 0 deletions src/quick-lint-js/fe/parse-statement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ void Parser::parse_and_visit_module(Parse_Visitor_Base &v) {
}
}
}
this->check_all_jsx_attributes();
v.visit_end_of_module();
}

Expand Down Expand Up @@ -4695,6 +4696,7 @@ void Parser::parse_and_visit_import(
// import "foo";
case Token_Type::string:
// Do not set is_current_typescript_namespace_non_empty_.
this->visited_module_import(this->peek());
this->skip();
this->consume_semicolon_after_statement();
return;
Expand Down Expand Up @@ -4853,6 +4855,7 @@ void Parser::parse_and_visit_import(

this->skip();
QLJS_PARSER_UNIMPLEMENTED_IF_NOT_TOKEN(Token_Type::string);
this->visited_module_import(this->peek());
this->skip();
QLJS_PARSER_UNIMPLEMENTED_IF_NOT_TOKEN(Token_Type::right_paren);
this->skip();
Expand Down Expand Up @@ -4918,6 +4921,7 @@ void Parser::parse_and_visit_import(
.declare_keyword = *declare_context.declare_namespace_declare_keyword,
});
}
this->visited_module_import(this->peek());
this->skip();
break;

Expand Down Expand Up @@ -5313,6 +5317,18 @@ void Parser::parse_and_visit_named_exports(
this->skip();
}

void Parser::visited_module_import(const Token &module_name) {
QLJS_ASSERT(module_name.type == Token_Type::string);
// TODO(#1159): Write a proper routine to decode string literals.
String8_View module_name_unescaped =
make_string_view(module_name.begin + 1, module_name.end - 1);
if (module_name_unescaped == u8"react"_sv ||
module_name_unescaped == u8"react-dom"_sv ||
starts_with(module_name_unescaped, u8"react-dom/"_sv)) {
this->imported_react_ = true;
}
}

void Parser::parse_and_visit_variable_declaration_statement(
Parse_Visitor_Base &v,
bool is_top_level_typescript_definition_without_declare_or_export) {
Expand Down
21 changes: 21 additions & 0 deletions src/quick-lint-js/fe/parse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,27 @@ Expression* Parser::build_expression(Binary_Expression_Builder& builder) {
}
}

void Parser::check_all_jsx_attributes() {
switch (this->options_.jsx_mode) {
case Parser_JSX_Mode::none:
break;

case Parser_JSX_Mode::auto_detect:
if (this->imported_react_) {
goto react;
}
break;

react:
case Parser_JSX_Mode::react:
this->jsx_intrinsic_attributes_.for_each(
[&](const Identifier& attribute_name) -> void {
this->check_jsx_attribute(attribute_name);
});
break;
}
}

QLJS_WARNING_PUSH
QLJS_WARNING_IGNORE_GCC("-Wnull-dereference")
void Parser::check_jsx_attribute(const Identifier& attribute_name) {
Expand Down
33 changes: 33 additions & 0 deletions src/quick-lint-js/fe/parse.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <optional>
#include <quick-lint-js/assert.h>
#include <quick-lint-js/container/hash-map.h>
#include <quick-lint-js/container/linked-vector.h>
#include <quick-lint-js/container/padded-string.h>
#include <quick-lint-js/diag/diag-reporter.h>
#include <quick-lint-js/diag/diagnostic-types.h>
Expand Down Expand Up @@ -55,11 +56,30 @@ enum class Parser_Top_Level_Await_Mode {
await_operator,
};

// How to interpret props for builtins such as <button onclick> and <span
// class>.
enum class Parser_JSX_Mode {
// Detect the JSX mode based on heuristics.
auto_detect = 0,

// Enforce no rules.
none,

// Use React's rules:
// * camelCase for event handler attributes starting with 'on'
// * certain camelCase attributes such as 'colSpan'
// * 'class' is named 'className'
react,
};

// TODO(#465): Accept parser options from quick-lint-js.config or CLI options.
struct Parser_Options {
Parser_Top_Level_Await_Mode top_level_await_mode =
Parser_Top_Level_Await_Mode::auto_detect;

// jsx_mode does not affect whether JSX is parsed or not. See this->jsx.
Parser_JSX_Mode jsx_mode = Parser_JSX_Mode::auto_detect;

// If true, parse JSX language extensions: https://facebook.github.io/jsx/
bool jsx = false;

Expand Down Expand Up @@ -608,6 +628,9 @@ class Parser {
void parse_and_visit_named_exports_for_typescript_type_only_import(
Parse_Visitor_Base &v, Source_Code_Span type_keyword);

// Precondition: module_name.type == Token_Type::string;
void visited_module_import(const Token &module_name);

// If set, refers to the first `export default` statement in this module. A
// module cannot contain more than one `export default`.
std::optional<Source_Code_Span>
Expand Down Expand Up @@ -887,6 +910,7 @@ class Parser {
Expression *parse_jsx_element_or_fragment(Parse_Visitor_Base &,
Identifier *tag,
const Char8 *less_begin);
void check_all_jsx_attributes();
void check_jsx_attribute(const Identifier &attribute_name);
Expression *parse_typescript_generic_arrow_expression(Parse_Visitor_Base &,
Precedence);
Expand Down Expand Up @@ -1092,6 +1116,15 @@ class Parser {
// variables) so that memory can be released in case we call setjmp.
Buffering_Visitor_Stack buffering_visitor_stack_;

// All parsed JSX attributes for intrinsic elements.
//
// Depending on Parser_JSX_Mode, diagnostics may or may not be reported for
// these attributes at the end of a module.
Linked_Vector<Identifier> jsx_intrinsic_attributes_{new_delete_resource()};

// Heuristic. True if React.js was imported.
bool imported_react_ = false;

bool in_top_level_ = true;
bool in_async_function_ = false;
bool in_generator_function_ = false;
Expand Down
110 changes: 91 additions & 19 deletions test/test-parse-jsx-react.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <quick-lint-js/container/concat.h>
#include <quick-lint-js/diag-matcher.h>
#include <quick-lint-js/parse-support.h>
#include <quick-lint-js/port/char8.h>
Expand All @@ -12,101 +13,172 @@ namespace quick_lint_js {
namespace {
class Test_Parse_JSX_React : public Test_Parse_Expression {};

constexpr Parser_Options jsx_react_options{
.jsx_mode = Parser_JSX_Mode::react,
.jsx = true,
};

constexpr Parser_Options jsx_auto_detect_options{
.jsx_mode = Parser_JSX_Mode::auto_detect,
.jsx = true,
};

constexpr Parser_Options jsx_none_options{
.jsx_mode = Parser_JSX_Mode::none,
.jsx = true,
};

TEST_F(Test_Parse_JSX_React, correctly_capitalized_attribute) {
test_parse_and_visit_module(u8R"(c = <td colSpan="2" />;)"_sv, no_diags,
jsx_options);
jsx_react_options);

test_parse_and_visit_module(u8R"(c = <div onClick={handler} />;)"_sv,
no_diags, jsx_options);
no_diags, jsx_react_options);
}

TEST_F(Test_Parse_JSX_React, event_attributes_should_be_camel_case) {
test_parse_and_visit_module(
u8"c = <div onclick={handler} />;"_sv, //
u8" ^^^^^^^ Diag_JSX_Event_Attribute_Should_Be_Camel_Case.attribute_name"_diag
u8"{.expected_attribute_name=onClick}"_diag, //
jsx_options);
jsx_react_options);

// TODO(strager): Should we also report that the handler's value is missing?
test_parse_and_visit_module(
u8"c = <div onclick />;"_sv, //
u8" ^^^^^^^ Diag_JSX_Event_Attribute_Should_Be_Camel_Case.attribute_name"_diag
u8"{.expected_attribute_name=onClick}"_diag, //
jsx_options);
jsx_react_options);

test_parse_and_visit_module(
u8"c = <div onmouseenter={handler} />;"_sv, //
u8" ^^^^^^^^^^^^ Diag_JSX_Event_Attribute_Should_Be_Camel_Case.attribute_name"_diag
u8"{.expected_attribute_name=onMouseEnter}"_diag, //
jsx_options);
jsx_react_options);

test_parse_and_visit_module(
u8"c = <div oncustomevent={handler} />;"_sv, //
u8" ^^^^^^^^^^^^^ Diag_JSX_Event_Attribute_Should_Be_Camel_Case.attribute_name"_diag
u8"{.expected_attribute_name=onCustomevent}"_diag, //
jsx_options);
jsx_react_options);
}

TEST_F(Test_Parse_JSX_React, miscapitalized_attribute) {
test_parse_and_visit_module(
u8"c = <td colspan=\"2\" />;"_sv, //
u8" ^^^^^^^ Diag_JSX_Attribute_Has_Wrong_Capitalization.attribute_name"_diag
u8"{.expected_attribute_name=colSpan}"_diag, //
jsx_options);
jsx_react_options);

test_parse_and_visit_module(
u8"c = <div onMouseenter={handler} />;"_sv, //
u8" ^^^^^^^^^^^^ Diag_JSX_Attribute_Has_Wrong_Capitalization.attribute_name"_diag
u8"{.expected_attribute_name=onMouseEnter}"_diag, //
jsx_options);
jsx_react_options);

test_parse_and_visit_module(
u8"c = <div onmouseENTER={handler} />;"_sv, //
u8" ^^^^^^^^^^^^ Diag_JSX_Attribute_Has_Wrong_Capitalization.attribute_name"_diag
u8"{.expected_attribute_name=onMouseEnter}"_diag, //
jsx_options);
jsx_react_options);
}

TEST_F(Test_Parse_JSX_React, commonly_misspelled_attribute) {
test_parse_and_visit_module(
u8"c = <span class=\"item\"></span>;"_sv, //
u8" ^^^^^ Diag_JSX_Attribute_Renamed_By_React.attribute_name"_diag
u8"{.react_attribute_name=className}"_diag, //
jsx_options);
jsx_react_options);
}

TEST_F(Test_Parse_JSX_React, attribute_checking_ignores_namespaced_attributes) {
test_parse_and_visit_module(u8R"(c = <div ns:onmouseenter={handler} />;)"_sv,
no_diags, jsx_options);
no_diags, jsx_react_options);
test_parse_and_visit_module(
u8R"(c = <div onmouseenter:onmouseenter={handler} />;)"_sv, no_diags,
jsx_options);
jsx_react_options);
test_parse_and_visit_module(u8R"(c = <div class:class="my-css-class" />;)"_sv,
no_diags, jsx_options);
no_diags, jsx_react_options);
}

TEST_F(Test_Parse_JSX_React, attribute_checking_ignores_namespaced_elements) {
test_parse_and_visit_module(u8R"(c = <svg:g onmouseenter={handler} />;)"_sv,
no_diags, jsx_options);
no_diags, jsx_react_options);
test_parse_and_visit_module(u8R"(c = <svg:g class="red" />;)"_sv, no_diags,
jsx_options);
jsx_react_options);
}

TEST_F(Test_Parse_JSX_React, attribute_checking_ignores_user_components) {
test_parse_and_visit_module(
u8R"(c = <MyComponent onmouseenter={handler} />;)"_sv, no_diags,
jsx_options);
jsx_react_options);

test_parse_and_visit_module(u8R"(c = <MyComponent class="red" />;)"_sv,
no_diags, jsx_options);
no_diags, jsx_react_options);

test_parse_and_visit_module(
u8R"(c = <mymodule.mycomponent onmouseenter={handler} />;)"_sv, no_diags,
jsx_options);
jsx_react_options);

test_parse_and_visit_module(
u8R"(c = <mymodule.mycomponent class="red" />;)"_sv, no_diags,
jsx_options);
jsx_react_options);
}

TEST_F(Test_Parse_JSX_React, no_diagnostic_if_not_react_mode) {
test_parse_and_visit_module(u8"c = <div onclick={handler} class=\"c\" />;"_sv,
no_diags, jsx_none_options);
}

TEST_F(Test_Parse_JSX_React,
no_diagnostic_if_auto_detect_and_react_not_imported) {
test_parse_and_visit_module(u8"c = <div onclick={handler} class=\"c\" />;"_sv,
no_diags, jsx_auto_detect_options);

for (String8_View import_code : {
// Non-React modules:
u8"import React from 'reactive-banana';"_sv,
u8"import ReactRouter from 'react-router';"_sv,
}) {
test_parse_and_visit_module(
concat(import_code, u8" c = <div onclick={handler} class=\"c\" />;"_sv),
no_diags, jsx_auto_detect_options);
}
}

TEST_F(Test_Parse_JSX_React, diagnostic_if_auto_detect_and_react_is_imported) {
for (String8_View import_code : {
u8"import React from 'react';"_sv,
u8"import React from \"react\";"_sv,
u8"import React from 'react-dom';"_sv,
u8"import React from 'react-dom/client';"_sv,
u8"import React from 'react-dom/server';"_sv,
u8"import someVariable from 'react';"_sv,
u8"import {someExport} from 'react';"_sv, u8"import 'react';"_sv,
// TODO(#1159): u8"import React from '\\u{72}eact';"_sv,
// TODO(strager): u8"const React = require('react');"
}) {
test_parse_and_visit_module(
concat(import_code, u8" c = <div onclick={handler} class=\"c\" />;"_sv),
u8"Diag_JSX_Event_Attribute_Should_Be_Camel_Case"_diag,
u8"Diag_JSX_Attribute_Renamed_By_React"_diag, jsx_auto_detect_options);
}

test_parse_and_visit_module(
u8"import React = require('react'); c = <div onclick={handler} class=\"c\" />;"_sv,
u8"Diag_JSX_Event_Attribute_Should_Be_Camel_Case"_diag,
u8"Diag_JSX_Attribute_Renamed_By_React"_diag,
Parser_Options{
.jsx_mode = Parser_JSX_Mode::auto_detect,
.jsx = true,
.typescript = true,
});

// import after the attribute should work too:
test_parse_and_visit_module(
u8"c = <div onclick={handler} class=\"c\" />; import React from 'react';"_sv,
u8"Diag_JSX_Event_Attribute_Should_Be_Camel_Case"_diag,
u8"Diag_JSX_Attribute_Renamed_By_React"_diag, jsx_auto_detect_options);
}
}
}

0 comments on commit a518996

Please sign in to comment.