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.

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

[Truffle] Add check_convert_type and check_funcall methods #4146

Merged
merged 7 commits into from Sep 16, 2016

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Sep 12, 2016

This is a work in progress. Please review.

This is for issue #3992.

I'm currently having difficulty here with 3 examples regressing. Here is a short example:

class Test
  def initialize(name, options={})
    @name = name
    @null = options[:null_object]
  end

  def method_missing(sym, *args, &block)
    if @null 
      self 
    else  
      super
    end
  end
  private :method_missing
end

puts [Test.new("foo")].flatten

Output:

/Users/brandonfish/Documents/jruby-patches/mmtest.rb:11:in `method_missing': undefined method `to_ary' for #<Test:0xcc @name="foo" @null=nil>:Test (NoMethodError)
    from /Users/brandonfish/Documents/jruby-patches/mmtest.rb:11:in `method_missing'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/type.rb:266:in `__send__'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/type.rb:266:in `check_funcall_missing'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/type.rb:250:in `check_funcall_default'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/type.rb:244:in `check_funcall'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/type.rb:233:in `convert_type'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/type.rb:224:in `rb_check_convert_type'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/array.rb:1196:in `block in recursively_flatten'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/thread.rb:102:in `detect_recursion'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/array.rb:1187:in `recursively_flatten'
    from /Users/brandonfish/Documents/jruby/truffle/src/main/ruby/core/array.rb:420:in `flatten'
    from /Users/brandonfish/Documents/jruby-patches/mmtest.rb:17:in `<main>'

Expected:

#<Test:0x007f863b051220>

@bjfish bjfish added the truffle label Sep 12, 2016
@eregon
Copy link
Member

eregon commented Sep 13, 2016

public void setRunning(boolean running) {
this.running = running;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is state in CoreLibrary for this 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've updated this to use state

@eregon
Copy link
Member

eregon commented Sep 13, 2016

Could you explain the semantics as you understand them?
They seem very strange and quite specific to Array#flatten.

@@ -398,7 +398,8 @@ private void createAccesor(DynamicObject module, String name) {
final RubyNode sequence = Translator.sequence(getContext(), sourceSection.getSource(), rubySourceSection, Arrays.asList(checkArity, accessInstanceVariable));
final RubyRootNode rootNode = new RubyRootNode(getContext(), sourceSection, null, sharedMethodInfo, sequence, false);
final CallTarget callTarget = Truffle.getRuntime().createCallTarget(rootNode);
final InternalMethod method = new InternalMethod(sharedMethodInfo, accessorName, module, visibility, callTarget);
final boolean builtIn = !getContext().isRunning();
final InternalMethod method = new InternalMethod(sharedMethodInfo, accessorName, module, visibility, builtIn, callTarget);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic could be part of addMethod or InternalMethod constructor to avoid duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this to the constructor

@eregon
Copy link
Member

eregon commented Sep 13, 2016

Just a thought: maybe check_funcall can be implemented with sth like m = obj.method(meth) and m.call(*args) ?

@bjfish
Copy link
Contributor Author

bjfish commented Sep 13, 2016

@eregon Array#flatten calls rb_check_convert_type(ary, T_ARRAY, "Array", "to_ary") and which calls convert_type , I think these methods are used by most implicit type conversions like NUM2INT , etc ... so these are not specific to flatten. I'm still working on understand the semantics for this and I think tracing this could offer better explanation than I could currently: https://github.com/ruby/ruby/blob/4915ce691d147a460e14531f4031d6934c5e8c5c/object.c#L2627

@bjfish
Copy link
Contributor Author

bjfish commented Sep 13, 2016

I don't believe BasicObject has this methodobj.method(meth) and I don't think method filters "basic" methods.

@eregon
Copy link
Member

eregon commented Sep 14, 2016

I don't believe BasicObject has this method obj.method(meth) and I don't think method filters "basic" methods.

I see.

so these are not specific to flatten.

Right, the Rubinius code seemed pretty specific but MRI isn't.

@eregon
Copy link
Member

eregon commented Sep 14, 2016

Let's look at the C then:

VALUE
rb_check_funcall(VALUE recv, ID mid, int argc, const VALUE *argv)
{
    return rb_check_funcall_default(recv, mid, argc, argv, Qundef);
}

VALUE
rb_check_funcall_default(VALUE recv, ID mid, int argc, const VALUE *argv, VALUE def)
{
    VALUE klass = CLASS_OF(recv);
    const rb_callable_method_entry_t *me;
    rb_thread_t *th = GET_THREAD();
    int respond = check_funcall_respond_to(th, klass, recv, mid);

    if (!respond) // if check_funcall_respond_to returns 0, return Qundef
    return def;

    me = rb_search_method_entry(recv, mid);
    if (!check_funcall_callable(th, me)) {
    return check_funcall_missing(th, klass, recv, mid, argc, argv,
                     respond, def);
    }
    stack_check(th);
    return vm_call0(th, recv, mid, argc, argv, me);
}

static int
check_funcall_respond_to(rb_thread_t *th, VALUE klass, VALUE recv, ID mid)
{
    return vm_respond_to(th, klass, recv, mid, TRUE);
}

static int
vm_respond_to(rb_thread_t *th, VALUE klass, VALUE obj, ID id, int priv)
{
    VALUE defined_class;
    const ID resid = idRespond_to;
// This is looking up respond_to? on the metaclass of the object
    const rb_method_entry_t *const me =
    method_entry_get(klass, resid, &defined_class);

    if (!me) return -1; // if no respond_to?, assume it's OK
    if (METHOD_ENTRY_BASIC(me)) {
// if respond_to? is built-in (maybe we could just check if it is Kernel#respond_to?
// since that is the only built-in respond_to?), return -1 which means "OK keep going"
    return -1;
    }
    else {
// Some arity checking, doesnt seem important for now
    int argc = 1;
    VALUE args[2];
    VALUE result;

    args[0] = ID2SYM(id);
    args[1] = Qtrue;
    if (priv) {
        argc = rb_method_entry_arity(me);
        if (argc > 2) {
        rb_raise(rb_eArgError,
             "respond_to? must accept 1 or 2 arguments (requires %d)",
             argc);
        }
        if (argc != 1) {
        argc = 2;
        }
        else if (!NIL_P(ruby_verbose)) {
        VALUE location = rb_method_entry_location(me);
        rb_warn("%"PRIsVALUE"%c""respond_to?(:%"PRIsVALUE") uses"
            " the deprecated method signature, which takes one parameter",
            (FL_TEST(klass, FL_SINGLETON) ? obj : klass),
            (FL_TEST(klass, FL_SINGLETON) ? '.' : '#'),
            QUOTE_ID(id));
        if (!NIL_P(location)) {
            VALUE path = RARRAY_AREF(location, 0);
            VALUE line = RARRAY_AREF(location, 1);
            if (!NIL_P(path)) {
            rb_compile_warn(RSTRING_PTR(path), NUM2INT(line),
                    "respond_to? is defined here");
            }
        }
        }
    }
// if obj's respond_to? is not Kernel's and is defined, actually call it
// aka !!meth.call(name)
    result = call_method_entry(th, defined_class, obj, resid, me, argc, args);
    return RTEST(result);
    }
}

def self.convert_type(obj, cls, meth, raise)
r = check_funcall(obj, meth)
if undefined.equal?(r)
if raise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't name it raise here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the name of this param

@@ -82,6 +82,16 @@ def self.object_respond_to?(obj, name, include_private = false)
Truffle.invoke_primitive :vm_object_respond_to, obj, name, include_private
end

def self.object_respond_to_no_built_in?(obj, name, include_private = false)
meth = Truffle.invoke_primitive :vm_method_lookup, obj, name
!meth.nil? && !(Truffle.invoke_primitive :vm_method_is_basic, meth)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move parenthesis around the call here: !Truffle.invoke_primitive(:vm_method_is_basic, meth)

r
end

def self.check_funcall(recv, meth, args = undefined)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args should probably default to [] or be removed if it's never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've update this to [] and used them later on to send *args


def self.check_funcall_missing(recv, meth, args, respond, default)
ret = basic_obj_respond_to_missing(recv, meth, false) #PRIV false
return default unless undefined.equal?(ret) || ret
Copy link
Member

@eregon eregon Sep 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is equivalent I think:

if (!RTEST(ret)) return def;

But this might be clearer:

res = basic_obj_respond_to_missing(recv, meth, false) #PRIV false
return default unless res

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this to your recommendation, thanks

return recv.__send__(:method_missing, meth) # , args
rescue NoMethodError
# TODO BJF usually more is done here
raise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the missing logic here is causing the failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eregon I think you right, I've added a new commit with more check_funcall_missing implemented which resolves the spec failures

begin
return recv.__send__(:method_missing, meth, *args)
rescue NoMethodError
# TODO BJF usually more is done here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that TODO addressed now?

Copy link
Contributor Author

@bjfish bjfish Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not completely, I think there's some cases that still aren't handled here yet: https://github.com/ruby/ruby/blob/dba03c38c54835248766f28033b5aeddc5f052ef/vm_eval.c#L369

@eregon
Copy link
Member

eregon commented Sep 16, 2016

Looks good, congrats for translating this fairly complex code!

@eregon eregon merged commit 33b1667 into truffle-head Sep 16, 2016
@eregon eregon deleted the truffle-check-funcall branch September 16, 2016 08:42
@bjfish
Copy link
Contributor Author

bjfish commented Sep 17, 2016

@eregon Thanks for the help with this. I forgot to mention, this was the last untagged core array spec, so array is at 100% now, 🎉

@enebo enebo modified the milestone: truffle-dev Nov 9, 2016
@enebo enebo added this to the Invalid or Duplicate milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants