get/set not working properly on values.

[wnederhof]

Hello,

I believe I found a problem with the org.eclipse.imp.pdb.facts.impl classes.

The BottomUpTransformer allows transformations on Rascal values. When no transformations are set to take place (i.e. when the BottomUpTransformer is not extended and the visitor-parameter in its constructor is set to the IdentityVisitor), it should therefore produce the exact same result as was provided in the input. However, using the following syntax definition:

lexical Integer = [0-9] !<< [0-9]+ !>> [0-9];
syntax Exp = "plus" "(" Exp e1 "," Exp e2 ")"
           | Integer;

and using the BottomUpTransformer composed with the IdentityVisitor and the concrete expression (Exp)'plus(2, 2)', I get the following result:

appl(prod(sort("Exp"),[lit("plus"),layouts("Whitespace"),lit("("),layouts("Whitespace"),label("e1",sort("Exp")),layouts("Whitespace"),lit(","),layouts("Whitespace"),label("e2",sort("Exp")),layouts("Whitespace"),lit(")")],{}),[[appl(prod(lit("plus"),[\char-class([range(112,112)]),\char-class([range(108,108)]),\char-class([range(117,117)]),\char-class([range(115,115)])],{}),[char(112),char(108),char(117),char(115)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(29,0)]])[@loc=|stdin:///|(29,0)],appl(prod(lit("("),[\char-class([range(40,40)])],{}),[char(40)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(30,0)]])[@loc=|stdin:///|(30,0)],appl(prod(sort("Exp"),[lex("Integer")],{}),[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[appl(regular(iter(\char-class([range(48,57)]))),[char(50)])[@loc=|stdin:///|(30,1)]])[@loc=|stdin:///|(30,1)]])[@loc=|stdin:///|(30,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(31,0)]])[@loc=|stdin:///|(31,0)],appl(prod(lit(","),[\char-class([range(44,44)])],{}),[char(44)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[char(32)])[@loc=|stdin:///|(32,1)]])[@loc=|stdin:///|(32,1)],appl(prod(sort("Exp"),[lex("Integer")],{}),[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[appl(regular(iter(\char-class([range(48,57)]))),[char(50)])[@loc=|stdin:///|(33,1)]])[@loc=|stdin:///|(33,1)]])[@loc=|stdin:///|(33,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(34,0)]])[@loc=|stdin:///|(34,0)],appl(prod(lit(")"),[\char-class([range(41,41)])],{}),[char(41)])]])[@loc=|stdin:///|(25,10)]

Whereas the original value was:

appl(prod(sort("Exp"),[lit("plus"),layouts("Whitespace"),lit("("),layouts("Whitespace"),label("e1",sort("Exp")),layouts("Whitespace"),lit(","),layouts("Whitespace"),label("e2",sort("Exp")),layouts("Whitespace"),lit(")")],{}),[appl(prod(lit("plus"),[\char-class([range(112,112)]),\char-class([range(108,108)]),\char-class([range(117,117)]),\char-class([range(115,115)])],{}),[char(112),char(108),char(117),char(115)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(29,0)]])[@loc=|stdin:///|(29,0)],appl(prod(lit("("),[\char-class([range(40,40)])],{}),[char(40)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(30,0)]])[@loc=|stdin:///|(30,0)],appl(prod(sort("Exp"),[lex("Integer")],{}),[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[appl(regular(iter(\char-class([range(48,57)]))),[char(50)])[@loc=|stdin:///|(30,1)]])[@loc=|stdin:///|(30,1)]])[@loc=|stdin:///|(30,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(31,0)]])[@loc=|stdin:///|(31,0)],appl(prod(lit(","),[\char-class([range(44,44)])],{}),[char(44)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[char(32)])[@loc=|stdin:///|(32,1)]])[@loc=|stdin:///|(32,1)],appl(prod(sort("Exp"),[lex("Integer")],{}),[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[appl(regular(iter(\char-class([range(48,57)]))),[char(50)])[@loc=|stdin:///|(33,1)]])[@loc=|stdin:///|(33,1)]])[@loc=|stdin:///|(33,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(34,0)]])[@loc=|stdin:///|(34,0)],appl(prod(lit(")"),[\char-class([range(41,41)])],{}),[char(41)])])[@loc=|stdin:///|(25,10)]

Looks quite similar, right? However, the identity transformation's result in Rascal is not printable. After comparing the two results, I found that the transformed result is 2 characters longer, and that the differences start at column 226. Prior to the transformation column 226 starts with:

[appl(prod(lit("plus"),...

While after the transformation column 226 starts with:

[[appl(prod(lit("plus"),...

Is this a bug in one of the org.eclipse.imp.pdb.facts.impl classes, or should I somehow flatten the concrete results after using the get/set-functions?

[jurgenvinju]

Looks like a bug in BottomUpTransformer to me!

[wnederhof]

I am very sorry, I failed to mention that I changed the visitConstructor method to:

for (int i = 0; i < o.arity(); i++) {
    o = o.set(i, o.get(i));
}
return o;

In order to test it out myself.

In the original version, the problem occured more than once in the tree. The result is 2562 characters long without the changes, while the pre-transformation value is 2520 characters long. Here are the trees without these changes:

Pre-transformation:

appl(prod(sort("Exp"),[lit("plus"),layouts("Whitespace"),lit("("),layouts("Whitespace"),label("e1",sort("Exp")),layouts("Whitespace"),lit(","),layouts("Whitespace"),label("e2",sort("Exp")),layouts("Whitespace"),lit(")")],{}),[appl(prod(lit("plus"),[\char-class([range(112,112)]),\char-class([range(108,108)]),\char-class([range(117,117)]),\char-class([range(115,115)])],{}),[char(112),char(108),char(117),char(115)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(29,0)]])[@loc=|stdin:///|(29,0)],appl(prod(lit("("),[\char-class([range(40,40)])],{}),[char(40)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(30,0)]])[@loc=|stdin:///|(30,0)],appl(prod(sort("Exp"),[lex("Integer")],{}),[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[appl(regular(iter(\char-class([range(48,57)]))),[char(50)])[@loc=|stdin:///|(30,1)]])[@loc=|stdin:///|(30,1)]])[@loc=|stdin:///|(30,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(31,0)]])[@loc=|stdin:///|(31,0)],appl(prod(lit(","),[\char-class([range(44,44)])],{}),[char(44)]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[char(32)])[@loc=|stdin:///|(32,1)]])[@loc=|stdin:///|(32,1)],appl(prod(sort("Exp"),[lex("Integer")],{}),[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[appl(regular(iter(\char-class([range(48,57)]))),[char(50)])[@loc=|stdin:///|(33,1)]])[@loc=|stdin:///|(33,1)]])[@loc=|stdin:///|(33,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[])[@loc=|stdin:///|(34,0)]])[@loc=|stdin:///|(34,0)],appl(prod(lit(")"),[\char-class([range(41,41)])],{}),[char(41)])])[@loc=|stdin:///|(25,10)]

Post-transformation:

appl(prod(sort("Exp"),[lit("plus"),layouts("Whitespace"),lit("("),layouts("Whitespace"),label("e1",sort("Exp")),layouts("Whitespace"),lit(","),layouts("Whitespace"),label("e2",sort("Exp")),layouts("Whitespace"),lit(")")],{}),[[appl(prod(lit("plus"),[\char-class([range(112,112)]),\char-class([range(108,108)]),\char-class([range(117,117)]),\char-class([range(115,115)])],{}),[[char(112),char(108),char(117),char(115)]]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[[]])[@loc=|stdin:///|(29,0)]]])[@loc=|stdin:///|(29,0)],appl(prod(lit("("),[\char-class([range(40,40)])],{}),[[char(40)]]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[[]])[@loc=|stdin:///|(30,0)]]])[@loc=|stdin:///|(30,0)],appl(prod(sort("Exp"),[lex("Integer")],{}),[[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[[appl(regular(iter(\char-class([range(48,57)]))),[[char(50)]])[@loc=|stdin:///|(30,1)]]])[@loc=|stdin:///|(30,1)]]])[@loc=|stdin:///|(30,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[[]])[@loc=|stdin:///|(31,0)]]])[@loc=|stdin:///|(31,0)],appl(prod(lit(","),[\char-class([range(44,44)])],{}),[[char(44)]]),appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[[char(32)]])[@loc=|stdin:///|(32,1)]]])[@loc=|stdin:///|(32,1)],appl(prod(sort("Exp"),[lex("Integer")],{}),[[appl(prod(lex("Integer"),[conditional(iter(\char-class([range(48,57)])),{\not-follow(\char-class([range(48,57)])),\not-precede(\char-class([range(48,57)]))})],{}),[[appl(regular(iter(\char-class([range(48,57)]))),[[char(50)]])[@loc=|stdin:///|(33,1)]]])[@loc=|stdin:///|(33,1)]]])[@loc=|stdin:///|(33,1)],appl(prod(layouts("Whitespace"),[\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))],{}),[[appl(regular(\iter-star(\char-class([range(9,10),range(13,13),range(32,32)]))),[[]])[@loc=|stdin:///|(34,0)]]])[@loc=|stdin:///|(34,0)],appl(prod(lit(")"),[\char-class([range(41,41)])],{}),[[char(41)]])]])[@loc=|stdin:///|(25,10)]

However, given the fact that there is only one very simple operation going on in my test version o = o.set(i, o.get(i)) that has any effect on o, I didn't think the problem was in the BottomUpTransformer.

After some more research this evening, I believe I found the problem. At line 1597 in RascalValueFactory$AnnotatedAbstractAppl, getInstance().appl is called with newChild as its second argument. As newChild is of type IList (which extends from IValue), the wrong appl function is called, i.e.:

ITree appl(IConstructor prod, IValue... args);

which creates a new list around args, instead of

ITree appl(IConstructor prod, IList args);

Therefore by casting the argument newChild to IList like in the labeled version (as it should always be an IList according to its definition), no new list is wrapped around args. Thus by replacing this line by:

case 1: return getInstance().appl(production, (IList) newChild);

Fixes the problem.

By accident, I believe I also found another bug: in visitTuple in the transformer class I found:

    @Override
    public IValue visitTuple(ITuple o) throws E {
        for (int i = 0; i < o.arity(); i++) {
            o.set(i, o.get(i).accept(this));
        }

        return fVisitor.visitTuple(o);
    }

Notice the o.set(...). Since IValues (from which ITuple extends) are immutable in Rascal and o.set returns an ITuple, shouldn't this be:

o = o.set(i, o.get(i).accept(this));

I really like the VisitorAdapter implementations, but can they be used for production? The reason why I am asking is that BottomUpTransformer (did not check the other visitors) is never used in Rascal and that the TraversalEvaluator works very differently. However, I personally find the Visitor/Transformer approach to be more elegant - and at first sight, I believe it should not be too hard to build a visit-expression using the VisitorAdapter (please correct me if I am wrong). What is the reason that the TraversalEvaluator is not modelled around the VisitorAdapter?

[jurgenvinju]

This bug and the wonderful analysis by @wnederhof has been waiting to long. Scheduling it for the release in March