Enhance the BuildContext with the discovered annotations by laeubi · Pull Request #676 · eclipse-jdt/eclipse.jdt.core

laeubi · 2023-01-31T09:30:47Z

What it does

Currently one can only get the information that annotations are present on a file in a BuildContext / CompilationParticipant but sometimes it is much more interesting to know if a specific annotation is present so one don't need to scan files unnecessary.

This captures the found low-level annotations and adds a new method based on this BuildContext.hasAnnotations(String) so that CompilationParticipant can check if the file is of any interest for them.

Fix #674

How to test

Have a CompilationParticipant
Have a project with a source file that contains an specific annotation and one without but containing another
in the method CompilationParticipant.processAnnotations(BuildContext[]) check that buildContext.hasAnnotations("...") returns true for you annotation of interest.

Author checklist

I have thoroughly tested my changes
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

laeubi · 2023-01-31T09:32:00Z

@stephan-herrmann or @iloveeclipse can you review this or suggest a reviewer?

This would be really helpful for PDE to not scan source files unnecessarily for annotations.

laeubi · 2023-01-31T10:26:14Z

The testfailure seems unrelated?

iloveeclipse · 2023-01-31T11:46:58Z

The testfailure seems unrelated?

Yes. Can you please add some tests using new API?

laeubi · 2023-01-31T13:02:54Z

I have now enhanced ParticipantBuildTests with a new testcase.

laeubi · 2023-01-31T15:13:13Z

Build has passed now! 🥳

laeubi · 2023-02-01T08:29:20Z

@iloveeclipse could this be merged or is anything left here?

iloveeclipse · 2023-02-01T10:18:32Z

Someone need to review this PR. I could do it later, this weekend or next week, I'm busy right now with internal HR work and mostly not have access to my workstation.

laeubi · 2023-03-27T04:29:07Z

@iloveeclipse any chance for this PR to be reviewed, or maybe suggest a reviewer for this?

jarthana · 2023-06-01T08:21:29Z

Let's consider this for M1.

laeubi · 2023-06-11T05:11:44Z

Let's consider this for M1.

Please let me know if anything has to be done from my side!

jarthana · 2023-06-15T04:25:24Z

I am considering this for merging this week. If anything, we can take up in follow up bugs. Let's see if we can get rid of the jenkins failures.

laeubi · 2023-06-15T13:29:20Z

I think I found the problem was some refactorings in the meantime and the change of the verions so there was an API error.

laeubi · 2023-06-19T04:40:45Z

@jarthana the testfailure seems unrelated as it can be seen in other PRs as well e.g. here: #1155

iloveeclipse · 2023-06-19T04:44:02Z

Please rebase to get rid of #1150 issue

laeubi · 2023-06-20T10:14:34Z

@iloveeclipse @jarthana this now is rebased and test are passing.

jarthana

This is just an observation. I see that we have the types already being added to the AbstractImageBuilder.newState.typeLocators. I don't see a direct way to use them, though.

jarthana · 2023-06-22T05:00:33Z

Now it fails with a different test. I see it passed locally, but the fact that it is not a test that has failed before and that it is related to the annotations, makes me wonder.

laeubi · 2023-06-22T05:07:43Z

I tried to enhance the assertin failure with some more information.

laeubi · 2023-06-26T07:56:25Z

The full failed exception trace is now shown as

junit.framework.AssertionFailedError
	at junit.framework.Assert.fail(Assert.java:55)
	at junit.framework.Assert.assertTrue(Assert.java:22)
	at junit.framework.Assert.assertNotNull(Assert.java:256)
	at junit.framework.Assert.assertNotNull(Assert.java:248)
	at junit.framework.TestCase.assertNotNull(TestCase.java:391)
	at org.eclipse.jdt.apt.tests.MirrorDeclarationTests$PackageInfoProc._process(MirrorDeclarationTests.java:343)
	at org.eclipse.jdt.apt.tests.annotations.generic.AbstractGenericProcessor.process(AbstractGenericProcessor.java:43)
	at org.eclipse.jdt.apt.core.internal.APTDispatchRunnable.dispatchToFileBasedProcessor(APTDispatchRunnable.java:668)
	at org.eclipse.jdt.apt.core.internal.APTDispatchRunnable.runAPTInFileBasedMode(APTDispatchRunnable.java:357)
	at org.eclipse.jdt.apt.core.internal.APTDispatchRunnable.build(APTDispatchRunnable.java:695)
	at org.eclipse.jdt.apt.core.internal.APTDispatchRunnable$1.run(APTDispatchRunnable.java:285)
	at org.eclipse.jdt.apt.core.internal.env.BuildEnv$CallbackRequestor.acceptBinding(BuildEnv.java:631)

laeubi · 2023-06-26T08:03:56Z

@jarthana I think the problem is the check for empty, because before the code was

if (result.hasAnnotations && this.filesWithAnnotations != null)

I will revert this, rebase (again) and then think it could be merged as is and if someone with deeper knowledge want to optimize it later on?

jarthana · 2023-06-26T09:16:33Z

@jarthana I think the problem is the check for empty, because before the code was

Yes, earlier we could easily detect annotations (that don't have have annotations themselves) by just the hasAnnotations flag. Now, it looks like we are going to have a depend on the presence of an empty []. Not a solid design, but I don't see this being fixed without putting the hasAnnotations back in the code.

I will revert this, rebase (again) and then think it could be merged as is and if someone with deeper knowledge want to optimize it later on?

Please fix this typo: precence => presence and then we are good to go.

Currently one can only get the information that annotations are present on a file in a BuildContext / CompilationParticipant but sometimes it is much more interesting to know if a specific annotation is present so one don't need to scann files unnecsaryly. This captures the found low-level annotations and adds a new method based on this BuildContext.hasAnnotations(String) so that CompilationParticipant can check fi the file is of any interest for them. Fix eclipse-jdt#674

laeubi · 2023-06-26T09:24:59Z

Please fix this typo: precence => presence and then we are good to go.

done!

…#676) Currently one can only get the information that annotations are present on a file in a BuildContext / CompilationParticipant but sometimes it is much more interesting to know if a specific annotation is present so one don't need to scann files unnecsaryly. This captures the found low-level annotations and adds a new method based on this BuildContext.hasAnnotations(String) so that CompilationParticipant can check fi the file is of any interest for them. Fix eclipse-jdt#674

laeubi force-pushed the issue_674 branch from e94887b to 92a3b8b Compare January 31, 2023 13:01

laeubi mentioned this pull request Feb 3, 2023

Enhance the BuildContext with the produced output class #694

Closed

3 tasks

laeubi force-pushed the issue_674 branch from 92a3b8b to e42e05b Compare March 27, 2023 04:28

laeubi force-pushed the issue_674 branch from e42e05b to ea801ca Compare March 27, 2023 04:54

jarthana added this to the 4.29 M1 milestone May 31, 2023

laeubi force-pushed the issue_674 branch from ea801ca to 70b003d Compare June 11, 2023 05:11

laeubi force-pushed the issue_674 branch 2 times, most recently from 9a4be66 to 7f4c05c Compare June 15, 2023 13:12

laeubi force-pushed the issue_674 branch from 7f4c05c to e4e7354 Compare June 17, 2023 09:01

laeubi force-pushed the issue_674 branch from e4e7354 to abe8d5e Compare June 19, 2023 04:46

laeubi force-pushed the issue_674 branch from abe8d5e to babc907 Compare June 20, 2023 10:14

jarthana requested changes Jun 21, 2023

View reviewed changes

Comment thread org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java

Comment thread ...lipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/CompilationParticipantResult.java Outdated

laeubi force-pushed the issue_674 branch from babc907 to 6b635d2 Compare June 21, 2023 15:35

laeubi requested a review from jarthana June 21, 2023 15:36

jarthana approved these changes Jun 21, 2023

View reviewed changes

laeubi force-pushed the issue_674 branch from 6b635d2 to 7a05144 Compare June 22, 2023 05:07

laeubi force-pushed the issue_674 branch from 7a05144 to aa74bfa Compare June 22, 2023 10:47

jarthana force-pushed the issue_674 branch from aa74bfa to 5042d9c Compare June 26, 2023 06:31

laeubi force-pushed the issue_674 branch from 5042d9c to 097d1ae Compare June 26, 2023 08:06

laeubi force-pushed the issue_674 branch from 097d1ae to 86b0820 Compare June 26, 2023 09:24

jarthana merged commit 8d512be into eclipse-jdt:master Jun 27, 2023

stephan-herrmann mentioned this pull request Oct 1, 2023

Add null guard for scope in SourceTypeBinding.storeAnnotations #1455

Closed

3 tasks

laeubi mentioned this pull request Oct 30, 2023

Always generate components by default eclipse-pde/eclipse.pde#841

Open

Conversation

laeubi commented Jan 31, 2023

What it does

How to test

Author checklist

Uh oh!

laeubi commented Jan 31, 2023

Uh oh!

laeubi commented Jan 31, 2023

Uh oh!

iloveeclipse commented Jan 31, 2023

Uh oh!

laeubi commented Jan 31, 2023

Uh oh!

laeubi commented Jan 31, 2023

Uh oh!

laeubi commented Feb 1, 2023

Uh oh!

iloveeclipse commented Feb 1, 2023

Uh oh!

laeubi commented Mar 27, 2023

Uh oh!

jarthana commented Jun 1, 2023

Uh oh!

laeubi commented Jun 11, 2023

Uh oh!

jarthana commented Jun 15, 2023

Uh oh!

laeubi commented Jun 15, 2023

Uh oh!

laeubi commented Jun 19, 2023

Uh oh!

iloveeclipse commented Jun 19, 2023

Uh oh!

laeubi commented Jun 20, 2023

Uh oh!

jarthana left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jarthana commented Jun 22, 2023

Uh oh!

laeubi commented Jun 22, 2023

Uh oh!

laeubi commented Jun 26, 2023

Uh oh!

laeubi commented Jun 26, 2023

Uh oh!

jarthana commented Jun 26, 2023

Uh oh!

laeubi commented Jun 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants