Ticket #1218 solution

[EverettHanke]

Gives a warning when a constructor is injectable but is not public/annotated with @activiate telling users to make constructor public.

[merks]

FYI. You can rename your local branch and create a new branch reusing the name you used for your in-progress PR. You can force push this to your fork to update the PR. I.e., it’s not necessary to close a PR and create a new one.

[github-actions]

Test Results

   285 files  ±0     285 suites  ±0   49m 48s ⏱️ - 3m 50s
 3 586 tests ±0   3 510 ✅ ±0   76 💤 ±0  0 ❌ ±0 
10 950 runs  ±0  10 719 ✅ ±0  231 💤 ±0  0 ❌ ±0 

Results for commit c9977e6. ± Comparison against base commit ef694eb.

[EverettHanke]

@merks thank you for letting me know. I didn't know how I would go about merging/reducing the commits into one. This makes sense I think. Sorry about the mess I've made here, it's not my intentions.

[merks]

No need to be sorry! Thanks for all the effort!!

[EverettHanke]

Could I get some help with why our PR keeps failing the Maven, Jenkins, and Compiler and API Tools tests. Our PR as far as I'm concerned has nothing to do with any of these technologies. I created a new development environment before this PR when applying these changes as a single commit. There shouldn't be any out-of-date source material. Could someone in this community potentially point me in the right direction as to why these tests are always failing?

[merks]

It's mostly complaining about "new issues" that don't actually appear to be related to your changes:

I'm rebuilding to see if they go away...

Note that we are in the freeze for RC2/final this week so we cannot move forward with this until development opens for the 4.35 release cycle.

[HannesWell]

As said in #1464 (review), in the current form, this unfortunately does not solve the issue.

I'll go into the details below, but first of all:
Please DO NOT CLOSE THIS PR and create a new one to apply the requested changes!!

Instead amend your commit locally and force-push the change commit to this PR's branch.
If you have problems with git or Github, please ask your Code-Day mentor to help you.

And regarding your question from #1474 (comment):

Hi there @HannesWell. I'm having an issue with the squash of the several commits. Also this commit should be addressing the requested changes asked on the #1464 changes requested. However if it appears that we didn't meet all the expectations for the ticket. Then I'd like to somehow get into contact with the community so I can discuss it and get a better understanding. Do you have a community discord by chance?

Yes there are more direct ways to get in contact, see eclipse-ide#help, for details. But for technical discussions like this, a PR or originating GH issue are the right place. So lets discuss this here:

If you have a very simple OSGi DS component like the following

package pack;

import org.osgi.framework.BundleContext;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.event.Event;
import org.osgi.service.event.EventHandler;

@Component
public class MyComponent implements EventHandler {

	@Activate
	MyComponent(BundleContext ctx) {
	}

	@Override
	public void handleEvent(Event event) {
	}
}

and step through the changed code you can see that the the changed branch is never hit because the class does not have a default constructor but has an injectable constructor, it's just not public which is a stricter requirement from the spec.

So in order to fix this you chould change hasInjectableConstructor() to getInjectableConstructor() and return the inject-able constructor or null if non is available. You can then add another else-if branch to the 'big'-if that checks if the inject-able constructor is not null but not public. If that's the case then problem type should be reported.

[EverettHanke]

Hi there @HannesWell, I'm curious. Since you mentioned that we want to flag this only if there is no default constructor but it has an injectable constructor maybe we shouldn't reinvent the wheel by creating a new function called getInjectableConstructor(). I'm thinking since hasInjectableConstructor needs to be true and we want to see if there is no default constructor to be injected into.

I apologize for my lack of a better explanation but like if we ran the if statement "if (hasInjectableConstructor && noDefaultConstructor) " and then under this if statement we can run the error. My reasoning behind this thinking is that the bool hasInjectableConstructor is just telling us if an injectable constructor is in use and the noDefaultConstructor is telling us whether or not we have a constructor. And because we only want to display the warning when these two criterias are met this would make sense for our if parameters.

However I am led to believe that neither of these can be the case because the entire if statement is wrapped inside a bigger if statement that already checks to determine if we are having no default constructor. on line 250 in AnnotationsVisitor.java we can see a if statement that already checks to see if we have no default constructor to inject to and within that if statement we have a chain of sub ifs and elses one of which would be for our has injectable constructor. So I was under the understanding that this is how we throw the error while meeting the goals of the ticket.

Again I apologize if I am missing the point of this ticket but I would like to learn if this is the correct path to go down or if I may still need some further explaining. Thank you for your patience with us CodeDay folk.