Repository: ruby/yarp
Pull request #1322 (2023-08-24 15:10:44)

Fix lex compat with BOM
* BOM should not impact looking for the encoding string
* We should re-encode tokens when the encoding changes
* BOM should change the column of comments only

Fixes one more file in the remaining parse errors.

kddnewton: > Do you have a test with the BOM?

We do now!

Patch:
diff --git a/ext/yarp/extension.c b/ext/yarp/extension.c
index 455cdcad..8aef456c 100644
--- a/ext/yarp/extension.c
+++ b/ext/yarp/extension.c
@@ -221,6 +221,20 @@ static void
 lex_encoding_changed_callback(yp_parser_t *parser) {
     lex_data_t *lex_data = (lex_data_t *) parser->lex_callback->data;
     lex_data->encoding = rb_enc_find(parser->encoding.name);
+
+    // Since we got a new encoding, we need to go back and change the encoding
+    // of the tokens that we've already lexed. This should be a tiny amount
+    // since encoding magic comments need to be the first or second line of the
+    // file.
+    VALUE tokens = lex_data->tokens;
+    for (long index = 0; index < RARRAY_LEN(tokens); index++) {
+        VALUE yields = rb_ary_entry(tokens, index);
+        VALUE token = rb_ary_entry(yields, 0);
+
+        VALUE value = rb_ivar_get(token, rb_intern("@value"));
+        rb_enc_associate(value, lex_data->encoding);
+        ENC_CODERANGE_CLEAR(value);
+    }
 }
 
 // Return an array of tokens corresponding to the given source.
diff --git a/include/yarp.h b/include/yarp.h
index 07848319..6b1f3b4e 100644
--- a/include/yarp.h
+++ b/include/yarp.h
@@ -30,7 +30,7 @@ void yp_serialize_content(yp_parser_t *parser, yp_node_t *node, yp_buffer_t *buf
 
 void yp_print_node(yp_parser_t *parser, yp_node_t *node);
 
-// Generate a scope node from the given node.
+// Generate a scope node for a DefNode and ClassNode
 void yp_scope_node_init(yp_node_t *node, yp_scope_node_t *dest);
 
 // The YARP version and the serialization format.
diff --git a/lib/yarp/lex_compat.rb b/lib/yarp/lex_compat.rb
index 8362b906..a9867737 100644
--- a/lib/yarp/lex_compat.rb
+++ b/lib/yarp/lex_compat.rb
@@ -574,19 +574,41 @@ module YARP
       result = YARP.lex(source, @filepath)
       result_value = result.value
       previous_state = nil
-
-      # If there's a UTF-8 byte-order mark as the start of the file, then ripper
-      # sets every token's on the first line back by 6 bytes. It also keeps the
-      # byte order mark in the first token's value. This is weird, and I don't
-      # want to mirror that in our parser. So instead, we'll match up the values
-      # here, and then match up the locations as we process the tokens.
-      bom = source.bytes[0..2] == [0xEF, 0xBB, 0xBF]
-      result_value[0][0].value.prepend("\xEF\xBB\xBF") if bom
+      bom = source.byteslice(0..2) == "\xEF\xBB\xBF"
 
       result_value.each_with_index do |(token, lex_state), index|
         lineno = token.location.start_line
         column = token.location.start_column
-        column -= index == 0 ? 6 : 3 if bom && lineno == 1
+
+        # If there's a UTF-8 byte-order mark as the start of the file, then for
+        # certain tokens ripper sets the first token back by 3 bytes. It also
+        # keeps the byte order mark in the first token's value. This is weird,
+        # and I don't want to mirror that in our parser. So instead, we'll match
+        # up the columns and values here.
+        if bom && lineno == 1
+          column -= 3
+
+          if index == 0 && column == 0
+            flushed =
+              case token.type
+              when :BACK_REFERENCE, :INSTANCE_VARIABLE, :CLASS_VARIABLE,
+                  :GLOBAL_VARIABLE, :NUMBERED_REFERENCE, :PERCENT_LOWER_I,
+                  :PERCENT_LOWER_X, :PERCENT_LOWER_W, :PERCENT_UPPER_I,
+                  :PERCENT_UPPER_W, :STRING_BEGIN
+                true
+              when :REGEXP_BEGIN, :SYMBOL_BEGIN
+                token.value.start_with?("%")
+              else
+                false
+              end
+
+            unless flushed
+              column -= 3
+              value = token.value
+              value.prepend(String.new("\xEF\xBB\xBF", encoding: value.encoding))
+            end
+          end
+        end
 
         event = RIPPER.fetch(token.type)
         value = token.value
@@ -668,6 +690,11 @@ module YARP
             end_offset = token.location.start_offset
 
             if previous_token.type == :COMMENT && start_offset < end_offset
+              if bom
+                start_offset += 3
+                end_offset += 3
+              end
+
               tokens << Token.new([[lineno, 0], :on_nl, source.byteslice(start_offset...end_offset), lex_state])
             end
 
diff --git a/src/yarp.c b/src/yarp.c
index 2a5a923c..e65453c3 100644
--- a/src/yarp.c
+++ b/src/yarp.c
@@ -536,73 +536,6 @@ yp_arguments_validate(yp_parser_t *parser, yp_arguments_t *arguments) {
     }
 }
 
-/******************************************************************************/
-/* Scope node functions                                                       */
-/******************************************************************************/
-
-// Generate a scope node from the given node.
-void
-yp_scope_node_init(yp_node_t *node, yp_scope_node_t *scope) {
-    scope->base.type = YP_NODE_SCOPE_NODE;
-    scope->base.location.start = node->location.start;
-    scope->base.location.end = node->location.end;
-
-    scope->parameters = NULL;
-    scope->body = NULL;
-    yp_constant_id_list_init(&scope->locals);
-
-    switch (YP_NODE_TYPE(node)) {
-        case YP_NODE_BLOCK_NODE: {
-            yp_block_node_t *cast = (yp_block_node_t *) node;
-            if (cast->parameters) scope->parameters = cast->parameters->parameters;
-            scope->body = cast->body;
-            scope->locals = cast->locals;
-            break;
-        }
-        case YP_NODE_CLASS_NODE: {
-            yp_class_node_t *cast = (yp_class_node_t *) node;
-            scope->body = cast->body;
-            scope->locals = cast->locals;
-            break;
-        }
-        case YP_NODE_DEF_NODE: {
-            yp_def_node_t *cast = (yp_def_node_t *) node;
-            scope->parameters = cast->parameters;
-            scope->body = cast->body;
-            scope->locals = cast->locals;
-            break;
-        }
-        case YP_NODE_LAMBDA_NODE: {
-            yp_lambda_node_t *cast = (yp_lambda_node_t *) node;
-            if (cast->parameters) scope->parameters = cast->parameters->parameters;
-            scope->body = cast->body;
-            scope->locals = cast->locals;
-            break;
-        }
-        case YP_NODE_MODULE_NODE: {
-            yp_module_node_t *cast = (yp_module_node_t *) node;
-            scope->body = cast->body;
-            scope->locals = cast->locals;
-            break;
-        }
-        case YP_NODE_PROGRAM_NODE: {
-            yp_program_node_t *cast = (yp_program_node_t *) node;
-            scope->body = (yp_node_t *) cast->statements;
-            scope->locals = cast->locals;
-            break;
-        }
-        case YP_NODE_SINGLETON_CLASS_NODE: {
-            yp_singleton_class_node_t *cast = (yp_singleton_class_node_t *) node;
-            scope->body = cast->body;
-            scope->locals = cast->locals;
-            break;
-        }
-        default:
-            assert(false && "unreachable");
-            break;
-    }
-}
-
 /******************************************************************************/
 /* Node creation functions                                                    */
 /******************************************************************************/
@@ -1083,6 +1016,75 @@ yp_block_argument_node_create(yp_parser_t *parser, const yp_token_t *operator, y
     return node;
 }
 
