Describe the bug Generated Java code for numb

This is a minimal viable test case: <div class="snippet-clipboard-content notransl

The commit that broke it is: <a class="commit-link" data-hovercard-type="commit" data-

<div class="highlight highlight-source-ruby notranslate position-relative overflow-auto" dir="auto"

Bug: Minus signs of number literals missing (e.g. -1 => 1) if comment contains umlaut (e.g. "ü") about opal HOT 5 CLOSED

sebnozzi commented on June 3, 2024

Bug: Minus signs of number literals missing (e.g. -1 => 1) if comment contains umlaut (e.g. "ü")

from opal.

Comments (5)

hmdne commented on June 3, 2024 3

To locate a part that is compiled to a switch while it shouldn't, I did the following:

I applied a patch:

diff --git a/lib/opal/nodes/if.rb b/lib/opal/nodes/if.rb
index ba8c302c1..3bf0e0ba9 100644
--- a/lib/opal/nodes/if.rb
+++ b/lib/opal/nodes/if.rb
@@ -266,6 +266,10 @@ module Opal
 
         return false unless valid_switch_body?(true_body)
 
+        disable = (rand(1..2) == 1)
+        p [@sexp.loc.expression, disable]
+        return false if disable
+
         could_become_switch_branch?(false_body)
       end

This randomly either enabled switch compilation or disabled it.

mkdir _res; cd _res
for i in `seq 1 80`; do echo $i; OPAL_CACHE_DISABLE=1 OPAL_PREFORK_DISABLE=1 ../bin/opal -rcorelib/string/unpack -rparser -rparser/ruby32 -e 'p Parser::Ruby32.parse("#ú\n-1")' > $i.ex; done

This did a lot of sample compilations and checked which one was good while dumping information about which case was compiled as switch (false) and which did not (true) and their locations.

I wrote a simple script that gave me info which particular part was badly compiled as a switch for the test to work:

fixes = Dir["*.ex"].map do |i|
  f = File.read(i).split("\n")

  last = f.pop

  correct = last == "s(:int, -1)"

  f.grep(/true/) if correct
end.compact

pp fixes.first.intersection(*fixes)

Then I ran it and got this output:

# ruby ./findpart.rb 
["[#<Parser::Source::Range parser/builders/default.rb 8257...8288>, true]"]

Let's try to locate that part:

# ruby -e 'puts File.read("/home/user/Code/.gem/gems/parser-3.2.2.1/lib/parser/builders/default.rb")[8257...8288]'
when '+'
        value = +value

Let's try to expand it to provide a bigger view:

    def unary_num(unary_t, numeric)
      value, = *numeric
      operator_loc = loc(unary_t)

      case value(unary_t)
      when '+'
        value = +value
      when '-'
        value = -value
      end

      numeric.updated(nil, [ value ],
        :location =>
          Source::Map::Operator.new(
            operator_loc,
            operator_loc.join(numeric.loc.expression)))
    end

This is correct to be compiled, isn't it? Let's try dig deeper.

Let's try to compile it to JS:

    $a = [].concat($to_a(numeric)), (value = ($a[0] == null ? nil : $a[0])), $a;
    operator_loc = self.$loc(unary_t);
    
    switch (self.$value(unary_t)) {
      case "+":
        value = value['$+@']()
        break;
      case "-":
        value = value['$-@']()
        break;
      default:
        nil
    };
    return numeric.$updated(nil, [value], $hash2(["location"], {"location": $$$($$$($$('Source'), 'Map'), 'Operator').$new(operator_loc, operator_loc.$join(numeric.$loc().$expression()))}));
  })

Nothing looks particularly wrong. I have patched that method to add this line of code:

`console.log(#{value(unary_t)})` if RUBY_ENGINE == 'opal'

[String: '-'] {
  internal_encoding: klass {
    '$$id': 14,
    name: 'UTF-8',
    names: [ 'UTF-8', 'CP65001' ],
    ascii: true,
    dummy: false
  }
}
s(:int, 1)

Aha! As I suspected, it's an Object-wrapped string (think: new String("123") vs "123"), since we need to store encoding info alongside a string (...a little known about JavaScript feature that we abuse for that matter). Let's try an interactive Node session:

> new String("abc")
[String: 'abc']
> "abc"
'abc'
>

See how to spot an object-wrapped string in Node?

> s = "abc"; switch(s) { case "abc": console.log("equal"); break; default: console.log("inequal"); }
equal
undefined
> s = new String("abc"); switch(s) { case "abc": console.log("equal"); break; default: console.log("inequal"); }
inequal
undefined
>

We can easily fix this issue by extracting a real JS string:

> s = new String("abc"); switch(s.valueOf()) { case "abc": console.log("equal"); break; default: console.log("inequal"); }
equal
undefined
>

Therefore a real fix is this:

diff --git a/lib/opal/nodes/if.rb b/lib/opal/nodes/if.rb
index ba8c302c1..b298964c3 100644
--- a/lib/opal/nodes/if.rb
+++ b/lib/opal/nodes/if.rb
@@ -349,7 +349,7 @@ module Opal
           @switch_additional_rules = sexp.meta[:switch_additional_rules]
           compile_switch_case(sexp.meta[:switch_test])
         else
-          line "switch (", expr(@switch_first_test), ") {"
+          line "switch (", expr(@switch_first_test), ".valueOf()) {"
           indent do
             compile_switch_case(@switch_test)
           end

And... it works:

# OPAL_CACHE_DISABLE=1 OPAL_PREFORK_DISABLE=1 bin/opal -rcorelib/string/unpack -rparser -rparser/ruby32 -e 'p Parser::Ruby32.parse("#ú\n-1")'
s(:int, -1)

from opal.

hmdne commented on June 3, 2024

Interesting case - my gut feeling is that parser gem when compiled with Opal loses the minus sign if UTF-8 characters are encountered...

This is not the case if Opal is ran on Linux.

from opal.

hmdne commented on June 3, 2024

This is a minimal viable test case:

# opal -rcorelib/string/unpack -rparser -rparser/ruby32 -e 'p Parser::Ruby32.parse("#ú\n-1")'
s(:int, 1)

The correct output should be:

# ruby -rparser -rparser/ruby32 -e 'p Parser::Ruby32.parse("#ú\n-1")'
s(:int, -1)

This is a regression since opal 1.5.0:

# opal _1.4.0_ -rcorelib/string/unpack -rparser -rparser/ruby32 -e 'p Parser::Ruby32.parse("#ú\n-1")'
Object freezing is not supported by Opal
s(:int, -1)
# opal _1.5.0_ -rcorelib/string/unpack -rparser -rparser/ruby32 -e 'p Parser::Ruby32.parse("#ú\n-1")'
Object freezing is not supported by Opal
s(:int, 1)
#

This is good news, since we can bisect and find an offending commit.

from opal.

hmdne commented on June 3, 2024

The commit that broke it is: 6d1b83d

[user@localhost opal]# git bisect good
6d1b83d1cc45b1fc062222029cca92e413da9ae9 is the first bad commit
commit 6d1b83d1cc45b1fc062222029cca92e413da9ae9
Author: hmdne <[email protected]>
Date:   Thu Mar 24 14:08:21 2022 +0100

    Try to compile case statements as switch
    
    This improves deeply nested `case` structures, like in... lexer.
    Compared to a tree of else ifs, `switch` is more like a goto, so more
    likely to be more performant.
    
    This makes our `ifs` (or `&&`, `||`) to be compiled to either `if`,
    ternary, `&&`, `||` or `switch`, making use of all the JS control flow
    features.

 lib/opal/ast/matcher.rb |  77 +++++++++++++++++
 lib/opal/nodes/if.rb    | 222 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 299 insertions(+)
 create mode 100644 lib/opal/ast/matcher.rb
[user@localhost opal]#

This is bad news, since it was a big change :(

from opal.

hmdne commented on June 3, 2024

diff --git a/lib/opal/nodes/if.rb b/lib/opal/nodes/if.rb
index ba8c302c1..109a026bc 100644
--- a/lib/opal/nodes/if.rb
+++ b/lib/opal/nodes/if.rb
@@ -252,6 +252,8 @@ module Opal
       end
 
       def could_become_switch?
+        return false
+
         return false if expects_expression?
 
         return true if sexp.meta[:switch_child]

This fixes the issue, but obviously this is not a solution. Let's try to find something better.

from opal.

Bug: Minus signs of number literals missing (e.g. -1 => 1) if comment contains umlaut (e.g. "ü") about opal HOT 5 CLOSED

Comments (5)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent