You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2020/02/22 11:48:09 UTC
[GitHub] [groovy] danielsun1106 opened a new pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
danielsun1106 opened a new pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171
…when resolving types
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382916363
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -106,6 +141,15 @@
return result;
}
+ @Override
+ public boolean resolveFromDefaultImports(ResolveVisitor resolveVisitor, final ClassNode type) {
Review comment:
what is java9 specific about this code, that it has to go into a VM Plugin?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r383456646
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
Understood.
BTW, we have 2 "Proxy" default imports, one from JDK, another from GDK...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382998864
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
> As 2 groovy elders worry about the impact of the PR, I close it now.
> Thanks for your revewing ;-)
I think the general idea is good, just I think we cannot rely on the cache in case of a miss. Could we forbid people putting classes in groovy.lang? In theory if we want to take the module system seriously then all the packages we use in core should be taboo for the users or else they would get the split package problem. But that also means to not allow people to fallback to the classpath environment / anonymous module Groovy actually needs atm. Such a decision should be done on the list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382927695
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -106,6 +141,15 @@
return result;
}
+ @Override
+ public boolean resolveFromDefaultImports(ResolveVisitor resolveVisitor, final ClassNode type) {
Review comment:
The PR is based on the classes found by `ClassFinder`, which is based on Java 9+ API, so it has to go into a VM plugin.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] blackdrag commented on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
blackdrag commented on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#issuecomment-589960378
is it really right to return false in case of the deterministic default imports fallback? You having had to change so many imports in tests hints, that this issue changes the resolving process in an undesirable way... or you forgot to remove that code, which you then should do ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382997861
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
Even if they are stored locally, there is no guarantee that there is a location or that the location uses the normal protocols. I am thinking here application servers for example. All I am saying is that if the map cannot be populated, it should not make the compiler fail to resolve classes. Otherwise extensive tests on for example JBoss (for example with Groovy as module) are in order. We have no coverage in the test suite for things like that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382997861
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
Even if they are stored locally, there is no guarantee that there is a location or that the location uses the normal protocols. I am thinking here application servers for example. All I am saying is that if the map cannot be populated, it should not make the compiler fail to resolve classes. Otherwise extensive tests on for example JBoss (for example with Groovy as module) are in order. We have no coverage in the test suite for things like that. If that is the case all fine
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382994421
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -106,6 +141,15 @@
return result;
}
+ @Override
+ public boolean resolveFromDefaultImports(ResolveVisitor resolveVisitor, final ClassNode type) {
Review comment:
> In fact we should move them out there as much as possible.
Understood. We use `ClassFinder` to find default imported classes within JRT as well, but visiting JRT is only available in Java 9+ :-(
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382986763
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -106,6 +141,15 @@
return result;
}
+ @Override
+ public boolean resolveFromDefaultImports(ResolveVisitor resolveVisitor, final ClassNode type) {
Review comment:
Actually, ClassFinder my have handling for module-info and jrt, but it would work just fine under Java8, if you added handling for jar urls I think. Are you using methods/classes in there that are Java9 specific? Did not spot any, but I was not looking too much into details.
Anyway, my thinking is that the plugins should not just bring compiler classes into them if not needed. In fact we should move them out there as much as possible. The Java5 generics code was a bit different since you needed to produce AST and required Java5 specific classes to produce it. But now that is no longer the case and a good portion of the methods should be no longer in a plugin
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 closed pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 closed pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382929102
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
> In general I am wondering what happens if you are in an environment which does not provide the classes from file based locations.
I assume that groovy-core classes are all in some directories or a jar file. Even if groovy-core classes are stored in a database, they will have to written to file system at last.
To be frank, I have not found users store groovy-core classes in some non file system, e.g. database... In order to cover the edge cases, the PR provides a option to `groovy.deterministic.default.imports` to fallback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r383087529
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
I agree with Jochen. More optimisation of imports is a good thing. Failing resolution for the case of a cache miss, rather than falling back to a slower path seems to me to be too big to do as a "tweak".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382956129
##########
File path: src/test/org/codehaus/groovy/runtime/FileAppendTest.groovy
##########
@@ -19,6 +19,7 @@
package org.codehaus.groovy.runtime
import groovy.test.GroovyTestCase
+import groovy.lang.DummyGStringBase
Review comment:
Understood.
In fact the classes are not real groovy-core classes, I call them fake core classes ;-)
I am not sure whether we support declaring arbitrary classes under groovy-core packages and importing by default. AFAIK, Java does not allow this kind of declaration...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r383572967
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
That's unfortunate. The Java one takes precedence and I suspect the Groovy one is very rarely used. I am inclined to deprecate the Groovy one. We can copy it some place else or rename to GroovyProxy (or both), or we could just delete it. We would normally prefer `@Delegate` these days but I guess it is a dynamic equivalent and we even have better dynamic alternatives like ProxyGenerator, albeit with additional complexity. I'll create an issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 edited a comment on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 edited a comment on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#issuecomment-589982874
> is it really right to return false in case of the deterministic default imports fallback? You having had to change so many imports in tests hints, that this issue changes the resolving process in an undesirable way... or you forgot to remove that code, which you then should do ;)
"the deterministic default imports" means the cache contains the **complete** default imported classes found by `ClassFinder` , in other words, groovy compiler knows all the default imported classes, so it does not have to gurantee some unknown default imported classes via looking up against all the default imported packages. Let's talk about the topic via some key code:
In the old resolving logic for default imports, we resolve the default imported classes by 2 steps:
1) Try to find the deterministic package name from the cache, if found, we do not need more looking up
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L618-L620
2) If the step 1 failed, we look up the type name against all the default imported packages
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L623-L625
In the new resolving logic for default imports, we eleminate the step 2 by default because the cache contains all default imported classes and step 1 will them find successfuly. If step 1 does not find, the type must not be default imported class, so step 2 is not necessary to process, i.e. we replace the step 2 by Java9 vmplugin's `return false` directly. The new logic of step 2 is shown as follows:
https://github.com/apache/groovy/blob/9d36f6dfcec2d165dd3dea9b97a2381600a2c0ca/src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java#L145-L151
The old logic of step 2 is here:
https://github.com/apache/groovy/blob/bb9d2b1e0ccde5267aba9cf83689ebfebdb0cf49/src/main/java/org/codehaus/groovy/vmplugin/VMPlugin.java#L137-L139
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382929102
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
> In general I am wondering what happens if you are in an environment which does not provide the classes from file based locations.
I assume that groovy-core classes are all in some directories or a jar file. Even if groovy-core classes are stored in a database, they will have to written to file system at last.
To be frank, I have not found users store groovy-core classes in some non file system, e.g. database... In order to cover the edge cases, the PR provides a option to `groovy.deterministic.default.imports` to fallback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 edited a comment on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 edited a comment on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#issuecomment-589982874
> is it really right to return false in case of the deterministic default imports fallback? You having had to change so many imports in tests hints, that this issue changes the resolving process in an undesirable way... or you forgot to remove that code, which you then should do ;)
"the deterministic default imports" means the default imported classes found by `ClassFinder` are complete, in other words, groovy compiler knows all the default imported classes, so it does not have to gurantee some unknown default imported classes via looking up against all the default imported packages. Let's talk about the topic via some key code:
In the old resolving logic for default imports, we resolve the default imported classes by 2 steps:
1) Try to find the deterministic package name from the cache, if found, we do not need more looking up
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L618-L620
2) If the step 1 failed, we look up the type name against all the default imported packages
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L623-L625
In the new resolving logic for default imports, we eleminate the step 2 by default because the cache contains all default imported classes and step 1 will them find successfuly. If step 1 does not find, the type must not be default imported class, so step 2 is not necessary to process, i.e. we replace the step 2 by Java9 vmplugin's `return false` directly. The new logic of step 2 is shown as follows:
https://github.com/apache/groovy/blob/9d36f6dfcec2d165dd3dea9b97a2381600a2c0ca/src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java#L145-L151
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382940149
##########
File path: src/test/org/codehaus/groovy/runtime/FileAppendTest.groovy
##########
@@ -19,6 +19,7 @@
package org.codehaus.groovy.runtime
import groovy.test.GroovyTestCase
+import groovy.lang.DummyGStringBase
Review comment:
I am not sure I like ever having to import something from groovy.lang or groovy.util.
We have IDEs and codenarc checks that are going to complain too.
I would expect just a slower path in those cases if not in a cache.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#issuecomment-590023191
As 2 groovy elders worry about the impact of the PR, I close it now.
Thanks for your revewing ;-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382956129
##########
File path: src/test/org/codehaus/groovy/runtime/FileAppendTest.groovy
##########
@@ -19,6 +19,7 @@
package org.codehaus.groovy.runtime
import groovy.test.GroovyTestCase
+import groovy.lang.DummyGStringBase
Review comment:
Understood.
In fact the classes are not real groovy-core classes, I call them fake core classes ;-)
I am not sure whether we should support declaring arbitrary classes under groovy-core packages and importing by default. AFAIK, Java does not allow this kind of declaration...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
blackdrag commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r382916001
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
I really wonder about this one. There is no gurantee that DefaultGroovyMethods.getLocation(clazz) will return something else than null, which would lead to a NPE here. In general I am wondering what happens if you are in an environment which does not provide the classes from file based locations. What if the classes actually come from a database? And yes, I mean the groovy-core classes itself. It is ok when it is slower then, as long as it still works. But the logic that handles the case it was not found in here it is not fully clear to me at this point actually
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#discussion_r383572967
##########
File path: src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java
##########
@@ -81,15 +91,40 @@
result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java", javaPns));
try {
- URI gsLocation = DefaultGroovyMethods.getLocation(GroovySystem.class).toURI();
- result.putAll(doFindClasses(gsLocation, "groovy", groovyPns));
+ List<URI> foundURIList = new LinkedList<>();
+ result.putAll(doFindGroovyClasses("groovy.lang.GroovySystem", groovyPns, foundURIList));
+
+ // in user production environment, Groovy core classes, e.g. `GroovySystem`(java class) and `GrapeIvy`(groovy class) are all packaged in the groovy core jar file,
+ // but in Groovy development environment, Groovy core classes are distributed in different directories
+ result.putAll(doFindGroovyClasses("groovy.grape.GrapeIvy", groovyPns, foundURIList));
} catch (Exception e) {
- System.err.println("[WARNING] Failed to get default imported groovy classes: " + e.getMessage());
+ throw new GroovyBugError("Failed to get default imported groovy classes", e);
}
return result;
}
+ private Map<String, Set<String>> doFindGroovyClasses(String cn, List<String> groovyPns, List<URI> foundURIList) throws URISyntaxException {
+ GroovyClassLoader groovyClassLoader = new GroovyClassLoader();
+ URI classpathEntryURI;
+ try {
+ Class<?> clazz = groovyClassLoader.loadClass(cn);
+ classpathEntryURI = DefaultGroovyMethods.getLocation(clazz).toURI();
Review comment:
That's unfortunate. The Java one takes precedence and I suspect the Groovy one is very rarely used. I am inclined to deprecate the Groovy one. We can copy it some place else or rename to GroovyProxy (or both), or we could just delete it. We would normally prefer @Delegate these days but I guess it is a dynamic equivalent and we even have better dynamic alternatives like ProxyGenerator, albeit with additional complexity. I'll create an issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 commented on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#issuecomment-589982874
> is it really right to return false in case of the deterministic default imports fallback? You having had to change so many imports in tests hints, that this issue changes the resolving process in an undesirable way... or you forgot to remove that code, which you then should do ;)
"the deterministic default imports" means the default imported classes found by `ClassFinder` are complete, in other words, groovy compiler knows all the default imported classes, so it does not have to gurantee some unknown default imported classes via looking up against all the default imported packages. Let's talk about the topic via some key code:
In the old resolving logic for default imports, we resolve the default imported classes by 2 steps:
1) Try to find the deterministic package name from the cache, if found, we do not need more looking up
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L618-L620
2) If the step 1 failed, we look up the type name against all the default imported packages
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L623-L625
In the new resolving logic for default imports, we eleminate the step 2 by default because the cache contains all default imported classes and step 1 will them find successfuly. If step 1 does not find, the type must not be default imported class, so step 2 is not necessary to process, i.e. we replace the step 2 by Java9 vmplugin's `return false` directly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [groovy] danielsun1106 edited a comment on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
Posted by GitBox <gi...@apache.org>.
danielsun1106 edited a comment on issue #1171: GROOVY-9416: Avoid unnecessary looking up non default import classes …
URL: https://github.com/apache/groovy/pull/1171#issuecomment-589982874
> is it really right to return false in case of the deterministic default imports fallback? You having had to change so many imports in tests hints, that this issue changes the resolving process in an undesirable way... or you forgot to remove that code, which you then should do ;)
"the deterministic default imports" means the default imported classes found by `ClassFinder` are complete, in other words, groovy compiler knows all the default imported classes, so it does not have to gurantee some unknown default imported classes via looking up against all the default imported packages. Let's talk about the topic via some key code:
In the old resolving logic for default imports, we resolve the default imported classes by 2 steps:
1) Try to find the deterministic package name from the cache, if found, we do not need more looking up
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L618-L620
2) If the step 1 failed, we look up the type name against all the default imported packages
https://github.com/apache/groovy/blob/GROOVY_3_0_1/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java#L623-L625
In the new resolving logic for default imports, we eleminate the step 2 by default because the cache contains all default imported classes and step 1 will them find successfuly. If step 1 does not find, the type must not be default imported class, so step 2 is not necessary to process, i.e. we replace the step 2 by Java9 vmplugin's `return false` directly. The new logic of step 2 is shown as follows:
https://github.com/apache/groovy/blob/9d36f6dfcec2d165dd3dea9b97a2381600a2c0ca/src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java#L145-L151
The old logic of step 2 is here:
https://github.com/apache/groovy/blob/bb9d2b1e0ccde5267aba9cf83689ebfebdb0cf49/src/main/java/org/codehaus/groovy/vmplugin/VMPlugin.java#L137-L139
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services