Repository: ruby/prism
Pull request #3168 (2024-10-08 14:49:32)

Avoid breaking code units offset on binary encoding
When a file is using binary encoding and contains multibyte characters, trying to call `encode` with one of the accepted LSP encodings (UTF-8, UTF-16 or UTF-32) will fail because it's not a valid conversion.

In situations like these, we want to avoid breaking, even if we can't provide the correct locations for nodes. In the included test, you can see that the locations are all using the number of bytes, rather than the code units.

eregon: `# encoding: binary` files with non-US-ASCII characters are kind of a broken concept/nasty edge case.
In both the example here and in https://github.com/Shopify/ruby-lsp/issues/2674 the actual encoding of the file is UTF-8, otherwise we wouldn't be able to show the ∂φ/∂x or smileys.

So maybe the best in that case is to consider the source as UTF-8 bytes, because BINARY is simply inaccurate and UTF-8 is by far the most likely actual encoding of the file?
I think it would then return the correct offsets for those two examples, yes?
eregon: > The issue is, if the file is Shift_JIS encoded (or some other encoding) then forcing it into UTF-8 is going to result in problems, even more so than replacing invalid/undef.

True, but does this ever happen?
At least so far all `# encoding: binary` files with non-US-ASCII characters I have seen are actually UTF-8.
And the reason for that `# encoding: binary` BTW in at least some of these ruby/spec files is to workaround a bug in the parser gem (used by RuboCop, used in ruby/spec CI), which couldn't handle things like "\xAA\xBB" with AA and BB some broken UTF-8:
https://github.com/whitequark/parser?tab=readme-ov-file#invalid-characters-inside-comments-and-literals

I wonder what editors do in such a case (since most would still display non-ASCII characters). Maybe they just assume the filesystem encoding which is UTF-8? (at least on Unix, not sure on Windows)
I think Ideally the LSP in this case would ask the editor which encoding it guessed and that would be passed to Prism, but I suspect there is no easy way to find that out.

I like your `def for(source)` approach, although I think we should go straight to `BrokenSource` if it's not valid UTF-8.

Patch:
diff --git a/config.yml b/config.yml
index 8bf62150..972f1e8e 100644
--- a/config.yml
+++ b/config.yml
@@ -141,7 +141,6 @@ errors:
   - INSTANCE_VARIABLE_BARE
   - INVALID_BLOCK_EXIT
   - INVALID_CHARACTER
-  - INVALID_COMMA
   - INVALID_ENCODING_MAGIC_COMMENT
   - INVALID_ESCAPE_CHARACTER
   - INVALID_FLOAT_EXPONENT
diff --git a/gemfiles/2.7/Gemfile.lock b/gemfiles/2.7/Gemfile.lock
index f0affb19..064e329b 100644
--- a/gemfiles/2.7/Gemfile.lock
+++ b/gemfiles/2.7/Gemfile.lock
@@ -12,10 +12,10 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
     rbs (3.1.3)
     test-unit (3.6.2)
diff --git a/gemfiles/3.0/Gemfile.lock b/gemfiles/3.0/Gemfile.lock
index 4d65d2ff..0793cc4a 100644
--- a/gemfiles/3.0/Gemfile.lock
+++ b/gemfiles/3.0/Gemfile.lock
@@ -17,12 +17,12 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
-    rbs (3.6.1)
+    rbs (3.6.0)
       logger
     ruby_memcheck (3.0.0)
       nokogiri
diff --git a/gemfiles/3.1/Gemfile.lock b/gemfiles/3.1/Gemfile.lock
index c2716a5e..d7364ee1 100644
--- a/gemfiles/3.1/Gemfile.lock
+++ b/gemfiles/3.1/Gemfile.lock
@@ -17,12 +17,12 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
-    rbs (3.6.1)
+    rbs (3.6.0)
       logger
     ruby_memcheck (3.0.0)
       nokogiri
diff --git a/gemfiles/3.2/Gemfile.lock b/gemfiles/3.2/Gemfile.lock
index e548d428..d7f117d0 100644
--- a/gemfiles/3.2/Gemfile.lock
+++ b/gemfiles/3.2/Gemfile.lock
@@ -17,12 +17,12 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
-    rbs (3.6.1)
+    rbs (3.6.0)
       logger
     ruby_memcheck (3.0.0)
       nokogiri
