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

Runtime initialize non-constant static finals #474

Merged
merged 2 commits into from Dec 14, 2016

Conversation

Projects
None yet
3 participants
@bjkail
Contributor

bjkail commented Dec 12, 2016

In my case, ExecutionDataWriter.FORMAT_VERSION is the problematic constant. We workaround by using reflection to access the field so that javac won't inline the value from the version of the library we use to compile.

I reviewed the org.jacoco.doc/target/apidocs/constant-values.html, and JaCoCo.ASM_API_VERSION seemed like the only other constant that was likely to cause similar problems for anyone.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 12, 2016

Member

@bjkail to confirm my understanding of

calling code must be recompiled when upgrading to a new version of the
library even if the caller would otherwise not require any changes

you are doing something like

if (ExecutionDataWriter.FORMAT_VERSION != 0x1007) {
  throw new RuntimeException();
}

this condition is always false since javac inlines FORMAT_VERSION. And from here comes the need of recompilation?

Member

Godin commented Dec 12, 2016

@bjkail to confirm my understanding of

calling code must be recompiled when upgrading to a new version of the
library even if the caller would otherwise not require any changes

you are doing something like

if (ExecutionDataWriter.FORMAT_VERSION != 0x1007) {
  throw new RuntimeException();
}

this condition is always false since javac inlines FORMAT_VERSION. And from here comes the need of recompilation?

@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Dec 12, 2016

Contributor

We have a client library that uses JaCoCo, but clients need to be able to choose their own JaCoCo version. We store execution data on a per-format folder basis, and the client library uses ExecutionDataWriter.FORMAT_VERSION so it can store new execution data files in the right place or so it only attempts to load execution data files using ExecutionDataReader that it will be able to read.

Contributor

bjkail commented Dec 12, 2016

We have a client library that uses JaCoCo, but clients need to be able to choose their own JaCoCo version. We store execution data on a per-format folder basis, and the client library uses ExecutionDataWriter.FORMAT_VERSION so it can store new execution data files in the right place or so it only attempts to load execution data files using ExecutionDataReader that it will be able to read.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 12, 2016

Member

Okay, so very similar to my example: due to inlining client library can claim that uses older format, while runtime dependency changed and it actually uses new format.

Member

Godin commented Dec 12, 2016

Okay, so very similar to my example: due to inlining client library can claim that uses older format, while runtime dependency changed and it actually uses new format.

@Godin

Also could you please fix your git email settings prior to other changes
email
and after the fix amend this commit first:

git commit --amend --reuse-message=HEAD --reset-author
git push --force-with-lease

?

Show outdated Hide outdated org.jacoco.core/src/org/jacoco/core/data/ExecutionDataWriter.java Outdated
Show outdated Hide outdated org.jacoco.core/src/org/jacoco/core/JaCoCo.java Outdated
@@ -24,7 +24,11 @@
IExecutionDataVisitor {
/** File format version, will be incremented for each incompatible change. */
public static final char FORMAT_VERSION = 0x1007;
public static final char FORMAT_VERSION;

This comment has been minimized.

@Godin

Godin Dec 12, 2016

Member

And comment here too.

@Godin

Godin Dec 12, 2016

Member

And comment here too.

This comment has been minimized.

@bjkail

bjkail Dec 12, 2016

Contributor

I added comment here and in JaCoCo. I wrote a test that uses TargetLoader to load JaCoCo and a test class that accesses JaCoCo.ASM_API_VERSION, and it uses ASM to rewrite JaCoCo to assign a different value. It seemed like overkill, but I can do it if you prefer.

@bjkail

bjkail Dec 12, 2016

Contributor

I added comment here and in JaCoCo. I wrote a test that uses TargetLoader to load JaCoCo and a test class that accesses JaCoCo.ASM_API_VERSION, and it uses ASM to rewrite JaCoCo to assign a different value. It seemed like overkill, but I can do it if you prefer.

This comment has been minimized.

@Godin

Godin Dec 13, 2016

Member

Yes - this seems like an overkill. Just comment looks fine and enough for me.

@Godin

Godin Dec 13, 2016

Member

Yes - this seems like an overkill. Just comment looks fine and enough for me.

@@ -142,7 +142,7 @@ public void testInvalidVersion() throws IOException {
buffer.write(ExecutionDataWriter.BLOCK_HEADER);
buffer.write(0xC0);
buffer.write(0xC0);
final char version = ExecutionDataWriter.FORMAT_VERSION - 1;
final char version = (char) (ExecutionDataWriter.FORMAT_VERSION - 1);

This comment has been minimized.

@Godin

Godin Dec 12, 2016

Member

Interesting that explicit cast not needed in case of inlined value. Learned something new today 😆

@Godin

Godin Dec 12, 2016

Member

Interesting that explicit cast not needed in case of inlined value. Learned something new today 😆

This comment has been minimized.

@bjkail

bjkail Dec 12, 2016

Contributor

I also learned this today :-). I guess constant fields are treated like literal constants in all regards.

@bjkail

bjkail Dec 12, 2016

Contributor

I also learned this today :-). I guess constant fields are treated like literal constants in all regards.

This comment has been minimized.

@Godin

Godin Dec 12, 2016

Member

There are implicit casts: char -> int -> subtraction , in first case value after subtraction is known at compile time and so one more safe implicit cast int -> char. In second case value not known and so javac says "possible lossy conversion from int to char".

@Godin

Godin Dec 12, 2016

Member

There are implicit casts: char -> int -> subtraction , in first case value after subtraction is known at compile time and so one more safe implicit cast int -> char. In second case value not known and so javac says "possible lossy conversion from int to char".

@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Dec 12, 2016

Contributor

I believe I fixed the issues, thanks. Sorry for the sloppiness with the author/committer email and file modes.

Contributor

bjkail commented Dec 12, 2016

I believe I fixed the issues, thanks. Sorry for the sloppiness with the author/committer email and file modes.

@Godin Godin added this to the 0.7.9 milestone Dec 13, 2016

@Godin Godin self-assigned this Dec 13, 2016

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 13, 2016

Member

To make it more explicit that this is not a constant value I would prefer a static method instead:

public static char ExecutionDataWriter.getFormatVersion()

This breaks the API but people using it currently ended up with broken code anyways.

Member

marchof commented Dec 13, 2016

To make it more explicit that this is not a constant value I would prefer a static method instead:

public static char ExecutionDataWriter.getFormatVersion()

This breaks the API but people using it currently ended up with broken code anyways.

@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Dec 13, 2016

Contributor

@marchof My code uses FORMAT_VERSION via reflection and is not broken, and other code that uses FORMAT_VERSION and uses the same JaCoCo version at compilation and runtime is not broken. Replacing FORMAT_VERSION with getFormatVersion() means more work for me to support multiple JaCoCo versions, so I would rather drop the pull request than do that.

Contributor

bjkail commented Dec 13, 2016

@marchof My code uses FORMAT_VERSION via reflection and is not broken, and other code that uses FORMAT_VERSION and uses the same JaCoCo version at compilation and runtime is not broken. Replacing FORMAT_VERSION with getFormatVersion() means more work for me to support multiple JaCoCo versions, so I would rather drop the pull request than do that.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 13, 2016

Member

@bjkail Many thanks for moving ASM_API_VERSION!

I see your point about compatibility: Another option would be to leave FORMAT_VERSION as is and deprecate it. In addition we add a new method getFormatVersion(). If I understand your case correctly you need to stick with reflection anyways to support old JaCoCo versions.

But I can also live with your PR as is.

Member

marchof commented Dec 13, 2016

@bjkail Many thanks for moving ASM_API_VERSION!

I see your point about compatibility: Another option would be to leave FORMAT_VERSION as is and deprecate it. In addition we add a new method getFormatVersion(). If I understand your case correctly you need to stick with reflection anyways to support old JaCoCo versions.

But I can also live with your PR as is.

@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Dec 13, 2016

Contributor

@marchof As soon as a JaCoCo is released with non-constant FORMAT_VERSION, I can updated my code to compile against that version, which means javac won't inline the value, so my .class file will contain a reference to FORMAT_VERSION, which will resolve the value at runtime regardless of whether the field is actually constant, so I can remove the reflection. The test I suggested to Evgeny did basically the same: compile a test class that uses FORMAT_VERSION, then use TargetLoader to load it alongside different versions of ExecutionDataWriter transformed via ASM to have FORMAT_VERSION with value 0 as either constant (simulating current JaCoCo) or non-constant (simulating future JaCoCo).

Contributor

bjkail commented Dec 13, 2016

@marchof As soon as a JaCoCo is released with non-constant FORMAT_VERSION, I can updated my code to compile against that version, which means javac won't inline the value, so my .class file will contain a reference to FORMAT_VERSION, which will resolve the value at runtime regardless of whether the field is actually constant, so I can remove the reflection. The test I suggested to Evgeny did basically the same: compile a test class that uses FORMAT_VERSION, then use TargetLoader to load it alongside different versions of ExecutionDataWriter transformed via ASM to have FORMAT_VERSION with value 0 as either constant (simulating current JaCoCo) or non-constant (simulating future JaCoCo).

bjkail added some commits Dec 14, 2016

Make FORMAT_VERSION non-constant
If a constant expression is assigned to a static final variable, javac
will inline the constant value in the caller, which means constants
should only be used for values that will never change.  Otherwise,
calling code must be recompiled when upgrading to a new version of the
library even if the caller would otherwise not require any changes.
@Godin

Godin approved these changes Dec 14, 2016

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 14, 2016

Member

@bjkail Not sure I fully understand why "more work". But in any case let's keep this as a field. @marchof other options seems overkill to me. I updated changelog, will merge after pass of builds.

@bjkail A kind of off topic: since you support multiple versions, may I ask out of curiosity - down to which one? Especially that we talk about version of format that last time was changed in 0.7.5 (2015/05/24)?

Member

Godin commented Dec 14, 2016

@bjkail Not sure I fully understand why "more work". But in any case let's keep this as a field. @marchof other options seems overkill to me. I updated changelog, will merge after pass of builds.

@bjkail A kind of off topic: since you support multiple versions, may I ask out of curiosity - down to which one? Especially that we talk about version of format that last time was changed in 0.7.5 (2015/05/24)?

@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Dec 14, 2016

Contributor

@Godin By "more work" I meant that I would have to update the current reflection to look for both getFormatVersion() and FORMAT_VERSION (assuming the former replaced the latter). It's not hard or complex work, it's just more :-).

I know 0.7.4 is still heavily used (the most recent for 0x1006), but I've seen older versions in use too. I have not encountered 0.4.x (0x1005) or older. Older versions are used both due to risk aversion and because upgrading creates a "flag day" for components that want to merge/intersect their execution data (if one component moves then they all need to move).

Contributor

bjkail commented Dec 14, 2016

@Godin By "more work" I meant that I would have to update the current reflection to look for both getFormatVersion() and FORMAT_VERSION (assuming the former replaced the latter). It's not hard or complex work, it's just more :-).

I know 0.7.4 is still heavily used (the most recent for 0x1006), but I've seen older versions in use too. I have not encountered 0.4.x (0x1005) or older. Older versions are used both due to risk aversion and because upgrading creates a "flag day" for components that want to merge/intersect their execution data (if one component moves then they all need to move).

@Godin Godin merged commit 055e8e4 into jacoco:master Dec 14, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.