GRECLIPSE
  1. GRECLIPSE
  2. GRECLIPSE-617

The problem of ASTs, transforms and how to do code resolution, content assist, etc.

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.1Release
    • Fix Version/s: 2.0.2Release
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      0

      Description

      Starting from the discussion in GRECLIPSE-610, this issue will describe the problem of how to handle ASTs that have transforms applied to them. The problem is that some IDE operations require the untransformed AST whereas others require the transformed AST. Groovy-Eclipse caches the transformed AST after each reconcile and uses that to perform most of its operations like inference, content assist, underlining, and code select. The problem is that if code blocks have been moved around, we are not always able to visit everything and know where to look for certain pieces of code.

      Here are several possibilities for fixing this:

      1. Always visit all parts of the AST for every visit and rather than cut a visit short after an element is found, continue visiting other (synthetic) methods just in case a more appropriate AST node is found. This can be complicated because things like inferencing and code select require the existence of a containing IJavaElement (ie- a code element that is in the Java model and not something that is transformed). This will drastically complicate the Inferencing visitor (and other similar visitors) because we always need to map back to something in the Java model even if the Groovy element has been transformed and moved to a synthetic groovy method outside of the model. However, this possibility is nice because it would not add any extra processing to to reconciles and memory footprint would not go up.
      2. Instead of caching a single instance of the (transformed) AST, cache both the untransformed AST and the transformed AST. This is nice because depending on the operation, we can choose to walk the appropriate AST. However, there are 3 problems here. First, it will take extra time to create the copy. Second, the size of a GroovyCompilationUnit will essentially double. Third, some operations would require both the transformed and the untransformed AST at the same time. It will be tricky to start from one, translate an AST node from one tree to the other (ie- what does equality mean between these two trees?) and then use the information from one tree to populate results of the other.
      3. Only cache the untransformed tree. If Peter is right in GRECLIPSE-610 and transforms that add declarations to be called in user code are rare, then perhaps we don't need to cache the transformed tree at all. Again, some extra time would be spent to create the copy of the AST, but perhaps we can be smart about it and only do a copy on write. Also, the size of GroovyCompilationUnits would not need to increase. The downside is that some very common transforms would need special handling for inferencing and content assist. One that comes to mind immediately is @Singleton. The getInstance method needs to be in content assist and it should not be underlined. So, we would need to create a special ITypeLookup and ContentAssistProvider for this.

      At this point, I am leaning towards #3, but I can also see the benefit of #1 where we do not need to special case AST Transforms that add user declarations.

        Issue Links

          Activity

          Hide
          Andrew Eisenberg added a comment - - edited

          Here is a list of the currently failing tests that are somewhat troubling:

          • InferencingTests.testStaticMethodCall3() : inferred type should have been string. The problem here is that a method call AST node should have been converted into a static method call AST node, but wasn't. This may have to happen in concrete asts.
          • FieldCompletionTests.testProperties1() : It looks like properties are not being generated for classes that have concrete ASTs requested. This is another thing that will need to be added to the groovy classes during concrete ast requesting.
          • CodeSelectFieldsTest.testCodeSelectOfGeneratedGetter() : This is occurring because properties are not being generated.
          • CodeSelectFieldsTest.testCodeSelectOfGeneratedSetter() : This is occurring because properties are not being generated.
          • TypeCompletionTests.testCompletionTypesInParameters4() : Looks like aproblem with default expressions of parameters
          • RenameTypeTests.testAnnotation2() : Renaming of an annotation reference inside another annotation is not being found. The second 'A' is not being renamed:
          @interface Main {
             A child() default @A("Void");
          }
          
          • RenameTypeTests.testAlias1() : Here, a constructor call is not being renamed (ie- the 'A' here):
             
            public A(int num) {
                fNum= num;
            }
            
          • RenameTypeTests.testEnum1() : Same problem as above.

          To summarize, when concrete asts are added, we need to ensure that getters and setters generated from properties are properly added to the class. Also, we need to ensure that static method calls are created where appropriate. For some of the other tests, I am concerned that some special logic we added when we were not dealing with concrete asts, is now causing us problems when we are. I am getting closer to a state where these changes are releasable, but not yet.

          Show
          Andrew Eisenberg added a comment - - edited Here is a list of the currently failing tests that are somewhat troubling: InferencingTests.testStaticMethodCall3() : inferred type should have been string. The problem here is that a method call AST node should have been converted into a static method call AST node, but wasn't. This may have to happen in concrete asts. FieldCompletionTests.testProperties1() : It looks like properties are not being generated for classes that have concrete ASTs requested. This is another thing that will need to be added to the groovy classes during concrete ast requesting. CodeSelectFieldsTest.testCodeSelectOfGeneratedGetter() : This is occurring because properties are not being generated. CodeSelectFieldsTest.testCodeSelectOfGeneratedSetter() : This is occurring because properties are not being generated. TypeCompletionTests.testCompletionTypesInParameters4() : Looks like aproblem with default expressions of parameters RenameTypeTests.testAnnotation2() : Renaming of an annotation reference inside another annotation is not being found. The second 'A' is not being renamed: @ interface Main { A child() default @A( " Void " ); } RenameTypeTests.testAlias1() : Here, a constructor call is not being renamed (ie- the 'A' here): public A( int num) { fNum= num; } RenameTypeTests.testEnum1() : Same problem as above. To summarize, when concrete asts are added, we need to ensure that getters and setters generated from properties are properly added to the class. Also, we need to ensure that static method calls are created where appropriate. For some of the other tests, I am concerned that some special logic we added when we were not dealing with concrete asts, is now causing us problems when we are. I am getting closer to a state where these changes are releasable, but not yet.
          Hide
          Andrew Eisenberg added a comment -

          The final solution that we are doing now is to selectively run AST transforms for different kinds of operations. For compiles, type hierarchy calculations, and method override calcluations, we are running the transformations, but for reconciles and all other operations that require an AST, transforms are not running. This appears to solve the problems described here. Closing this bug. If there are any new problems to come of this, it will be in a new issue.

          Show
          Andrew Eisenberg added a comment - The final solution that we are doing now is to selectively run AST transforms for different kinds of operations. For compiles, type hierarchy calculations, and method override calcluations, we are running the transformations, but for reconciles and all other operations that require an AST, transforms are not running. This appears to solve the problems described here. Closing this bug. If there are any new problems to come of this, it will be in a new issue.
          Hide
          Andrew Eisenberg added a comment -

          We've done enough work here. Future work will go into new issues.

          Show
          Andrew Eisenberg added a comment - We've done enough work here. Future work will go into new issues.
          Hide
          Andy Clement added a comment -

          just to note, this solution currently breaks @Grab related reconciling.

          Show
          Andy Clement added a comment - just to note, this solution currently breaks @Grab related reconciling.
          Hide
          Carl Parisi added a comment -

          Is the
          request.concrete.ast=true
          still supported?

          Show
          Carl Parisi added a comment - Is the request.concrete.ast=true still supported?

            People

            • Assignee:
              Andrew Eisenberg
              Reporter:
              Andrew Eisenberg
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: