setters are generated for List and Opt NTA children

[Axel Mårtensson]

An evaluated NTA should be immutable, however setters are generated for List and Opt NTA children, i.e. given the grammar {A ::= /[B]/; B;}, a method A.setB(); is generated. Couldn't setting the child node of an NTA be seen as a mutation of the NTA?

[Jesper Öqvist]

I think JastAdd should not generate mutator methods for NTAs, i.e. it should not generate setX() or addX() or any other mutator methods for list/opt NTAs. NTAs are attribtutes, and attribute values should never be modified after evaluation.

As you said Axel, these methods are currently used in JModelica. So if we remove them it would be good to find an alternative pattern that could be used in JModelica. Also, we should probably remove them in a major release.

[Jesper Mattsson]

They are? That's not good. Do you know where they are used? I can take a look at it and see what we actually intended to do there. Maybe I can get rid of them relatively quickly. (I can't see an easy way to search for them.)

[Jesper Öqvist]

Axel mentioned that an Opt sublcass was used in NTAs (/ [..] / NTAs) in JModelica, and then a generated setter method was used to replace the contents of the Opt NTA. Maybe he can point to some instances of that in JModelica.

[Jesper Mattsson]

Ah, yes - I know what you mean. Yes, that needs a discussion on how to properly do what we wanted.

The base need is that you have a newly constructed node, where we need to call some method on it that might call an inherited attribute before we add it to its final place the tree. What is the recommended way to do that? Simply setting parent? Not doing it? (I don't think we can swing avoiding it.)

[Axel Mårtensson]

Ok, so I found a few places where the mutator methods are used. I created a new branch no-nta-setters in my fork of jastadd2 where I remove the mutator methods. When i use this version of jastadd2 to compile JModelica, I get compile-time errors where the removed mutator methods are used on NTAs:

    [javac] /home/axel/workspace-neon/JModelica/Compiler/ModelicaCompiler/src/java-generated/org/jmodelica/modelica/compiler/FRelExp.java:200: error: cannot find symbol
    [javac]         setIndicator(v.createUseExp());
    [javac]         ^
    [javac]   symbol:   method setIndicator(FIdUseExp)
    [javac]   location: class FRelExp
    [javac] /home/axel/workspace-neon/JModelica/Compiler/ModelicaCompiler/src/java-generated/org/jmodelica/modelica/compiler/Program.java:1346: error: cannot find symbol
    [javac]                         addAnonymousClass(ParserHandler.parseAnonymousClassString(code, restriction, targetName));
    [javac]                         ^
    [javac]   symbol:   method addAnonymousClass(BaseClassDecl)
    [javac]   location: class Program
    [javac] /home/axel/workspace-neon/JModelica/Compiler/ModelicaCompiler/src/java-generated/org/jmodelica/modelica/compiler/InstProgramRoot.java:1670: error: cannot find symbol
    [javac]                     addInstLibClassDecl(icd);
    [javac]                     ^
    [javac]   symbol:   method addInstLibClassDecl(InstClassDecl)
    [javac]   location: class InstProgramRoot
    [javac] /home/axel/workspace-neon/JModelica/Compiler/ModelicaCompiler/src/java-generated/org/jmodelica/modelica/compiler/InstProgramRoot.java:1963: error: cannot find symbol
    [javac]                         addInstAnonymousClass(createInstClassDecl(cl));
    [javac]                         ^
    [javac]   symbol:   method addInstAnonymousClass(InstClassDecl)
    [javac]   location: class InstProgramRoot

[Jesper Mattsson]

OK, I think all of those can probably be expressed better, I'll try to get some time to take a look at it. The Opt thing that I commented on above is probably more fundamental.

That was a good list, thanks.

[Jesper Mattsson]

So, @joqvist, do you have any comment or answer to what I wrote above about the Opt subclass?

[Jesper Öqvist]

@jespermattsson, I don't really understand the problem. I usually use an NTA to dynamically construct nodes in the AST, then I can evaluate any attribute on the resulting node.

This is my understanding of your issue:

• You create a new node as a non-NTA, and then,
• you want to attach the new node to the AST, but first evaluate some attribute on it.

Is it really necessary to evaluate attributes on the node before it is inserted in the AST? I can't think of a simple way to "fake"-insert it in the AST. Maybe the inherited attributes you want to evaluate could instead be evaluated on the node where you would want to fake-attach the new node?

[Jesper Mattsson]

@joqvist, a typical use case is a symbolic manipulation of an expression (most notably scalarization and differentiation), where you delegate to the sub-expressions and then combine the results in some way. How you do the combination might depend on properties of the newly generated sub-expression, and calculating the properties might in turn require inherited methods (typically name lookup). In these cases being able to operate on partial results like that reduces the complexity of the algorithm and the code duplication by a lot.

There are of course also places where this mechanism was simply convenient to use, since it is already there.

[Jesper Mattsson]

A quick search shows that the expression version of this is used 133 times in the compiler, nearly half in the differentiation code.

[Axel Mårtensson]

We are talking about dynamicFExpOpt(), right? I might have some time to look into alternatives for this pattern next week.

[Jesper Mattsson]

Yes, that's right. We do have an alternate pattern that we could switch to. The old mechanism was simply setting the parent field of the new node, but we felt that that was pretty raw and didn't want to use JastAdd internal fields. So my question was not "Is there another way to do this?", but rather "Is there a recommended way to do this?".

It strikes me now that we could probably use rewrites to achieve this, i.e. any node that needs information from its new children to be able to decide what new node to create will instead create a temporary node that will be rewritten to the final target. The downside is that we would get a lot of temporary AST node classes, probably at least a hundred.