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

Cannot close over local variable #924

Closed
wants to merge 3 commits into from

Conversation

atorstling
Copy link
Contributor

This is a reproducing test and suggested fix for issue #916. The problem turned out to be that the information read out of the ConstantPool didn't actually represent the type signature of the lambda class when we had closed over local variables.

I also saw issue #912 which is related to that the constant pool interface isn't uniform or stable, so I decided to try to avoid it altogether.

So instead of trying to deduce the type signature of the body class, I look at the concrete types of the "accept" method parameters. I'm not sure if this is equivalent to the old method and if there are some cases which it doesn't cover, but at least all tests are passing in the project.

This fix also leads to the TypeIntrospector being unused, but I didn't remove it in the pull request to avoid cluttering.

@atorstling
Copy link
Contributor Author

This is also related to #914 - that fix tries to solve a related problem, if not the same.

Given("^this data table:$", (DataTable peopleTable) -> {
List<Person> people = peopleTable.asList(Person.class);
assertEquals("Hellesøy", people.get(0).last);
});
Integer alreadyHadThisManyCukes = 1;
Given("^I have 42 cukes in my belly$", () -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like your change breaks arguments. Try changing this line to the following to reproduce:

Given("^I have (\\d+) cukes in my belly$", (Integer n) -> {

Any thoughts about how to deal with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I didn't expect it to break that easily. What happens? I can have a look in a couple of days, but not right now unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda parameters are reported as java.lang.Object, so the type conversion breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh well, so much for that technique then ;) If its not caused by the interface methods being declared accepting Object, I have no immediate ideas for workarounds. Maybe merge the other fix and I'll see if there is a way around it at nearest occation. Should have tested it better :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's funny though - the documentation for getGenericParameterTypes says "If a formal parameter type is a parameterized type, the Type object returned for it must accurately reflect the actual type parameters used in the source code.".

@atorstling
Copy link
Contributor Author

I found that https://github.com/jhalterman/typetools/blob/master/src/main/java/net/jodah/typetools/TypeResolver.java also uses the constant pool to resolve lambda arguments, so maybe your old method is the only viable. http://stackoverflow.com/questions/21887358/reflection-type-inference-on-java-8-lambdas certainly seems to implicate that. Perhaps using typetools is a good idea, so that support for new JDKs can be added there?

In either case, dcowboys fix seems to be the right one (it's the same method as used by TypeResolver).

@atorstling atorstling closed this Oct 30, 2015
@atorstling
Copy link
Contributor Author

See #929 for a new attempt

brasmusson added a commit that referenced this pull request Jul 14, 2016
brasmusson added a commit that referenced this pull request Jul 31, 2016
@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants