Fix type of constructor calls in noclasspathmode

[MartinWitt]

See #4643
Okay this fix has the useable stage reached.

[slarse]

A couple of comments on what's there right now with some suggestions. I haven't taken the time yet to actually verify that this is the correct way to go about it, but at first glance it looks reasonable.

I think it's very important that going forward we clearly separate the noclasspath workarounds in the code from the "happy path" of all types actually being available. Noclasspath workarounds often look cryptic, and can change from one JDT release to another as the behavior changes as it's technically speaking bad input to JDT.

[munnasyed]

Hi,
Can you please merge the changes in master. So that, we can run the testcases in our system.

[MartinWitt]

I think it's very important that going forward we clearly separate the noclasspath workarounds in the code from the "happy path" of all types actually being available

Should we start locating these temporary fixes in a separate class like NoClassPathUtils, where we clearly mark them as unstable and best effort? Most fixes are almost static methods and as you said workarounds for JDT.

[I-Al-Istannen]

Hi,
Can you please merge the changes in master. So that, we can run the testcases in our system.

@munnasyed, @MartinWitt's branch is publicly accessible. If you want to try it out, I'd suggest cloning it and building spoon yourself using mvn package. In its current state the quality of the fix is not good enough to be included in master.

[slarse]

I think it's very important that going forward we clearly separate the noclasspath workarounds in the code from the "happy path" of all types actually being available

Should we start locating these temporary fixes in a separate class like NoClassPathUtils, where we clearly mark them as unstable and best effort? Most fixes are almost static methods and as you said workarounds for JDT.

I would say, probably not. It's first of all pretty unlikely that we'd ever achieve any kind of consistency in using said helper class, as there is so much legacy and new contributors will likely not be aware of them. "Noclasspath" in itself is also not necessarily a good unifying quality for functionality. The workarounds are also, typically, incredibly context dependent and only used in a single place, and so the definition of the method used to handle that very specific case should imo be as close as possible to the use.

What we should definitely agree upon is a convention for structuring noclasspath workarounds. We might want to have a naming convention for helper methods, or something like that. I think at the very least we should not litter the "core logic" with the nitty-gritty details of these workarounds as it makes the core logic somewhat hard to follow.

[MartinWitt]

It's first of all pretty unlikely that we'd ever achieve any kind of consistency in using said helper class, as there is so much legacy and new contributors will likely not be aware of them.

You are correct, but we could decide that it is time for a change in our code style, and we start marking them from now on?

What we should definitely agree upon is a convention for structuring noclasspath workarounds. We might want to have a naming convention for helper methods, or something like that.

This clearly sounds like the use case for a marker annotation. Something like this as documenting annotation would enforce to link the related issue. We can mark all new methods resolving issues with noclasspath mode and clearly document that these fixes/workarounds are unstable.

@interface NoClasspathWorkaround {

 int issueNumber();
}

I think at the very least we should not litter the "core logic" with the nitty-gritty details of these workarounds as it makes the core logic somewhat hard to follow.

100% correct.

[slarse]

You are correct, but we could decide that it is time for a change in our code style, and we start marking them from now on?

Yes, absolutely. What I meant was specifically about the idea of "all methods doing X go into helper class Y". If you don't achieve consistency with such a solution (which, due to legacy, we never will), it's of little value: you'll never know where to look for those helpers. On the other hand, adopting a coding guideline of extracting noclasspath stuff into separate methods and marking them somehow improves code quality locally, even if it's not used everywhere.

This clearly sounds like the use case for a marker annotation. Something like this as documenting annotation would enforce to link the related issue. We can mark all new methods resolving issues with noclasspath mode and clearly document that these fixes/workarounds are unstable.

@interface NoClasspathWorkaround {

 int issueNumber();
}

I like that idea, let's do that. Introduce it in this PR perhaps? I'm thinking if perhaps issueNumber isn't the best attribute to have. It might not always be applicable, and regardless I think we should use @GitHubIssue for that, you could just stack the annotations.

[MartinWitt]

It might not always be applicable, and regardless I think we should use @GitHubIssue for that, you could just stack the annotations

@GitHubIssue is currently only available for test classes. We could change this, but currently @GitHubIssue has many JUnit dependencies in it. I'm unsure if we want to refactor our test tooling.

[slarse]

@GitHubIssue is currently only available for test classes.

We could easily move it.

@GitHubIssue has many JUnit dependencies in it

You're right, I was apparently on an out-of-date branch that didn't have the JUnit stuff when I was looking at it, I forgot we added that. With the fixed attribute on it it doesn't really make sense to use for production code even if we were to move the JUnit stuff to another file.

How about just having a String reason in NoClasspathWorkaround? I don't like the idea of explicitly tying the annotation itself to GitHub Issues, it seems a bit too restrictive to me. I would also say that reason should be required.

[MartinWitt]

How about just having a String reason in NoClasspathWorkaround? I don't like the idea of explicitly tying the annotation itself to GitHub Issues, it seems a bit too restrictive to me. I would also say that reason should be required.

I have added a field reason toNoClasspathWorkaroundannotation. I believe a link to the original issue is desirable, or shall we link the pull request?

[slarse]

Sorry, missed this completely. I've been preoccupied and sick over the past month so my open sourcing has suffered.

I have added a field reason toNoClasspathWorkaroundannotation. I believe a link to the original issue is desirable, or shall we link the pull request?

We typically refer to the issue, but honestly I think either is fine as they are linked anyway.

[slarse]

@MartinWitt I think it looks good, just thinking that we might want to put the annotation in the internal package?

[slarse]

I hate to come with yet another thing after not saying anything for so long, but should we not perhaps put this in the internal package? It seems like something we don't want to share with the world, but only use internally in Spoon.

[slarse]

Actually, spoon.testing.utils is probably the place we want to put it, considering the naming of that package.

[MartinWitt]

Using a testing package seems wrong for this. As this is not really related to testing or? An internal package, yes indeed.

[slarse]

Using a testing package seems wrong for this. As this is not really related to testing or?

I have no idea of what last week me was thinking. It was close to midnight so I suspect: not much.

An internal package, yes indeed.

Yes. That.

[MartinWitt]

How about we do this in spoon.experimental and see if we have enough classes to create an internal package in the future?

[slarse]

@MartinWitt Ugh, missed this comment.

But yes, sure. Essentially, if you put it anywhere where we can freely break it I'm satisfied.

[MartinWitt]

Ugh, missed this comment.

No worries, I also had a lengthy summer vacation and enjoyed the nice weather.
I moved the class to the experimental package and added Experimental and Internal. More evidence that we can and will break this annotation's package is not possible.

[slarse]

LGTM, thanks @MartinWitt. Let's try to start adopting the NoClasspathWorkaround annotation as we go along and see how it fares.

@monperrus note this new annotation that should be used when we do something strange in noclasspath mode. It's to avoid littering otherwise sane code with insane fixes.