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

[ji] make inspect on Java proxies work #5219

Merged
merged 7 commits into from Oct 13, 2018

Conversation

Projects
None yet
2 participants
@kares
Copy link
Member

kares commented Jun 15, 2018

... inspect will now call toString on a Java proxy (unless its java.lang.Object#toString in which case we do the same "bare" hashy-inspect)

the motivation here is #5182 ... have seen "useless" hashy-inspect on Java proxied types too many times.

the approach here is the following: proxies use a call-site to determine whether toString is overriden.
the side effect is that I reverted an old 'feature' where Java (instance) methods, such as toString, only show-up at the lowest level in the Ruby class hierarchy - as explained on #5182 (comment).

there's likely a "cleaner" way -> having inspect alias-ed whenever a type we process has toString, maybe we shall eventually refactor to that but there's a much more interesting feature that I am pursuing :

current default inspect (for any Java class missing a custom toString) does only hashy-inspect without any details. would like to be able to reflectively inspect such hierarchies similar to how Ruby shows all instance variables - while I got it working its not currently usable as there's inconsistencies that I need to think through (more on that elsewhere).

in terms of compatibility there's some "breaking" change related to java.util.Map proxies, which seem to have been historically approached differently than the others - the internally sync state with a Hash.

  1. this remains the case however due noted above - there was an internal "bug" where colliding methods would prefer Hash-like ones instead of the explicit Java override (e.g. clear had the same effect but did not return void as is prescribed by java.util.Map but instead self from RubyHash).
  2. the inspect for map proxy needed to be changed, otherwise it would be weird to have a special exception for the Map hierarchy from inspect doing toString on Java proxies (methods for restoring previous behavior provided).
@kares

This comment has been minimized.

Copy link
Member Author

kares commented Jun 28, 2018

ping @headius @enebo thoughts, concerns about shipping at least what we do have atm, around here?

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Jul 16, 2018

any thoughts, would love to ship a better JI support in tools such as rspec?

@kares kares requested a review from headius Sep 11, 2018

@kares kares force-pushed the ji-inspect branch from 62df676 to 51b4249 Sep 11, 2018

kares added some commits Jun 14, 2018

[ji] let's not remove duplicate methods from hierarchy
e.g. previously a instance_method(:toString) would only exists on
java.lang.Object - custom impls in more concrete classes would not show
up. which is fine to save space due Java's dispatch rules but the proxy
class hierarachy doesn't match reality which we'll need for Java inspect
-> toString
[ji] inspect for Java proxies -> toString or a custom fallback
need a base to_s impl on java.lang.Object - calling toString()
refactored the Java object proxy class tracking in JavaSupport
[test] split and adjust Hash-like expectations
inspect/to_s are expected to behave like rest of Java proxies
(delegating to String) while also Java APIs should always win when
there's a conflict in naming e.g. Map#clear vs Hash#clear
[test] JI spec how inspect -> toString
this would be a resolution for weird non-readable rspec output
... with Java types - GH-5182
revert reflective inspect for now
... as there are a few 'uniformity' pieces to figure out
[test] add and adjust java fields
... we'll reuse to test out reflective inspect
@headius
Copy link
Member

headius left a comment

I approve. I only have one question... if a Java type defines its own inspect, that should override the default one from JavaProxy, yes?

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Oct 12, 2018

if a Java type defines its own inspect, that should override the default one from JavaProxy, yes?

yeah, that is always the case with these proxy methods but just to be sure I have added some specs

@kares kares added this to the JRuby 9.2.1.0 milestone Oct 12, 2018

@kares kares merged commit f880e60 into master Oct 13, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@kares kares deleted the ji-inspect branch Oct 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.