Skip to content

WIP: Bugs and TODOs of Templates and TemplateMatcher #1342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

WIP: Bugs and TODOs of Templates and TemplateMatcher #1342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
678a417
bugs and todos of Substitution
pvojtechovsky Jun 12, 2017
dc23ffe
bugs and todos of Parameters
pvojtechovsky Jun 12, 2017
94f365b
bugs and todos of TemplateMatcher
pvojtechovsky Jun 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/main/java/spoon/support/template/Parameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ private static Object getValue(Template<?> template, String parameterName, Field
if (Modifier.isFinal(rtField.getModifiers())) {
Map<String, Object> m = finals.get(template);
if (m == null) {
//BUG: parameters marked as final will always return null, even if they have a value!
return null;
}
return m.get(parameterName);
Expand Down Expand Up @@ -268,6 +269,7 @@ public static boolean isParameterSource(CtFieldReference<?> ref) {
//the reference to this is not template parameter
return false;
}
//BUG: it returns true only for parameters of type TemplateParameter, because interface TemplateParameter can never be a subtype of something else
if (getTemplateParameterType(ref.getFactory()).isSubtypeOf(ref.getType())) {
//the type of template field is or extends from class TemplateParameter.
return true;
Expand Down
1 change: 1 addition & 0 deletions src/main/java/spoon/template/Substitution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ public static <T> CtConstructor<T> insertConstructor(CtClass<T> targetClass, Tem
if (newConstrutor.getParameters().isEmpty()) {
CtConstructor<?> c = targetClass.getConstructor();
if (c != null && c.isImplicit()) {
//BUG: it removes nothing, because temporary collection is returned
targetClass.getConstructors().remove(c);
}
}
Expand Down
89 changes: 86 additions & 3 deletions src/main/java/spoon/template/TemplateMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,21 @@ private List<String> getTemplateNameParameters(CtClass<? extends Template<?>> te
return Parameters.getNames(templateType);
}

/**
* Collects all AST nodes, which has to be substituted, because they represents a template parameter declared by field annotated by {@link Parameter}
*
* TODO test it: This code is probably wrong, or I did not understood it...
* @param templateType CtClass model of {@link Template}
* @return ??
*/
private List<CtTypeReference<?>> getTemplateTypeParameters(final CtClass<? extends Template<?>> templateType) {

final List<CtTypeReference<?>> ts = new ArrayList<>();
final Collection<String> c = Parameters.getNames(templateType);
new CtScanner() {
@Override
public void visitCtTypeParameterReference(CtTypeParameterReference reference) {
//BUG? Parameters#isParameterSource() avoids CtTypeParameterReference ... so is it correct?
if (c.contains(reference.getSimpleName())) {
ts.add(reference);
}
Expand All @@ -108,6 +116,7 @@ public <T> void visitCtTypeReference(CtTypeReference<T> reference) {

/**
* Looks for fields of type {@link CtStatementList} in the template and returns these fields,
* BUG: ? It does not care about annotation Parameter, so there is actually not possible to generate field of type CtStatementList as part of code generate by template
* @param root CtClass model of {@link Template}
* @param variables
* @return returns for fields of type {@link CtStatementList} in the template
Expand All @@ -117,6 +126,8 @@ private List<CtFieldReference<?>> getVarargs(CtClass<? extends Template<?>> root
for (CtFieldReference<?> field : root.getAllFields()) {
if (field.getType().getActualClass() == CtStatementList.class) {
boolean alreadyAdded = false;
//BUG: alreadyAdded can be never true, because `variables` are collected from fields of type `TemplateParameters`,
//so their type can never be CtStatementList ...
for (CtInvocation<?> invocation : variables) {
alreadyAdded |= ((CtFieldAccess<?>) invocation.getTarget()).getVariable().getDeclaration().equals(field);
}
Expand Down Expand Up @@ -202,6 +213,22 @@ public TemplateMatcher(CtElement templateRoot) {
private boolean addMatch(Object template, Object target) {
Object inv = matches.get(template);
Object o = matches.put(template, target);
/*
* BUG: it always returns true, because inv==o. It is contract of Map.
* The correct code is probably:
*
* Object inv = matches.get(template);
* if (inv != null && inv.equals(target) == false) {
* //another value would be inserted. TemplateMatcher does not support matching of different values for the same template parameter
* return false;
* }
* matches.put(template, target)
* return true;
* Object inv = matches.put(template, target);
* return (null == inv) || inv.equals(target);
*
* But callers of addMatch does not handle return value consistently to this contract ...
*/
return (null == inv) || inv.equals(o);
}

Expand All @@ -212,8 +239,10 @@ private boolean addMatch(Object template, Object target) {
*/
private CtElement checkListStatements(List<?> teList) {
for (Object tem : teList) {
if (variables.contains(tem) && (tem instanceof CtInvocation)) {
//TODO: simplify, if it is same like an item of variables, then it must be a CtInvocation
if (containsSame(variables, tem) && (tem instanceof CtInvocation)) {
CtInvocation<?> listCand = (CtInvocation<?>) tem;
//BUG: it returns true only for parameters of type TemplateParameter, because interface TemplateParameter can never be a subtype of something else
boolean ok = listCand.getFactory().Type().createReference(TemplateParameter.class).isSubtypeOf(listCand.getTarget().getType());
return ok ? listCand : null;
}
Expand Down Expand Up @@ -360,6 +389,8 @@ private boolean helperMatch(Object target, Object template) {
boolean add = invokeCallBack(target, template);
if (add) {
//ParameterMatcher matches the target too, add that match
//BUG: if addMatch returns false, then report it as
//Launcher.LOGGER.debug("incongruent match");
return addMatch(template, target);
}
return false;
Expand All @@ -380,9 +411,18 @@ private boolean helperMatch(Object target, Object template) {
* after replacing of variables in template name
*/
boolean ok = matchNames(tRef.getSimpleName(), ((CtReference) target).getSimpleName());
/*
* TODO comment: In what case the template.equals(target) == true??
*/
if (ok && !template.equals(target)) {
boolean remove = !invokeCallBack(target, template);
if (remove) {
/*
* BUG: if ParameterMatcher does not agrees then it should remove a match,
* but the match was inserted with different key by matchNames!
* The best solution would be to add the match only after it is agreed by ParameterMatcher.
* It avoids replacing of correct match by incorrect match in `matches`
*/
matches.remove(tRef.getSimpleName());
return false;
}
Expand All @@ -391,6 +431,10 @@ private boolean helperMatch(Object target, Object template) {
}

if (template instanceof CtNamedElement) {
/*
* same code like above, with same bugs
* TODO use a shared function called from both places and fix it once.
*/
CtNamedElement named = (CtNamedElement) template;
boolean ok = matchNames(named.getSimpleName(), ((CtNamedElement) target).getSimpleName());
if (ok && !template.equals(target)) {
Expand Down Expand Up @@ -432,6 +476,7 @@ private boolean helperMatch(Object target, Object template) {
}

if (target instanceof CtElement) {
//TODO cache relevant fields for a spoon model class in a static Map
for (Field f : RtHelper.getAllFields(target.getClass())) {
f.setAccessible(true);
if (Modifier.isStatic(f.getModifiers())) {
Expand Down Expand Up @@ -475,6 +520,8 @@ private boolean helperMatch(Object target, Object template) {
* @param target a potentially matching element
* @param template a matching parameter, which may define extra {@link ParameterMatcher}
* @return true if {@link ParameterMatcher} of `template` matches on `target`
*
* TODO: rename this method to #checkParameterMatcher
*/
private boolean invokeCallBack(Object target, Object template) {
try {
Expand Down Expand Up @@ -512,9 +559,11 @@ private boolean invokeCallBack(Object target, Object template) {

/**
* Detects whether `object` represent a template variable `inMulti`
* TODO: rename to isCurrentTemplateParameter ?
*/
private boolean isCurrentTemplate(Object object, CtElement inMulti) {
if (object instanceof CtInvocation<?>) {
//BUG: should use == instead of equals?
return object.equals(inMulti);
}
if (object instanceof CtParameter) {
Expand Down Expand Up @@ -562,7 +611,7 @@ private boolean matchCollections(Collection<?> target, Collection<?> template) {
if (teList.size() != taList.size()) {
return false;
}

//TODO simplify the cycle. Use one index for both lists
for (int te = 0, ta = 0; (te < teList.size()) && (ta < taList.size()); te++, ta++) {
if (!helperMatch(taList.get(ta), teList.get(te))) {
return false;
Expand All @@ -584,22 +633,35 @@ private boolean matchCollections(Collection<?> target, Collection<?> template) {
return false;
}
boolean ret = addMatch(inMulti, multi);
//BUG: if addMatch returns false, then report it as
//Launcher.LOGGER.debug("incongruent match");
return ret;
}
//there is next template parameter. Move to it
te++;
//adds all target list items, which are not matching to next template parameter, to the actual template parameter
/*
* IMPROVE:
* - do not check (te < teList.size()), because it is already tested above
* - get teList.get(te) into local variable
* then it will be clear that this cycle iterates over taList only
*/
while ((te < teList.size()) && (ta < taList.size()) && !helperMatch(taList.get(ta), teList.get(te))) {
multi.add(taList.get(ta));
ta++;
}
//BUG: te-- ?? Or do not increase te above at all?

//we have found first target parameter, which fits to next template parameter
//create statement list for previous parameter and add it's match
CtStatementList tpl = templateType.getFactory().Core().createStatementList();
tpl.setStatements((List<CtStatement>) (List<?>) multi);
if (!invokeCallBack(tpl, inMulti)) {
return false;
}
//BUG: why we do not care about return value here?
// if addMatch returns false, then report it as
//Launcher.LOGGER.debug("incongruent match");
addMatch(inMulti, tpl);
// update inMulti
inMulti = nextListStatement(teList, inMulti);
Expand All @@ -609,24 +671,35 @@ private boolean matchCollections(Collection<?> target, Collection<?> template) {
if (!helperMatch(taList.get(ta), teList.get(te))) {
return false;
}
//TODO: make condition more readable. E.g. ta+1>=taList.size()
if (!(ta + 1 < taList.size()) && (inMulti != null)) {
/*
* there is no next target item in taList,
* but there is still some template parameter,
* which expects one
*/
CtStatementList tpl = templateType.getFactory().Core().createStatementList();
//BUG: it looks like `multi` must be always empty
//TODO: delete this cycle
for (Object o : multi) {
tpl.addStatement((CtStatement) o);
}
//so it returns empty statement list
//so it returns empty statement list - might be OK
if (!invokeCallBack(tpl, inMulti)) {
return false;
}
//BUG: why we do not care about return value here?
// if addMatch returns false, then report it as
//Launcher.LOGGER.debug("incongruent match");
addMatch(inMulti, tpl);
// update inMulti
inMulti = nextListStatement(teList, inMulti);
multi = new ArrayList<>();
/*
* BUG: if there is next `inMulti` template parameter,
* then it is not checked whether it matches empty statement list,
* because ta+1==taList.size() and it finishes the main cycle.
*/
}
}
}
Expand All @@ -639,6 +712,8 @@ private boolean matchCollections(Collection<?> target, Collection<?> template) {
* @param templateName the name from template
* @param elementName the name from target
* @return true if matching
*
* TODO fix BUG: refactor this method and callers, to call addMatch correctly
*/
private boolean matchNames(String templateName, String elementName) {

Expand Down Expand Up @@ -667,20 +742,28 @@ private boolean matchNames(String templateName, String elementName) {

/**
* returns next ListStatement parameter from teList
*
* BUG: it works only for first and second parameter. The 3rd call will return first parameter again!
*
* @param teList
* @param inMulti TODO replace by int index
* @return TODO return int index of found statement or -1 if there is no next one
*/
private CtElement nextListStatement(List<?> teList, CtElement inMulti) {
if (inMulti == null) {
return checkListStatements(teList);
}
List<?> teList2 = new ArrayList<Object>(teList);
if (inMulti instanceof CtInvocation) {
//BUG: we should use removeSame, which uses "==" instead of "equals"
teList2.remove(inMulti);
} else if (inMulti instanceof CtVariable) {
CtVariable<?> var = (CtVariable<?>) inMulti;
for (Iterator<?> iter = teList2.iterator(); iter.hasNext();) {
CtVariable<?> teVar = (CtVariable<?>) iter.next();
if (teVar.getSimpleName().equals(var.getSimpleName())) {
iter.remove();
//BUG? Should it really remove all variables of the same name, or should it remove first found?
}
}
}
Expand Down