AIOOBE in ReferenceExpression.sIsMoreSpecific when saving incomplete … #1419
Conversation
|
Thanks for the patch! Could you provide a regression test too? |
| for (int i = 0; i < sParams.length; i++) { | ||
| if (TypeBinding.notEquals(sParams[i], tParams[i])) | ||
| return false; | ||
| if(sParams.length == tParams.length) { |
There was a problem hiding this comment.
Comment withdrawn, as most of this is already discussed in the linked issue #1330.
Still we should probably check this against JLS (entry should be 18.5.4).
There was a problem hiding this comment.
JLS §18.5.4. refers to §15.12.2.5 for this part.
In JLS version 8 (from which this method was first implemented) I found:
(where U1 ... Uk and R1 are the parameter types and return type of the function type of the capture of S, and V1 ... Vk and R2 are the parameter types and return type of the function type of T)
To me this seems to say that also JLS 8 assumed both function types to have the same number of parameters (k).
Interestingly, this parenthesis can no longer be found in JLS 20. But still in version 20, the definitions of P1, ..., Pn and P1', ..., Pn' use the same n.
Looking at the part specific to method reference expressions, JLS 8 defined for all i (1 ≤ i ≤ k), Ui is the same as Vi,. Very interestingly, I cannot find anything like that in JLS 20. If that was an intentional deletion in JLS, then also we should deleted the code comparing parameter types.
For clarity, I'm referring to this:
JLS 8:
If e is an exact method reference expression (§15.13.1), then i) for all i (1 ≤ i ≤ k), Ui is the same as Vi, and ii) one of the following is true: ...
e is an exact method reference expression (§15.13.1), and one of the following is true: ...
There was a problem hiding this comment.
Is removing the code right solution? As it sill works for other versions like 1.8 and I checked the tests are failing.
There was a problem hiding this comment.
Is removing the code right solution? As it sill works for other versions like 1.8 and I checked the tests are failing.
Look at the uses of org.eclipse.jdt.internal.compiler.impl.CompilerOptions.complianceLevel in the compiler. By checking the compliance levels, it is possible to conditionally alter the behavior. Eventually, the behavior should match corresponding javac versions (ideally unless it is accepted to a defective unhelpful behavior) and should come along with a regression test that establishes appropriate behavior at different JDK levels
Do we really need the regression test since we are deleting this part of code? |
Well, to confirm that someone would not break the use case again (by adding same code again for example, or just by any other change), regression test is needed. |
|
I am trying to write a test case to generate this error (pop-window). Has anyone any idea how to capture this pop-up window ( I can't find any similar test in project )
|
This is "too late" for the test, as the fix is in jdt.core, the test should be there too (not in UI layer). |
Take a look at org.eclipse.jdt.core.tests.compiler.regression.LambdaExpressionsTest and |
Without my fix, the error is resolved into this popup and there is nothing in the console. I tried to find a similar example in both the files you mentioned. The AIOOBE is resolved and its shown on a pop-up dialogue box not in console. All I get is
|
The first order business is to write a test even before attempting a fix. JDT/Core tests are run in "headless" mode i.e the UI is not running when the automated tests are running (mostly). There is some mystery here - I haven't read through all the comments here and elsewhere - but the missing semicolon seems necessary to reproduce the problem - Why ?? Did you find out ? Also if I add a regression test in LET with the snippet from above, with or without the missing semicolon I don't see any AIOOB - this needs to be analyzed (debugged) to fully understand the problem landscape. So we do have one interactive way of reproducing the problem - why does the problem vanish when adding a junit ?? Can you study the difference by debugging and comparing ? |
|
(If after trying, you are still not able to make headway, let me know - I can spend some cycles on unblocking you next week) |
Yes I tried to debug both the cases. I am not entirely sure why it is not resolved earlier (before sIsMoreSpecific Method is called. |
|
@srikanth-sankaran I would be surprised if the change in JLS consisted only in deleting this one sentence. Shouldn't we check the vicinity and refresh our implementation from current JLS? |
You need to peel the next layer and investigate why in one case it doesn't consider a particular candidate while in the other it does |
Yes, finding the root cause for the issue is essential to properly solve it. I just wanted to let you know that we (@ShahzaibIbrahim, @fedejeanne, @HeikoKlare) had to push the issue a bit down in our backlog since it seems to require quite some additional effort (at least for us not being so deep into JDT yet). But we did not drop the topic and will proceed work as soon as possible. |
…statement eclipse-jdt#1330 While searching for the correct method binding, the method ReferenceExpression::sIsMoreSpecific() is executed with type bindings for the two accept methods of FileNameFilter and FileFilter. Fix: Added a check before iterating over the arguments for both message to avoid AIOOBE. eclipse-jdt#1330
statement eclipse-jdt#1330 As per findings, JLS 20 removed this iteration as compared to JLS 20, that why removing this specific code block which checks the parameter for choosing the most specific functions (see reference §15.13.1) eclipse-jdt#1330
statement eclipse-jdt#1330 Adding Compatibility Check for JDK 1.8 eclipse-jdt#1330
jukzi
added
needinfo
|
This PR still misses a Junit Test |
|
Thank you for the reminder. Unfortunately, we have many higher priority topics than this, so I do not expect us to come to this in the near (or even not-so-near) future. If I am not mistaken, the currently proposed solution is inappropriate, so we both need a regression test and an improved solution. The issue is still reproducible in the way explained in #1330. There is now another report of the same issue in #3359. |
Using the example from #1330 I cannot reproduce using the batch compiler, so regular compilation is not sufficient. The stacktrace in the issue, however, indicates that the exception was raised while requesting DOM AST. |
…statement #1330
While searching for the correct method binding, the method ReferenceExpression::sIsMoreSpecific() is executed with type bindings for the two accept methods of FileNameFilter and FileFilter.
Fix: Added a check before iterating over the arguments for both message to avoid AIOOBE.
#1330
What it does
How to test
Author checklist