Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moditect removes import clauses from input file #90

Open
GedMarc opened this issue Jan 16, 2019 · 15 comments

Comments

Projects
None yet
4 participants
@GedMarc
Copy link

commented Jan 16, 2019

Hi there,

Have a strange scenario where the imports clauses on a module-info file (not placed in any meta-inf/versions for version specific integration) is not coming in

Repo : https://github.com/GedMarc/GuiceInjection

original_module_info.txt

import com.jwebmp.guicedinjection.abstractions.GuiceInjectorModule;
import com.jwebmp.guicedinjection.implementations.GuiceDefaultModuleExclusions;
import com.jwebmp.guicedinjection.injections.ContextBinderGuice;
import com.jwebmp.guicedinjection.injections.JPMSGuiceASM;
import com.jwebmp.guicedinjection.interfaces.*;

module com.jwebmp.guicedinjection {

output (https://jwebmp.com/artifactory/webapp/#/artifacts/browse/tree/General/libs-snapshot-local/com/jwebmp/guiced-injection/0.63.0.52-SNAPSHOT/guiced-injection-0.63.0.52-20190116.085626-1.jar)

Has no imports clauses in generated module-info (See attached)

module com.jwebmp.guicedinjection {

decompiled_module_info.txt

Plugin Settings

<plugin>
                        <groupId>org.moditect</groupId>
                        <artifactId>moditect-maven-plugin</artifactId>
<version>1.0.0.Beta2</version>
                        <executions>
                            <execution>
                                <id>add-module-infos</id>
                                <phase>package</phase>
                                <goals>
                                    <goal>add-module-info</goal>
                                </goals>
                                <configuration>
                                    <overwriteExistingFiles>true</overwriteExistingFiles>
                                    <module>
                                        <moduleInfoFile>
                                            src/jre11/java/module-info.java
                                        </moduleInfoFile>
                                    </module>
                                </configuration>
                            </execution>
                        </executions>
                    </plugin>

The snapshot is available on sonartype-snapshot - (I have to change it so I can build)

  <dependency>
            <groupId>com.jwebmp</groupId>
            <artifactId>guiced-injection</artifactId>
            <version>0.63.0.52-SNAPSHOT</version>
        </dependency>
@GedMarc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

The error produced on build is

java.lang.module.FindException: Error reading module: C:\java\repositories\apache-maven-repository\com\jwebmp\guiced-injection\0.63.0.52-SNAPSHOT\guiced-injection-0.63.0.52-20190116.085659-1.jar
Caused by: java.lang.module.InvalidModuleDescriptorException: IPackageContentsScanner: is not a qualified name of a Java class in a named package

@siordache

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Your module-info.java contains unqualified service names:

uses IPackageContentsScanner;
...
provides IGuicePreStartup with JPMSGuiceASM;
...

You must fully qualify them:

uses com.jwebmp.guicedinjection.interfaces.IPackageContentsScanner;
...
provides com.jwebmp.guicedinjection.interfaces.IGuicePreStartup with com.jwebmp.guicedinjection.injections.JPMSGuiceASM
...
@gunnarmorling

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

contains unqualified service names

He actually imported these types; I didn't even know you could do that in a module descriptor, but if you can, this should be supported. The current implementation doesn't take care of resolving imports. So as a workaround, just not using imports should work, but eventually it's something that the plug-in should support.

@gunnarmorling

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

That said, this might become a bit more complex. Perhaps the symbol resolver from JavaParser can be used to resolve unqualified types against import statements (@matozoid may clarify that), but we're out of luck when using * imports. The current parser/compiler implementation has no knowledge about the code base going beyond the module-info.java file itself, that'd have to change for making star imports work. I'm not sure whether it's worth the effort, but supporting regular imports might be doable with reasonable effort.

@GedMarc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

Was wondering if I would have to do that xD there's a good 130 projects i will have to do this xD Let the hunt for the IDE formatting property begin xD

I think put a notice on the readme for now?

I agree, that wildcards should not be accepted for imports, and should stick with the strict declarations or complete import package statements - This matches the patterns for JPMS, and is not so much a nice to have as it is a bloat-the-module-loader with unnecessary searches.

@matozoid

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

Symbol solver questions are usually forwarded to @ftomassetti :-) It won't solve symbols if the imports aren't there though.

Maybe try an 80% solution where you index all the classes in the project and their packages so you know which unqualified class needs which imports? Classes outside the main source directory would be impossible to find, of course.

@GedMarc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

@matozoid This would be 100% safe due to the JPMS restrictions

  1. No uses or provides clause can reference any class not found inside the declaring module
  2. No package can be exported or opened that does not contain any classes within the declaring module
  3. Exporting "to" modules are string-name based and no validation is performed on compilation or execution to the existence of the declared module

In essence, in JPMS, there will never be any scenario where any scanning of external classes would ever be necessary or required.
In instances where it is specified (as moditect allows any specification -> VERY IMPORTANT TO HAVE THIS) an execution error will present itself as an invalid module declaration.

It is important to me to allow moditect to allow any specification valid or not, in porting Guice over to JPMS, we JarJar ASM and CGLIB into the project, and the development module-info is different to the packaged module-info, and the current structure works perfectly for this.

@matozoid

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

The only downside would be that scanning sourcecode tends to be slow :-(

@GedMarc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

@matozoid I think that can be mitigated though -

try (ScanResult sr = new ClassGraph().whitelistPackagesNonRecursive("specified.package").scan()) {
			for (ClassInfo allClass : sr.getAllClasses()) {
				System.out.println("import " + allClass.loadClass().getCanonicalName() + ";\n");
			}
		}
<dependency>
            <groupId>io.github.classgraph</groupId>
            <artifactId>classgraph</artifactId>
<version>4.6.18</version>
            <type>jar</type>
        </dependency>

@lukehutch Would this be feasible in this situation?

@siordache

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

@GedMarc

  1. No uses or provides clause can reference any class not found inside the declaring module

I think this is not true.

You cannot find java.sq.Driver inside the declaring module, but you can write:

import java.sql.*;

module com.jwebmp.guicedinjection {
    uses Driver;
    provides Driver with com.jwebmp.guicedinjection.injections.MyDriver;
}
@GedMarc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

@siordache
I wish that were true, the MyDriver class has to be located in the module com.jwebmp.guicedinjection

or you get the following exception on execution (it will compile, it will never run)

 Provider class MyDriver not in module com.jwebmp.guicedinjection

This is one of those not-documented only know when developed things.

@siordache

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

I assumed the MyDriver exists in your code. My question was how do you detect that Driver means java.sql.Driver.

@GedMarc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

You mean if structure is as below?

import java.sql.*;
module driver.registration {
requires java.sql;
provides Driver with something.MyDriver;
}

Hmm, interesting situation.

The more I think about this, the more i believe that not supporting wildcard entries in import clauses is more justified...
I did a quick test now,

requires java.sql;
requires test.create.driver.class;

image

Ok here's where we get funky -
It actually only picks up the Provider from sql automagically, i'm guessing from the uses clause in java.sql
Now when we use double wild card expressions -

![image](https://user-images.githubusercontent.com/5367513/51253787-01635d00-19a8-11e9-983d-8f66bffb4281.png)

In a valid decompiled valid module-info with wildcard imports - same project same module info compiled with the original module-info on the JDK 11 platform (https://mvnrepository.com/artifact/com.jwebmp.jre11/guiced-injection/0.60.0.1):

module com.jwebmp.guicedinjection {
    requires com.google.guice;
    requires io.github.classgraph;
    requires java.validation;
    requires com.fasterxml.jackson.core;
    requires com.fasterxml.jackson.databind;
    requires com.fasterxml.jackson.datatype.jdk8;
    requires com.fasterxml.jackson.datatype.jsr310;
    requires java.logging;
    requires com.jwebmp.logmaster;
    requires aopalliance;
    requires javax.inject;
    requires com.fasterxml.jackson.annotation;
    requires com.google.common;

    exports com.jwebmp.guicedinjection;
    exports com.jwebmp.guicedinjection.interfaces;
    exports com.jwebmp.guicedinjection.abstractions;
    exports com.jwebmp.guicedinjection.pairing;
    exports com.jwebmp.guicedinjection.properties;

    uses com.jwebmp.guicedinjection.interfaces.IPackageContentsScanner;
    uses com.jwebmp.guicedinjection.interfaces.IFileContentsScanner;
    uses com.jwebmp.guicedinjection.interfaces.IGuiceConfigurator;
    uses com.jwebmp.guicedinjection.interfaces.IGuiceDefaultBinder;
    uses com.jwebmp.guicedinjection.interfaces.IGuicePreStartup;
    uses com.jwebmp.guicedinjection.interfaces.IGuiceModule;
    uses com.jwebmp.guicedinjection.interfaces.IGuicePostStartup;
    uses com.jwebmp.guicedinjection.interfaces.IPathContentsScanner;
    uses com.jwebmp.guicedinjection.interfaces.IPathContentsBlacklistScanner;
    uses com.jwebmp.guicedinjection.interfaces.IGuiceScanJarExclusions;
    uses com.jwebmp.guicedinjection.interfaces.IGuiceScanModuleExclusions;

    provides com.jwebmp.guicedinjection.interfaces.IGuicePreStartup with com.jwebmp.guicedinjection.injections.JPMSGuiceASM;
    provides com.jwebmp.guicedinjection.interfaces.IGuiceScanJarExclusions with com.jwebmp.guicedinjection.implementations.GuiceDefaultJARExclusions;
    provides com.jwebmp.guicedinjection.interfaces.IGuiceScanModuleExclusions with com.jwebmp.guicedinjection.implementations.GuiceDefaultModuleExclusions;
    provides com.jwebmp.guicedinjection.interfaces.IGuiceDefaultBinder with com.jwebmp.guicedinjection.injections.ContextBinderGuice;
    provides com.jwebmp.guicedinjection.interfaces.IGuiceModule with com.jwebmp.guicedinjection.abstractions.GuiceInjectorModule;
}

Everything is exploded, and no import clause is there.

The imports clause in module-info is therefore purely cosmetic to the compiler, and is not in anyway actually read or used by the JVM.

So back to the original suggestions I think, disallow wildcard import statements? Or completely disallow the import statement in the moditect files?

@GedMarc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

Hmm picture broke,

Basically the module-info does not compile, but it also highlights that the class Driver can only exist in one imported package regardless of the 'uses' clause, which is a full contradiction of the first scenario lol

I don't think wildcards should be supported by moditect based on this.

@gunnarmorling

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

So back to the original suggestions I think, disallow wildcard import statements? Or completely disallow the import statement in the moditect files?

Yes, so my vote is definitely to disallow wildcard ones. On regular ones, I'm not sure whether allowing them adds much value: what are the chances that you'd reference the same type more than once? But if we wanted to allow them, they should simply be handled as a "stupid" source of resolving type references in other places in the parsed file. We shouldn't scan or parse other parts of the source folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.