Community
Participate
Working Groups
Created attachment 244239 [details] test case exposing the bug In a batched build, annotation processing may not run on all compilation units. Please take a look at the attached test case that exposes the problem. In the test case there are 2 classes, Foo and Bar that have references to each other. The batch size is set to 1. Suppose that Foo is in the first batch. Foo has a reference to Bar, so both Foo and Bar are compiled in the first batch, but only Foo was actually passed to the compiler, so only Foo has annotation processing run on it. Then in the second batch, we detect that Bar was compiled in the first batch, so we are done with compiling. The problem is that then Bar never has annotation processing run on it.
Gentle ping. The provided test case is reproducible and I have dozens of users running into this problem.
This looks like same as bug 407841. Can you confirm if the fix works for you? This was fixed for Luna.
I just re-ran the test case and it still fails. I can confirm that I have the patch from bug 407841 in my workspace.
I think the problem is that when processAnnotations() is called in the compiler, it checks for units that have already been processed. But if units from the next batch are processed after processAnnotations() is called, which happens later in the compiler's compile() method with the ProcessTaskManager, then the units from the next batch that are brought in are ignored by processAnnotations(). This seems to be what is happening in the provided test case, at least.
(In reply to Harry Terkelsen from comment #4) > I think the problem is that when processAnnotations() is called in the > compiler, it checks for units that have already been processed. But if units > from the next batch are processed after processAnnotations() is called, > which happens later in the compiler's compile() method with the > ProcessTaskManager, then the units from the next batch that are brought in > are ignored by processAnnotations(). This seems to be what is happening in > the provided test case, at least. Hmm.. that sounds like the same scenario that I fixed. If you look at the patch, the fix was to get the units that were compiled after processAnnotations() was called and include them in the next batch of processAnnotations() call. Anyway, I will take a look at your attached project and see what's going on. Will put in M3 to keep it on my radar.
I think this is an edge case where there are classes marked to be processed in the next processAnnotations() call but the next batch never happens because all files have been processed implicitly
(In reply to Harry Terkelsen from comment #6) > I think this is an edge case where there are classes marked to be processed > in the next processAnnotations() call but the next batch never happens > because all files have been processed implicitly I see the problem. Thanks for the test case. I think the fix should be along the lines of the alternate solution I talked about in bug 407841, comment #12. But I recall this being tricky. Will see what I can do. Meanwhile, if anyone has the time, patches are welcome.
I have a draft patch. I will work on this post M4.
Created attachment 249260 [details] Draft patch under test Patch I was talking about. Still some rough edges, needs to be closely reviewed as it touches the compilation process. As said, will take it up for M5.
I made some changes to the fix and pushed to gerrit: https://git.eclipse.org/r/#/c/38310/ And hooked the new test into the JDT framework, it's here: https://git.eclipse.org/r/#/c/38311/ Shankha, can you please review, thanks!
In the interest of getting this tested sooner than later, I have release the fix and test through respective commits: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=2812d86d568ba2c20ada48601e951ea64c90a8dd http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=29f4739da22539e75a2d7ec3c39641dd2a5bf473 Shankha, review this when you find time.
This patch causes annotation processing after STB.scope has been nulled (from CUD.cleanUp()). This is dangerous - and causes NPE when combined with Bug 436486. I'll try to add protection in the other bug, but I'm afraid, that NPE could surface in *many* places accessing STB.scope! The dangerous call is in Compiler.process() line 543. Would it make sense to let processAnnotations temporarily restore the STB.scope link?
(In reply to Stephan Herrmann from comment #12) > This patch causes annotation processing after STB.scope has been nulled > (from CUD.cleanUp()). This is dangerous - and causes NPE when combined with > Bug 436486. I'll try to add protection in the other bug, ... Being tested via https://git.eclipse.org/r/#/c/38731/ I'll release if successful, but I'd like to revert that change if the situation can be avoided from this bug ... I'm aware that STB.scope can be null on a few control flows from DOM into the compiler. But the change in this bug seems to open much wider doors.
(In reply to Stephan Herrmann from comment #13) > (In reply to Stephan Herrmann from comment #12) > > This patch causes annotation processing after STB.scope has been nulled > > (from CUD.cleanUp()). This is dangerous - and causes NPE when combined with > > Bug 436486. I'll try to add protection in the other bug, ... > > Being tested via https://git.eclipse.org/r/#/c/38731/ > I'll release if successful, but I'd like to revert that change if the > situation can be avoided from this bug ... temporarily released via commit dbdde44f24ace4ee47bccce2cfe484eeeb8013f3 Re-opening this bug so we don't forget to settle this conflict :)
(In reply to Stephan Herrmann from comment #14) > temporarily released via commit dbdde44f24ace4ee47bccce2cfe484eeeb8013f3 Thanks for the fix, Stephan. My bad, I should have run the tests after rebasing. Will take a closer look at the fix again.
(In reply to Jay On Vacation till Jan 5 2015 from comment #15) > (In reply to Stephan Herrmann from comment #14) > > temporarily released via commit dbdde44f24ace4ee47bccce2cfe484eeeb8013f3 > > Thanks for the fix, Stephan. My bad, I should have run the tests after > rebasing. Objection: that mistake was mine :) (since your commit was released first). OTOH, what are the odds for two consecutive changes interacting with each other in this way? I think that's a risk we can live with.
(In reply to Stephan Herrmann from comment #16) > OTOH, what are the odds for two consecutive changes interacting with each > other in this way? I think that's a risk we can live with. Yep, this is not something that we can overcome. In the past many times I run into non fast-forward commit issues because some commit has come while the tests were running. I just rebase, push and move on :) BTW, I am testing a fix for the regression. I will revert commit dbdde44f24ace4ee47bccce2cfe484eeeb8013f3 and add my fix.
Reverted the stop-gap fix with this: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=4b2447f980b1f004d5d76b9cf416fd4398082a5b And fixed with this: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=9ab1542db1d40bdde11e7c06d8c1a2b268c6c336
Verified for 4.5 M5 using I20150127-0900 build
