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

NaN while scanning for annotations with "double" fields #339

Closed
danwallach opened this issue Apr 18, 2019 · 5 comments
Closed

NaN while scanning for annotations with "double" fields #339

danwallach opened this issue Apr 18, 2019 · 5 comments

Comments

@danwallach
Copy link

danwallach commented Apr 18, 2019

I'm working on a Java-based autograder for the class that I'll be teaching this fall. The broad idea is that you put annotations on unit tests to say how many points they're worth.

Our annotations look something like this:

@GradeTopic(project = "Sorting", topic = "HeapSort")
public class HeapSortTest {
  @Test
  @Grade(project = "Sorting", topic = "HeapSort", points = 0.4)
  public void heapSortStrings() {
    TestAnySorter.exerciseStrings(new HeapSort<>());
  }

  @Test
  @Grade(project = "Sorting", topic = "HeapSort", points = 0.4)
  public void heapSortIntegers() {
    TestAnySorter.exerciseIntegers(new HeapSort<>());
  }
}

The corresponding definition of @Grade is pretty straightforward:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
@Documented
@Repeatable(Grades.class)
public @interface Grade {
  String project();
  String topic();
  double points();
  double maxPoints() default 0.0;
}

What happens:

When I've got double values like the number of points, I've found that sometimes I get NaN rather than the number. You can see this in action if you fetch this commit and run ./gradlew :brokenExampleSort:autograderDebugAnnotations:

https://github.com/RiceComp215-Staff/RiceChecks/tree/c6d5d26024b2a0dc3b89e998e70ca899c26fd6a6

The output will have ClassGraph logging mixed in with my own logging, but you'll see something like this:

2019-04-18T14:30:02.753-0500	ClassGraph	------------------ Method: @org.junit.jupiter.api.Test @edu.rice.autograder.annotations.Grade(project="Sorting", topic="HeapSort", points=NaN) public void heapSortStrings()

In this case, something about 0.4 points in HeapSortTest.java is causing it to become NaN. If you look instead at the adjacent InsertionSortTest.java, where it's using 0.5 points, then everything seems completely fine. If you edit those 0.4 numbers and change them to 0.5, then everything again works.

I've found that integers never cause a problem. The problems only manifest with fractions between 0 and 1, and exactly which fractions are accepted and which are rejected is hard to predict. Pick a number and flip a coin for whether it's accepted or rejected.

FWIW, I'm running with OpenJDK 11 (Amazon Corretto) on macOS 10.14.4 with Kotlin 1.3.30 and the latest ClassGraph 4.8.24.

@lukehutch
Copy link
Member

@danwallach I'm happy to hear you're using ClassGraph at Rice -- I was accepted there for my PhD, and came very close to attending, but ultimately picked another institution. I was really impressed by the Rice CS Department though!

Thanks for the detailed and helpful bug report and test case. I was able to reproduce this. There was a bug in the 64 bit (long) big endian deserialization code -- longs were being deserialized as two 32-bit big endian ints, then both halves were being converted to longs and the top int shifted up by 32 bits before they were or'd together. However, sign extension during casting from int to long was causing all the bits in the top half to be set to 1 when the most significant bit was set in the bottom half. (When reading a double, first the value was read as a 64-bit long, then the bits were interpreted as an IEEE 754 double, hence the NaN you ran into.)

Fixed and released in 4.8.25.

(Incidentally this would also affect the reading of Zip64 files, and longs and doubles in other class metadata, so it was an important fix.)

@danwallach
Copy link
Author

That's great. And thanks for the fast turnaround.

@lukehutch
Copy link
Member

You're welcome!

@danwallach
Copy link
Author

Verified: fixes the bug.

@lukehutch
Copy link
Member

Thanks for verifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants