feat(fe): warn on confusing operator precedence of & and >>

closes #1056

Co-authored-by: Matthew "strager" Glazar <[email protected]>

docs/errors/E0716.md

@@ -0,0 +1,25 @@
+# E0716: unintuitive operator precedence when using & and << or >>
+
+```config-for-examples
+{
+  "globals": {
+    "another_variable": true
+  }
+}
+```
+
+JavaScript's operator precedence can cause confusing situations and bugs when it
+comes to bitshifting with a bitwise and.
+```js
+let a_bitwise_variable = another_variable & 0x1 << 0x2
+```
+The code above looks like it evaluates `another_variable & 0x1` first,
+but it instead evaluates `0x1 << 0x2` first. 
+If this is intended behavior, it may be easier to read if you write parentheses:
+```js
+another_variable & (0x1 << 0x2)
+```
+Otherwise, to get the order of operations correct, add parentheses around the operands of `&`:
+```js
+(another_variable & 0x1) << 0x2
+```

po/messages.pot

@@ -2089,6 +2089,14 @@ msgstr ""
 msgid "export default previously appeared here"
 msgstr ""
 
+#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
+msgid "unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &"
+msgstr ""
+
+#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
+msgid "'&' here"
+msgstr ""
+
 #: test/test-diagnostic-formatter.cpp
 #: test/test-vim-qflist-json-diag-reporter.cpp
 msgid "something happened"

src/quick-lint-js/diag/diagnostic-metadata-generated.cpp

@@ -6425,6 +6425,24 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = {
         },
       },
     },
+
+    // Diag_Unintuitive_Bitshift_Precedence
+    {
+      .code = 716,
+      .severity = Diagnostic_Severity::warning,
+      .message_formats = {
+        QLJS_TRANSLATABLE("unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &"),
+        QLJS_TRANSLATABLE("'&' here"),
+      },
+      .message_args = {
+        {
+          Diagnostic_Message_Arg_Info(offsetof(Diag_Unintuitive_Bitshift_Precedence, bitshift_operator), Diagnostic_Arg_Type::source_code_span),
+        },
+        {
+          Diagnostic_Message_Arg_Info(offsetof(Diag_Unintuitive_Bitshift_Precedence, and_operator), Diagnostic_Arg_Type::source_code_span),
+        },
+      },
+    },
 };
 }
 

src/quick-lint-js/diag/diagnostic-metadata-generated.h

@@ -440,10 +440,11 @@ namespace quick_lint_js {
   QLJS_DIAG_TYPE_NAME(Diag_Class_Generator_On_Getter_Or_Setter) \
   QLJS_DIAG_TYPE_NAME(Diag_Class_Async_On_Getter_Or_Setter) \
   QLJS_DIAG_TYPE_NAME(Diag_Multiple_Export_Defaults) \
+  QLJS_DIAG_TYPE_NAME(Diag_Unintuitive_Bitshift_Precedence) \
   /* END */
 // clang-format on
 
-inline constexpr int Diag_Type_Count = 429;
+inline constexpr int Diag_Type_Count = 430;
 
 extern const Diagnostic_Info all_diagnostic_infos[Diag_Type_Count];
 }

src/quick-lint-js/diag/diagnostic-types-2.h

@@ -3318,6 +3318,18 @@ struct Diag_Multiple_Export_Defaults {
   Source_Code_Span second_export_default;
   Source_Code_Span first_export_default;
 };
+
+struct Diag_Unintuitive_Bitshift_Precedence {
+  [[qljs::diag("E0716", Diagnostic_Severity::warning)]]  //
+  // clang-format off
+  [[qljs::message("unintuitive operator precedence when using & and '{0}'; "
+                  "'{0}' evaluates before &",
+                  ARG(bitshift_operator))]]         //
+  // clang-format on
+  [[qljs::message("'&' here", ARG(and_operator))]]  //
+  Source_Code_Span bitshift_operator;
+  Source_Code_Span and_operator;
+};
 }
 QLJS_WARNING_POP
 

src/quick-lint-js/fe/parse-expression.cpp

@@ -73,6 +73,8 @@ void Parser::visit_expression(Expression* ast, Parse_Visitor_Base& v,
         expression_cast<Expression::Binary_Operator*>(ast));
     this->warn_on_xor_operator_as_exponentiation(
         expression_cast<Expression::Binary_Operator*>(ast));
+    this->warn_on_unintuitive_bitshift_precedence(
+        expression_cast<Expression::Binary_Operator*>(ast));
     break;
   case Expression_Kind::Trailing_Comma: {
     auto& trailing_comma_ast =

src/quick-lint-js/fe/parse.cpp

@@ -360,7 +360,24 @@ void Parser::warn_on_comma_operator_in_index(Expression* ast,
     }
   }
 }
-
+void Parser::warn_on_unintuitive_bitshift_precedence(Expression* ast) {
+  if (ast->kind() != Expression_Kind::Binary_Operator) return;
+  if (ast->child_count() <= 2) return;
+  auto* binary_op = static_cast<Expression::Binary_Operator*>(ast);
+  Source_Code_Span left_op = binary_op->operator_spans_[0];
+  Source_Code_Span right_op = binary_op->operator_spans_[1];
+  if (left_op.string_view() == u8"&"_sv &&
+      (right_op.string_view() == u8">>"_sv ||
+       right_op.string_view() == u8"<<"_sv)) {
+    if (binary_op->child(0)->kind() == Expression_Kind::Variable &&
+        binary_op->child(1)->kind() == Expression_Kind::Literal &&
+        binary_op->child(2)->kind() == Expression_Kind::Literal) {
+      this->diag_reporter_->report(
+          quick_lint_js::Diag_Unintuitive_Bitshift_Precedence{
+              .bitshift_operator = right_op, .and_operator = left_op});
+    }
+  }
+}
 void Parser::error_on_pointless_string_compare(
     Expression::Binary_Operator* ast) {
   auto is_comparison_operator = [](String8_View s) {

src/quick-lint-js/fe/parse.h

@@ -526,6 +526,7 @@ class Parser {
   void warn_on_comma_operator_in_conditional_statement(Expression *);
   void warn_on_comma_operator_in_index(Expression *, Source_Code_Span);
   void warn_on_xor_operator_as_exponentiation(Expression::Binary_Operator *);
+  void warn_on_unintuitive_bitshift_precedence(Expression *ast);
   void error_on_pointless_string_compare(Expression::Binary_Operator *);
   void error_on_pointless_compare_against_literal(
       Expression::Binary_Operator *);

src/quick-lint-js/i18n/translation-table-generated.cpp

@@ -18,7 +18,8 @@ const Translation_Table translation_data = {
         {74, 87, 79, 56, 0, 59},             //
         {71, 80, 60, 58, 0, 52},             //
         {0, 0, 0, 0, 0, 28},                 //
-        {31, 56, 0, 32, 0, 63},              //
+        {0, 0, 0, 0, 0, 63},                 //
+        {31, 56, 0, 32, 0, 9},               //
         {0, 0, 0, 0, 0, 67},                 //
         {0, 0, 0, 70, 0, 26},                //
         {79, 25, 30, 63, 49593, 66},         //
@@ -499,7 +500,8 @@ const Translation_Table translation_data = {
         {180, 42, 159, 156, 171, 156},       //
         {0, 0, 0, 0, 0, 65},                 //
         {92, 45, 78, 81, 70, 43},            //
-        {98, 37, 86, 82, 83, 77},            //
+        {0, 0, 0, 0, 0, 77},                 //
+        {98, 37, 86, 82, 83, 81},            //
         {38, 35, 17, 23, 13, 14},            //
         {38, 27, 34, 28, 33, 27},            //
         {26, 41, 26, 32, 0, 22},             //
@@ -1813,6 +1815,7 @@ const Translation_Table translation_data = {
         u8"\"globals\" descriptor must be a boolean or an object\0"
         u8"\"globals\" must be an object\0"
         u8"'!' here treated as the TypeScript non-null assertion operator\0"
+        u8"'&' here\0"
         u8"'**' operator cannot be used after unary '{1}' without parentheses\0"
         u8"',' should be ';' instead\0"
         u8"'.' is not allowed after generic arguments; write [\"{1}\"] instead\0"
@@ -2294,6 +2297,7 @@ const Translation_Table translation_data = {
         u8"unexpected token in variable declaration; expected variable name\0"
         u8"unexpected whitespace between '!' and '=='\0"
         u8"unicode byte order mark (BOM) cannot appear before #! at beginning of script\0"
+        u8"unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &\0"
         u8"unmatched '}'\0"
         u8"unmatched indexing bracket\0"
         u8"unmatched parenthesis\0"

src/quick-lint-js/i18n/translation-table-generated.h

@@ -18,8 +18,8 @@ namespace quick_lint_js {
 using namespace std::literals::string_view_literals;
 
 constexpr std::uint32_t translation_table_locale_count = 5;
-constexpr std::uint16_t translation_table_mapping_table_size = 524;
-constexpr std::size_t translation_table_string_table_size = 80043;
+constexpr std::uint16_t translation_table_mapping_table_size = 526;
+constexpr std::size_t translation_table_string_table_size = 80133;
 constexpr std::size_t translation_table_locale_table_size = 35;
 
 QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
@@ -33,6 +33,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
           "\"globals\" descriptor must be a boolean or an object"sv,
           "\"globals\" must be an object"sv,
           "'!' here treated as the TypeScript non-null assertion operator"sv,
+          "'&' here"sv,
           "'**' operator cannot be used after unary '{1}' without parentheses"sv,
           "',' should be ';' instead"sv,
           "'.' is not allowed after generic arguments; write [\"{1}\"] instead"sv,
@@ -514,6 +515,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
           "unexpected token in variable declaration; expected variable name"sv,
           "unexpected whitespace between '!' and '=='"sv,
           "unicode byte order mark (BOM) cannot appear before #! at beginning of script"sv,
+          "unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &"sv,
           "unmatched '}'"sv,
           "unmatched indexing bracket"sv,
           "unmatched parenthesis"sv,

src/quick-lint-js/i18n/translation-table-test-generated.h

@@ -27,7 +27,7 @@ struct Translated_String {
 };
 
 // clang-format off
-inline const Translated_String test_translation_table[523] = {
+inline const Translated_String test_translation_table[525] = {
     {
         "\"global-groups\" entries must be strings"_translatable,
         u8"\"global-groups\" entries must be strings",
@@ -105,6 +105,17 @@ inline const Translated_String test_translation_table[523] = {
             u8"'!' here treated as the TypeScript non-null assertion operator",
         },
     },
+    {
+        "'&' here"_translatable,
+        u8"'&' here",
+        {
+            u8"'&' here",
+            u8"'&' here",
+            u8"'&' here",
+            u8"'&' here",
+            u8"'&' here",
+        },
+    },
     {
         "'**' operator cannot be used after unary '{1}' without parentheses"_translatable,
         u8"'**' operator cannot be used after unary '{1}' without parentheses",
@@ -5396,6 +5407,17 @@ inline const Translated_String test_translation_table[523] = {
             u8"unicode byte ordningsm\u00e4rke (BOM) kan inte f\u00f6rekomma f\u00f6re #! i b\u00f6rjan av skript",
         },
     },
+    {
+        "unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &"_translatable,
+        u8"unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &",
+        {
+            u8"unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &",
+            u8"unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &",
+            u8"unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &",
+            u8"unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &",
+            u8"unintuitive operator precedence when using & and '{0}'; '{0}' evaluates before &",
+        },
+    },
     {
         "unmatched '}'"_translatable,
         u8"unmatched '}'",

test/test-parse-warning.cpp

@@ -436,6 +436,27 @@ TEST_F(Test_Parse_Warning, warn_on_xor_operation_used_as_exponentiation) {
   test_parse_and_visit_expression(u8"4 ^ 3"_sv, no_diags);
   test_parse_and_visit_expression(u8"(x+2)^a"_sv, no_diags);
 }
+TEST_F(Test_Parse_Warning, warn_on_unintuitive_precedence_when_using_bitshift) {
+  test_parse_and_visit_expression(
+      u8"var1 & 0x01 >> 0x02"_sv,
+      u8"            ^^ Diag_Unintuitive_Bitshift_Precedence.bitshift_operator\n"_diag
+      u8"     ^ .and_operator"_diag);
+  test_parse_and_visit_expression(
+      u8"var2 & 0x1234 << 0x4321"_sv,
+      u8"              ^^ Diag_Unintuitive_Bitshift_Precedence.bitshift_operator\n"_diag
+      u8"     ^ .and_operator"_diag);
+  test_parse_and_visit_statement(
+      u8"const x = a & 10 << 12"_sv,
+      u8"                 ^^ Diag_Unintuitive_Bitshift_Precedence.bitshift_operator\n"_diag
+      u8"            ^ .and_operator"_diag);
+  test_parse_and_visit_expression(u8"0x111 << 0x222 & var1"_sv, no_diags);
+  test_parse_and_visit_expression(u8"a&b>>c"_sv, no_diags);
+  test_parse_and_visit_expression(u8"x & y << z"_sv, no_diags);
+  test_parse_and_visit_expression(u8"0xABCD & 0xDCBA << 0xFFFF"_sv, no_diags);
+  test_parse_and_visit_expression(u8"(a & 0o12) >> 0xBEEF"_sv, no_diags);
+  test_parse_and_visit_expression(u8"a & (b >> 0xBEEF)"_sv, no_diags);
+  test_parse_and_visit_expression(u8"a & b >> c"_sv, no_diags);
+}
 }
 }