Methods belonging to a different source unit get visited and report errors at the wrong place

Cedric Champeau added a comment - 06/Jan/12 8:13 AM Partially resolved, see https://github.com/groovy/groovy-core/commit/455b8a10b1790a53f8f6b0c9ba808769c885302c Especially the TODO. Copy of a discussion by mail: The general problem to be solved is that one: @AnnotationTriggeringTypeChecking class A { def foo() { ... b.bar() ... } } @AnotherAnnotationTriggeringTypeChecking class B { def bar() { ... a.foo() ...} } In most situations, both annotations will either be @TypeChecked or @CompileStatic , but you can have a mix, or custom annotations (including @TypeChecked or @CompileStatic with custom plugin factories). When class A is visited, it eventually discovers a call to b.bar(). B is not defined in the same source unit and we need to infer the return type of b.bar(). It is necessary to determine the return type of bar() at the time of the visit of foo() because the result operation can be chained. Without recursion, think of : class A { def foo() { b.bar().toUpperCase() } } class B { def bar() { 'bar' } } Also, the return type of bar() may include generic type information that must be inferred at that time too. Note that type inference must be triggered if, and only if, the target method is itself type checked. This means that when b.bar() is checked, we must: determine if method bar() is type checked launch type checking, and only type checking, at that moment We cannot use the visitor used by A for two reasons: as the annotations for A and B can be different, there's no reason why A and B should be type checked the same way. Worse, it may introduce failures. error reporting will not use the correct source unit The second point can easily be solved. The first one is the trickiest. Imagine B uses @CompileStatic . The visitor from A could check every annotation bar() or its declaring class (as well as possible outer classes), and verify that the transformation class used by this annotation is derived from StaticTypesTransformation. This is the case, for example, of the @CompileStatic annotation, which defines its own transformation class extending StaticTypesTransformation. This transformation does multiple things: add node metadata (static compilation flags, custom writer factory) override protected StaticTypeCheckingVisitor newVisitor(final SourceUnit unit, final ClassNode node, final TypeCheckerPluginFactory pluginFactory) to provide a static compilation visitor which performs type checking and specialized checks for static compilation performs AST modifications in order to facilitate static compilation If the transformation does not extend StaticTypesTransformation but uses a type checking visitor (that's possible), then there's not much we can do... Obviously, there are things that must not be done by a transformation triggered from a type checking only step. We need a way to separate those operations and this is not always easy to do. For example, if the static type checker only called the newVisitor() method on the AST transformation class, it would get a visitor which does exactly what's needed, but also checks that every method call has a target method set... More, the visitor from A cannot know by advance what parameters should be provided to newVisitor(), especially the plugin factory part, because the logic of that is in the public void visit(final ASTNode[] nodes, final SourceUnit source) method of the AST transformation. We could imagine that we forget this and use the same visitor as the visitor from A. That would help: detecting cycles (as every visited method is remembered) doing type checking only But we would have a type checking result different from what we would obtain if we compiled B alone, with its own annotation... Another problem is that performing type checking on a node (be it a class node or a method node) "pollutes" it with type checking meta data. If the visitor which is used is not correct, or not instantiated with the correct configuration, then the AST of B will contain erroneous type information. Second (and this is a problem which exists in any case), the type checking step will be started a second time when the compiler will compile class B. If type information is already present in the AST of B, the result of type checking is very likely to be wrong. To be short, we need either a way of remembering the state of the AST before the visitor from A generates a visitor for B and adds metadata, or we need to work on a clone of the AST of B, so that when the visit ends, even if, for example, the @CompileStatic annotation transforms the AST, the original AST of B remains untouched. Unfortunately, cloning an AST is not necessarily easy... Last but not least, we must also consider the case where the AST of B represents a precompiled node. In that case, the code of method will be empty, and type inference would fail. This means that we must add type inference metadata at the bytecode level (in any encoded form) which can be used in that situation. As the problem is complex, I will only commit a "hotfix" for now, which should be suitable for most situations, which implies only methods from the same source unit can have their return type inferred (which is not perfect). This shouldn't be a problem in most situations, and as I will have some work on presentations to do, it will let me some time to work on more critical bugs.