Skip to content

Conversation

@jonnybot0
Copy link
Contributor

Helpful background may be found in the comments of https://issues.apache.org/jira/browse/GROOVY-9541.

As @eric-milles noted there, we may need to do a more careful consideration of each consumer of StaticTypeCheckingSupport#evaluateExpression, since the source object's classloader may not actually be correct in every case.

I can attest that letting org.codehaus.groovy.transform.ASTTransformationCollectorCodeVisitor#findCollectedAnnotations use the context classloader will result in difficult-to-resolve cases of GROOVY-9541.

A word about 4616fc8: without that, you'll get a cast_failure when trying to compile the groovy-macro module. I was a bit nervous about it, but I think java.util.Optional#map provides enough null safety that it should work.

My local test suite is still finishing execution, but I thought a concrete diff & a PR would be helpful for the discussion.

@sonatype-lift
Copy link

sonatype-lift bot commented Oct 25, 2022

⚠️ 185 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@eric-milles
Copy link
Member

eric-milles commented Oct 25, 2022

Could you recreate the PR against the master branch? That is where we would first evaluate it and then we can cherry-pick it back to 4_0_X and 3_0_X for you if risk is minimal.

The changes look fine in general. Have you searched for each use of evaluateExpression or is there a subset that was handled?

@eric-milles
Copy link
Member

The cast failure implies that you used a different class loader to create the instance of AnnotationCollectorMode than the loader the compiler used to load that class. The transform loader is what you want if the compiler is going to be assigning references that aren't just strings and numbers.

@jonnybot0
Copy link
Contributor Author

Have you searched for each use of evaluateExpression or is there a subset that was handled?

Yes, this is all the usages in the source code I could find.

The cast failure implies that you used a different class loader to create the instance of AnnotationCollectorMode than the loader the compiler used to load that class. The transform loader is what you want if the compiler is going to be assigning references that aren't just strings and numbers.

Right. That's why I moved to using .valueOf(val.toString()) to get the annotation value without a cast. But I see what you mean. I should be able to use the transformLoader on the class. I'll do that in my follow-up PR to master.

@jonnybot0 jonnybot0 closed this Oct 26, 2022
@jonnybot0
Copy link
Contributor Author

I'll close this and reopen once I can get the patch working against the master branch.

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.

2 participants