natalie-lang / natalie

a work-in-progress Ruby compiler, written in Ruby and C++
https://natalie-lang.org
MIT License
937 stars 63 forks source link

Symbol deduplication in CPP code has some missing cases #2048

Closed herwinw closed 4 months ago

herwinw commented 4 months ago

I was just looking at the generated code for CONST ||= 1; 2 (the ; 2 is to turn the used mode off, I guess the last line is always used. This is without #2047, but the same things happens with that PR):

    symbols[0] = SymbolObject::intern("CONST", 5);
    symbols[1] = SymbolObject::intern("CONST", 5);

            Value is_defined_result1;
            try {
                auto const2 =  self->const_find_with_autoload(env, self, symbols[0]/*:CONST*/, Object::ConstLookupSearchMode::NotStrict);
                const2;
                is_defined_result1 = new StringObject("constant");
            } catch (ExceptionObject *) {
                is_defined_result1 = NilObject::the();
            }
            Value if_result3;
            if (is_defined_result1.public_send(env, "!"_s)->is_truthy()) {
                if_result3 = is_defined_result1.public_send(env, "!"_s);
            } else {
                auto const4 =  self->const_find_with_autoload(env, self, symbols[0]/*:CONST*/, Object::ConstLookupSearchMode::NotStrict);
                if_result3 = const4.public_send(env, "!"_s);
            }
            Value if_result5;
            if (if_result3->is_truthy()) {
                if_result5 =  self->const_set(symbols[1]/*:CONST*/, Value::integer(1));
            } else {
                ;
            }

The two calls to const_find_with_autoload both use the first symbol, whe call to const_set uses the second one. Both symbols are the same, and the code is created by the same node.name calls. So we should be able to merge these two values.

herwinw commented 4 months ago

With the power of $stderr.puts sprinkled in your code (who needs a debugger):

{:CONST=>0, "CONST"=>1}

ConstFindInstruction calls to_sym on the input, ConstSetInstruction calls to_s

So I guess we should fix this using https://api.rubyonrails.org/classes/ActiveSupport/HashWithIndifferentAccess.html (grabs popcorn)