Skip to content

Fixes #343 withincode doesn't work as specified #345

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jafarre-bi
Copy link

@jafarre-bi jafarre-bi commented Jul 15, 2025

Pointcuts of the form withincode(MethodSignature) don't match method-execution joinpoints, which is in contradiction with the documentation. Moreover, both constructor-execution and initialization joinpoints are matched by pointcuts of the form withincode(ConstructorSignature). This agrees with the documentation, but is inconsistent with the behaviour regarding method execution.
This PR allows withincode pointcuts to match method-execution joinpoints and modifies the corresponding unit tests.

@jafarre-bi
Copy link
Author

My doubt here is whether withincode should match initialization joinpoints or not, as explained in the comment. What do you think @kriegaex?

@kriegaex
Copy link
Contributor

kriegaex commented Jul 16, 2025

@jafarre-bi, Being inactive at the moment, I really have not looked into your issue and this PR, validating whether there is in fact a real problem that needs to be solved, maybe just a documentation issue, or neither. But if you still think that your PR solves a problem, please note that it causes test failures. Please check if either your code or the tests need to be adjusted.

@jafarre-bi jafarre-bi force-pushed the bugfix/343withincode branch from 171bee3 to e3e1fb8 Compare July 16, 2025 00:11
@jafarre-bi
Copy link
Author

@jafarre-bi, Being inactive at the moment, I really have not looked into your issue and this PR, validating whether there is in fact a real problem that needs to be solved, maybe just a documentation issue, or neither. But if you still think that your PR solves a problem, please note that it causes test failures. Please check if either your code or the tests need to be adjusted.

Yes, that's why it's still a draft. I didn't fully understand how that worked. Now I do.
In the end, it's simply adding method execution to the joinpoint kinds matched by withincode pointcuts.
Surprisingly, there are unit tests checking specifically that they don't match. Difficult to understand, as it's inconsistent with constructor and initialization pointcuts, which do match, and directly in contradiction with the documentation.
I just pushed the changes with unit-test fixes. The tests don't pass in my local environment because of unexpected warnings from the compiler, which are probably due to the version of my JDK. Let's see if they pass here.

@kriegaex
Copy link
Contributor

Let's see if they pass here.

They did. Next, you probably want to take a look at the failing integration tests.

@kriegaex
Copy link
Contributor

Regarding 26caa9f, there are still failing ITs. Furthermore, you seem to have committed a few files with only whitespace changes. You should fix up the commit to no longer contain those changes and force-push. BTW, I am still not saying anything about the changes in the code and tests, because I really have not looked into any of it.

@jafarre-bi
Copy link
Author

jafarre-bi commented Jul 18, 2025

In the first commit, I committed indentation that mixed spaces with the original tab-based indentation, and it wasn't aligned. That's why there are whitespace-only changes in the second commit. If you look into the changes as a whole, there are no whitespace-only changes.

@jafarre-bi
Copy link
Author

I finally managed to get all tests to pass in my local environment by changing the JDK to version 21. I also removed all changes in white space.
While I believe this is a bug fix, it may also be a breaking change, as some pointcuts will match joinpoints they didn't match before. However, the more I think about it, the clearer it becomes that the problem isn't with the documentation. It would not be easy to document the current behaviour solidly.
There's something else I haven't looked into yet. I took a quick look at the implementation of the @withincode annotation's behaviour, and I believe it currently behaves differently from the withincode clause. It appears to exclude all container-type joinpoints, even constructor execution and initialisation. The difference would be more pronounced with the changes in this PR.

@jafarre-bi jafarre-bi force-pushed the bugfix/343withincode branch from 4a8e430 to 1f79352 Compare July 20, 2025 21:53
@jafarre-bi jafarre-bi marked this pull request as ready for review July 23, 2025 16:13
@jafarre-bi jafarre-bi changed the title Fix 343 withincode doesn't work as specified Fixes eclipse-aspectj/aspectj/#343 withincode doesn't work as specified Jul 24, 2025
@jafarre-bi jafarre-bi changed the title Fixes eclipse-aspectj/aspectj/#343 withincode doesn't work as specified Fixes eclipse-aspectj/aspectj#343 withincode doesn't work as specified Jul 24, 2025
@jafarre-bi jafarre-bi changed the title Fixes eclipse-aspectj/aspectj#343 withincode doesn't work as specified Fixes #343 withincode doesn't work as specified Jul 24, 2025
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