Skip to content

wildcard resolved first instead of current package #223

Open
@asafbennatan

Description

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

Activity

asafbennatan

asafbennatan commented on Dec 22, 2021

@asafbennatan
Author

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

asafbennatan commented on Dec 22, 2021

@asafbennatan
Author

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?

added a commit that references this issue on Dec 22, 2021

Fixes forge#223 - tries to resolve classes in current package before …

d5632f6
gastaldi

gastaldi commented on Dec 22, 2021

@gastaldi
Member

@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

asafbennatan commented on Dec 22, 2021

@asafbennatan
Author

@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

gastaldi commented on Dec 22, 2021

@gastaldi
Member

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

gastaldi commented on Dec 22, 2021

@gastaldi
Member

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

asafbennatan commented on Dec 22, 2021

@asafbennatan
Author

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

asafbennatan commented on Dec 22, 2021

@asafbennatan
Author

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

gastaldi commented on Dec 22, 2021

@gastaldi
Member

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

gastaldi

gastaldi commented on Dec 22, 2021

@gastaldi
Member

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

asafbennatan commented on Dec 22, 2021

@asafbennatan
Author

@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

asafbennatan commented on Dec 22, 2021

@asafbennatan
Author

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

asafbennatan commented on Dec 25, 2021

@asafbennatan
Author

@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

added a commit that references this issue on Dec 25, 2021

Fixes forge#223 - tries to resolve classes in current package before …

1b93944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      wildcard resolved first instead of current package · Issue #223 · forge/roaster