WiP: fix replacing of field access

[pvojtechovsky]

No description provided.

[pvojtechovsky]

This PR changes behavior of the Templates. So it is not backward compatible.
See the first commit to reproduce the problems: Substitution of field access and local variable access behaves different:
A) substitution of template parameter of type String replaces whole field access with String literal
B) substitution of template parameter of type String replaces local variable/method/... name only.

I suggest to make it consistent and to always replace name of the named element.
If one needs to generate String literal by template s/he can simply use string literal in template:

@Parameter
String $string$ = "x"
...
{
   System.out.println("$string$"); //which substitutes to System.out.println("x");
   //this was legacy implementation which produced same result like line above
// System.out.println($string$); //but now it substitutes to System.out.println(x);
}

Another way how to substitute string literal which still works is:

@Parameter
CtLiteral<String> $string$;  //<!--- note CtLiteral as parameter type
...
{
   System.out.println($string$);
}

WDYT?

[surli]

I'm a bit confused about the quote usage in your example:

@Parameter
String $string$ = "x";

@Parameter
CtLiteral<String>  $otherString$; // create proper ctLiteral with x value

...
{
   System.out.println("$string$"); // now produce: System.out.println(x)
   System.out.println($string$); // now produce? System.out.println(x)? error? 
   System.out.println($otherString$); // now produce: System.out.println("x")
   System.out.println("$otherString$"); // now produce? System.out.println(""x"") ? 
}

I agree the behaviour should be consistent, and then do your example works the same way with other primitive types?

@Parameter
int $int$ = 42;

@Parameter
CtLiteral<Integer>  $otherInt$; // create proper ctLiteral with 42 value

...
{
   System.out.println("$int$"); // now produce: System.out.println(42) ? 
   System.out.println($otherInt$); // now produce: System.out.println(42) too? 
}

[pvojtechovsky]

After this PR is merged it will behave like this

@Parameter
String $string$ = "x";

@Parameter
CtLiteral<String>  $otherString$; // create proper ctLiteral with x value

...
{
   System.out.println("$string$"); // now produce: System.out.println("x")
   System.out.println($string$); // now produce: System.out.println(x)
   System.out.println($otherString$); // now produce: System.out.println("x")
   System.out.println("$otherString$"); // now produce: System.out.println("x")
}

and with int I have not tested it ... but it might behave like this:

@Parameter
int $int$ = 42;

@Parameter
CtLiteral<Integer>  $otherInt$; // create proper ctLiteral with 42 value

...
{
   System.out.println("$int$"); // now produce: System.out.println("42") 
   System.out.println($int$); // now produce: System.out.println(42)
   System.out.println($otherInt$); // now produce: System.out.println(42)
}

[surli]

Ok thanks for clarifying.
I'm still wondering about this PR: I understand that it can be useful to be able to have this behaviour System.out.println($string$); // now produce: System.out.println(x) but I find it really counter intuitive. In my point of view it does not make sense that after the substitution the method will call a completely different field, because the name was contained inside a string. I would expect the function to call the content of the field, as the old behavior did.

WDYT @tdurieux @monperrus?

[pvojtechovsky]

I came with this PR mainly because of this use case - I need to generate a class with many fields and appropriate accessor methods. But actually it is not easily possible, because all field references are replaced by string literal :( ... OK, there is workaround - to use TemplateParameter ... but it is not so nice like the solution which is possible after this PR.

See this example template:

@Parameter("$name$")
String name = "x";

int $name$;
int m_$name$;
{
    System.out.println($name$+m_$name$)
}

This template is substituted like this by legacy spoon code

int x;
int m_x;
{
    System.out.println("x"+m_x)
}

and after this PR is applied it produces this:

int x;
int m_x;
{
    System.out.println(x+m_x)
}

which is correct ;-)

[monperrus]

Very interesting discussion, thanks Pavel for shaking the template engine. Let's discuss first #1457 and then com back to this one.

[monperrus]

Why do you change example classes in src/test/java/spoon/test/template/testclasses?

if we change the existing behavior, this should only be seen in the expected values of assertions.

[pvojtechovsky]

if we change the existing behavior, this should only be seen in the expected values of assertions.

Interesting idea. I will extend the test instead of changing it. I have modified it to show how to migrate old templates to get the same behavior.

I just do not know, when I will have time for that.

[monperrus]

Looking again at the example in your last comment, I agree with the principle of your proposed behavioral change.

[surli]

@pvojtechovsky so, following the discussion I added a new test corresponding to the example below and I changed SubsitutionVisitor to check between case A and case B to treat them differently. I don't know much about the helpers provided in template mechanism so maybe my solution is a bit complicated.

[pvojtechovsky]

I would preffer to have this check/decision sooner - somewhere in the Parameters class, where Map of substitution parameters is collected. Here we migh create value of type string literal and then code here would know what to do.

Why?

• it will perform faster
• it will not bad influence new substitution API feature: add fluent API for templates #1458 where client can decide whether s/he wants to modify field name or substitute by string literal early - during definition of map of parameters

[pvojtechovsky]

I will try to move that code to Paramters#getNamesToValues today

[pvojtechovsky]

I have copied FieldAccessTemplate into FieldAccessOfInnerClassTemplate with similar field name substitution, but this time in inner class. It shows that

• Template parameter restrictions introduced by fix: clarifies the contract of proxy template parameters #1457 are too strict
• Simon's try to search for Field in declaring type does not work here

My try to move Change SubstitutionVisitor to treat differently case A and case B code into Parameters#getNamesToValues failed on InvocationTemplate

public class InvocationTemplate extends ExtensionTemplate {
	IFace iface;
	
	void invoke() {
		iface.$method$();
	}
	@Parameter
	String $method$;
...
}

because I have converted "$method$" parameter value from String to StringLiteral, which is not correct in this case.

The solution for all these problems would be to modify the meaning of the proxy parameter like this: "Use the proxy parameter value if you want to replace simple name of the reference."

I will make new PR based on this PR which will show this solution soon.

[pvojtechovsky]

The tests actually fails here
TemplateTest.testFieldAccessNameSubstitutionInInnerClass:844 expected:<[value] = 7> but was:<["value"] = 7>
... as expected and explained in my previous comment. The possible solution is in #1476. I added it into another PR, because it rolls back some of Martin's and Simon's changes and modifies meaning of template Parameter#value.

[surli]

I close this one as you pick back the work in #1476