Fix issues discovered using Error Prone #4860

nick-someone · 2017-11-21T02:54:02Z

Hello!

As part of the investigation that led to #4844, I integrated Error Prone (http://errorprone.info) into the JRuby compiler. (See configuration in https://github.com/nglorioso/jruby/commit/ae3a5cb605b2b8b31ba0fae66332086ce01730f9).

This found a number of bugs (ranging in severity/nagginess). I'm happy to revert or change any of these, but all of them represent real defects:

RubyEnumerable - each_with_index19 is trivially infinitely-recursive. I assumed that each_with_index is appropriate
StructLayout/SymbolProc - hashCode() ends up RSHIFT'ing an int value by 32 bits, which always ends up resulting in 0. I changed to 16 bits, but if you needed a stable hash code, I can change to XOR 0
RipperContext/FullInterpreterContext - Array.toString() is unhelpful for error messages. Manually unrolled them with Arrays.toString.
RealClassGenerator - an exception was created and thrown on the floor in an exceptional condition. I threw it instead.
Pack - ulMask was an int constant left shifted by 56, but since it was an int, it shifts out to 0 instead of 0xfe000000.
SelectExecutor - Here, I don't have enough context to make the right changes, but the errorKeyList.contains() calls will never return true, since the file descriptor objects can't be contained in errorKeyList

kares · 2017-11-21T06:11:36Z

these are looking really good ... @headius let's have these for 9.1.15 shall we?
have tried find bugs + IDEA's analysis previously but the amount was just too much to find such real gems.

enebo · 2017-11-21T15:57:58Z

The ripper suggestion is wrong even though it did point out there was a bug there. These all look good except that one. That one should be something like:

StringBuilder buf = new StringBuilder();
buf.append((char) end);

@nglorioso change to that or omit that change otherwise I think it all looks good to me. Although I surprising bad at bit math stuff so the StructLayout change would be nice if someone else said that is ok.

headius

About half reasonable changes, half unnecessary or incorrect.

headius · 2017-11-21T18:13:59Z

core/src/main/java/org/jruby/ext/ffi/StructLayout.java

@@ -658,7 +658,7 @@ public boolean equals(Object obj) {

        @Override
        public int hashCode() {
-            return 53 * 5 + (int) (this.offset ^ (this.offset >>> 32)) ^ type.hashCode();
+            return 53 * 5 + (int) (this.offset ^ (this.offset >>> 16)) ^ type.hashCode();


This change fixes the offset int getting shifted to oblivion. However, I'd prefer to replace this with a hashCode generated by IntelliJ as follows:

diff --git a/core/src/main/java/org/jruby/ext/ffi/StructLayout.java b/core/src/main/java/org/jruby/ext/ffi/StructLayout.java index f7e269d777..4d5bfdf0a8 100644 --- a/core/src/main/java/org/jruby/ext/ffi/StructLayout.java +++ b/core/src/main/java/org/jruby/ext/ffi/StructLayout.java @@ -658,7 +658,10 @@ public final class StructLayout extends Type { @Override public int hashCode() { - return 53 * 5 + (int) (this.offset ^ (this.offset >>> 32)) ^ type.hashCode(); + int result = super.hashCode(); + result = 31 * result + type.hashCode(); + result = 31 * result + offset; + return result; } /**

headius · 2017-11-21T18:14:47Z

core/src/main/java/org/jruby/ext/ripper/StringTerm.java

@@ -146,7 +146,7 @@ public int parseString(RipperLexer lexer, LexerSource src) throws IOException {
    }

    private String parseRegexpFlags(RipperLexer lexer, LexerSource src) throws IOException {
-        StringBuilder buf = new StringBuilder(end);
+        StringBuilder buf = new StringBuilder();


I don't understand this change.

http://errorprone.info/bugpattern/StringBuilderInitWithChar

The prior version of the code pre-allocated the StringBuilder to the length of the int represented by the char. (Not starting pre-initialized to a single-character).

From the rest of the code, it doesn't appear that 'end' was intended to show up in the actual output, so I removed the eager preallocation here.

I made a general comment in the PR about this. The result of that string ends up appended to another StringBuilder (which indicates perhaps this could get cleaned up) but 'end' should definitely be in the string. It was likely me assuming character was an acceptable constructor and because this was ported C code we saved the char as an int and confusion ensued.

@nglorioso Actually just remove this. The existing code should be passing in the StringBuilder from the previous builder in. I will refactor this a tiny bit and that StringBuilder will go away.

@nglorioso and tip of the hat for saying it is not in the output. We append the original StringBuilder to an outer StringBuilder but in the case of RegexpEnd end we throw out all that constructed output! Since it never used it did not matter what was in either of these strings.

Addressed in 58cd133 on jruby-9.1 and cpd over to master.

headius · 2017-11-21T18:15:48Z

core/src/main/java/org/jruby/ir/interpreter/FullInterpreterContext.java

@@ -188,6 +189,6 @@ public int determineRPC(int ipc) {
            if (ipc <= rescueIPCs[i]) return rescueIPCs[i + 1];
        }

-        throw new RuntimeException("BUG: no RPC found for " + getFileName() + ":" + getName() + ":" + ipc + getInstructions());
+        throw new RuntimeException("BUG: no RPC found for " + getFileName() + ":" + getName() + ":" + ipc + Arrays.toString(getInstructions()));


This is wrong both ways. The array will just be a mangled Instr]@0x1234 sort of toString, and the Arrays version will include ALL of the instructions in the error message.

Hrm. Perhaps do you want me to replace getInstructions() with rescueIPCs and Arrays.toString that?

@nglorioso just remove getInstructions() altogether. Originally we only saw this while developing our new runtime so we probably never cared about anything other than ipc since we could just run the program again and look at the instructions on a second run. If we corrected this to print out all instrs I think users would complain about a gazillion lines of output.

As an added note we should consider possibly changing this to be an hserr-like output in addition to notifying the user we hit an internal bug. We could then dump CFG, instrs, ipc, etc into a file so they could paste that into an issue (sorry for adding this to an incidental PR review but sometimes inspiration strikes!). I am not asking that here though :)

headius · 2017-11-21T18:16:07Z

core/src/main/java/org/jruby/ir/operands/SymbolProc.java

@@ -34,7 +34,7 @@ public Object createCacheObject(ThreadContext context) {

    @Override
    public int hashCode() {
-        return 47 * 7 + (int) (this.name.hashCode() ^ (this.encoding.hashCode() >>> 32));
+        return 47 * 7 + (int) (this.name.hashCode() ^ (this.encoding.hashCode() >>> 16));


Same reason for fix and argument for generated hashCode as for StructLayout.

headius · 2017-11-21T18:16:34Z

core/src/main/java/org/jruby/java/codegen/RealClassGenerator.java

@@ -144,7 +144,7 @@ public static Class createRealImplClass(Class superClass, Class[] interfaces, Ru

        Class newClass = defineRealImplClass(ruby, name, superClass, superTypeNames, simpleToAll);
        if (!newClass.isAssignableFrom(interfaces[0])) {
-            new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());
+            throw new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());


This should never happen anyway, but it's a good change.

Looks like there a number of failures on the Travis build that are triggered by this change: https://travis-ci.org/jruby/jruby/builds/305072247

Haven't dug into this, but I can revert this change if there are too many failing tests to deal with.

Still waiting on this. Can revert the change to not shake up the CI, or leave it in :)

Revert this change and file a bug about this useless line.

#4880 filed

headius · 2017-11-21T18:17:19Z

core/src/main/java/org/jruby/util/io/SelectExecutor.java

@@ -209,6 +209,7 @@ IRubyObject selectInternal(ThreadContext context) throws IOException {
                RubyIO io = TypeConverter.ioGetIO(runtime, obj);
                RubyIO write_io = io.GetWriteIO();
                fptr = io.getOpenFileChecked();
+                /* BUG: errorKeyList (SelectionKey) can't contain fptr.fd() (ChannelFD) */


This should be excluded. I suppose it objects to a comment starting with "BUG".

Yeah, sorry, should have cleared up during the PR comment.

There exists a bug here, and I wanted to mark it. I'm happy to remove the comment from this PR. Should I file a distinct issue to look into this?

FIled #4867 for this.

Thank you. Remove this from the PR and we'll discuss it separately.

headius

Might as well squash all these into one commit, but it looks good to me.

headius

Few more tweaks and we can merge.

headius · 2017-11-27T23:39:47Z

core/src/main/java/org/jruby/java/codegen/RealClassGenerator.java

@@ -144,7 +144,7 @@ public static Class createRealImplClass(Class superClass, Class[] interfaces, Ru

        Class newClass = defineRealImplClass(ruby, name, superClass, superTypeNames, simpleToAll);
        if (!newClass.isAssignableFrom(interfaces[0])) {
-            new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());
+            throw new RuntimeException(newClass.getInterfaces()[0].getClassLoader() + " " + interfaces[0].getClassLoader());


Revert this change and file a bug about this useless line.

headius · 2017-11-27T23:40:54Z

core/src/main/java/org/jruby/ext/ripper/RipperLexer.java

@@ -2349,7 +2350,7 @@ public boolean tokenAddMBC(int first_byte, ByteList buffer) {
        int length = precise_mbclen();

        if (length <= 0) {
-            compile_error("invalid multibyte char (" + getEncoding().getName() + ")");
+            compile_error("invalid multibyte char (" + Arrays.toString(getEncoding().getName()) + ")");


I didn't notice this before, but this should just be getEncoding(). The default toString for Encoding is to return the name of the encoding.

headius · 2017-11-27T23:41:26Z

core/src/main/java/org/jruby/util/io/SelectExecutor.java

@@ -209,6 +209,7 @@ IRubyObject selectInternal(ThreadContext context) throws IOException {
                RubyIO io = TypeConverter.ioGetIO(runtime, obj);
                RubyIO write_io = io.GetWriteIO();
                fptr = io.getOpenFileChecked();
+                /* BUG: errorKeyList (SelectionKey) can't contain fptr.fd() (ChannelFD) */


Thank you. Remove this from the PR and we'll discuss it separately.

nick-someone · 2017-12-01T17:29:26Z

Thanks! Extraneous things removed, should be good to merge (assuming no other CI failures)

headius · 2017-12-05T20:09:05Z

Thanks for your patience and help!

nick-someone · 2017-12-05T21:45:56Z

Thank you! Glad to help improve things :)

headius reviewed Nov 21, 2017

View reviewed changes

headius approved these changes Nov 21, 2017

View reviewed changes

nick-someone mentioned this pull request Nov 26, 2017

Definite bug in core/src/main/java/org/jruby/util/io/SelectExecutor.java #4867

Closed

headius requested changes Nov 27, 2017

View reviewed changes

Fix issues discovered using http://errorprone.info

0e959fe

headius merged commit 0e959fe into jruby:master Dec 5, 2017

nick-someone deleted the bugfixes branch December 5, 2017 21:45

enebo added this to the JRuby 9.1.15.0 milestone Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Fix issues discovered using Error Prone #4860

Fix issues discovered using Error Prone #4860

nick-someone commented Nov 21, 2017

kares commented Nov 21, 2017

enebo commented Nov 21, 2017

headius left a comment

headius Nov 21, 2017

headius Nov 21, 2017

nick-someone Nov 21, 2017

enebo Nov 21, 2017

enebo Nov 21, 2017

enebo Nov 21, 2017

enebo Nov 21, 2017

headius Nov 21, 2017

nick-someone Nov 21, 2017

enebo Nov 21, 2017

enebo Nov 21, 2017

headius Nov 21, 2017

headius Nov 21, 2017

nick-someone Nov 21, 2017

nick-someone Nov 26, 2017

headius Nov 27, 2017

nick-someone Dec 1, 2017

headius Nov 21, 2017

nick-someone Nov 21, 2017

nick-someone Nov 26, 2017

headius Nov 27, 2017

headius left a comment

headius left a comment

headius Nov 27, 2017

headius Nov 27, 2017

headius Nov 27, 2017

nick-someone commented Dec 1, 2017

headius commented Dec 5, 2017

nick-someone commented Dec 5, 2017

Fix issues discovered using Error Prone #4860

Fix issues discovered using Error Prone #4860

Conversation

nick-someone commented Nov 21, 2017

kares commented Nov 21, 2017

enebo commented Nov 21, 2017

headius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

headius left a comment

Choose a reason for hiding this comment

headius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-someone commented Dec 1, 2017

headius commented Dec 5, 2017

nick-someone commented Dec 5, 2017