Skip to content

Commit

Permalink
Merge pull request #2309 from natalie-lang/rb/defined-const-missing
Browse files Browse the repository at this point in the history 
Fix `defined?` to not call `const_missing`
  • Loading branch information
@richardboehme
richardboehme authored Nov 8, 2024
2 parents 06720cd + 23ed3f0 commit af62600
Show file tree
Hide file tree
Showing 7 changed files with 80 additions and 51 deletions.
7 changes: 5 additions & 2 deletions include/natalie/module_object.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ class ModuleObject : public Object {

Value is_autoload(Env *, Value) const;

virtual Value const_find(Env *, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::Raise) override;
virtual Value const_find_with_autoload(Env *, Value, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::Raise) override;
virtual Value const_find(Env *, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::ConstMissing) override;
virtual Value const_find_with_autoload(Env *, Value, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::ConstMissing) override;
virtual Value const_get(SymbolObject *) const override;
virtual Value const_fetch(SymbolObject *) override;
virtual Value const_set(SymbolObject *, Value) override;
Expand Down Expand Up @@ -176,6 +176,9 @@ class ModuleObject : public Object {
snprintf(buf, len, "<ModuleObject %p name=(none)>", this);
}

private:
Value handle_missing_constant(Env *, Value, ConstLookupFailureMode);

protected:
Constant *find_constant(Env *, SymbolObject *, ModuleObject **, ConstLookupSearchMode = ConstLookupSearchMode::Strict);

Expand Down
5 changes: 3 additions & 2 deletions include/natalie/object.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class Object : public Cell {
enum class ConstLookupFailureMode {
Null,
Raise,
ConstMissing,
};

Object()
Expand Down Expand Up @@ -248,8 +249,8 @@ class Object : public Cell {
Value extend(Env *, Args);
void extend_once(Env *, ModuleObject *);

virtual Value const_find(Env *, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::Raise);
virtual Value const_find_with_autoload(Env *, Value, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::Raise);
virtual Value const_find(Env *, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::ConstMissing);
virtual Value const_find_with_autoload(Env *, Value, SymbolObject *, ConstLookupSearchMode = ConstLookupSearchMode::Strict, ConstLookupFailureMode = ConstLookupFailureMode::ConstMissing);
virtual Value const_get(SymbolObject *) const;
virtual Value const_fetch(SymbolObject *);
virtual Value const_set(SymbolObject *, Value);
Expand Down
18 changes: 16 additions & 2 deletions lib/natalie/compiler/instructions/const_find_instruction.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
module Natalie
class Compiler
class ConstFindInstruction < BaseInstruction
def initialize(name, strict:, file: nil, line: nil)
def initialize(name, strict:, failure_mode: 'ConstMissing', file: nil, line: nil)
@name = name.to_sym
@strict = strict
@failure_mode = failure_mode

# source location info
@file = file
Expand All @@ -26,7 +27,16 @@ def generate(transform)

namespace = transform.pop
search_mode = @strict ? 'Strict' : 'NotStrict'
transform.exec_and_push(:const, "#{namespace}->const_find_with_autoload(env, self, #{transform.intern(name)}, Object::ConstLookupSearchMode::#{search_mode})")
transform.exec_and_push(
:const,
"#{namespace}->const_find_with_autoload(" \
'env, ' \
'self, ' \
"#{transform.intern(name)}, " \
"Object::ConstLookupSearchMode::#{search_mode}, " \
"Object::ConstLookupFailureMode::#{@failure_mode}" \
')'
)
end

def execute(vm)
Expand All @@ -52,6 +62,10 @@ def find_object_with_constant(obj)
end while (obj = parent(obj))
end

def raise_if_missing!
@failure_mode = 'Raise'
end

def serialize(rodata)
position = rodata.add(@name.to_s)

Expand Down
44 changes: 23 additions & 21 deletions lib/natalie/compiler/pass1.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -952,7 +952,7 @@ def transform_constant_or_write_node(node, used:)
# defined?(CONST)
IsDefinedInstruction.new(type: 'constant'),
PushSelfInstruction.new,
ConstFindInstruction.new(node.name, strict: false),
ConstFindInstruction.new(node.name, strict: false, failure_mode: 'Raise'),
EndInstruction.new(:is_defined),
DupInstruction.new,

Expand Down Expand Up @@ -1072,35 +1072,35 @@ def transform_constant_path_or_write_node(node, used:)
name, _is_private, prep_instruction = constant_name(node.target)
# FIXME: is_private shouldn't be ignored I think
#
# This describes the stack for the three distinct paths
instructions = [ # if !defined?(tmp::CONST) if defined?(tmp::CONST) && !tmp::CONST if defined(tmp::CONST) && tmp::CONST
prep_instruction, # [tmp] [tmp] [tmp]
DupInstruction.new, # [tmp, tmp] [tmp, tmp] [tmp, tmp]
IsDefinedInstruction.new(type: 'constant'), # [tmp, tmp, is_defined] [tmp, tmp, is_defined] [tmp, tmp, is_defined]
SwapInstruction.new, # [tmp, is_defined, tmp] [tmp, is_defined, tmp] [tmp, is_defined, tmp]
ConstFindInstruction.new(name, strict: true), # [tmp, is_defined, tmp, CONST] [tmp, is_defined, tmp, CONST] [tmp, is_defined, tmp, CONST]
EndInstruction.new(:is_defined), # [tmp, false] [tmp, true] [tmp, true]
IfInstruction.new, # [tmp] [tmp] [tmp]
DupInstruction.new, # [tmp, tmp] [tmp, tmp]
ConstFindInstruction.new(name, strict: true), # [tmp, false] [tmp, tmp::CONST]
# This describes the stack for the three distinct paths
instructions = [ # if !defined?(tmp::CONST) if defined?(tmp::CONST) && !tmp::CONST if defined(tmp::CONST) && tmp::CONST
prep_instruction, # [tmp] [tmp] [tmp]
DupInstruction.new, # [tmp, tmp] [tmp, tmp] [tmp, tmp]
IsDefinedInstruction.new(type: 'constant'), # [tmp, tmp, is_defined] [tmp, tmp, is_defined] [tmp, tmp, is_defined]
SwapInstruction.new, # [tmp, is_defined, tmp] [tmp, is_defined, tmp] [tmp, is_defined, tmp]
ConstFindInstruction.new(name, strict: true, failure_mode: 'Raise'), # [tmp, is_defined, tmp, CONST] [tmp, is_defined, tmp, CONST] [tmp, is_defined, tmp, CONST]
EndInstruction.new(:is_defined), # [tmp, false] [tmp, true] [tmp, true]
IfInstruction.new, # [tmp] [tmp] [tmp]
DupInstruction.new, # [tmp, tmp] [tmp, tmp]
ConstFindInstruction.new(name, strict: true), # [tmp, false] [tmp, tmp::CONST]
ElseInstruction.new(:if),
PushFalseInstruction.new, # [tmp, false]
PushFalseInstruction.new, # [tmp, false]
EndInstruction.new(:if),
IfInstruction.new, # [tmp] [tmp] [tmp]
IfInstruction.new, # [tmp] [tmp] [tmp]
]
if used
instructions << ConstFindInstruction.new(name, strict: true) # [tmp::Const]
instructions << ConstFindInstruction.new(name, strict: true) # [tmp::Const]
else
instructions << PopInstruction.new # []
instructions << PopInstruction.new # []
end
instructions << ElseInstruction.new(:if)
instructions << DupInstruction.new if used # [tmp, tmp] [tmp, tmp]
instructions << DupInstruction.new if used # [tmp, tmp] [tmp, tmp]
instructions.append(
transform_expression(node.value, used: true), # [tmp, tmp, value] [tmp, tmp, value]
SwapInstruction.new, # [tmp, value, tmp] [tmp, value, tmp]
ConstSetInstruction.new(name), # [tmp] [tmp]
transform_expression(node.value, used: true), # [tmp, tmp, value] [tmp, tmp, value]
SwapInstruction.new, # [tmp, value, tmp] [tmp, value, tmp]
ConstSetInstruction.new(name), # [tmp] [tmp]
)
instructions << ConstFindInstruction.new(name, strict: true) if used # [value] [value]
instructions << ConstFindInstruction.new(name, strict: true) if used # [value] [value]
instructions << EndInstruction.new(:if)
instructions
end
Expand Down Expand Up @@ -1258,6 +1258,8 @@ def transform_defined_node(node, used:)
body[index] = [GlobalVariableDefinedInstruction.new(instruction.name), instruction]
when InstanceVariableGetInstruction
body[index] = [InstanceVariableDefinedInstruction.new(instruction.name), instruction]
when ConstFindInstruction
body[index].raise_if_missing!
when SendInstruction
if index == body.length - 1
body[index] = instruction.to_method_defined_instruction
Expand Down
14 changes: 4 additions & 10 deletions spec/language/optional_assignments_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -667,9 +667,7 @@ module ConstantSpecs

it 'correctly defines non-existing constants' do
ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT1 ||= :assigned
NATFIXME "Don't use const_missing in defined?(const)", exception: SpecFailedException do
ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT1.should == :assigned
end
ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT1.should == :assigned
end

it 'correctly overwrites nil constants' do
Expand All @@ -684,9 +682,7 @@ module ConstantSpecs
x = 0
(x += 1; ConstantSpecs::ClassA)::OR_ASSIGNED_CONSTANT2 ||= :assigned
x.should == 1
NATFIXME "Don't use const_missing in defined?(const)", exception: SpecFailedException do
ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT2.should == :assigned
end
ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT2.should == :assigned
end

it 'causes side-effects of the module part to be applied only once (for nil constant)' do
Expand All @@ -708,10 +704,8 @@ module ConstantSpecs
}.should raise_error(Exception)

x.should == 1
NATFIXME 'it does not evaluate the right-hand side if the module part raises an exception (for undefined constant)', exception: SpecFailedException do
y.should == 0
defined?(ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT3).should == nil
end
y.should == 0
defined?(ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT3).should == nil
end

it 'does not evaluate the right-hand side if the module part raises an exception (for nil constant)' do
Expand Down
33 changes: 19 additions & 14 deletions src/module_object.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,11 +227,8 @@ Value ModuleObject::const_find_with_autoload(Env *env, Value self, SymbolObject
ModuleObject *module = nullptr;
auto constant = find_constant(env, name, &module, search_mode);

if (!constant) {
if (failure_mode == ConstLookupFailureMode::Null)
return nullptr;
return send(env, "const_missing"_s, { name });
}
if (!constant)
return handle_missing_constant(env, name, failure_mode);

if (constant->needs_load()) {
assert(module);
Expand All @@ -245,15 +242,26 @@ Value ModuleObject::const_find_with_autoload(Env *env, Value self, SymbolObject
Value ModuleObject::const_find(Env *env, SymbolObject *name, ConstLookupSearchMode search_mode, ConstLookupFailureMode failure_mode) {
auto constant = find_constant(env, name, nullptr, search_mode);

if (!constant) {
if (failure_mode == ConstLookupFailureMode::Null)
return nullptr;
return send(env, "const_missing"_s, { name });
}
if (!constant)
return handle_missing_constant(env, name, failure_mode);

return constant->value();
}

Value ModuleObject::handle_missing_constant(Env *env, Value name, ConstLookupFailureMode failure_mode) {
if (failure_mode == ConstLookupFailureMode::Null)
return nullptr;

if (failure_mode == ConstLookupFailureMode::Raise) {
auto name_str = name->to_s(env);
if (this == GlobalEnv::the()->Object())
env->raise_name_error(name_str, "uninitialized constant {}", name_str->string());
env->raise_name_error(name_str, "uninitialized constant {}::{}", inspect_str(), name_str->string());
}

return send(env, "const_missing"_s, { name });
}

Value ModuleObject::const_set(SymbolObject *name, Value val) {
std::lock_guard<std::recursive_mutex> lock(g_gc_recursive_mutex);

Expand Down Expand Up @@ -325,10 +333,7 @@ Value ModuleObject::constants(Env *env, Value inherit) const {
}

Value ModuleObject::const_missing(Env *env, Value name) {
auto name_str = name->to_s(env);
if (this == GlobalEnv::the()->Object())
env->raise_name_error(name_str, "uninitialized constant {}", name_str->string());
env->raise_name_error(name_str, "uninitialized constant {}::{}", inspect_str(), name_str->string());
return handle_missing_constant(env, name, ConstLookupFailureMode::Raise);
}

void ModuleObject::make_method_alias(Env *env, SymbolObject *new_name, SymbolObject *old_name) {
Expand Down
10 changes: 10 additions & 0 deletions test/natalie/defined_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ class Bar
end
end

class ConstMissingConst
def self.const_missing(const)
const
end
end

def foo; end

describe 'defined?' do
Expand All @@ -19,6 +25,10 @@ def foo; end
defined?(NonExistent).should == nil
end

it 'does not call const_missing' do
defined?(ConstMissingConst::Bar).should == nil
end

it 'recognizes namespaced constants' do
defined?(Foo::Bar).should == 'constant'
defined?(Food::Bar).should == nil
Expand Down

0 comments on commit af62600

Please sign in to comment.