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

Runtime exception: Structural identity doesn't work for method references #64

Closed
marksto opened this issue Apr 12, 2019 · 8 comments

Comments

Projects
None yet
2 participants
@marksto
Copy link

commented Apr 12, 2019

Describe the bug
items.stream().map(item -> item.getId()).forEach(System.out::println) βœ…
items.stream().map(Item::getId).forEach(System.out::println); πŸ†˜

To Reproduce
Test project: https://github.com/marksto/manifold-cast-exception
mvn clean install

Expected behavior
Structural identity works for method references in Java 8+.

Desktop (please complete the following information):

  • OS Type & Version: "mac os x", version: "10.14.3", arch: "x86_64", family: "mac"
  • Java/JDK version: Java version: 1.8.0_172, vendor: Oracle Corporation
  • IntelliJ IDEA version: 2018.1.2
  • Manifold version: 0.59-alpha
  • Manifold IntelliJ plugin version: 0.59-alpha

Stack trace

Exception in thread "main" java.lang.ClassCastException: manifold.ext.DataBindings cannot be cast to abc.schema.Item
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1382)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
	at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418)
	at abc.RunMe.loadItemsArray_PlainInterface(RunMe.java:39)
	at abc.RunMe.main(RunMe.java:22)
@marksto

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

BTW, my final goal was to do the following simple typed caching...

List<Product> products = IJsonList.<Product>load().fromJson(jsonStr);
productsCache.putAll(products.stream().collect(toMap(Product::getId, identity())));

... and then something even more fantastic:

Map<String, Product> productsMap = productsCache.getAllPresent(productsKeys);
return productsMap.values().stream()
        .collect(groupingBy(product -> product.getCategory().getCode()));

The second fails with ClassCastException as well. No rest for the wicked, huh?

Much 😞 if this is a "no fix" type of things.
For me, it would mean that it's time to come back home to my wife, children, and Clojure.

@rsmckinney

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Ah, nice one. Method references are difficult to rewrite in the Java compiler, but it's possible. In the meantime you can simply use a lambda in place of method references e.g. replace Item::getId with item -> item.getId(). Not so bad, is it?

I personally don't like method refs because in my view they are difficult to read -- the small amount of space they save is typically significant in terms of type information. Perhaps my brain's Java parser isn't as efficient as most.

@rsmckinney rsmckinney added the bug label Apr 12, 2019

@rsmckinney

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

What I can do in the meantime is add a compiler error so this type of code can be easily discovered and changed to lambda format. I'll add this error to both the javac plugin and the IJ plugin.

Thanks for reporting this!

You can use IJ's quick fix feature to easily change a method ref to a lambda expression:

image

@marksto

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

In the meantime you can simply use a lambda in place of method references

Yep, that was my first shot.

But then I'd like to do this kind of stuff:

Map<String, Product> productsMap = productsCache.getAllPresent(productsKeys);
return productsMap.values().stream()
        .collect(groupingBy(product -> product.getCategory().getCode()));

And it seems completely impossible to achieve this with properties of "type": "object".
I get the following error even for the same replacement trick.
debug-err

@rsmckinney

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Yeah... the expression evaluator in IJ's debugger doesn't yet work with Manifold. Basically when you eval an expression while debugging it's not actually compiled and executed, it's locally interpreted in terms of Java Debug Interface (JDI) mirrors. This is why conditional breakpoints slow down execution so much, this is also why expressions involving Manifold do not work -- the javac compiler is not involved.

@marksto

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

Ooo-key... I managed to figure out this inconsistency between debugger interpretation and the code being executed too (regardless the error in evaluator the app itself was running fine), and in the end this last ClassCastException turned out to be an invalid JSON Schema issue. πŸ€¦β€β™‚οΈ

Product was parsed with:

{
  ...
  "category": "string-value",
  ...
}

while the schema used for loading had:

{
  ...
  "category": {
    "type": "object",
    ...
  }
  ...
}

I've covered this scenario with a separate test case in updated test project, but still was not able to reproduce the exact same exception (in tests it fails with manifold.internal.javac.JavaCompileIssuesException: Error compiling Java class: abc.schema.java_lang_String_structuralproxy_abc_schema_Category).

rsmckinney added a commit that referenced this issue May 4, 2019

#64
- provide a compile-time error indicating a method reference is not supported on a structural interface method, instead a lambda expression must be used
@rsmckinney

This comment has been minimized.

Copy link
Member

commented May 8, 2019

In progress

@rsmckinney

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Fix available in release 0.65-alpha

@rsmckinney rsmckinney closed this May 8, 2019

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.