Skip to content

Issue with Parameter of type array with generics wildcard vs raw type #415

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

Closed
NicoKiaru opened this issue Dec 22, 2020 · 7 comments
Closed

Comments

@NicoKiaru
Copy link
Contributor

NicoKiaru commented Dec 22, 2020

Hi,

I noticed a strange behaviour with scijava parameters. It looks like the handling of array parameters are very different if they contain a generics wildcard or not:

Let's imagine a class:

public class MyClass<T> {

}

And a scijava converter from String to MyClass<?>[] (not sure the wildcard changes anything if present in the converter)

This:

@Parameter
MyClass[] myobjects;

behaves correctly with regards to the scijava mechanism. The correct converter is called.

However this:

@Parameter
MyClass<?>[] myobjects;

completely fails : in the test case I have, the scijava mechanism attempts to put a File[] object into myobjects, obviously failing.

I narrowed down the issue to the CommandModuleItem class. The line

final Class<?> type = Types.raw(Types.fieldType(field, getDelegateClass()));
returns the type of the parameter. In case the Parameter has no generics wildcard, then the returned type is [L MyClass, while if it has a generics widlcard, this same line returns [L Object, wrongfully allowing irrelevant converters (thus the one from String to File array).

I have not made a minimal example of this issue, but one test in bigdataviewer-playground can be used to reproduce the issue : https://github.com/bigdataviewer/bigdataviewer-playground/blob/fixes-rawtype/src/test/src/sc/fiji/bdvpg/scijava/ScijavaShowDemo.java.

The demo fails. But if you remove the wildcard in the line https://github.com/bigdataviewer/bigdataviewer-playground/blob/daab0a336831b89c44d13932c243dd41e493dbe5/src/main/java/sc/fiji/bdvpg/scijava/command/bdv/BdvSourcesAdderCommand.java#L49:

SourceAndConverter[] sacs;

Then everything works.

How can this be solved ? I'm also open to suggestions on my code. I tend to fear that modifying scijava common is risky. So if there's a solution on bigdataviewer-playground side I would rather do it. I can think of 3 options:

  • keeping raws type in the parameters : this creates annoying issues when converting to List + we are not supposed to use raw types in Java as far as I understood
  • making a container class : SourceAndConverterList which would hold a single field List<SourceAndConverter>. This seems the most easy thing to do a provides a lot of control on the scijava side - but it's a bit inelegant
  • managing to make a working List<SourceAndConverter<?>> scijava parameter, but I have no idea if this is possible. To me, because the generic types are erased at runtime, this looks impossible, but is it really ?

Here's some maybe related issues:

#172

#118

@frauzufall
Copy link
Member

Hi @NicoKiaru, I don't really have an answer, but I tried to make a minimal example. It prints the field type and the output of Types.raw() for arrays and lists, typed and raw, of class fields. The output for lists looks fine so I guess the issue with the typed List parameters is somewhere else, maybe you can describe more precisely what's going wrong with typed List parameters.
Regarding arrays - not sure if they are supposed to work as parameters. @ctrueden @imagejan

import org.scijava.util.Types;
import java.lang.reflect.Type;
import java.util.List;

public class FieldTest {

	public static void main(String...args) throws NoSuchFieldException {
		printFieldTypes("listTyped");
		printFieldTypes("listRaw");
		printFieldTypes("arrayTyped");
		printFieldTypes("arrayRaw");
	}

	private static void printFieldTypes(String varName) throws NoSuchFieldException {
		Type type = Types.fieldType(MyCommand.class.getField(varName), MyCommand.class);
		System.out.println(varName + ": " + type + " " + Types.raw(type));
	}
 
	public class MyClass<T> {}
	public class MyCommand {
		public List<MyClass<?>> listTyped;
		public List<MyClass> listRaw;
		public MyClass<?>[] arrayTyped;
		public MyClass[] arrayRaw;
	}

}

This is the output:

listTyped: java.util.List<FieldTest.MyClass<?>> interface java.util.List
listRaw: java.util.List<FieldTest.MyClass<T>> interface java.util.List
arrayTyped: FieldTest.MyClass<?>[] class [Ljava.lang.Object;
arrayRaw: class [LFieldTest$MyClass; class [LFieldTest$MyClass;

@ctrueden
Copy link
Member

ctrueden commented Jan 2, 2021

I would prefer to fix this on the SJC side, but I won't have time to work on it for at least a few weeks. But I appreciate the detailed bug report and analysis, @NicoKiaru! Thank you. And thank you @frauzufall for attempting to create an MCVE!

@NicoKiaru
Copy link
Contributor Author

I've added a (currently failing) test which reproduces the error in the PR #416. I'm not sure it can be more minimal. Also I noticed that I can overcome this issue by assigning a higher priority to the converter I'd like to use, but still, scijava should not try to put a File array into another sort of array, so it's still worth investigating.

@imagejan
Copy link
Member

imagejan commented Jan 5, 2021

I tried to add a really minimal test here: #417

It seems the issue is in the private implementation of GenericTypeReflector (forked from GenTyRef), I wonder whether it might have been fixed upstream (or in GeAnTyRef?) already?

@NicoKiaru
Copy link
Contributor Author

NicoKiaru commented Jan 5, 2021

There are some weird things happening with arrays and generics. I can't find it again, but there was an explanation of the reason why an object of class MyClass<?>[] had to be considered as Object[] at runtime. This link may be a starting point https://www.oreilly.com/library/view/learning-java-4th/9781449372477/ch08s10.html, as well as the article mentioned by Curtis in gitter : https://www.ibm.com/developerworks/java/library/j-jtp01255/index.html

imagejan added a commit that referenced this issue Jan 5, 2021
Also add minimal test for array types with generics.

See #415
@imagejan
Copy link
Member

imagejan commented Jan 5, 2021

I improved the test and updated #417 with a fix to Types.raw().

That fix also makes your test pass, so I rebased #416 onto my PR to see if it passes on Travis as well.

imagejan added a commit that referenced this issue Jan 5, 2021
Also add minimal test for array types with generics.

See #415
hinerm pushed a commit that referenced this issue Jan 19, 2021
Also add minimal test for array types with generics.

See #415
@hinerm
Copy link
Member

hinerm commented Jan 19, 2021

Closed in f6d8749

@hinerm hinerm closed this as completed Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants