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

Ignore only the valueOf methods used for boxing. #32

Merged
merged 2 commits into from Oct 27, 2016

Conversation

csoroiu
Copy link
Contributor

@csoroiu csoroiu commented Oct 21, 2016

With this commit all tests are passing when retrolambda is used in combination with IntelliJ & jacoco code coverage.
Without this commit the test LambdaTest.shouldResolveSubclassArgumentsForMethodRefs was failing when ran with code coverage after retrolambda was used.

Basically, we need to check if the valueOf method matches the signature of methods used for boxing operations such as Integer.valueof(int), Boolean.valueOf(boolean) etc.

Integer class contains 3 valueOf methods out of which only Integer.valueof(int) is used for boxing.

Thanks

@codecov-io
Copy link

codecov-io commented Oct 21, 2016

Current coverage is 79.09% (diff: 57.14%)

Merging #32 into master will decrease coverage by 1.46%

@@             master        #32   diff @@
==========================================
  Files             1          1          
  Lines           216        220     +4   
  Methods           0          0          
  Messages          0          0          
  Branches         77         79     +2   
==========================================
  Hits            174        174          
  Misses           14         14          
- Partials         28         32     +4   

Powered by Codecov. Last update f4a5330...9b88eb6

@csoroiu csoroiu force-pushed the master branch 2 times, most recently from b435b95 to 44c883e Compare October 21, 2016 18:54
@csoroiu
Copy link
Contributor Author

csoroiu commented Oct 24, 2016

@jhalterman, I feel that I should have written a more detailed explanation of the current changes.
Basically, the test LambdaTest.shouldResolveSubclassArgumentsForMethodRefs, when it is ran without retrolambda, but with IntelliJ Idea coverage enabled, the getMemberRef method processes the following ConstantPool values(backwards, same as the main iteration):

method: public static Integer valueOf(java.lang.String)
  declared in class: class java.lang.Integer
constructor: public Object()
  declared in class: class java.lang.Object

And the return value is:

method: public static Integer valueOf(java.lang.String)
  declared in class: class java.lang.Integer

But, with retrolambda and with IntelliJ Idea coverage, the ConstantPool processed values are the following:

constructor (added by retrolambda): private LambdaTest$$Lambda$23() []
  declared in class: class net.jodah.typetools.functional.LambdaTest$$Lambda$23
method: public static Integer valueOf(java.lang.String)
  declared in class: class java.lang.Integer
constructor: public Object()
  declared in class: class java.lang.Object
method added by coverage tool: public static Object loadClassData(java.lang.String)
  declared in class: class com.intellij.rt.coverage.data.ProjectData

Without the fix return value is:

method: public static Object loadClassData(java.lang.String)
  declared in class: class com.intellij.rt.coverage.data.ProjectData

With the fix return value is:

method: public static Integer valueOf(java.lang.String)
  declared in class: class java.lang.Integer

In the first case (without retrolambda), IntelliJ's coverage tool does not instrument the runtime generated lambda class, but in the second case, the class is not runtime generated anymore, it is generated by retrolambda in the compilation step, and even if it is marked as a synthetic class, it is still instrumented by IntelliJ's coverage tool.

With this change the type detection is more stable for lambdas and I couldn't think yet to some other failing scenario.
Also, it seems that lambda type detection will not work on Android or Java 9 because they do not have an accessible ConstantPool object.

Cheers

@jhalterman
Copy link
Owner

@csoroiu AFAIK Oracle/Open JDK 9 should still have ConstantPool - is that not the case?

http://hg.openjdk.java.net/jdk9/dev/jdk/file/e6b6ca2d616e/src/java.base/share/classes/java/lang/Class.java#l2810

@jhalterman jhalterman merged commit 8318bfe into jhalterman:master Oct 27, 2016
@jhalterman
Copy link
Owner

@csoroiu This stuff looks good - thanks! Is there anything else still TODO regarding retrolambda support that should be looked at before the next release? Possibly related would be ProGuard, though I haven't looked at that at all yet...

@csoroiu
Copy link
Contributor Author

csoroiu commented Oct 28, 2016

@jhalterman regarding the ConstantPool in jdk9, indeed there is one, but due to project jigsaw is not accessible. It has been moved to jdk.internal.reflect package. I try changing only the import, but the code does not compile. When using reflection it throws an exception when you try to invoke the methods on the constantpool object. If a remember correctly it was an IllegalAccessException.

I haven't worked with ProGuard yet. I'm not an Android guy, but the issue #3 relates to Android. The least thing we should do is to create a simple android project and run the typetools unit tests on that platform with and without ProGuard. I could try to do this, but it may take a while as I'm not an Android developer.

@jhalterman
Copy link
Owner

@csoroiu I'm not an Android developer either. I expect none of the lambda/method-ref capabilities to work since Android is still on 1.6 AFAIK. So, no need to worry about that stuff.

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

Successfully merging this pull request may close these issues.

None yet

3 participants