Avoid dodgy cast in InferenceContext18.solve()

[mbazley2]

What it does

InferenceContext18.solve() was assuming that it is always safe to cast from InvocationSite to ASTNode. These two classes are not actually related, but the only thing the resulting variable was being used for was to call sourceStart() and sourceEnd(), two methods that are defined on both classes.

A cleaner solution is to use the existing InvocationSite.EmptyWithAstNode wrapper class, which includes a passthrough implementation of the two relevant methods.

How to test

This should be functionally equivalent to the previous code, but the old cast was very inconvenient for my project that uses the ECJ as a base for implementing operator overloading in Java.

Author checklist

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

[iloveeclipse]

@mbazley2 :

1. You need to sign ECA. See https://api.eclipse.org/git/eca/status/gh/eclipse-jdt/eclipse.jdt.core/4522
2. Could you please provide a reproducer for the problem & a regresion test?

[mbazley2]

1. I have signed the ECA, but it looks like Git is using an old email address for some reason. Is there any way to get it to recognise my current one? The "Common troubleshooting tips" section hints that there is, but doesn't actually say how.
2. The reproducer for the problem is to create a fake InvocationSite for e.g. transparently converting the code "a + b" into "a.add(b)", which is something I assume you don't want in the main repository. I am not aware of any way to cause the ClassCastException I suffered using only vanilla Java code.

[iloveeclipse]

1. Is there any way to get it to recognise my current one

Yes, change the author email in your git commit, amend and force push it again.

[iloveeclipse]

You can also set it permanently in .git/config file for your forked JDT repo:

[user]
	email = your@mail

[stephan-herrmann]

Thanks for the PR

InferenceContext18.solve() was assuming that it is always safe to cast from InvocationSite to ASTNode. These two classes are not actually related,

True, but since all these types are internal, we can easily see that "almost all" classes implementing InvocationSite actually extend (directly or indirectly) ASTNode. This leaves us with only 3 classes (EmptyWithAstNode + 2 anonymous classes), for which it should be possible to argue that their instances will never find their way into InferenceContext18.solve().

but the only thing the resulting variable was being used for was to call sourceStart() and sourceEnd(), two methods that are defined on both classes.

A cleaner solution is to use the existing InvocationSite.EmptyWithAstNode wrapper class, which includes a passthrough implementation of the two relevant methods.

Using such a wrapper in itself feels a bit like a workaround, too. Maybe it's time to invent an interface Location and wire it into the existing hierarchy, so that InferenceContext18.solve() can simple be written as solve(boolean,Location), leaving its callers unchanged. WDYT?

[stephan-herrmann]

I just came across another candidate that might benefit from the proposed interface Location: ProblemReporter.isClassPathCorrect(char[][], CompilationUnitDeclaration, Object, boolean, ReferenceBinding) 😄

[mbazley2]

Maybe it's time to invent an interface Location and wire it into the existing hierarchy, so that InferenceContext18.solve() can simple be written as solve(boolean,Location), leaving its callers unchanged. WDYT?

The same thought occurred to me, but I was trying to submit the minimum possible diff in the hopes that it would be more likely to get accepted. I could try implementing such a thing instead if you want.

[stephan-herrmann]

Maybe it's time to invent an interface Location and wire it into the existing hierarchy, so that InferenceContext18.solve() can simple be written as solve(boolean,Location), leaving its callers unchanged. WDYT?

The same thought occurred to me, but I was trying to submit the minimum possible diff in the hopes that it would be more likely to get accepted. I could try implementing such a thing instead if you want.

Small is beautiful. Small without a wrapper is even more beautiful. 😄 I don't think introducing a slim interface Location will bloat the change (the biggest part might be the copyright header which you may copy-adjust from any existing file). So, if you want to give it a try this will be appreciated.

As a proof of its usefulness you might even want to use it also in ProblemReporter.isClassPathCorrect() (see above).

[mbazley2]

Looks like the pipeline fell victim to another occurrence of eclipse-platform/eclipse.platform.releng.aggregator#2660. Rebased again to see if that fixes it.

[stephan-herrmann]

essentially looks good.

After we discussed that small is beautiful I believe you could still shrink the change a bit, see detailed comments.

[stephan-herrmann]

I don't see much benefit in declaring nameSourceStart()/End() here. Those don't seem to be useful for ASTNode and many of its subclasses. Nor do I see this called via the Location interface. Am I missing anything?

[stephan-herrmann]

this change looks unmotivated to me. Wouldn't just changing the signature of isClassPathCorrect() simplify things already?
I don't think that method has use for the LookupEnvironment itself.