+static void
+yp_scope_node_create(yp_parameters_node_t *parameters, yp_node_t *body, yp_constant_id_list_t locals, const char *start, const char *end, yp_scope_node_t *dest)
+{
+    *dest = (yp_scope_node_t) {
+         {
+             .type = YP_NODE_SCOPE_NODE,
+             .location = { .start = start, .end = end },
+         },
+        .parameters = parameters,
+        .body = body,
+        .locals = locals,
+    };
+    return;
+}
+
+// TODO: Implement this for all other nodes which can have a scope
+void
+yp_scope_node_init(yp_node_t *node, yp_scope_node_t *dest) {
+    yp_parameters_node_t *parameters = NULL;
+    yp_node_t *body;
+    yp_constant_id_list_t locals;
+    const char *start = node->location.start;
+    const char *end = node->location.end;
+
+    switch (node->type) {
+      case YP_NODE_BLOCK_NODE: {
+          yp_block_node_t *block_node = (yp_block_node_t *) node;
+          body = block_node->body;
+          locals = block_node->locals;
+          if (block_node->parameters) {
+              parameters = block_node->parameters->parameters;
+          }
+          break;
+      }
+      case YP_NODE_CLASS_NODE: {
+          yp_class_node_t *class_node = (yp_class_node_t *) node;
+          body = class_node->body;
+          locals = class_node->locals;
+          break;
+      }
+      case YP_NODE_DEF_NODE: {
+          yp_def_node_t *def_node = (yp_def_node_t *) node;
+          parameters = def_node->parameters;
+          body = def_node->body;
+          locals = def_node->locals;
+          break;
+      }
+      case YP_NODE_MODULE_NODE: {
+          yp_module_node_t *module_node = (yp_module_node_t *) node;
+          body = module_node->body;
+          locals = module_node->locals;
+          break;
+      }
+      case YP_NODE_SINGLETON_CLASS_NODE: {
+          yp_singleton_class_node_t *singleton_class_node = (yp_singleton_class_node_t *) node;
+          body = singleton_class_node->body;
+          locals = singleton_class_node->locals;
+          break;
+      }
+
+      default:
+        assert(false && "unreachable");
+        return;
+    }
+
+    yp_scope_node_create(parameters, body, locals, start, end, dest);
+    return;
+}
+
 // Allocate and initialize a new BlockNode node.
 static yp_block_node_t *
 yp_block_node_create(yp_parser_t *parser, yp_constant_id_list_t *locals, const yp_token_t *opening, yp_block_parameters_node_t *parameters, yp_node_t *body, const yp_token_t *closing) {
@@ -12876,6 +12878,8 @@ yp_parser_metadata(yp_parser_t *parser, const char *metadata) {
 // Initialize a parser with the given start and end pointers.
 YP_EXPORTED_FUNCTION void
 yp_parser_init(yp_parser_t *parser, const char *source, size_t size, const char *filepath) {
+    assert(source != NULL);
+
     // Set filepath to the file that was passed
     if (!filepath) filepath = "";
     yp_string_t filepath_string;
@@ -12944,14 +12948,15 @@ yp_parser_init(yp_parser_t *parser, const char *source, size_t size, const char
     size_t newline_size = size / 22;
     yp_newline_list_init(&parser->newline_list, source, newline_size < 4 ? 4 : newline_size);
 
-    assert(source != NULL);
+    // Skip past the UTF-8 BOM if it exists.
     if (size >= 3 && (unsigned char) source[0] == 0xef && (unsigned char) source[1] == 0xbb && (unsigned char) source[2] == 0xbf) {
-        // If the first three bytes of the source are the UTF-8 BOM, then we'll skip
-        // over them.
         parser->current.end += 3;
-    } else if (size >= 2 && source[0] == '#' && source[1] == '!') {
-        // If the first two bytes of the source are a shebang, then we'll indicate
-        // that the encoding comment is at the end of the shebang.
+        parser->encoding_comment_start += 3;
+    }
+
+    // If the first two bytes of the source are a shebang, then we'll indicate
+    // that the encoding comment is at the end of the shebang.
+    if (peek(parser) == '#' && peek_offset(parser, 1) == '!') {
         const char *encoding_comment_start = next_newline(source, (ptrdiff_t) size);
         if (encoding_comment_start) {
             parser->encoding_comment_start = encoding_comment_start + 1;
diff --git a/templates/src/node.c.erb b/templates/src/node.c.erb
index d288628f..03b720ab 100644
--- a/templates/src/node.c.erb
+++ b/templates/src/node.c.erb
@@ -118,7 +118,7 @@ yp_node_memsize_node(yp_node_t *node, yp_memsize_t *memsize) {
         // We do not calculate memsize of a ScopeNode
         // as it should never be generated
         case YP_NODE_SCOPE_NODE:
-            return;
+          return;
         <%- nodes.each do |node| -%>
 #line <%= __LINE__ + 1 %> "<%= File.basename(__FILE__) %>"
         case <%= node.type %>: {
diff --git a/templates/src/prettyprint.c.erb b/templates/src/prettyprint.c.erb
index cf2f12f2..e4403d0e 100644
--- a/templates/src/prettyprint.c.erb
+++ b/templates/src/prettyprint.c.erb
@@ -19,7 +19,7 @@ prettyprint_node(yp_buffer_t *buffer, yp_parser_t *parser, yp_node_t *node) {
         // We do not need to print a ScopeNode as it's not part
         // of the AST
         case YP_NODE_SCOPE_NODE:
-            return;
+          return;
         <%- nodes.each do |node| -%>
         case <%= node.type %>: {
             yp_buffer_append_str(buffer, "<%= node.name %>(", <%= node.name.length + 1 %>);


Test patch:
diff --git a/test/bom_test.rb b/test/bom_test.rb
new file mode 100644
index 00000000..7dc7eabe
--- /dev/null
+++ b/test/bom_test.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+# Don't bother checking this on these engines, this is such a specific Ripper
+# test.
+return if RUBY_ENGINE == "jruby" || RUBY_ENGINE == "truffleruby"
+
+require "yarp_test_helper"
+
+class BOMTest < Test::Unit::TestCase
+  def test_ident
+    assert_bom("foo")
+  end
+
+  def test_back_reference
+    assert_bom("$+")
+  end
+
+  def test_instance_variable
+    assert_bom("@foo")
+  end
+
+  def test_class_variable
+    assert_bom("@@foo")
+  end
+
+  def test_global_variable
+    assert_bom("$foo")
+  end
+
+  def test_numbered_reference
+    assert_bom("$1")
+  end
+
+  def test_percents
+    assert_bom("%i[]")
+    assert_bom("%r[]")
+    assert_bom("%s[]")
+    assert_bom("%q{}")
+    assert_bom("%w[]")
+    assert_bom("%x[]")
+    assert_bom("%I[]")
+    assert_bom("%W[]")
+    assert_bom("%Q{}")
+  end
+
+  def test_string
+    assert_bom("\"\"")
+    assert_bom("''")
+  end
+
+  private
+
+  def assert_bom(source)
+    bommed = "\xEF\xBB\xBF#{source}"
+    assert_equal YARP.lex_ripper(bommed), YARP.lex_compat(bommed).value
+  end
+end