NPE while dup-ing guava's ImmutableList #4721
Comments
hakanai
changed the title
|
After about half a day, I figured out what is going on here, and it's kind of awful. :D Seemingly, Our own test doesn't call |
hakanai
changed the title
…a clone method ... we can do so if we're able to allocate a new instance of the same class
|
And then on digging a bit further in our own code, it turns out that the actual class being returned isn't So perhaps the best fix is, continue to do the public constructor search - if none are found, fall back to the old logic (where presumably it returned the original list?) |
|
we're following general Java collection "conventions" here, implementing
not sure, there's almost no stuff in Ruby (only Symbols?) where it would return the very same on on the other hand maybe we can assume a coll to be immutable if it returned itself on there's still the potential to do the dup in weird cases using hacks such as serialization but I believe it wouldn't do much good. the exception message could get improved thought although we do not want logic too specific to one impl. |
|
I think it's fair to say that if a collection class implements For our own list class I found, we don't implement But this call to |
|
|
could we understand why it needs to be doing that so we can decide if we could maybe raise a concern? |
|
Not sure about RSpec, really. I'd assume they're working around people returning mutable objects or something. But as for (Likewise, just because it's possible to implement |
|
OK, let's see your proposal including a stubbed test for the case you describe - so we have a place to start |
|
To answer my unanswered question above - what are we supposed to do if a Ruby type cannot be duped? Well, apparently some types are already doing this: So what I'm leaning towards writing in my specs:
I'll probably still have to go to RSpec to tell them that some collection types might not support |
|
Could you point me towards where the existing specs live for this method? I can't seem to find them in spec/java_integration... |
|
... just check-out the commit you commented 😉 (put new collection impls under java_integration/fixtures) |
kares
changed the title
|
Ah, indeed... I see what was wrong now, my local copy was out of date. >_> I have two examples for #dup and two for #clone now, but haven't quite figured out the target to actually run the specs. I'm just waiting for a giant sync to go through first though, because my master was apparently very out of date. Something I did notice while looking at the code though. IDEA shows some NPE warnings on the same place where I get the NPE, and when I tried adding the null check there to fix the warnings, I realised that it would fall through to the lower fallback where it looks for the no-arg constructor. And then, lo and behold, that lower fallback is already returning the collection itself as a last resort fallback. So at the moment, the least-resistance fix seems to be to simply put in the missing null check (while maybe also replacing the call to |
|
@trejkaz was mostly interested in showcasing the issue - a failure spec with a stub-ed |
|
The sync finished overnight the previous night (it really was that slow...) and then for a day I couldn't do anything, so... Should capture the spirit of it, but I'd feel better if maven had listed the task to run the specs so that I could check whether the error these get would be the same. |
|
o.k. thanks ... will look around (e.g. how Scala's collections are setup). |
|
have pushed a fix for this issue - there's actually two parts 7307dea and 1ea71f4 |
|
pushed the first commit to jruby-9.1 ... edd18dd |
Environment
JRuby 9.1.12.0
Java 1.8.0_92
Mac OS X
rspec 3.6.0
Expected Behavior
It's complicated. The spec should probably fail, but in a different way which is more our fault.
I was still in the process of figuring out why this only happened for one spec file, because it doesn't look any stranger than any other spec file.
Actual Behavior
hooks.rbline 464 is where it's running the "around example" hooks.The text was updated successfully, but these errors were encountered: