Signature failure does not halt compilation

[lukehutch]

When using Animal Sniffer 1.18, even with a failure like the following, compilation continues and succeeds:

[INFO] --- maven-enforcer-plugin:3.0.0-M3:enforce (check-signatures) @ classgraph ---
[INFO] Checking unresolved references to org.codehaus.mojo.signature:java17:1.0
[INFO] /home/luke/Work/classgraph/src/main/java/nonapi/io/github/classgraph/fileslice/reader/RandomAccessArrayReader.java:107: Covariant return type change detected: java.nio.Buffer java.nio.ByteBuffer.limit(int) has been changed to java.nio.ByteBuffer java.nio.ByteBuffer.limit(int)

The fact that the build did not stop on this error has caused me to push out releases that broke runtime compatibility with JDK 1.7/1.8.

[famod]

Are you sure your enforcer config is set to fail (which is the default, though)?

What happens when you use the check-Mojo instead?
E.g.:

<plugin>
    <groupId>org.codehaus.mojo</groupId>
    <artifactId>animal-sniffer-maven-plugin</artifactId>
    <executions>
        <execution>
            <id>check-java-version-compatibility</id>
            <phase>test</phase>
            <goals>
               <goal>check</goal>
            </goals>
            <configuration>
                <signature>
                    <groupId>org.codehaus.mojo.signature</groupId>
                    <artifactId>java18</artifactId>
                    <version>1.0</version>
                </signature>
            </configuration>
        </execution>
    </executions>
</plugin>

[lukehutch]

This is what I'm using:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-enforcer-plugin</artifactId>
    <dependencies>
        <dependency>
            <groupId>org.codehaus.mojo</groupId>
            <artifactId>animal-sniffer-enforcer-rule</artifactId>
            <version>1.18</version>
        </dependency>
    </dependencies>
    <executions>
        <execution>
            <id>check-signatures</id>
            <phase>compile</phase>
            <goals>
                <goal>enforce</goal>
            </goals>
            <configuration>
                <rules>
                    <checkSignatureRule
                        implementation="org.codehaus.mojo.animal_sniffer.enforcer.CheckSignatureRule">
                        <signature>
                            <groupId>org.codehaus.mojo.signature</groupId>
                            <artifactId>java17</artifactId>
                            <version>1.0</version>
                        </signature>
                    </checkSignatureRule>
                </rules>
            </configuration>
        </execution>
    </executions>
</plugin>

I still get this if there's a signature issue, without failure:

[INFO] --- maven-enforcer-plugin:3.0.0-M3:enforce (check-signatures) @ classgraph ---
[INFO] Checking unresolved references to org.codehaus.mojo.signature:java17:1.0
[INFO] /home/luke/Work/classgraph/src/main/java/nonapi/io/github/classgraph/fileslice/reader/ClassfileReader.java:268: Covariant return type change detected: java.nio.Buffer java.nio.ByteBuffer.position(int) has been changed to java.nio.ByteBuffer java.nio.ByteBuffer.position(int)

I tried changing the phase to test and the goal to check for check-signatures, and I get the following with ./mvnw clean package:

[ERROR] Could not find goal 'check' in plugin org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3 among available goals display-info, enforce, help -> [Help 1]

[lukehutch]

Adding the following to the configuration section does not change the behavior:

                                <fail>true</fail>
                                <failFast>true</failFast>
                                <ignoreCache>true</ignoreCache>
                                <skip>false</skip>

[lukehutch]

Narrowing this down further -- this does not seem to be a configuration problem, but rather a problem that covariant return types are not flagged as errors.

I am compiling with JDK 1.8, using -source 1.7 -target 1.7.

The following code is correct:

public void setPos(ByteBuffer buf, long pos) throws IOException {
    ((Buffer) buf).position(pos);
}

The following gives the Covariant return type change detected warning reported previously, when checked with the Java 1.7 signatures, but the build doesn't fail (however the code will fail at runtime):

public void setPos(ByteBuffer buf, long pos) throws IOException {
    buf.position(pos);
}

The following code does report an error and halt the build, since Stream was added in Java 1.8:

Arrays.asList(1, 2, 3).stream().forEach(new Consumer<Integer>() {
    @Override
    public void accept(Integer e) {
        System.out.println(e);
    }
});

[INFO] --- maven-enforcer-plugin:3.0.0-M3:enforce (check-signatures) @ classgraph ---
[INFO] Checking unresolved references to org.codehaus.mojo.signature:java17:1.0
[ERROR] /home/luke/Work/classgraph/src/main/java/nonapi/io/github/classgraph/fileslice/reader/ClassfileReader.java:270: Undefined reference: java.util.stream.Stream
[ERROR] /home/luke/Work/classgraph/src/main/java/nonapi/io/github/classgraph/fileslice/reader/ClassfileReader.java:270: Undefined reference: java.util.stream.Stream java.util.List.stream()
[ERROR] /home/luke/Work/classgraph/src/main/java/nonapi/io/github/classgraph/fileslice/reader/ClassfileReader.java:270: Undefined reference: void java.util.stream.Stream.forEach(java.util.function.Consumer)

[famod]

I had a quick look at where this Covariant return type change detected message is coming from: https://github.com/mojohaus/animal-sniffer/blob/animal-sniffer-parent-1.18/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureChecker.java#L600

So it is not coming from the signature itself.

[lukehutch]

@famod Thanks. So return true should be return false instead?

https://github.com/mojohaus/animal-sniffer/blob/animal-sniffer-parent-1.18/animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/SignatureChecker.java#L603

[famod]

@lukehutch

@famod Thanks. So return true should be return false instead?

Well, that would be wrong for the 1.8 signature, wouldn't it?

[lukehutch]

@famod I think line 603 should say return false, and a line needs to be inserted after line 606 saying return true. But I'm only guessing based on what the rest of the method does.

[lukehutch]

@famod Please see PR #87.

[lukehutch]

Actually somebody already addressed this in #83.

[cpovirk]

We likewise pushed out a release that broke Java 7/8 compatibility, so we're now looking at this issue. Is there any chance that #83 (or a similar PR) might be merged in the near future, or should we look into alternative ways of detecting this problem? (We'd use javac --release, but that doesn't work with code that uses sun.misc.Unsafe when available.)

Here's an example of the problem:

$ cat src/main/java/com/foo/Bar.java 
package com.foo;

class Bar {
  public static void main(String[] args) {
    java.nio.ByteBuffer.allocate(0).position(0);
  }
}

$ cat pom.xml 
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>
 
  <groupId>com.foo</groupId>
  <artifactId>bar</artifactId>
  <version>1.0-SNAPSHOT</version>

  <properties>
    <maven.compiler.source>1.8</maven.compiler.source>
    <maven.compiler.target>1.8</maven.compiler.target>
  </properties>

  <build>
    <plugins>
      <plugin>
        <groupId>org.codehaus.mojo</groupId>
        <artifactId>animal-sniffer-maven-plugin</artifactId>
        <version>1.19</version>
        <configuration>
          <signature>
            <groupId>org.codehaus.mojo.signature</groupId>
            <artifactId>java18</artifactId>
            <version>1.0</version>
          </signature>
        </configuration>
      </plugin>
    </plugins>
  </build>
</project>

$ JAVA_HOME=/usr/local/buildtools/java/jdk11 mvn clean install animal-sniffer:check | grep -e Covariant -e SUCCESS
[INFO] /usr/local/google/home/cpovirk/code/animalsnifferbytebuffer/src/main/java/com/foo/Bar.java:5: Covariant return type change detected: java.nio.Buffer java.nio.ByteBuffer.position(int) has been changed to java.nio.ByteBuffer java.nio.ByteBuffer.position(int)
[INFO] BUILD SUCCESS

$ /usr/local/buildtools/java/jdk8/bin/java -cp $HOME/.m2/repository/com/foo/bar/1.0-SNAPSHOT/bar-1.0-SNAPSHOT.jar com.foo.Bar
Exception in thread "main" java.lang.NoSuchMethodError: java.nio.ByteBuffer.position(I)Ljava/nio/ByteBuffer;
        at com.foo.Bar.main(Bar.java:5)

As in #83 (comment), I wasn't able to find much detail about MANIMALSNIFFER-49, only an archive of the brief original post.

[cpovirk]

Slightly more history:

does it matter for JVM what kind of Object is returned, even though it's not used?

The question wasn't directly answered, and I think Animal Sniffer came away assuming that the answer is "no," leading to:

An info-message is added when Animal Sniffer hits this kind of valid method change.

I believe my snippet above gives a more direct answer of "yes."

[onacit]

When will we have java18:1.1? Thanks.

[cpovirk]

I just got it into my head to try using old version 1.13 (before the bug was introduced) as a workaround. That turns out to produce an IllegalArgumentException in ASM, which maybe could be fixed.

But the real discovery was that, when I was ensuring that I'd actually reintroduced the problem, I found that the current version of Animal Sniffer catches it! It looks like @olamy merged @ogolberg's fix for this in ba0d71e, and apparently I'd neglected to subscribe to that PR thread way back when. I've confirmed that the first fixed version is 1.21. Thanks for taking care of that!