Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix defined? to not call const_missing #2309

Merged
merged 2 commits into from
Nov 8, 2024
+80 −51
Merged

Fix defined? to not call const_missing #2309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1aaeb26
Do not call `const_missing` when using `defined?`
richardboehme Nov 8, 2024
23ed3f0
Fix conditional constant assignment with const_missing
richardboehme Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions include/natalie/module_object.hpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading