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

RubyHash doesn't implement the contract of java.util.Map correctly #4916

Closed
robertpanzer opened this Issue Dec 28, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@robertpanzer

robertpanzer commented Dec 28, 2017

Environment

JRuby 9.1.13.0
OS X

(The sources show though that the same behavior should observable with the current master)

Expected Behavior

The Javadoc of java.util.Map.put() defines this:

Returns:
the previous value associated with key, or null if there was no mapping for key. (A null return can also indicate that the map previously associated null with key, if the implementation supports null values.)

Therefore I would expect these 2 tests to pass:

  @Test
  public void test1() {
    Ruby ruby = Ruby.newInstance();
    IRubyObject hash = ruby.evalScriptlet("{ \"Jane Doe\" => 10, \"Jim Doe\" => 6 }");
    assertThat(hash, instanceOf(RubyHash.class));
    RubyHash rubyHash = (RubyHash) hash;
    // Should return previous value 10, but returns the new value 42
    assertThat(rubyHash.put("Jane Doe", 42), is((Object) 10)); 
  }

  @Test
  public void test2() {
    Ruby ruby = Ruby.newInstance();
    IRubyObject hash = ruby.evalScriptlet("{ \"Jane Doe\" => 10, \"Jim Doe\" => 6 }");
    assertThat(hash, instanceOf(RubyHash.class));
    RubyHash rubyHash = (RubyHash) hash;
    // Should return null because the key wasn't present before, but returns the new value 42
    assertThat(rubyHash.put("Robert Panzer", 42), nullValue());
  }

Actual Behavior

The tests fail, because RubyHash.put() returns the value passed to this call:

@Override
public Object put(Object key, Object value) {
internalPut(JavaUtil.convertJavaToUsableRubyObject(getRuntime(), key), JavaUtil.convertJavaToUsableRubyObject(getRuntime(), value));
return value;
}

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 25, 2018

Member

Ah, very good. A simple fix for once!

Member

headius commented Jan 25, 2018

Ah, very good. A simple fix for once!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 25, 2018

Member

Ok, not totally simple, since internalPut and downstream methods do not return any existing value. But I'll have a patch shortly.

Member

headius commented Jan 25, 2018

Ok, not totally simple, since internalPut and downstream methods do not return any existing value. But I'll have a patch shortly.

headius added a commit that referenced this issue Jan 25, 2018

headius added a commit that referenced this issue Jan 25, 2018

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 25, 2018

Member

Fix and tests (adapted from yours) pushed in db06305.

Member

headius commented Jan 25, 2018

Fix and tests (adapted from yours) pushed in db06305.

@headius headius closed this Jan 25, 2018

@headius headius added this to the JRuby 9.1.16.0 milestone Jan 25, 2018

@robertpanzer

This comment has been minimized.

Show comment
Hide comment
@robertpanzer

robertpanzer Jan 25, 2018

Thanks! 🎉

robertpanzer commented Jan 25, 2018

Thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment