Bug 536860 - JDT compiler fails to infer correctly generic types in Eclipse Photon
Summary: JDT compiler fails to infer correctly generic types in Eclipse Photon
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.8   Edit
Hardware: PC Linux
: P3 normal with 1 vote (vote)
Target Milestone: 4.11 RC1   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 538126 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-07-10 01:16 EDT by Lyor Goldstein CLA
Modified: 2020-01-30 17:26 EST (History)
7 users (show)

See Also:
manoj.palat: review+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lyor Goldstein CLA 2018-07-10 01:16:39 EDT
Example #1:

public <T> EntityReader<T, Path> createEntityReader(
    Class<T> entity, Path path, CoreCSVSettings settings,
    Function<? super String, ? extends String> colNameMapper,
    Function<? super String[], ? extends T> instantiator,
    Map<String, ?> runtimeValues, LoadValueConverter converter)
         throws IOException {


        /*
         * Eclipse's error:
         *
         * The method createEntityStreamReader(Class<T>, Callable<InputStream>, CoreCSVSettings, Function<? super String,? extends String>, Function<? super String[],? extends T>, Map<String,?>, LoadValueConverter)
         * in the type FilePersisterFactoryImpl is not applicable for the arguments
         * (Class<T>, Callable<InputStream>, CoreCSVSettings, Function<capture#9-of ? super String,capture#10-of ? extends String>, Function<capture#11-of ? super String[],capture#12-of ? extends T>, Map<String,capture#13-of ?>, LoadValueConverter)
         */

        EntityReader<T, ?> streamReader =
            createEntityStreamReader(entity, ExtraIOUtils.getInputStreamProvider(path),   // returns a Callable<InputStream>
                settings, colNameMapper, instantiator, runtimeValues, converter);
        ...
}


public <T> EntityReader<T, Callable<InputStream>> createEntityStreamReader(
        Class<T> entity, Callable<InputStream> streamProvider, CoreCSVSettings settings,
        Function<? super String, ? extends String> colNameMapper, Function<? super String[], ? extends T> instantiator,
        Map<String, ?> runtimeValues, LoadValueConverter converter)
            throws IOException {
    ... whatever ...
}

------------------------------------------------------------

Example #2:

public <T> EntityReader<T, Callable<InputStream>> createEntityStreamReader(
    Class<T> entity, Callable<InputStream> streamProvider, CoreCSVSettings settings,
    Function<? super String, ? extends String> colNameMapper, Function<? super String[], ? extends T> instantiator,
    Map<String, ?> runtimeValues, LoadValueConverter converter)
            throws IOException {

        /*
         * Eclipse's error:
         *
         *    The method resolveInstantiator(Class<T>, Function<? super String[],? extends T>)
         *    in the type FilePersisterFactoryImpl is not applicable for the arguments
         * (Class<T>, Function<capture#29-of ? super String[],capture#30-of ? extends T>)
         */
        Function<? super String[], ? extends T> factory = resolveInstantiator(entity, instantiator);
        ....
}

protected <T> Function<? super String[], ? extends T> resolveInstantiator(
    Class<T> entity, Function<? super String[], ? extends T> instantiator) {
        .... whatever ....
}

--------------------------------------------------------

* I am using OpenJDK Java 8 build 172 running on Fedora 27
* The code compiles just fine outside of Eclipse via the Java compiler
* Eclipse Oxygen (!) has no problems with this code...
Comment 1 Lyor Goldstein CLA 2018-07-10 01:22:05 EDT
I marked it as "Critical" since it is a regression problem which makes using Eclipse Photon on existing code problematic to say the least. For now, I worked around it by using raw class casts since (for now...) it occurs only in these 2 locations and I can live with it for the time being. However, this is not likely to hold for long and we are likely to encounter code that we cannot (or do not want) to "spoil" by such un-necessary casts...
Comment 2 Stephan Herrmann CLA 2018-07-10 07:30:23 EDT
I briefly tried to reproduce, but there's way too much stuff missing from the code in comment 0 for me to be able to guess.

Please provide sufficient stubs for all types and methods involved.
Comment 3 Lyor Goldstein CLA 2018-07-11 05:39:03 EDT
Here is the necessary stubbing which should suffix (IMO) to reproduce - as can be plainly seen they are all interfaces - I have listed all of them - including the non-generic ones

public interface CoreCSVSettings {
    /**
     * Default charset if none specified
     */
    Charset DEFAULT_CHARSET = Charset.forName(DEFAULT_CODE_PAGE);

    String getCodePage();

    /**
     * Default delimiter if none specified
     */
    Character DEFAULT_DELIMITER = ',';

    Character getColumnDelimiter();

    /**
     * Default assumption about the first row being headers or not
     */
    Boolean DEFAULT_HEADER_ROW = Boolean.TRUE;

    Boolean getFirstRowIsHeader();  
}

--------------------------------------------------------

/**
 * Used to convert values read from CSV into ones compatible with the
 * expected database column type
 */
public interface LoadValueConverter {

