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

Improve condy (constant dynamic) support #139

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

kriegaex
Copy link
Contributor

Fixes https://issues.apache.org/jira/browse/BCEL-362.

There was a problem when processing JaCoCo-instrumented code with BCEL, e.g. when passing it through example class JasminVisitor, causing

java.lang.ClassCastException: class org.apache.bcel.classfile.ConstantMethodref cannot be cast to class org.apache.bcel.classfile.ConstantClass (org.apache.bcel.classfile.ConstantMethodref and org.apache.bcel.classfile.ConstantClass are in unnamed module of loader 'app')
    at org.apache.bcel.generic.ConstantPoolGen.<init>(ConstantPoolGen.java:153)
    at org.apache.bcel.generic.ConstantPoolGen.<init>(ConstantPoolGen.java:211)
    at de.example.JasminVisitor.<init>(JasminVisitor.java:70)

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #139 (735abe4) into master (8b0b168) will increase coverage by 0.04%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master     #139      +/-   ##
============================================
+ Coverage     44.08%   44.12%   +0.04%     
- Complexity     2521     2527       +6     
============================================
  Files           362      362              
  Lines         16322    16327       +5     
  Branches       2126     2127       +1     
============================================
+ Hits           7195     7205      +10     
+ Misses         8398     8394       -4     
+ Partials        729      728       -1     
Impacted Files Coverage Δ
...n/java/org/apache/bcel/classfile/ConstantPool.java 64.66% <0.00%> (-2.01%) ⬇️
.../java/org/apache/bcel/generic/ConstantPoolGen.java 68.98% <100.00%> (+0.21%) ⬆️
src/main/java/org/apache/bcel/util/Class2HTML.java 53.19% <0.00%> (-0.50%) ⬇️
src/main/java/org/apache/bcel/Repository.java 14.28% <0.00%> (ø)
src/main/java/org/apache/bcel/util/CodeHTML.java 50.17% <0.00%> (ø)
src/main/java/org/apache/bcel/util/MethodHTML.java 56.06% <0.00%> (ø)
...c/main/java/org/apache/bcel/util/ConstantHTML.java 95.04% <0.00%> (ø)
.../main/java/org/apache/bcel/util/AttributeHTML.java 56.38% <0.00%> (ø)
src/main/java/org/apache/bcel/util/ClassPath.java 42.63% <0.00%> (+1.16%) ⬆️
.../main/java/org/apache/bcel/classfile/Constant.java 57.14% <0.00%> (+2.38%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@garydgregory
Copy link
Member

garydgregory commented Aug 14, 2022

Hello @kriegaex,

Thank you for you PR!

You'll need update this PR with some kind of test, otherwise, this is a regression waiting to happen. It could be as simple as parsing a class file that fails without applying the "main" side for the PR. The test should reflect what you say happens, namely:

java.lang.ClassCastException: class org.apache.bcel.classfile.ConstantMethodref cannot be cast to class org.apache.bcel.classfile.ConstantClass (org.apache.bcel.classfile.ConstantMethodref and org.apache.bcel.classfile.ConstantClass are in unnamed module of loader 'app')
    at org.apache.bcel.generic.ConstantPoolGen.<init>(ConstantPoolGen.java:153)
    at org.apache.bcel.generic.ConstantPoolGen.<init>(ConstantPoolGen.java:211)
    ...

Take notice that Codecov commented that coverage has decreased due to this PR not being tested:

- Coverage     44.08%   44.08%   -0.01%  

In fact, that is often what I do to validate a PR:

  • Apply the test side of the PR,
  • Watch the test fail,
  • Apply the main side of the PR, and
  • Watch the test pass.
  • Merge the PR

Thank you again for helping out 😄

@kriegaex
Copy link
Contributor Author

kriegaex commented Aug 14, 2022

So, instead of being happy that someone provides a PR at all after me having begged the contributors for a bugfix, and writing a test by yourself, now the PR I was asked to contribute before is not good enough still, and I should provide a test, too? That is quite an interesting experience with this project.

What exactly do maintainers do around here? Simply wait for users to do everything? Actually, I do have a test, but it requires JaCoCo in my Maven project. I checked whether you have a Maven Invoker test suite, but you do not. So there is no easy way to provide a test without digging into BCEL's intestines and writing a separate test, similar to the ones you already have with extra precompiled class files that can be processed by BCEL. I do not have the cycles for that.

Actually, I only wanted to fix a problem in AspectJ, asking the BCEL team for help. I got none, so I had to fix it myself in AspectJ. I was even nice enough to provide a bugfix for the upstream project, and now that is not good enough. Uh-huh.

Fixes https://issues.apache.org/jira/browse/BCEL-362.

There was a problem when processing JaCoCo-instrumented code with BCEL,
e.g. when passing it through example class JasminVisitor, causing

java.lang.ClassCastException:
  class org.apache.bcel.classfile.ConstantMethodref cannot be cast to
  class org.apache.bcel.classfile.ConstantClass
    (org.apache.bcel.classfile.ConstantMethodref and
    org.apache.bcel.classfile.ConstantClass are in unnamed module
    of loader 'app')
  at org.apache.bcel.generic.ConstantPoolGen.<init>(ConstantPoolGen.java:153)
  at org.apache.bcel.generic.ConstantPoolGen.<init>(ConstantPoolGen.java:211)
  at de.example.JasminVisitor.<init>(JasminVisitor.java:70)
@kriegaex
Copy link
Contributor Author

kriegaex commented Aug 15, 2022

FWIW, I added a test case - not with the goal to increase coverage but to prevent future regressions. Reverse the order of the two commits, and the test before the fix will fail.

Instead, we document contributions in `changes.xml` which I'll update after I merge PR.
@garydgregory garydgregory merged commit 3c38bf6 into apache:master Aug 15, 2022
asfgit pushed a commit that referenced this pull request Aug 15, 2022
@kriegaex kriegaex deleted the BCEL-362 branch August 15, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants