wildcard resolved first instead of current package

[asafbennatan]

when looking for super type of a type we try to resolve the canonical name of that type. the issue is that we are trying to resolve wildcard imports before checking in the current package first. example of the issue:

package org.jboss.forge.grammar.java;

import java.net.http.*;
import java.util.*;

public class MockWildcardClass {

    public List<String> getList(){
        return new ArrayList<>();
    }
    public HttpRequest getHttpRequest(){
        return new HttpRequest();
    }
}

and in the same package:

package org.jboss.forge.grammar.java;


public class HttpRequest {

   private String fakeField;


    public String getFakeField() {
        return fakeField;
    }

    public <T extends HttpRequest> T setFakeField(String fakeField) {
        this.fakeField = fakeField;
        return (T) this;
    }
}

roaster resolves HttpRequest as java.net.http.HttpRequest even though it should be org.jboss.forge.grammar.java.HttpRequest

[asafbennatan]

seems like this will be hard to solve , as far as i see roaster parses a single file this means we lose context of other files within the same package - this is also why the resolving of wildcard imports is incorrect since it tries to find the class by using ContextualClassLoader.loadClass. but this will only work for classes which are also in the runtime environment....

[asafbennatan]

here is the solution i have implemented on my fork of jboss: added an interface:

public interface ParsingContext {

    <T extends JavaType<?>> T parse(final Class<T> type, final String data);
    <T extends JavaType<?>> T parse(final Class<T> type, final File data) throws IOException;
    <T extends JavaType<?>> T parse(final Class<T> type, final InputStream data);

    <T extends JavaType<?>> T getType(String canonicalName);


}

and

package org.jboss.forge.roaster.model;

public interface ContextCapable<T extends JavaType<T>> {

    ParsingContext getParsingContext();
    void setParsingContext(ParsingContext parsingContext);

}

now JavaType implements ContextCapable. and in resolveType we do:

  // no need to check for a import with getImport becuase than a fqn name is needed
      // search for an existing import with the same simple name
      for (Import imprt : imports)
      {
         if (imprt.getSimpleName().equals(result))
         {
            return imprt.getQualifiedName();
         }
      }
      //resolve the current package if exists
      if (getPackage() != null)
      {
         String className = getPackage() + "." + result;
         ParsingContext parsingContext = getParsingContext();
         if(parsingContext !=null&& parsingContext.getType(className)!=null)
         {
            return className;
         }
      }

wildcard imports check is later. the usage of the library is done via:

       ParsingContext parsingContext=Roaster.createParsingContext();
       File file = new File("C:\\Users\\Asaf\\IdeaProjects\\roaster\\tests\\src\\test\\resources\\org\\jboss\\forge\\grammar\\java\\MockWildcardClass.java");
       javaClass = parsingContext.parse(JavaClassSource.class, file);

       File second = new File("C:\\Users\\Asaf\\IdeaProjects\\roaster\\tests\\src\\test\\resources\\org\\jboss\\forge\\grammar\\java\\HttpRequest.java");

       ClassLoaderWildcardImportResolverTest.second= parsingContext.parse(JavaClassSource.class, second);

should i create a pull request?

[gastaldi]

@asafbennatan If I understand it correctly, this won't solve the problem because in your ParsingContext implementation you're storing in a Map every .java file that was already parsed previously, therefore you can get a different result if you forget to add the imported type to the Map.

We had a similar use case for resolving types out of wildcard imports, which is why we introduced org.jboss.forge.roaster.spi.WildcardImportResolver SPI using the ServiceLoader feature. Can you check if that could work for you?

[asafbennatan]

@gastaldi my idea is that once you create the parsing context you should only parse using the parsingContext.parse - which parses the source and adds it to its internal map. as for org.jboss.forge.roaster.spi.WildcardImportResolver - how should i test it with the service location feature ? - if i understand correctly you mean i have to implement WildcardImportResolver which will find the correct implementation? - this means in that implementation i will first check if that source was already loaded before ?

[gastaldi]

I think the Roaster.parse method needs a context object parameter that can provide implementations of WildcardImportResolvers and other features. For example, you could have something like:

ParsingContext context = 
	ParsingContextBuilder.create()
	 .addImportResolver(new ProjectImportResolver(Path.of("/foo"))
	 .build();

// Introduce a new parse method:
Roaster.parse(ParsingContext context, final Class<T> type, final InputStream data);

[gastaldi]

as for org.jboss.forge.roaster.spi.WildcardImportResolver - how should i test it with the service location feature ? - if i understand correctly you mean i have to implement WildcardImportResolver which will find the correct implementation? - this means in that implementation i will first check if that source was already loaded before ?

I think you'll need to resolve the current project directory sources and check if the type exists in the path.

[asafbennatan]

hmm , will that work ? since i will need to parse the source code to actually know it contains the class I'm looking for (i think we cant just rely on the file name -right ? ) this means every time we run the resolver it will parse it - unless we cache the result . IMHO its quite a lot of code for what should be the default implementation (because that's how java does it when it searches for the class) - IMHO accomplishing it should be easy- perhaps if ProjectImportResolver was provided by roaster - but this solution has the problem of working only for files and directory (and not for strings and input streams).

[asafbennatan]

I think the Roaster.parse method needs a context object parameter that can provide implementations of WildcardImportResolvers and other features. For example, you could have something like:

ParsingContext context = 
	ParsingContextBuilder.create()
	 .addImportResolver(new ProjectImportResolver(Path.of("/foo"))
	 .build();

// Introduce a new parse method:
Roaster.parse(ParsingContext context, final Class<T> type, final InputStream data);

how about :

ParsingContext context = new ParsingContext();
Roaster.parse(ParsingContext context, final Class<T> type, final InputStream data);

and Roaster.parse will be incharge of adding the parsed source into the parsingContext

[gastaldi]

Right, now we'll need to define what's in the ParsingContext and how would it be used during the parsing

[gastaldi]

I like the idea of having a ParsingContext and building it (like adding ImportResolvers for example). That would provide a neat abstraction instead of having a default implementation storing parsed sources in memory. WDYT?

[asafbennatan]

@gastaldi in general yes i think its a good idea. you think we should use import resolvers to solve the 'same package' issue ? i mean it would be straight forward if the javaSourceImpl would search for classes in the required order: 1..x. import rules which i dont remember x+1.explicit imports(aka com.x.y.z.ClassName) x+2.same package classes x+3.wildcard import. where if we are using import resolvers some1 looking the code of javaSourceImpl will see: 1..x. import rules which i dont remember x+1.explicit imports(aka com.x.y.z.ClassName) x+2.wildcard import. (containing the special resolver to fix the same package issue)

in addition there might be other locations where you would like to use context from other parsing instances ? ( though im not sure if there are actual or not ). other then that - seems like a valid solution.

[asafbennatan]

also keep in mind that even we if are solving it with import resolvers - the implementation for the import resolver will have to keep track of the previously parsed classes anyway

[asafbennatan]

@gastaldi noted another issue with type resolution - java.lang classes are being resolved out of order - if there's specific import of my.package.Object or if in the same package it should have priority over java.lang.Object