diff --git a/gemfiles/3.3/Gemfile.lock b/gemfiles/3.3/Gemfile.lock
index 6748849c..0267cfee 100644
--- a/gemfiles/3.3/Gemfile.lock
+++ b/gemfiles/3.3/Gemfile.lock
@@ -17,12 +17,12 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
-    rbs (3.6.1)
+    rbs (3.6.0)
       logger
     ruby_memcheck (3.0.0)
       nokogiri
diff --git a/gemfiles/3.4/Gemfile.lock b/gemfiles/3.4/Gemfile.lock
index 9b4fe386..146a4d7b 100644
--- a/gemfiles/3.4/Gemfile.lock
+++ b/gemfiles/3.4/Gemfile.lock
@@ -17,12 +17,12 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
-    rbs (3.6.1)
+    rbs (3.6.0)
       logger
     ruby_memcheck (3.0.0)
       nokogiri
diff --git a/gemfiles/jruby/Gemfile.lock b/gemfiles/jruby/Gemfile.lock
index 2ef9948e..d17b1448 100644
--- a/gemfiles/jruby/Gemfile.lock
+++ b/gemfiles/jruby/Gemfile.lock
@@ -10,10 +10,10 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1-java)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
     test-unit (3.6.2)
       power_assert
diff --git a/gemfiles/truffleruby/Gemfile.lock b/gemfiles/truffleruby/Gemfile.lock
index 05a6af5f..d8fc6255 100644
--- a/gemfiles/truffleruby/Gemfile.lock
+++ b/gemfiles/truffleruby/Gemfile.lock
@@ -10,10 +10,10 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
+    power_assert (2.0.3)
     racc (1.8.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
     test-unit (3.6.2)
       power_assert
diff --git a/gemfiles/typecheck/Gemfile.lock b/gemfiles/typecheck/Gemfile.lock
index 1812bb06..459bc463 100644
--- a/gemfiles/typecheck/Gemfile.lock
+++ b/gemfiles/typecheck/Gemfile.lock
@@ -22,7 +22,7 @@ GEM
     erubi (1.13.0)
     ffi (1.17.0)
     fileutils (1.7.2)
-    i18n (1.14.6)
+    i18n (1.14.5)
       concurrent-ruby (~> 1.0)
     json (2.7.2)
     language_server-protocol (3.17.0.3)
@@ -36,40 +36,40 @@ GEM
     parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
-    power_assert (2.0.4)
-    prism (1.1.0)
+    power_assert (2.0.3)
+    prism (1.0.0)
     racc (1.8.1)
     rainbow (3.1.1)
     rake (13.2.1)
-    rake-compiler (1.2.8)
+    rake-compiler (1.2.7)
       rake
     rb-fsevent (0.11.2)
     rb-inotify (0.11.1)
       ffi (~> 1.0)
-    rbi (0.2.1)
+    rbi (0.2.0)
       prism (~> 1.0)
       sorbet-runtime (>= 0.5.9204)
-    rbs (3.6.1)
+    rbs (3.6.0)
       logger
     ruby_parser (3.21.1)
       racc (~> 1.5)
       sexp_processor (~> 4.16)
     securerandom (0.3.1)
     sexp_processor (4.17.2)
-    sorbet (0.5.11597)
-      sorbet-static (= 0.5.11597)
-    sorbet-runtime (0.5.11597)
-    sorbet-static (0.5.11597-universal-darwin)
-    sorbet-static (0.5.11597-x86_64-linux)
-    sorbet-static-and-runtime (0.5.11597)
-      sorbet (= 0.5.11597)
-      sorbet-runtime (= 0.5.11597)
-    spoom (1.5.0)
+    sorbet (0.5.11577)
+      sorbet-static (= 0.5.11577)
+    sorbet-runtime (0.5.11577)
+    sorbet-static (0.5.11577-universal-darwin)
+    sorbet-static (0.5.11577-x86_64-linux)
+    sorbet-static-and-runtime (0.5.11577)
+      sorbet (= 0.5.11577)
+      sorbet-runtime (= 0.5.11577)
+    spoom (1.4.2)
       erubi (>= 1.10.0)
       prism (>= 0.28.0)
       sorbet-static-and-runtime (>= 0.5.10187)
       thor (>= 0.19.2)
-    steep (1.8.0)
+    steep (1.8.0.dev.2)
       activesupport (>= 5.1)
       concurrent-ruby (>= 1.1.10)
       csv (>= 3.0.9)
@@ -80,12 +80,12 @@ GEM
       logger (>= 1.3.0)
       parser (>= 3.1)
       rainbow (>= 2.2.2, < 4.0)
-      rbs (~> 3.6.0)
+      rbs (>= 3.5.0.pre)
       securerandom (>= 0.1)
       strscan (>= 1.0.0)
       terminal-table (>= 2, < 4)
     strscan (3.1.0)
-    tapioca (0.16.3)
+    tapioca (0.16.2)
       bundler (>= 2.2.25)
       netrc (>= 0.11.0)
       parallel (>= 1.21.0)
@@ -101,7 +101,7 @@ GEM
     thor (1.3.2)
     tzinfo (2.0.6)
       concurrent-ruby (~> 1.0)
-    unicode-display_width (2.6.0)
+    unicode-display_width (2.5.0)
     yard (0.9.37)
     yard-sorbet (0.9.0)
       sorbet-runtime
diff --git a/java-wasm/pom.xml b/java-wasm/pom.xml
index 62ba45c9..2bbf67dd 100644
--- a/java-wasm/pom.xml
+++ b/java-wasm/pom.xml
@@ -15,7 +15,7 @@
     <maven.compiler.source>11</maven.compiler.source>
     <maven.compiler.target>11</maven.compiler.target>
 
-    <junit.version>5.11.2</junit.version>
+    <junit.version>5.11.1</junit.version>
   </properties>
 
   <dependencies>
@@ -37,7 +37,7 @@
       <plugin>
           <groupId>org.apache.maven.plugins</groupId>
           <artifactId>maven-surefire-plugin</artifactId>
-          <version>3.5.1</version>
+          <version>3.5.0</version>
       </plugin>
     </plugins>
   </build>
diff --git a/lib/prism/parse_result.rb b/lib/prism/parse_result.rb
index ae026b42..aea5dee9 100644
--- a/lib/prism/parse_result.rb
+++ b/lib/prism/parse_result.rb
@@ -90,7 +90,7 @@ module Prism
     # concept of code units that differs from the number of characters in other
     # encodings, it is not captured here.
     def code_units_offset(byte_offset, encoding)
-      byteslice = (source.byteslice(0, byte_offset) or raise).encode(encoding)
+      byteslice = (source.byteslice(0, byte_offset) or raise).encode(encoding, invalid: :replace, undef: :replace)
 
       if encoding == Encoding::UTF_16LE || encoding == Encoding::UTF_16BE
         byteslice.bytesize / 2
diff --git a/src/prism.c b/src/prism.c
index 208b88d9..b526b599 100644
--- a/src/prism.c
+++ b/src/prism.c
@@ -2107,6 +2107,14 @@ pm_array_node_create(pm_parser_t *parser, const pm_token_t *opening) {
     return node;
 }
 
+/**
+ * Return the size of the given array node.
+ */
+static inline size_t
+pm_array_node_size(pm_array_node_t *node) {
+    return node->elements.size;
+}
+
 /**
  * Append an argument to an array node.
  */
@@ -9877,10 +9885,6 @@ escape_read(pm_parser_t *parser, pm_buffer_t *buffer, pm_buffer_t *regular_expre
         }
         case 'c': {
             parser->current.end++;
-            if (flags & PM_ESCAPE_FLAG_CONTROL) {
-                pm_parser_err_current(parser, PM_ERR_ESCAPE_INVALID_CONTROL_REPEAT);
-            }
-
             if (parser->current.end == parser->end) {
                 pm_parser_err_current(parser, PM_ERR_ESCAPE_INVALID_CONTROL);
                 return;
@@ -9894,6 +9898,10 @@ escape_read(pm_parser_t *parser, pm_buffer_t *buffer, pm_buffer_t *regular_expre
                     return;
                 }
                 case '\\':
+                    if (flags & PM_ESCAPE_FLAG_CONTROL) {
+                        pm_parser_err_current(parser, PM_ERR_ESCAPE_INVALID_CONTROL_REPEAT);
+                        return;
+                    }
                     parser->current.end++;
 
                     if (match(parser, 'u') || match(parser, 'U')) {
@@ -9927,10 +9935,6 @@ escape_read(pm_parser_t *parser, pm_buffer_t *buffer, pm_buffer_t *regular_expre
         }
         case 'C': {
             parser->current.end++;
-            if (flags & PM_ESCAPE_FLAG_CONTROL) {
-                pm_parser_err_current(parser, PM_ERR_ESCAPE_INVALID_CONTROL_REPEAT);
-            }
-
             if (peek(parser) != '-') {
                 size_t width = parser->encoding->char_width(parser->current.end, parser->end - parser->current.end);
                 pm_parser_err(parser, parser->current.start, parser->current.end + width, PM_ERR_ESCAPE_INVALID_CONTROL);
@@ -9951,6 +9955,10 @@ escape_read(pm_parser_t *parser, pm_buffer_t *buffer, pm_buffer_t *regular_expre
                     return;
                 }
                 case '\\':
+                    if (flags & PM_ESCAPE_FLAG_CONTROL) {
+                        pm_parser_err_current(parser, PM_ERR_ESCAPE_INVALID_CONTROL_REPEAT);
+                        return;
+                    }
                     parser->current.end++;
 
                     if (match(parser, 'u') || match(parser, 'U')) {
@@ -9985,10 +9993,6 @@ escape_read(pm_parser_t *parser, pm_buffer_t *buffer, pm_buffer_t *regular_expre
         }
         case 'M': {
             parser->current.end++;
-            if (flags & PM_ESCAPE_FLAG_META) {
-                pm_parser_err_current(parser, PM_ERR_ESCAPE_INVALID_META_REPEAT);
-            }
-
             if (peek(parser) != '-') {
                 size_t width = parser->encoding->char_width(parser->current.end, parser->end - parser->current.end);
                 pm_parser_err(parser, parser->current.start, parser->current.end + width, PM_ERR_ESCAPE_INVALID_META);
@@ -10004,6 +10008,10 @@ escape_read(pm_parser_t *parser, pm_buffer_t *buffer, pm_buffer_t *regular_expre
             uint8_t peeked = peek(parser);
             switch (peeked) {
                 case '\\':
+                    if (flags & PM_ESCAPE_FLAG_META) {
+                        pm_parser_err_current(parser, PM_ERR_ESCAPE_INVALID_META_REPEAT);
+                        return;
+                    }
                     parser->current.end++;
 
                     if (match(parser, 'u') || match(parser, 'U')) {
@@ -10046,8 +10054,6 @@ escape_read(pm_parser_t *parser, pm_buffer_t *buffer, pm_buffer_t *regular_expre
         default: {
             if (parser->current.end < parser->end) {
                 escape_write_escape_encoded(parser, buffer);
-            } else {
-                pm_parser_err_current(parser, PM_ERR_INVALID_ESCAPE_CHARACTER);
             }
             return;
         }
@@ -12983,7 +12989,7 @@ pm_binding_powers_t pm_binding_powers[PM_TOKEN_MAXIMUM] = {
     [PM_TOKEN_PERCENT] = LEFT_ASSOCIATIVE(PM_BINDING_POWER_FACTOR),
     [PM_TOKEN_SLASH] = LEFT_ASSOCIATIVE(PM_BINDING_POWER_FACTOR),
     [PM_TOKEN_STAR] = LEFT_ASSOCIATIVE(PM_BINDING_POWER_FACTOR),
-    [PM_TOKEN_USTAR] = RIGHT_ASSOCIATIVE_UNARY(PM_BINDING_POWER_FACTOR),
+    [PM_TOKEN_USTAR] = LEFT_ASSOCIATIVE(PM_BINDING_POWER_FACTOR),
 
     // -@
     [PM_TOKEN_UMINUS] = RIGHT_ASSOCIATIVE_UNARY(PM_BINDING_POWER_UMINUS),
@@ -14113,8 +14119,8 @@ static void
 parse_arguments(pm_parser_t *parser, pm_arguments_t *arguments, bool accepts_forwarding, pm_token_type_t terminator, uint16_t depth) {
     pm_binding_power_t binding_power = pm_binding_powers[parser->current.type].left;
 
-    // First we need to check if the next token is one that could be the start
-    // of an argument. If it's not, then we can just return.
+    // First we need to check if the next token is one that could be the start of
+    // an argument. If it's not, then we can just return.
     if (
         match2(parser, terminator, PM_TOKEN_EOF) ||
         (binding_power != PM_BINDING_POWER_UNSET && binding_power < PM_BINDING_POWER_RANGE) ||
@@ -14302,32 +14308,23 @@ parse_arguments(pm_parser_t *parser, pm_arguments_t *arguments, bool accepts_for
         // If parsing the argument failed, we need to stop parsing arguments.
         if (PM_NODE_TYPE_P(argument, PM_MISSING_NODE) || parser->recovering) break;
 
-        // If the terminator of these arguments is not EOF, then we have a
-        // specific token we're looking for. In that case we can accept a
-        // newline here because it is not functioning as a statement terminator.
-        bool accepted_newline = false;
-        if (terminator != PM_TOKEN_EOF) {
-            accepted_newline = accept1(parser, PM_TOKEN_NEWLINE);
-        }
+        // If the terminator of these arguments is not EOF, then we have a specific
+        // token we're looking for. In that case we can accept a newline here
+        // because it is not functioning as a statement terminator.
+        if (terminator != PM_TOKEN_EOF) accept1(parser, PM_TOKEN_NEWLINE);
 
         if (parser->previous.type == PM_TOKEN_COMMA && parsed_bare_hash) {
-            // If we previously were on a comma and we just parsed a bare hash,
-            // then we want to continue parsing arguments. This is because the
-            // comma was grabbed up by the hash parser.
-        } else if (accept1(parser, PM_TOKEN_COMMA)) {
-            // If there was a comma, then we need to check if we also accepted a
-            // newline. If we did, then this is a syntax error.
-            if (accepted_newline) {
-                pm_parser_err_previous(parser, PM_ERR_INVALID_COMMA);
-            }
+            // If we previously were on a comma and we just parsed a bare hash, then
+            // we want to continue parsing arguments. This is because the comma was
+            // grabbed up by the hash parser.
         } else {
-            // If there is no comma at the end of the argument list then we're
-            // done parsing arguments and can break out of this loop.
-            break;
+            // If there is no comma at the end of the argument list then we're done
+            // parsing arguments and can break out of this loop.
+            if (!accept1(parser, PM_TOKEN_COMMA)) break;
         }
 
-        // If we hit the terminator, then that means we have a trailing comma so
-        // we can accept that output as well.
+        // If we hit the terminator, then that means we have a trailing comma so we
+        // can accept that output as well.
         if (match1(parser, terminator)) break;
     }
 }
@@ -14484,14 +14481,13 @@ parse_parameters(
     bool accepts_blocks_in_defaults,
     uint16_t depth
 ) {
-    pm_do_loop_stack_push(parser, false);
-
     pm_parameters_node_t *params = pm_parameters_node_create(parser);
-    pm_parameters_order_t order = PM_PARAMETERS_ORDER_NONE;
+    bool looping = true;
 
-    while (true) {
-        bool parsing = true;
+    pm_do_loop_stack_push(parser, false);
+    pm_parameters_order_t order = PM_PARAMETERS_ORDER_NONE;
 
+    do {
         switch (parser->current.type) {
             case PM_TOKEN_PARENTHESIS_LEFT: {
                 update_parameter_state(parser, &parser->current, &order);
@@ -14626,7 +14622,7 @@ parse_parameters(
                     // then we can put a missing node in its place and stop parsing the
                     // parameters entirely now.
                     if (parser->recovering) {
-                        parsing = false;
+                        looping = false;
                         break;
                     }
                 } else if (order > PM_PARAMETERS_ORDER_AFTER_OPTIONAL) {
@@ -14684,7 +14680,7 @@ parse_parameters(
                         context_pop(parser);
 
                         if (uses_parentheses) {
-                            parsing = false;
+                            looping = false;
                             break;
                         }
 
@@ -14728,7 +14724,7 @@ parse_parameters(
                         // then we can put a missing node in its place and stop parsing the
                         // parameters entirely now.
                         if (parser->recovering) {
-                            parsing = false;
+                            looping = false;
                             break;
                         }
                     }
@@ -14830,31 +14826,14 @@ parse_parameters(
                     }
                 }
 
-                parsing = false;
+                looping = false;
                 break;
         }
 
-        // If we hit some kind of issue while parsing the parameter, this would
-        // have been set to false. In that case, we need to break out of the
-        // loop.
-        if (!parsing) break;
-
-        bool accepted_newline = false;
-        if (uses_parentheses) {
-            accepted_newline = accept1(parser, PM_TOKEN_NEWLINE);
-        }
-
-        if (accept1(parser, PM_TOKEN_COMMA)) {
-            // If there was a comma, but we also accepted a newline, then this
-            // is a syntax error.
-            if (accepted_newline) {
-                pm_parser_err_previous(parser, PM_ERR_INVALID_COMMA);
-            }
-        } else {
-            // If there was no comma, then we're done parsing parameters.
-            break;
+        if (looping && uses_parentheses) {
+            accept1(parser, PM_TOKEN_NEWLINE);
         }
-    }
+    } while (looping && accept1(parser, PM_TOKEN_COMMA));
 
     pm_do_loop_stack_pop(parser);
 
@@ -17993,31 +17972,19 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, b
             bool parsed_bare_hash = false;
 
             while (!match2(parser, PM_TOKEN_BRACKET_RIGHT, PM_TOKEN_EOF)) {
-                bool accepted_newline = accept1(parser, PM_TOKEN_NEWLINE);
-
                 // Handle the case where we don't have a comma and we have a
                 // newline followed by a right bracket.
-                if (accepted_newline && match1(parser, PM_TOKEN_BRACKET_RIGHT)) {
+                if (accept1(parser, PM_TOKEN_NEWLINE) && match1(parser, PM_TOKEN_BRACKET_RIGHT)) {
                     break;
                 }
 
                 // Ensure that we have a comma between elements in the array.
-                if (array->elements.size > 0) {
-                    if (accept1(parser, PM_TOKEN_COMMA)) {
-                        // If there was a comma but we also accepts a newline,
-                        // then this is a syntax error.
-                        if (accepted_newline) {
-                            pm_parser_err_previous(parser, PM_ERR_INVALID_COMMA);
-                        }
-                    } else {
-                        // If there was no comma, then we need to add a syntax
-                        // error.
-                        const uint8_t *location = parser->previous.end;
-                        PM_PARSER_ERR_FORMAT(parser, location, location, PM_ERR_ARRAY_SEPARATOR, pm_token_type_human(parser->current.type));
+                if ((pm_array_node_size(array) != 0) && !accept1(parser, PM_TOKEN_COMMA)) {
+                    const uint8_t *location = parser->previous.end;
+                    PM_PARSER_ERR_FORMAT(parser, location, location, PM_ERR_ARRAY_SEPARATOR, pm_token_type_human(parser->current.type));
 
-                        parser->previous.start = location;
-                        parser->previous.type = PM_TOKEN_MISSING;
-                    }
+                    parser->previous.start = location;
+                    parser->previous.type = PM_TOKEN_MISSING;
                 }
 
                 // If we have a right bracket immediately following a comma,
@@ -21765,11 +21732,8 @@ parse_expression(pm_parser_t *parser, pm_binding_power_t binding_power, bool acc
     // Otherwise we'll look and see if the next token can be parsed as an infix
     // operator. If it can, then we'll parse it using parse_expression_infix.
     pm_binding_powers_t current_binding_powers;
-    pm_token_type_t current_token_type;
-
     while (
-        current_token_type = parser->current.type,
-        current_binding_powers = pm_binding_powers[current_token_type],
+        current_binding_powers = pm_binding_powers[parser->current.type],
         binding_power <= current_binding_powers.left &&
         current_binding_powers.binary
      ) {
@@ -21810,13 +21774,6 @@ parse_expression(pm_parser_t *parser, pm_binding_power_t binding_power, bool acc
         // If the operator is nonassoc and we should not be able to parse the
         // upcoming infix operator, break.
         if (current_binding_powers.nonassoc) {
-            // If this is a non-assoc operator and we are about to parse the
-            // exact same operator, then we need to add an error.
-            if (match1(parser, current_token_type)) {
-                PM_PARSER_ERR_TOKEN_FORMAT(parser, parser->current, PM_ERR_NON_ASSOCIATIVE_OPERATOR, pm_token_type_human(parser->current.type), pm_token_type_human(current_token_type));
-                break;
-            }
-
             // If this is an endless range, then we need to reject a couple of
             // additional operators because it violates the normal operator
             // precedence rules. Those patterns are:
@@ -21826,7 +21783,7 @@ parse_expression(pm_parser_t *parser, pm_binding_power_t binding_power, bool acc
             //
             if (PM_NODE_TYPE_P(node, PM_RANGE_NODE) && ((pm_range_node_t *) node)->right == NULL) {
                 if (match4(parser, PM_TOKEN_UAMPERSAND, PM_TOKEN_USTAR, PM_TOKEN_DOT, PM_TOKEN_AMPERSAND_DOT)) {
-                    PM_PARSER_ERR_TOKEN_FORMAT(parser, parser->current, PM_ERR_NON_ASSOCIATIVE_OPERATOR, pm_token_type_human(parser->current.type), pm_token_type_human(current_token_type));
+                    PM_PARSER_ERR_TOKEN_FORMAT(parser, parser->current, PM_ERR_NON_ASSOCIATIVE_OPERATOR, pm_token_type_human(parser->current.type), pm_token_type_human(parser->previous.type));
                     break;
                 }
 
@@ -21948,7 +21905,6 @@ wrap_statements(pm_parser_t *parser, pm_statements_node_t *statements) {
             ));
 
             pm_arguments_node_arguments_append(arguments, (pm_node_t *) keywords);
-            pm_node_flag_set((pm_node_t *) arguments, PM_ARGUMENTS_NODE_FLAGS_CONTAINS_KEYWORDS);
         }
 
         pm_statements_node_t *wrapped_statements = pm_statements_node_create(parser);
diff --git a/templates/src/diagnostic.c.erb b/templates/src/diagnostic.c.erb
index c0ec6965..d2b7b4f6 100644
--- a/templates/src/diagnostic.c.erb
+++ b/templates/src/diagnostic.c.erb
@@ -223,7 +223,6 @@ static const pm_diagnostic_data_t diagnostic_messages[PM_DIAGNOSTIC_ID_MAX] = {
     [PM_ERR_INCOMPLETE_VARIABLE_INSTANCE]       = { "'%.*s' is not allowed as an instance variable name", PM_ERROR_LEVEL_SYNTAX },
     [PM_ERR_INSTANCE_VARIABLE_BARE]             = { "'@' without identifiers is not allowed as an instance variable name", PM_ERROR_LEVEL_SYNTAX },
     [PM_ERR_INVALID_BLOCK_EXIT]                 = { "Invalid %s", PM_ERROR_LEVEL_SYNTAX },
-    [PM_ERR_INVALID_COMMA]                      = { "invalid comma", PM_ERROR_LEVEL_SYNTAX },
     [PM_ERR_INVALID_ESCAPE_CHARACTER]           = { "Invalid escape character syntax", PM_ERROR_LEVEL_SYNTAX },
     [PM_ERR_INVALID_FLOAT_EXPONENT]             = { "invalid exponent", PM_ERROR_LEVEL_SYNTAX },
     [PM_ERR_INVALID_LOCAL_VARIABLE_READ]        = { "identifier %.*s is not valid to get", PM_ERROR_LEVEL_SYNTAX },


Test patch:
diff --git a/test/prism/api/command_line_test.rb b/test/prism/api/command_line_test.rb
index e53d1870..a8c43551 100644
--- a/test/prism/api/command_line_test.rb
+++ b/test/prism/api/command_line_test.rb
@@ -52,9 +52,6 @@ module Prism
       assert_kind_of CallNode, predicate
       assert_equal :gets, predicate.name
 
-      arguments = predicate.arguments
-      assert arguments.contains_keywords?
-
       arguments = predicate.arguments.arguments
       assert_equal 2, arguments.length
       assert_equal :$/, arguments.first.name
diff --git a/test/prism/errors/arguments_invalid_comma.txt b/test/prism/errors/arguments_invalid_comma.txt
deleted file mode 100644
index 4e136058..00000000
--- a/test/prism/errors/arguments_invalid_comma.txt
+++ /dev/null
@@ -1,4 +0,0 @@
-p(a,b
-,)
-^ invalid comma
-
diff --git a/test/prism/errors/array_invalid_comma.txt b/test/prism/errors/array_invalid_comma.txt
deleted file mode 100644
index 2f52a253..00000000
--- a/test/prism/errors/array_invalid_comma.txt
+++ /dev/null
@@ -1,4 +0,0 @@
-[a
-,]
-^ invalid comma
-
diff --git a/test/prism/errors/binary_range_with_left_unary_range.txt b/test/prism/errors/binary_range_with_left_unary_range.txt
index 85cf55fb..37e41f39 100644
--- a/test/prism/errors/binary_range_with_left_unary_range.txt
+++ b/test/prism/errors/binary_range_with_left_unary_range.txt
@@ -2,7 +2,6 @@
    ^~ unexpected range operator; .. and ... are non-associative and cannot be chained
 ...1..
     ^~ unexpected range operator; .. and ... are non-associative and cannot be chained
-    ^~ unexpected ..; .. is a non-associative operator
     ^~ unexpected .., expecting end-of-input
     ^~ unexpected .., ignoring it
 
diff --git a/test/prism/errors/non_assoc_equality.txt b/test/prism/errors/non_assoc_equality.txt
index 9b3f1375..6ce8da88 100644
--- a/test/prism/errors/non_assoc_equality.txt
+++ b/test/prism/errors/non_assoc_equality.txt
@@ -1,25 +1,19 @@
 1 == 2 == 3
-       ^~ unexpected '=='; '==' is a non-associative operator
        ^~ unexpected '==', expecting end-of-input
        ^~ unexpected '==', ignoring it
 1 != 2 != 3
-       ^~ unexpected '!='; '!=' is a non-associative operator
        ^~ unexpected '!=', expecting end-of-input
        ^~ unexpected '!=', ignoring it
 1 === 2 === 3
-        ^~~ unexpected '==='; '===' is a non-associative operator
         ^~~ unexpected '===', expecting end-of-input
         ^~~ unexpected '===', ignoring it
 1 =~ 2 =~ 3
-       ^~ unexpected '=~'; '=~' is a non-associative operator
        ^~ unexpected '=~', expecting end-of-input
        ^~ unexpected '=~', ignoring it
 1 !~ 2 !~ 3
-       ^~ unexpected '!~'; '!~' is a non-associative operator
        ^~ unexpected '!~', expecting end-of-input
        ^~ unexpected '!~', ignoring it
 1 <=> 2 <=> 3
-        ^~~ unexpected '<=>'; '<=>' is a non-associative operator
         ^~~ unexpected '<=>', expecting end-of-input
         ^~~ unexpected '<=>', ignoring it
 
diff --git a/test/prism/errors/parameters_invalid_comma.txt b/test/prism/errors/parameters_invalid_comma.txt
deleted file mode 100644
index 2d22f7ca..00000000
--- a/test/prism/errors/parameters_invalid_comma.txt
+++ /dev/null
@@ -1,4 +0,0 @@
-def f(a
-,b);end
-^ invalid comma
-
diff --git a/test/prism/errors/range_and_bin_op.txt b/test/prism/errors/range_and_bin_op.txt
index 55928c40..4a7a396d 100644
--- a/test/prism/errors/range_and_bin_op.txt
+++ b/test/prism/errors/range_and_bin_op.txt
@@ -1,5 +1,4 @@
 1..2..3
-    ^~ unexpected ..; .. is a non-associative operator
     ^~ unexpected .., expecting end-of-input
     ^~ unexpected .., ignoring it
 
diff --git a/test/prism/errors/range_and_bin_op_2.txt b/test/prism/errors/range_and_bin_op_2.txt
index 6ca91a26..f2a31dcf 100644
--- a/test/prism/errors/range_and_bin_op_2.txt
+++ b/test/prism/errors/range_and_bin_op_2.txt
@@ -1,5 +1,4 @@
 1..2..
-    ^~ unexpected ..; .. is a non-associative operator
     ^~ unexpected .., expecting end-of-input
     ^~ unexpected .., ignoring it
 
diff --git a/test/prism/ruby/location_test.rb b/test/prism/ruby/location_test.rb
index fc80a5b8..e360a0db 100644
--- a/test/prism/ruby/location_test.rb
+++ b/test/prism/ruby/location_test.rb
@@ -140,6 +140,25 @@ module Prism
       assert_equal 7, location.end_code_units_column(Encoding::UTF_32LE)
     end
 
+    def test_code_units_handles_binary_encoding_with_multibyte_characters
+      # If the encoding is set to binary and the source contains multibyte
+      # characters, we avoid breaking the code unit offsets, but they will
+      # still be incorrect.
+
+      program = Prism.parse(<<~RUBY).value
+        # -*- encoding: binary -*-
+
+        😀 + 😀
+      RUBY
+
+      # first 😀
+      location = program.statements.body.first.receiver.location
+
+      assert_equal 4, location.end_code_units_column(Encoding::UTF_8)
+      assert_equal 4, location.end_code_units_column(Encoding::UTF_16LE)
+      assert_equal 4, location.end_code_units_column(Encoding::UTF_32LE)
+    end
+
     def test_chop
       location = Prism.parse("foo").value.location