import groovysh Completion works for all jar files on CLASSPATH

[tkruse]

merge with caution, I am new to Groovy, and Classloading

I don't know how well this will generally work, but I think it will be a huge Improvement over the status quo

[PascalSchumacher]

Hello Thibault,
sadly I'm not qualified to review your pull request, but I created a Jira Issue (https://jira.codehaus.org/browse/GROOVY-6074) for your enhancement.

[tkruse]

Note that while the delay on my PC is negligeable, it might considerably slow down startup on slower machines. There should be some room for improvements for matching strings, and else maybe a command line option could disable the class searching. the same goes for memory consumption, as groovysh would hold a lot of classnames in memory. For my purposes of learning Groovy, having great completion outweights the disadvantages

[andresteingress]

hi! thanks for the contribution, you're very welcome :-)

I had a look at the code and the following enhancements came to my mind:

• test cases: although the groovyshell is not the best-tested code out there, we should attempt to reach out for this goal. For example, this would be a starting point for testing the ReflectionCompleter:

class ReflectionCompletorTests extends GroovyTestCase {

    Groovysh shell
    ReflectionCompletor reflectionCompletor

    @Override protected void setUp() throws Exception {
        shell = new Groovysh()

        reflectionCompletor = new ReflectionCompletor(shell, ClassNameCompletor.classNames)
    }

    void testPreimportedClassNamesContainGroovyPreimportPackages() {
        assert 'String' in reflectionCompletor.preimportedClassNames
        assert 'FileInputStream' in reflectionCompletor.preimportedClassNames
        assert 'BigDecimal' in reflectionCompletor.preimportedClassNames
        assert 'BigInteger' in reflectionCompletor.preimportedClassNames
        assert 'ArrayList' in reflectionCompletor.preimportedClassNames
        assert 'InetAddress' in reflectionCompletor.preimportedClassNames
        assert 'Closure' in reflectionCompletor.preimportedClassNames
        assert 'AntBuilder' in reflectionCompletor.preimportedClassNames
    }

    void testInvalidImportMustNotThrowCompilationException() {
        assert -1 == reflectionCompletor.complete("import java.", 11, [])
    }

    void testFindMatchingVariables() {
        shell.interp.context.variables << [test: 42]

        def candidates = []
        reflectionCompletor.complete("tes", 2, candidates)

        assert candidates == ['test']
    }
}

Of course, you only have to write tests for the new/changed functionality.

• ReflectionCompleter should be split up into two Completer classes: VariableCompleter and ClassNameCompleter, maybe with a common base class. That will make testing easier and the completer code is smaller and better maintainable.
• ClassNameCompleter should use org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS to load only the classes of the default import packages (currently around 780 classes compared to 10000 * x classes in the entire class path). That will highly decreased memory consumption and increase startup performance.
• ImportCommand shouldn't use jline.ClassNameCompletor. Maybe it's better to first complete the package name and as a second step complete the class name from within the given package.

This (or something) like that would be the completer code to get all package from the current GroovyClassLoader:

class ImportPackageCompletor extends SimpleCompletor {

   ImportPackageCompletor(GroovyClassLoader groovyClassLoader) throws IOException {
        this(groovyClassLoader, null)
    }

    ImportPackageCompletor(GroovyClassLoader groovyClassLoader, SimpleCompletorFilter filter)
        throws IOException {
        super(getPackageNames(groovyClassLoader), filter)


        setDelimiter(".")
    }

    static String[] getPackageNames(GroovyClassLoader groovyClassLoader) throws IOException {
        def urls = new HashSet<URL>()

        for (ClassLoader loader = groovyClassLoader; loader != null; loader = loader.parent) {
            if (!(loader instanceof URLClassLoader)) {
                continue
            }

            urls.addAll(loader.URLs)
        }

        def systemClasses = [String.class, javax.swing.JFrame.class] as Class[]
        systemClasses.each { Class systemClass ->
            def classURL = systemClass.getResource("/${systemClass.name.replace('.', '/')}.class")
            if (classURL != null) {
                def uc = classURL.openConnection()
                if (uc instanceof JarURLConnection) {
                    urls.add(((JarURLConnection) uc).getJarFileURL())
                }
            }
        }

        return new URLClassLoader(urls as URL[]).packages*.name
    }
}

and the test case:

class ImportPackageCompletorTests extends GroovyTestCase {

    void testGetAllPackages() {
        final classLoader = new GroovyShell().classLoader
        def packageNames = ImportPackageCompletor.getPackageNames(classLoader) as List

        assert 'groovy.lang' in packageNames
        assert 'groovy.util' in packageNames
        assert 'java.util' in packageNames
        assert 'java.math' in packageNames
        assert 'javax.accessibility' in packageNames
    }
}

