Refactor Kernel#respond_to? specializations

[ssnickolay]

Related to #2120 (comment)

The test output before the PR changes:

1)
Kernel#respond_to? throws a type error if argument can't be coerced into a Symbol ERROR
Expected TypeError ((?-mix:is not a symbol nor a string))
but got: TypeError (TruffleRuby doesn't have a case for the org.truffleruby.core.kernel.KernelNodesFactory$RespondToNodeFactory$RespondToNodeGen node with values of type KernelSpecs::A(org.truffleruby.core.basicobject.RubyBasicObject) Object(org.truffleruby.core.basicobject.RubyBasicObject) java.lang.Boolean=false
	from org.truffleruby.core.kernel.KernelNodesFactory$RespondToNodeFactory$RespondToNodeGen.executeAndSpecialize(KernelNodesFactory.java:5238)
	from org.truffleruby.core.kernel.KernelNodesFactory$RespondToNodeFactory$RespondToNodeGen.execute(KernelNodesFactory.java:5210)
	from org.truffleruby.language.arguments.CheckArityNode.execute(CheckArityNode.java:41)
	from org.truffleruby.language.methods.ExceptionTranslatingNode.execute(ExceptionTranslatingNode.java:33)
	from org.truffleruby.language.RubyRootNode.execute(RubyRootNode.java:64)
	from com.oracle.truffle.api.impl.DefaultCallTarget.callDirectOrIndirect(DefaultCallTarget.java:84))
/Users/ssnickolay/Projects/oss/graal/truffleruby-ws-2/truffleruby/spec/ruby/core/kernel/respond_to_spec.rb:28:in `respond_to?'

Not sure if I should add a note to the Changelog

[eregon]

toString() should not be called in PE code, because the receiver class is not known so it would be a virtual call, which is a performance warning.
So I'd suggest passing object to typeErrorIsNotAOrB() (then toString() is called behind a TruffleBoundary and that's OK).

We should probably use the new CoreException#inspectReceiver() helper there instead of toString(), but the PR adding it is not merged yet (should be soon).

[ssnickolay]

Hm, it seems we have a few places with similar.toString():

truffleruby/src/main/java/org/truffleruby/core/cast/LongCastNode.java

Lines 47 to 51 in 314f8ae

	protected long doBasicObject(Object value,
	@CachedContext(RubyLanguage.class) RubyContext context) {
	throw new RaiseException(
	context,
	context.getCoreExceptions().typeErrorIsNotA(value.toString(), "Integer (fitting in long)", this));

and

truffleruby/src/main/java/org/truffleruby/core/cast/BigIntegerCastNode.java

Lines 68 to 70 in 4acd2a8

	private RubyException notAnInteger(RubyContext context, Object object) {
	return context.getCoreExceptions().typeErrorIsNotA(
	object.toString(),

here we use Utils.toString():

truffleruby/src/main/java/org/truffleruby/core/cast/IntegerCastNode.java

Lines 54 to 58 in 314f8ae

	protected int doBasicObject(Object value,
	@CachedContext(RubyLanguage.class) RubyContext context) {
	throw new RaiseException(
	context,
	context.getCoreExceptions().typeErrorIsNotA(Utils.toString(value), "Integer (fitting in int)", this));

Is this will be a part of

We should probably use the new CoreException#inspectReceiver() helper there instead of toString(), but the PR adding it is not merged yet (should be soon).

or we need to refactor these places too (probably not in this PR)?

[eregon]

Those cases are behind a @TruffleBoundary, so compilation-wise they are fine. The case here isn't.
I think -H:+TruffleCheckBlackListedMethods which is used in jt build --env native would actually detect as an error at image build time.

In terms of error message, inspectReceiver() is better. I used that to replace toString() in CoreExceptions, but not in other places (yet).

[eregon]

The PR I was talking about has been merged now, so if you rebase you can use inspectReceiver():
25eacca
As usual, new code should use the correct approach, but fixing old code is out of scope.

[ssnickolay]

Those cases are behind a @TruffleBoundary, so compilation-wise they are fine

Ah, that was obvious, sorry

The PR I was talking about has been merged now, so if you rebase you can use inspectReceiver():

No problem, but I should still pass Object over String, right?:

public RubyException typeErrorIsNotAOrB(Object value, String expectedTypeA, String expectedTypeB, Node currentNode) {
   # use inspectReceiver(value) instead of value.toString()

[eregon]

Yes, I think in the future we should remove those cases where the receiver is typed as String, conversation should be done in CoreExeptions or later.

[eregon]

Not sure if I should add a note to the Changelog

You could add one like

* Fix error message when the method name is not a Symbol or String for `Kernel#respond_to?`

(the previous message was essentially a bug)

[ssnickolay]

@eregon the PR ready for the final review

[eregon]

Thank you for the refactoring!