review: fix replacing of field access by relaxed Parameter value contract

[pvojtechovsky]

This is the alternative to the #1444 with

• relaxed Template parameter constraint
• early detection of string literal parameter value applied to each Parameter without value

[pvojtechovsky]

Travis CI is broken...

[monperrus]

Thanks Pavel.
I don't understand the goal of this PR. The title of the PR suggests it's a fix. What is the wrong behavior and why?

[pvojtechovsky]

what is wrong behavior?

This PR is alternative solution to #1444. The problems are described there.

Short description: The proxy template parameter is needed not only in case of name conflict with existing field name, but is needed

• for field name in nested class (see new test case in#1444)
• for case when client wants to rename method name of method invocation - there is already older test for that.

Therefore I suggest to release the template parameter constraint you have introduced. It is the change introduced by this PR.
Optionally you can try to solve that problem represented by actually failing test in #1444 by another way - continue with #1444

[monperrus]

I see, I have to deeply understand the problem of #1444 then. However, I'm traveling, it may require some time I don't have yet.

[pvojtechovsky]

Yes, the deeper understanding is needed.

These are the concepts which have to be understood:

C1) Template and SubstitutionVisitor are no more deeply bound together. The only binding is in constructor of SubstitutionVisitor, where Template is converted to substitution parameters represented by Map<String, Object>.

C2) If the substitution parameter value is CtLiteral of type String then parameter CtFieldReference is replaced by that CtLiteral. So the substitution produces a string literal.

C3) If the substitution parameter value is String then simple name of the parameter CtFieldReference is substituted. So the substitution produces a field reference to field whose name is defined by template parameter.

These 3 concepts were origin target of the PR #1444. But then Simon came with interesting idea (commit Change SubstitutionVisitor to treat differently case A and case B) (if I understood him well) to make my solution more backward compatible.
The Simon's solution has these problems:

• it does not follows clear rules C2 and C3, but tries to detect intention of the Template designer by detecting how the field reference is used in generated code.
• it does not work for field references in inner classes. See commit named "test: field access in inner class"

Therefore I suggested to detect intention of the Template designer sooner - during extraction of substitution parameters from the Template (C1). See commit convert String to Literal parameter value automatically. This solution is compatible with the concepts C2 and C3.

It introduces concept

C4) If the template parameter has a proxy name, then it is not substituted as String literal, but it is substituted in a simple name of the field/executable reference.

Concept C4 can be used for field parameter in Template, but it cannot be used for field parameter in inner class (See TemplateTest#testFieldAccessNameSubstitutionInInnerClass) and it cannot be used for replacing of method invocation names (See TemplateTest#testTemplateInvocationSubstitution)
because it is not compatible with Template parameter rule in Substition#checkTemplateContracts(). Therefore I disabled this rule in commit "disable one Substitution#checkTemplateContracts contract"

Final I adapted the tests InvocationTemplate and FieldAccessOfInnerClassTemplate to use C4.

I hope this PR is understandable now and we can continue here.

[pvojtechovsky]

I need C1, C2, C3. I base my templates on these rules.

I am not sure concerning backward compatibility introduced by Simon and I am not sure whether C4 is the understandable solution. Do you see a better one?

[pvojtechovsky]

I like it as it is now. It is not compatible with some legacy templates, but it is more clear what template does.
@surli Is it the solution you mean in your last comment?

[surli]

This one bothers me: one of the main interest of Spoon template, is that they are java typed. Here using return PARAM; is right concerning the typing, but throws an exception as there's no field with the new name in the test.
With the proposed solution, (i.e. string parameters are used for renaming fields/methods), I don't think we can keep the typing safety for String (like here).
However we should provide an immediate feedback, when applying the template: I'd like to do it when a String parameter is used in this kind of case but it seems really complicated, maybe we can do it only when an error occured, instead of throwing an NPE, WDYT?

[pvojtechovsky]

maybe we can do it only when an error occurred, instead of throwing an NPE?

Good idea! I have added error handler which throws SpoonException instead of NPE + test of this situation.

[surli]

Good idea! I have added error handler which throws SpoonException instead of NPE + test of this situation.

Nice job! It's far more clear like that for users I think.

So, if I sum up String parameters are now used exclusively for replacing variable or method names, and we got a clear error if you use them to return a string for example. And if you want to use a parameter to return a string, you have to use a TemplateParameter or a CtLiteral, right?

Then we are potentially breaking template clients here, but regarding all the bugs templates had we can assume that really few people used templates.

I propose to conclude this PR that you update the javadoc of Template because it shows an example with String parameter which is not good anymore. And as we potentatially break client I really wonder if we put this PR in the next minor release or if we do a minor release with the most recent bug fixes and we keep this PR and some others (like the recent one with imports) for a major release in september. WDYT @pvojtechovsky @monperrus @tdurieux?

[monperrus]

ok for me

[pvojtechovsky]

update documentation

I believe that current implementation is better then before, but thinking about documenation, it looks it is not good intuitive/easily understandable. There is missing a concept, which would explicitly state the meaning of template String parameter. Whether
A) it is understood as string literal
B) it is the (part of the) simple name of an model element

The inconsistency / unfriendliness is that parameter of type int is automatically handled as int literal, while string is/was ambiguous - how to explain that in documentation?

May be the solution would be, to introduce another annotation, which would explicitly express that parameter value is not a literal, but will be used as (part of the) simple name.

Something like:

@NameParameter(_name_)
String name;

optional solution is to mark old templates as deprecated and to introduce template builder, which does not have this problem.

[surli]

while string is/was ambiguous - how to explain that in documentation?

I agree it was ambiguous but for me, String usage is now pretty clear: you use String type for a parameter only if you want to rename some element of the code. Else you use a CtLiteral. And we should document it like that.

May be the solution would be, to introduce another annotation, which would explicitly express that parameter value is not a literal, but will be used as (part of the) simple name.
Something like:

@NameParameter(_name_)
String name;

It looks like a good idea indeed to use another annotation for stating that a parameter will be used only to rename code elements. I'm not sure about the usage of the argument inside the annotation: it should follow the same rule as standard parameter then.
But then it means that we go back to the previous usage for standard parameters with String (same behaviour as CtLiteral), right?
Honestly I'm not completely sure about this solution, especially since I was pretty satisfied about the solution we have here. WDYT @monperrus ?

[monperrus]

I would prefer to keep only one annotation for templates.

So I would stick and merge this solution for now

[pvojtechovsky]

So I would stick and merge this solution for now

I agree. It is a good step in good direction.

The template documentation should be updated too. But current examples with template parameter of type "int", which is automatically converted to CtLiteral is confusing if it will be extended with template parameter of type "String", which is NOT automatically converted to CtLiteral ... because of reasons we 3 understand, but which is hard to explain for me in documentation ...

[surli]

because of reasons we 3 understand, but which is hard to explain for me in documentation

If you already started to change the documentation, push your changes, we'll be able to help you.

In my point of view, we can keep things simple to explain the stuff around String parameters:

String parameters are not working like other primitive type parameters, since we're using String parameters only to rename elements of the code like fields and methods. To use a parameter with a type String like other primitive types, use CtLiteral<String>.

And give examples: anyway dev will only read the examples ;-)

[pvojtechovsky]

If you already started to change the documentation

no, I did not.

[pvojtechovsky]

Please check the updated documentation and if it is OK, then this PR seems to be ready for merge.

[surli]

@pvojtechovsky I fixed the failing test case and I just updated the javadoc of Template.java could you validate this last change then I'll merge the PR. Finally :)

[pvojtechovsky]

here is a typo mistake in method name.

[pvojtechovsky]

It looks OK for me. Thank you for all your input and fixing documentation ;-)