ImportPackageCompletor would have to be extended to complete to class names within an already specified package or to .*.

I hope that wasn't too much information, if you need help or if i should implement certain parts just reply here or give me a ping on twitter/adn @asteingr.

[tkruse]

Thanks for the feedback. I am already writing unit tests, and will try to do integrate your comments. One problem I could not easily solve was to provide valid root packages for import. So when you type:
"import "
I'd like to see:
"com. org. sun. java. javax...."

And for that, I'd need to load all classes I guess, to infer the root packages. But I see the problem with memory consumption, so I'll make the default without this completion, and maybe a preference that can eagerly load packages. I will ping you once I have a better state.

[tkruse]

Hm, I notice that java.math. is not part of:
groovy:000> org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS.collect({it})
===> [java.lang., java.io., java.net., java.util., groovy.lang., groovy.util.]

Seems wrong to me.

[andresteingress]

It's not the entire java.math package that is included. it's just java.math.bigdecimal and java.math.biginteger - see the comment above this line in ResolveVisitor:

// note: BigInteger and BigDecimal are also imported by default
    public static final String[] DEFAULT_IMPORTS = {"java.lang.", "java.io.", "java.net.", "java.util.", "groovy.lang.", "groovy.util."};

[andresteingress]

Ad root package: it's just a first thought, but why isn't it sufficient to load just the packages on the class path (like the example above shows). One why to structure the package names would be in a tree structure. So when "import " is typed, all names of the first level (after root) can be suggested. The choices will decrease along the way down to a concrete leave.

[tkruse]

Re root packages: To determine which root packages are available, i still have to go through all jars in the classpath. Sure I can do so and just store root packages, but then the next time the user presses tab, I would have to go through the jars again. Maybe I can store a map from the first 3 levels of packages to jarfile, so that the number of jars to go through are less...

[andresteingress]

This line loads all available packages from the given URLs as shown in the code sample above:

return new URLClassLoader(urls as URL[]).packages*.name

True, we need to find out about all packages, but we'll do that only once. The thing is, the number of packages (~180 in my tests) and the memory consumption for this information is much smaller than keeping all class names (~48.000 in my tests) around.

[tkruse]

Hi, so I tried doing most of what you said, or at least what I understood of it. I did not splitup ReflectionCompletor because of all the other stuff that happens in there in my other pull request. For maintenance and testing, things in there can still well be moved into smaller and static methods.

I copied the code from above and from JLine ClassCompletor into class PackageHelper to be reused by multiple Completors. So I stroe in memory only package names in their base form, and look up classes on demand looking only into the respective jars. Not sure whether all the logic I used there was correct.

Unit tests are not complete, and i want to add an Preference option to disable all classlookup. But maybe you can have a look still.

[andresteingress]

Thanks! I will have a look at your changes when I am back from a client.

[tkruse]

In particular look at how you like the lazy loading. To be honest, it
annoys me, I't rather wait longer for startup than on the first time I
press Tab

On Mon, Apr 8, 2013 at 8:17 AM, Andre Steingress
notifications@github.comwrote:

Thanks! I will have a look at your changes when I am back from a client.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/153#issuecomment-16034317
.

[tkruse]

My Latest and greatest is at my branch: https://github.com/tkruse/groovy-core/tree/fix_and_imports, which is on top of pull request #150. Many more unit tests added now.
In that branch, I have nicely split up ReflectionCompleter as well, based also on completely new code for evaluating safe parts of the line only (If I find the time, I'll work on using evaluation even less, it is soo careless to eval).

But it is too bothersome for me to backport that branch to master, I really wish someone just picket up #150 (or most of of it anyway). So anyway, if you review code, rather look at the fix_and_imports branch. i could do a pull request for it as well, but on github it will show as 60 commits or so because tit includes #150

[tkruse]

closing this until #150 is resolved, will rebase then.

[tkruse]

@andresteingress:
HI, I need your help with classloading again. Basically you suggestion does not really work with non-system jars:

url = new URL("file:///path/to/log4j-1.2.16.jar")
urls = [url]
new URLClassLoader(urls as URL[]).packages*.name

This should give me all packages inside log4j, but instead it gives me the System packages. I checked the URL was right.

In Classloader.java it reads:

 if (parent != null) {
      pkgs = parent.getPackages();
  } else {
     pkgs = Package.getSystemPackages();
  }

This however works:

new URLClassLoader(urls as URL[]).findClass("org.apache.log4j.Level")
===> class org.apache.log4j.Level

So findClass() is properly implemented, but URLClassLoader.packages never contains the packages of the URL. Do you have another idea of how to list just the packages?