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

SpEL: Compiled OpNE should favor equals() to != [SPR-14863] #19429

Closed
spring-projects-issues opened this issue Oct 31, 2016 · 2 comments
Closed

SpEL: Compiled OpNE should favor equals() to != [SPR-14863] #19429

spring-projects-issues opened this issue Oct 31, 2016 · 2 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Oct 31, 2016

Denis Zhdanov opened SPR-14863 and commented

There is a problem with != SpEL in compiled form. It compares strings using reference identity instead of equals().

I.e. the following byte code is generated from the following expression data['my-key'] != 'my-value' (note if_acmpeq instruction usage)

invokevirtual org/denis/spring/spel/Test$MyContext/getData()Ljava/util/Map;
checkcast java/util/Map
ldc "my-key"
invokeinterface java/util/Map/get(Ljava/lang/Object;)Ljava/lang/Object; 2
ldc "my-value"
if_acmpeq 11
iconst_1
goto 12
iconst_0
invokestatic java/lang/Boolean/valueOf(Z)Ljava/lang/Boolean;
areturn

A complete standalone project which illustrates the problem can be found here

I checked spring source code and the problem is in the OpNE class.

There was a similar report for the OpEQ class (#13832) and it was fixed - OpEQ


Affects: 4.3.3

Issue Links:

  • #13832 SpEL: OpEQ should use equals()
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 1, 2016

Andy Clement commented

Thanks for the testcase, very useful.

The old referenced issue #13832 does talk about the == to equals() switch but not in the context of compilation.

I've used this bug as a chance to revisit compilation across both OpNE and OpEQ. Both of these delegate to the equalityCheck() method in Operator for the interpreted case. That method is non trivial and coping with all the nuances in bytecode generation would be tedious and hard to maintain. So to fix the issue here, rather than increase the complexity of the bytecode generation I drastically simplified it. I threw away all the special case handling and have modified it so the generated compiled code delegates to the same equalityCheck. This ensures the behaviour in the compiler and non-compiled case should always match. In this case there is no need to be clever in compiling this operator because it typically isn't the place that benefits from SpEL compilation. It is the expression components that lead to reflective invocation that need special handling for compilation. I also took the chance to simplify OpEQ in the same way. Testcases have been added to cover representative cases and all the existing tests pass.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 2, 2016

Denis Zhdanov commented

Hi Andy Clement,

Cool, many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants