review fix: Invalid use of type access generics

[pvojtechovsky]

Here is the failing test, which shows that Spoon prints actual type arguments in class access: List<Object>.class

Is it problem of pretty printer or SubstitutionVisitor?

[surli]

Is it problem of pretty printer or SubstitutionVisitor?

For me it looks like a SubstitutionVisitor problem: apparently it tries to substitute someMethod with o because o is contained in the map of parameters and "someMethod" contains "o"...
It happens in substituteName, the exception is thrown when calling getParameterValueAsString: I'm really not sure why o belongs to the map of parameters, is it intended?

[pvojtechovsky]

The o is in the map because there is a bug in Parameters#isParameterSource

if (getTemplateParameterType(ref.getFactory()).isSubtypeOf(ref.getType())) {

which returns true on Object ... I am going to fix it too, but it was tooooo late yesterday.

[pvojtechovsky]

For me it looks like a SubstitutionVisitor problem

I though that too (see my branch with fix of SubstitutionVisitor), but later I found that other type access is handled correctly by pretty printer. So may be this one should be also fixed on pretty printer.

[pvojtechovsky]

apparently it tries to substitute someMethod with o because o is contained in the map of parameters and "someMethod" contains "o"...

I added PR #1461, which fixes this problem.

[pvojtechovsky]

After rebase it fails on new template Parameter contract "when Parameter is proxy then it's type must be a String". But how to define this template, without removing the constraint?

public class TypeReferenceClassAccessTemplate extends ExtensionTemplate {
	Object o;

	$Type$ someMethod($Type$ param) {
		o = $Type$.out;
		o = $Type$.currentTimeMillis();
		o = $Type$.class;
		o = o instanceof $Type$;
		Supplier<Long> p = $Type$::currentTimeMillis;
		return new $Type$();
	}
	
	@Local
	public TypeReferenceClassAccessTemplate(CtTypeReference<?> typeRef) {
		this.typeRef = typeRef;
	}
	
	@Parameter("$Type$")
	CtTypeReference<?> typeRef;

	@Local
	static class $Type$ {
		static final String out = "";
		static long currentTimeMillis(){
			return 0;
		}
	}

	public static class Example<T> {
		static final String out = "";
		static long currentTimeMillis(){
			return 0;
		}
	}
}

typeRef has to Substitute for $Type$. If I rename the field typeRef to $Type$ then there is ambiguity and compiler uses field and not Type in body of someMethod - and it is really not wanted.

So it seems to me like these new constrains introduced by @monperrus in Substitution.checkTemplateContracts are too strict.

[surli]

@pvojtechovsky do you agree that this one could be closed now?

[pvojtechovsky]

I think it cannot be closed. The origin problem is different and still exists. There was conflict with template implementation. I will check how it behaves now when templates are fixed. I will let you know

[pvojtechovsky]

My comment above from 1 Aug is still relevant: the template Parameter contract "when Parameter is proxy then it's type must be a String" is too strict and avoids to implement such templates.
I suggest to release that constraint somehow ... for example: "when Parameter is proxy then it's type must be a String or CtTypeReference"
@monperrus @surli WDYT?

[surli]

"when Parameter is proxy then it's type must be a String or CtTypeReference"

So if I'm trying to sum up, here we are in a special case because what you want to use inside a parameter is not a reference to a field of the template, neither a name to rename field/methods, but a type to be able to generate automatically some codes using different types.

So, we have three cases:

• either we use a String parameter, then the value will be used only to rename field/method: the proxy can be used to avoid conflict with existing field name
• either we use a CtTypeReference because we want to template a type
• either we use any other type for the template parameter which will be used directly inside the code

So, why do we need a proxy on the CtTypeReference: can't we make an exception if the type of the template parameter is a CtTypeReference to use the parameter directly to template a Type.
Can you imagine an example where we would want to use a CtTypeReference for anything else?

I tend to agree with you on relaxing the constraint, I just need to see an example with the usage of CtTypeReference with and without a proxy to really see how it can be used.

[pvojtechovsky]

I just need to see an example

See /src/test/java/spoon/test/template/testclasses/NtonCodeTemplate.java
There is

	@Parameter
    CtTypeReference<?> _TargetType_;

[pvojtechovsky]

So, why do we need a proxy on the CtTypeReference: can't we make an exception if the type of the template parameter is a CtTypeReference to use the parameter directly to template a Type.

In this case it is not possible because

@Parameter
CtTypeReference<?> $Type$;

will be in conflict with

@Local
static class $Type$ {...}

So I see no good way how to implement such template without relaxing that template parameter constraint.

[surli]

Ok I'm still trying to explain the problem, in order to be sure I fully understand its limit and to explain in documentation why there are exceptions in using template parameters:

We cannot template a class name with a CtTypeReference and use it in the same time because the template wouldn't be compilable: it breaks the first rule of Spoon template which is "a template should be correctly typed and compilable". Then we have to use a proxy parameter on CtTypeReference to solve this problem.

So the constraint is:

when Parameter is proxy then it's type must be a String or CtTypeReference, and it should exists an element inside the template with the proxyname (i.e. it's useless to have a proxyparameter in your example if the classe $Type$ don't exist)

[pvojtechovsky]

We cannot template a class name with a CtTypeReference and use it in the same time because the template wouldn't be compilable: it breaks the first rule of Spoon template which is "a template should be correctly typed and compilable". Then we have to use a proxy parameter on CtTypeReference to solve this problem.

yes it is correct.

So the constraint is:

when Parameter is proxy then it's type must be a String or CtTypeReference, and it should exists an element inside the template with the proxyname (i.e. it's useless to have a proxyparameter in your example if the classe $Type$ don't exist)

... and it should exists an element inside the template with the proxyname ...
why you are so strict? ;-) It was so nice before when I was free to use proxy name whenever I found it helpful to make it more readable! ... and not only when it was absolutely necessary to make it compilable .

[pvojtechovsky]

I have fixed the next problem found by this PR in #1535.

Now this PR finally fails on the main problem of this PR.

Spoon prints actual type arguments in class access:

List<Object>.class

And main question of this PR: Is it problem of pretty printer or SubstitutionVisitor?
I need to have agreed answer before I can implement correct solution:

Simon wrote:
For me it looks like a SubstitutionVisitor problem

Pavel wrote: I though that too (see my branch with fix of SubstitutionVisitor), but later I found that other type access is handled correctly by pretty printer. So may be this one should be also fixed on pretty printer.

WDYT?

[pvojtechovsky]

I have fixed that problem on DJPP side, because I believe it is it's responsibility to print correctly the sources independently on CtTypeReference has generics or not.

[monperrus]

it seems there are two independent fixes here: the one with forceWildcardGenerics and the ones with modifying the preconditions of ignoreGenerics. Correct?

Is the bug specific to using the template engine or can it happen in a regular situation?

[pvojtechovsky]

there are two independent fixes here

yes, both are related to use of generics

Is the bug specific to using the template engine or can it happen in a regular situation?

it is not related to template engine. Client can came into same problems by manipulating of AST too. Anyway these problems can be fixed by two ways
A) by more clever DJPP code = this PR
B) by letting client create AST which contains CtTypeReferences, with generics arguments which fits to printed java code.

[surli]

Thanks @pvojtechovsky !