    /**
     * @param pathIndicator The file path indicator from which the value originates.
     * <B>Note:</B> the actual type depends on the implementation, it is highly
     * recommended though that the {@code toString()} method yield a useful path value
     * @param lineNumber <P>The line number from which this column value was read.<B>Note(s):</B></BR>
     * <UL>
     *      <LI>
     *      The line numbers start at <B>1</B> - i.e., 1, 2, 3
     *      </LI>
     *
     *      <LI>
     *      The line numbers refer to <U>data</U> lines - i.e., if the CSV file
     *      has a header line it is not counted and the &quot;first&quot; line
     *      is still reported as {@code 1} even though it is physically the 2nd one.
     *      </LI>
     * </UL>
     * @param tableName The table name into which the data is being loaded
     * @param colName The <U>mapped</U> column name - as it appears in the database
     * @param colType The column type
     * @param value The read value
     * @return The equivalent database object to use
     */
    Object convert(Object pathIndicator, long lineNumber, String tableName, String colName, Class<?> colType, String value);
}

----------------------------------------------------------------------------

/**
 * @param <T> Type of entity being managed
 */
public interface EntityIndicator<T> {
    Class<T> getEntity();
}

----------------------------------------------------------------------------

/**
 * @param <S> Persistence storage type - e.g., file, database, etc.
 */
public interface PersisterStorage<S> extends java.nio.channels.Channel {
    /**
     * @return Type of backing storage for the persister - file, database, etc.
     */
    Class<S> getStorageType();

    /**
     * @return The persisted storage instance that is actually used - file, database, etc.
     */
    S getStorageInstance();

    /**
     * @return A <U>snapshot</U> of the original settings used to instantiate this object
     */
    CoreCSVSettings getSettings();

    /**
     * @return The current read/write number <U>excluding</U> the headers line (if any)
     */
    int getCurrentLineNumber();
}

----------------------------------------------------------------------------

/**
 * Represents a read or write entity persister resources
 *
 * @param <T> Type of entity being persisted
 * @param <S> Persistence storage type - e.g., file, database, etc.
 */
public interface EntityPersister<T, S> extends EntityIndicator<T>, PersisterStorage<S> {
    Callable<T> getEntityDescriptor();
}

----------------------------------------------------------------------------


/**
 * Represents a persister resource for reading entities from the persistence storage
 *
 * @param <T> Type of entity being persisted
 * @param <S> Persistence storage type - e.g., file, database, etc.
 */
public interface EntityReader<T, S> extends EntityPersister<T, S> {
    /**
     * Reads an instance from the persistence storage
     *
     * @return The read instance - {@code null} if EOF reached
     * @throws IOException If reflection API related issues encountered
     */
    T read() throws IOException;

    /**
     * Reads all remaining instances
     *
     * @return A {@link Collection} of the read values until EOF is reached
     * @throws IOException If reflection API related issues encountered
     */
    default Collection<T> readAll() throws IOException {
        Collection<T> values = new LinkedList<>();
        for (T v = read(); v != null; v = read()) {
            values.add(v);
        }
        return values;
    }
}


----------------------------------------------------------------------------
Comment 4 Lyor Goldstein CLA 2018-07-11 05:40:20 EDT
As far as providing stubs for the methods - it is irrelevant - you can simply do "return null" - the problem is that they don't compile...
Comment 5 Lyor Goldstein CLA 2018-07-30 06:19:26 EDT
Any progress on this issue ? If there is any more information missing please let me know...
Comment 6 Jay Arthanareeswaran CLA 2018-07-30 09:36:11 EDT
I see the error

"The method createEntityStreamReader(Class<T>, Callable<InputStream>, CoreCSVSettings, Function<? super String,? extends String>, Function<? super String[],? extends T>, Map<String,?>, LoadValueConverter) in the type Snippet is not applicable for the arguments (Class<T>, Callable<InputStream>, CoreCSVSettings, Function<capture#1-of ? super String,capture#2-of ? extends String>, Function<capture#3-of ? super String[],capture#4-of ? extends T>, Map<String,capture#5-of ?>, LoadValueConverter)"

Is this what you see too? And I can confirm Javac doesn't report any error.
Comment 7 Lyor Goldstein CLA 2018-08-02 00:30:03 EDT
Yes, this is exactly the one - see also the initial comment on the issue where this exact error is recorded on this example and also another.
Comment 8 Thomas Naskali CLA 2018-10-17 04:10:08 EDT
The same error can be reproduced with this code (requires guava 18.0 on the classpath)  :

example #1 (error):

public final class Entity implements Comparable {

	private int index = 1;

	public Entity(int index) {
		this.index = index;
	}

	public int compareTo(Object o) {
		return index;
	}

}

import com.google.common.collect.Range;

public class Test {

	public static void main(String[] args) {
		Entity d1 = new Entity(1);
		Entity d2 = new Entity(2);
		Range<Entity> r = Range.closed(d1, d2);
	}

}


This only happens if you don't specify an argument type for the Comparable generic interface in the Entity class.

example #2 (no error):

public final class Entity implements Comparable<Entity> {

	private int index = 1;

	public Entity(int index) {
		this.index = index;
	}

	public int compareTo(Entity o) {
		return index;
	}

}

import com.google.common.collect.Range;

public class Test {

	public static void main(String[] args) {
		Entity d1 = new Entity(1);
		Entity d2 = new Entity(2);
		Range<Entity> r = Range.closed(d1, d2);
	}

}


Interestingly if you declare the Entity class as a static inner class within the main class where it's used, then the error disappears !

example #3 (no error)

public class Test {

	public static void main(String[] args) {
		Entity d1 = new Entity(1);
		Entity d2 = new Entity(2);
		Range<Entity> r = Range.closed(d1, d2);
	}

	public static final class Entity implements Comparable {

		private int index = 1;

		public Entity(int index) {
			this.index = index;
		}

		public int compareTo(Object o) {
			return index;
		}

	}
	
}
Comment 9 Thomas Naskali CLA 2018-10-17 04:25:34 EDT
Any progress on this issue ? We have dozens of similar errors in our enterprise application when upgarding to Photon. It means a no-go for us because the non-generic Comparable interface comes from an external library (joda-time 1.6.2) and we cannot afford to upgrade now. Thank you !
Comment 10 Till Brychcy CLA 2018-10-17 04:31:19 EDT
(In reply to Thomas Naskali from comment #9)
> Any progress on this issue ? We have dozens of similar errors in our
> enterprise application when upgarding to Photon. It means a no-go for us
> because the non-generic Comparable interface comes from an external library
> (joda-time 1.6.2) and we cannot afford to upgrade now. Thank you !

Your issue seems to be caused by mixture of raw types and generics, but the issue from comment#0 not, so please create a separate bug.
Comment 11 Thomas Naskali CLA 2018-10-17 05:00:12 EDT
Thank you for the answer, I just opened bug#540208.
Comment 12 Lyor Goldstein CLA 2019-02-25 03:59:20 EST
FYI still occurs in Eclipse 2018-12. As a veteran supporter of Eclipse and what it represents I would like to mention that IntelliJ IDEA does not have this problem and many of my R&D team members are simply ditching Eclipse because of this type of continued issues (and sadly, I am inclined to follow). Not only due to this issue, but quite a few others related to the fact that perfectly good code needs to be "twisted" just to be able to use it in Eclipse - e.g., 537593, 544757. Not only that, but with each new Eclipse release it seems that more bugs of this type are popping up.
Comment 13 Eclipse Genie CLA 2019-02-25 17:59:30 EST
New Gerrit change created: https://git.eclipse.org/r/137578
Comment 14 Stephan Herrmann CLA 2019-02-25 18:11:21 EST
It still took me a while to reproduce the error, but finally I succeeded to get a self-contained 30-something-lines example. [1]

NB for future bug reports: if not possible to produce a complete single file example (please incl. imports) it's best to attach a configured zip Eclipse project. This greatly reduces risk of a report falling between the cracks.


Once I had the test case it was quite straight-forward, to locate where ecj fails to recognize that

   String[] <: capture-of ? String[]

should evaluate to TRUE.

Strictly speaking this is not a regression, the faulty method has been like this since bug 428019 (5 years ago). It is still possible, that some other fix between then and now increased the chance of hitting this bug.

I'm scheduling this for 4.12 since 4.11 is already cooling down.

Still, if s.o. from the team supports the fix it should be ready, if jenkins approves. The fix is simply done by "borrowing" a corresponding paragraph from method isCompatibleWith().

[1] https://git.eclipse.org/r/#/c/137578/1/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/GenericsRegressionTest_1_8.java
Comment 15 Stephan Herrmann CLA 2019-02-25 19:21:09 EST
(In reply to Stephan Herrmann from comment #14)
> ... if jenkins approves ...

Jenkins gave +1
Comment 16 Stephan Herrmann CLA 2019-02-25 20:06:49 EST
*** Bug 544757 has been marked as a duplicate of this bug. ***
Comment 17 Manoj N Palat CLA 2019-02-26 00:49:11 EST
(In reply to Stephan Herrmann from comment #14)

> I'm scheduling this for 4.12 since 4.11 is already cooling down.
> 
> Still, if s.o. from the team supports the fix it should be ready, if jenkins
> approves.

+1 review

+1 for 4.11 as well
Comment 19 Manoj N Palat CLA 2019-02-26 02:11:22 EST
(In reply to Eclipse Genie from comment #18)
> Gerrit change https://git.eclipse.org/r/137578 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=51935ee9d486419f4f1f74060e0b14409482d023

Thanks Stephan - rebased and committed for RC1
Comment 20 Stephan Herrmann CLA 2019-02-26 06:07:40 EST
(In reply to Manoj Palat from comment #19)
> (In reply to Eclipse Genie from comment #18)
> > Gerrit change https://git.eclipse.org/r/137578 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> > ?id=51935ee9d486419f4f1f74060e0b14409482d023
> 
> Thanks Stephan - rebased and committed for RC1

Thanks Manoj
Comment 21 Jay Arthanareeswaran CLA 2019-02-28 02:23:11 EST
Verified for 4.11 RC1 using build I20190227-1800
Comment 22 Stephan Herrmann CLA 2020-01-30 17:26:13 EST
*** Bug 538126 has been marked as a duplicate of this bug. ***