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

          Andrew Eisenberg made changes -
          Field Original Value New Value
          Link This issue is depended upon by GRECLIPSE-648 [ GRECLIPSE-648 ]
          Hide
          Andrew Eisenberg added a comment -

          I am playing around locally with creating a copy of the ast that will remain 'concrete'. No ast transforms will be applied to this tree. However, it will be resolved. This appears to be sufficient.

          I have not checked performance, but I guess that it will be horrendous.

          Show
          Andrew Eisenberg added a comment - I am playing around locally with creating a copy of the ast that will remain 'concrete'. No ast transforms will be applied to this tree. However, it will be resolved. This appears to be sufficient. I have not checked performance, but I guess that it will be horrendous.
          Hide
          Andrew Eisenberg added a comment -

          This solution is still a quick and dirty way to test out a way to better handle AST transforms in the UI. Essentially, what I have now is Option 2. We will only create the AST copy during reconcile. This means that full compiles will not take a hit on ast caching. Testing this out locally, I found that Grails took approx 2x longer to build when running with the AST caching compared to without.

          In order to enable AST caching, before starting Eclipse, you must set framework/system property

          request.concrete.ast=true 
          

          I am not ready to commit what I have. I need to make sure that it is not running havoc on some other part of the code.

          Show
          Andrew Eisenberg added a comment - This solution is still a quick and dirty way to test out a way to better handle AST transforms in the UI. Essentially, what I have now is Option 2. We will only create the AST copy during reconcile. This means that full compiles will not take a hit on ast caching. Testing this out locally, I found that Grails took approx 2x longer to build when running with the AST caching compared to without. In order to enable AST caching, before starting Eclipse, you must set framework/system property request.concrete.ast= true I am not ready to commit what I have. I need to make sure that it is not running havoc on some other part of the code.
          Hide
          Andrew Eisenberg added a comment - - edited

          Here are the situations where we would like the transformed ast only:

          • problem finding
          • build structure
          • reconciling (actually, the top two are really special cases of reconciling)
          • indexing

          Here are the situations where we would like the concrete ast:

          • compilation
          • hierarchy resolving
          • related method finding (ie- searching for method declaration matches while refactoring)
          Show
          Andrew Eisenberg added a comment - - edited Here are the situations where we would like the transformed ast only: problem finding build structure reconciling (actually, the top two are really special cases of reconciling) indexing Here are the situations where we would like the concrete ast: compilation hierarchy resolving related method finding (ie- searching for method declaration matches while refactoring)
          Hide
          Andy Clement added a comment -

          When we were switching backwards and forwards on whether to transform or not as part of reconciling, these situations were not considered separately. Now you have distilled them out (is that definetly all of them?), we can possibly turn off transforms for some but not others and avoid ast copying. Although it avoids the expensive copy, it may still make a right mess of jdt.core as the switches will need to be in place to ensure requestors to specify what they want. If we default one way, I suppose that actually halves the problem as only those who don't want it have to specify they don't.

          Show
          Andy Clement added a comment - When we were switching backwards and forwards on whether to transform or not as part of reconciling, these situations were not considered separately. Now you have distilled them out (is that definetly all of them?), we can possibly turn off transforms for some but not others and avoid ast copying. Although it avoids the expensive copy, it may still make a right mess of jdt.core as the switches will need to be in place to ensure requestors to specify what they want. If we default one way, I suppose that actually halves the problem as only those who don't want it have to specify they don't.
          Hide
          Andrew Eisenberg added a comment -

          OK. A few more improvements here.

          1. When the secret system property to enable requesting of concrete asts is set, the there are info messages sent to the log, that notfies you when a concrete or non-concrete ast is requested. This is only a temporary measure because I imagine for regular use, this will get annoying.

          2. I added some assertions to ensure that a concrete ast is never requested when the system property is left unset.

          3. All tests pass when the property is usset.

          Unfortunately, I still have some test failures when the system property is set. I'm still exploring why.

          Show
          Andrew Eisenberg added a comment - OK. A few more improvements here. 1. When the secret system property to enable requesting of concrete asts is set, the there are info messages sent to the log, that notfies you when a concrete or non-concrete ast is requested. This is only a temporary measure because I imagine for regular use, this will get annoying. 2. I added some assertions to ensure that a concrete ast is never requested when the system property is left unset. 3. All tests pass when the property is usset. Unfortunately, I still have some test failures when the system property is set. I'm still exploring why.
          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.
          Andrew Eisenberg made changes -
          Fix Version/s 2.0.2Release [ 16150 ]
          Affects Version/s 2.0.1Release [ 16074 ]
          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.
          Andrew Eisenberg made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          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: