Code review for jigsaw/jake -> jdk9/dev sync up

[Paul Sandoz]

For the JDK patch:

MethodHandles
—

 176     public static Lookup privateLookupIn(Class<?> targetClass, Lookup lookup) throws IllegalAccessException {
 177         SecurityManager sm = System.getSecurityManager();
 178         if (sm != null) sm.checkPermission(ACCESS_PERMISSION);
 179         if (targetClass.isPrimitive())
 180             throw new IllegalArgumentException(targetClass + " is a primitive class");
 181         Module targetModule = targetClass.getModule();
 182         Module callerModule = lookup.lookupClass().getModule();
 183         if (callerModule != targetModule && targetModule.isNamed()) {
 184             if (!callerModule.canRead(targetModule))
 185                 throw new IllegalAccessException(callerModule + " does not read " + targetModule);
 186             Class<?> clazz = targetClass;
 187             while (clazz.isArray()) {
 188                 clazz = clazz.getComponentType();
 189             }

What happens if you pass a primitive array?

I think you need to specify what happens if an array class is passed and how the target class is obtained, and an IAE if the "element type" of the array is a primitive class.

(Separately i am looking forward to using this method to replace some Unsafe usages between Thread and other classes within the base module.)


ModuleDescriptor
—

 241         private <T> Stream<String> toStringStream(Set<T> s) {
 242             return s.stream().map(e -> e.toString().toLowerCase());
 243         }
 244
 245         private <M> String toString(Set<M> mods, String what) {
 246             return (Stream.concat(toStringStream(mods), Stream.of(what)))
 247                     .collect(Collectors.joining(" "));
 248         }

The above occurs three times. Suggest moving to the enclosing class as package private static methods.


Resolver
—

 449                     for (Configuration parent: parents) {
 450                         Optional<ResolvedModule> om = parent.findModule(dn);
 451                         if (om.isPresent()) {
 452                             other = om.get().reference();
 453                             break;
 454                         }
 455                     }

Replace with

  ResolvedModule rm = findInParent(dm);
  if (rm != null)
    other = rm.reference();


 532             Set<ResolvedModule> requiresTransitive= new HashSet<>();

Formating glitch for “=“


ServiceLoader
—

1331         @Override
1332         @SuppressWarnings("unchecked")
1333         public boolean tryAdvance(Consumer<? super Provider<T>> action) {
1334             if (ServiceLoader.this.reloadCount != expectedReloadCount)
1335                 throw new ConcurrentModificationException();
1336             Provider<T> next = null;
1337             if (index < loadedProviders.size()) {
1338                 next = (Provider<T>) loadedProviders.get(index);
1339             } else if (iterator.hasNext()) {
1340                 next = iterator.next();
1341             } else {
1342                 loadedAllProviders = true;
1343             }
1344             index++;

Move the index increment into the top if block, thereby it only gets increment appropriately (no crazy overflow possible).


1353         @Override
1354         public int characteristics() {
1355             // need to decide on DISTINCT
1356             // not IMMUTABLE as structural interference possible
1357             return Spliterator.ORDERED + Spliterator.NONNULL;
1358         }

Should probably be consistent (subset of) with the characteristics reported for loadedProviders.stream(), thus DISTINCT would not be appropriate in this case unless you changed the optimal case of when all providers are loaded.



1552     public Optional<S> findFirst() {
1553         Iterator<S> iterator = iterator();
1554         if (iterator.hasNext()) {
1555             return Optional.of(iterator.next());
1556         } else {
1557             return Optional.empty();
1558         }
1559     }

 return stream().findFirst() ?


Paul.

> On 24 Nov 2016, at 07:25, Alan Bateman <Alan.Bateman at oracle.com> wrote:
> 
> Folks on jigsaw-dev will know that we are on a mission to bring the changes accumulated in the jake forest to jdk9/dev. We can think of this as a refresh of the module system in JDK 9, the last big refresh was in May with many small updates since then.
> 
> The focus this time is to bring the changes that are tied to JSR issues into jdk9/dev, specifically the issues that are tracked on the JSR issues list [1] as:
> 
> #CompileTimeDependences
> #AddExportsInManifest
> #ClassFileModuleName
> #ClassFileAccPublic
> #ServiceLoaderEnhancements
> #ResourceEncapsulation/#ClassFilesAsResources
> #ReflectiveAccessToNonExportedTypes
> #AwkwardStrongEncapsulation
> #ReadabilityAddedByLayerCreator
> #IndirectQualifiedReflectiveAccess (partial)
> #VersionsInModuleNames
> #NonHierarchicalLayers
> #ModuleAnnotations/#ModuleDeprecation
> #ReflectiveAccessByInstrumentationAgents
> 
> Some of these issues are not "Resolved" yet, meaning there is still ongoing discussion on the EG mailing list. That is okay, there is nothing final here. If there are changes to these proposals then the implementation changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we will keep the jake forest open for ongoing prototyping and iteration, also ongoing implementation improvements where iteration or bake time is important.
> 
> For the code review then the focus is therefore on sanity checking the changes that we would like to bring into jdk9/dev. We will not use this review thread to debate alternative designs or other big implementation changes that are more appropriate to bake in jake.
> 
> To get going, I've put the webrevs with a snapshot of the changes in jake here:
>    http://cr.openjdk.java.net/~alanb/8169069/0/
> 
> The changes are currently sync'ed against jdk-9+146 and will be rebased (and re-tested) against jdk9/dev prior to integration. There are a number of small changes that need to be added to this in the coming days, I will refresh the webrev every few days to take account of these updates.
> 
> 
> A few important points to mention, even if you aren't reviewing the changes:
> 
> 1. This refresh requires a new version of jtreg to run the tests. The changes for this new version are in the code-tools/jtreg repository and the plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then we'll update the requiredVersion property in each TEST.ROOT to force everyone to update.
> 
> 2. For developers trying out modules with the main line JDK 9 builds then be aware that `requires public` changes to `requires transitive` and the `provides` clause changes to require all providers for a specific service type to be in the same clause. Also be aware that the binary form of the module declaration (module-info.class) changes so you will need to recompile any modules.
> 
> 3. Those running existing code on JDK 9 and ignoring modules will need to be aware of a disruptive change in this refresh. The disruptive change is #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it can't be used to break into non-public fields/methods of JDK classes. This change is going to expose a lot of hacks in existing code. We plan to send mail to jdk9-dev in advance of this integration to create awareness of this change. As per the original introduction of strong encapsulation then command line options (and now the manifest of application JAR files) can be used to keep existing code working. The new option is `--add-opens` to open a package in a module for deep reflection by other modules. As an example, if you find yourself with code that hacks into the private `comparator` field in java.util.TreeMap then running with `--add-opens java.base/java.util=ALL-UNNAMED` will keep that code working.
> 
> 
> A few miscellaneous notes for those that are reviewing:
> 
> 1. We have some temporary/transition code in the top-level repo to deal with the importing of the JavaFX modules. This will be removed once the changes are in JDK 9 for the OpenJFX project to use.
> 
> 2. In the jdk repo then it's important to understand that the module system is initialized at startup and there are many places where we need to keep startup performance in mind. This sometimes means less elegant code than might be used if startup wasn't such a big concern.
> 
> 3. The changes in the jaxws repo make use of new APIs that means the code doesn't compile with JDK 7 or JDK 8. Our intention is to work with the JAXB and JAX-WS maintainers to address the issues in the upstream project and then bring those changes into jdk9/dev to replace the patches that we are forced to push for the short term.
> 
> 4. You will see several tests where the value of the @modules tag has `:open` or `:+open`. This is new jtreg speak. The former means the test is run with --add-opens to open the package, the latter means the test is exported at compile-time and exported + open at run-time (the latter usage will be rare, it's where tests have static references to JDK internal types and are also doing deep reflection with setAccessible).
> 
> 
> In terms of dates then we are aiming to integrate these changes into jdk9/dev in early December. I will send a follow-up mail next week on this as we work through the logistics.
> 
> -Alan
> 
> [1] http://openjdk.java.net/projects/jigsaw/spec/issues/
> [2] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-November/010219.html