GROOVY-7558 Add private field accessors and bridge methods to dynamic…

[shils]

… classes with statically compiled inner classes

[shils]

adding a public method here is not ideal, though it seems like the alternative would be to create a separate class responsible for adding bridge methods, which would be used by both StaticCompilationVisitor and the phase operation added for dynamic outer classes.

[blackdrag]

So basically you find, you need to generate those bridge methods, and leave it to a later visitor to actually produce that code... The alternative would be to write a visitor that can adds this information and is run from StaticCompilationVisitor, to let the actual adding of those methods be done by the StaticCompilationVisitor later on. But actually the list of inner classes in the classnode is supposed to contain all inner classes at this stage, so I am not sure you even need a full blown visitor for this. What do you think

[shils]

If I understand correctly, your suggestion is to have the StaticCompilationVisitor for an inner class or method add bridge methods to its outer class? The issue there is that there may be multiple statically compiled inner classes/methods, each possibly accessing different private members of the outer class, so we would have to try adding bridge methods (to the outer class) for each statically compiled inner node and keep track of which private members we'd already added bridge methods for. This would add some complexity to the implementation, but I think it would be ok. Does that sound like what you had in mind?

[PascalSchumacher]

@blackdrag Any feedback for @shils?

[blackdrag]

Sorry, for not answering earlier. It might be more easy to explain by example... Look at StaticTypeCheckingVisitor.VariableExpressionTypeMemoizer. The memoizer is used in the visitor to track something, even though StaticTypeCheckingVisitor is itself a visitor. The memoizer here does a specific task, by collecting information in the subtrees of the AST, and feeds that to the original visitor. IN your case that would be the information of what bridge methods do need to be added

[shils]

@blackdrag That's alright, I've been busier than usual as of late anyway. Thanks for the feedback, I'll take a look.

[shils]

@blackdrag I'm not sure I understand your comment. Dynamic classes are not visited by any static compilation visitor so the bridge method collecting visitor you suggest would have to be run from some other visitor.. or from a statically compiled inner class. The collecting visitor would also have to perform method selection across all of the statically compiled inner classes (or be run after method selection.)

In any case, I changed the implementation to not add a public method at the cost of possibly misusing phase operations.

[paulk-asert]

+1, the use of a callback in metadata is a little novel but I can't think of a better way right now and I think correctness trumps the added complexity in this case. We can always refactor and simplify if we can think of an improvement.

[shils]

Agreed, thanks for reviewing this.