Bugzilla – Full Text Bug Listing
| Summary: | No hover on inner type constructor | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Tobias Widmer <tobias_widmer> | ||||
| Component: | UI | Assignee: | JDT-UI-Inbox <jdt-ui-inbox> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | frederic_fusier, gturski, jerome_lanneluc, markus.kell.r, martinae, philippe_mulet, pwebster, thomas.a.cook | ||||
| Version: | 3.1 | ||||||
| Target Milestone: | 3.3 M4 | ||||||
| Hardware: | PC | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Tobias Widmer
simpler test case:
public class C {
private class Inner {
public Inner(int i) {}
void innerFoo(){}
}
void foo() {
Inner i= new Inner(1);
i.innerFoo();
}
}
Our label provider fails as the number of parameters receieed from the binding key is not the same as the number of parameter names received from the IMethod. bining key: 'Lpack/C$Inner;.(Lpack/C;I)V' Should be only 1 parameter. Thread [Text Viewer Hover Presenter] (Suspended (exception ArrayIndexOutOfBoundsException)) JavaElementLabels.getMethodLabel(IMethod, long, StringBuffer) line: 518 JavaElementLabels.getElementLabel(IJavaElement, long, StringBuffer) line: 386 JavaElementLabels.getElementLabel(IJavaElement, long) line: 363 JavadocHover.getInfoText(IJavaElement) line: 142 JavadocHover.getHoverInfo(IJavaElement[]) line: 118 JavadocHover(AbstractJavaEditorTextHover).getHoverInfo(ITextViewer, IRegion) line: 120 BestMatchHover.getHoverInfo(ITextViewer, IRegion) line: 102 JavaEditorTextHoverProxy.getHoverInfo(ITextViewer, IRegion) line: 69 TextViewerHoverManager$4.run() line: 160 Modified our code to no crash but have an extra check and write to the log > 20050609 This is not a regression compare to RC1. M7 was also broken. *** Bug 99639 has been marked as a duplicate of this bug. *** The binding key reflects resolved info as it would appear in classfile. Therefore, it doesn't (and shouldn't) hide synthetics which are mandated. Also this guarantees independance in between source/binary form. *** Bug 100773 has been marked as a duplicate of this bug. *** This problem apprears in all cases where we have a constructor invocation for non static inner classes. The problem is that the binding key is one for the synthetic construcutor taking TestClass and the signature is one for the method as it appears in source. What are we going to do here: the fact that we get elements from different models here (source versus binray) makes it complicated. At least the binding key should tell us that it is "synthetic" and JDT/Core should provide methods to convert it into a "non" synthetic key. Otherwise we can never use the key to render "source" information in this cases. What are the options for 3.1: - we don't log the error - we don't log the error in cases of a constructor Opinions ? I would vote for not logging the error since we correctly behave in most cases (at least in the case in comment #1, the hover works with RC3, same with case in 100173) *** Bug 123529 has been marked as a duplicate of this bug. *** I've tried with 3.1.2 and was not able to reproduce the problem (comment 0, comment 1 and duplicates) => close as worksforme as WORKSFORME as I cannot put any entry in buildnotes for this bug JDT/UI does not log the error any more (bug 101029 removed the logging in JavaElementLabels (line 543 in rev. 1.39)). The key from IMethod#getKey() still contains the synthetic parameter type when decoded via Signature.getParameterTypes(new BindingKey(key).toSignature()). The effect can be seen in 3.1.2 and in HEAD with the example from comment 1 when hovering over "Inner" in "new Inner(1)": - Expected: "xy.C.Inner.Inner(int i)" - Is: "xy.C.Inner.Inner(C, int)" Our current fallback in JavaElementLabels is to just render the parameter types, but not the parameter names. (In reply to comment #12) > JDT/UI does not log the error any more (bug 101029 removed the logging in > JavaElementLabels (line 543 in rev. 1.39)). > > The key from IMethod#getKey() still contains the synthetic parameter type when > decoded via Signature.getParameterTypes(new BindingKey(key).toSignature()). > > The effect can be seen in 3.1.2 and in HEAD with the example from comment 1 > when hovering over "Inner" in "new Inner(1)": > - Expected: "xy.C.Inner.Inner(int i)" > - Is: "xy.C.Inner.Inner(C, int)" > > Our current fallback in JavaElementLabels is to just render the parameter > types, but not the parameter names. > Not really sure to understand your fall back here... Why, when parameter types and parameter names have different lengthes, don't you use the method.getParameterTypes() instead of Signature.getParameterTypes() result? Then you would have: if (getFlag(flags, M_PARAMETER_NAMES) && method.exists()) { names= method.getParameterNames(); if (types == null) { nParams= names.length; } else if (nParams != names.length) { if (resolvedSig == null) { // nothing can really be done in this case... names = null; } else { // get types from model instead of signature types = method.getParameterTypes(); if (types == null) { nParams = names.length; else if (types.length == names.length) { nParams = types.length; } else { // nothing can really be done in this case... names = null; } } } } As the model is already populated with method.exists() call, there's no real performance side effect with this implementation. This snippet shows that clients can distinguish constructors with synthetic parameters. Then, I think that trying to add information in unique key for this piece of information would pollute it unnecessarily. Markus, what's your mind on this? The problem is that clients can only do the distinction if they have both a binding key and the corresponding IMethod. If they use BindingKey#toSignature() in a different context, they still get the wrong signature. Re our current fallback: We didn't try to find a "smart" workaround -- we just didn't want to be completely screwed up in case this happens. Consider this as a limitation of the unique key: we can't remove the synthetic parameters from the it has they come from the method signature. By the way, the unique key is not supposed to provide all the information of the IMethod. So, unfortunately, if there are cases where clients only have this binding key, then they will not be able to distinguish these synthetic parameters. However, I can't see in any duplicate bugs such a test case... Note that, talking with Jerome, he said that getParameterTypes on IMethod gives less information than those got from binding key, so the comment 13 snippet can be improved => I'll attach a patch... Created attachment 54656 [details]
Proposed patch for JDT/UI
This patch was done on version v20061128-0800 of org.eclipse.jdt.ui project.
Markus, As the attached patch fixes issue described in this bug and considering that unique key has this limitation on synthetic parameters, I move this bug to JDT/UI Thanks Frederic for the patch. I changed our code to do what you suggested. Its clear to me that from a signature it isn't possible to find out anymore what parameters are synthetic. But I think it would make sense to add that to the Javadoc to 'Signature.getParameterTypes' to say that the returned types can also contain synthetic parameter that are not returned from API in the Java model or AST. I guess you could solve the problem in the binding key where the format is under your control and you could offer (new API) BindingKey.getParameterTypes that does not return these types. But I must say that I'm fine if we ignore this issue for now until we detect other problems. The workaround will do fine. moving to jdt.core to decide if they want to offer new API in BindingKey to solve this, or if the current solution is good enough As I said in comment 17, we currently consider this as a limitation of BindingKey and do not plan to add any new API to solve this peculiar issue until there's a strong need on it... I'd rather prefer to assign this bug to JDT/UI as we didn't change any code in JDT/Core to solve this problem. You can then close it as FIXED and add a specific test for it. If you want to keep trace of the BindingKey limitation on this subject, then open a separate bug and I'll close it as WONTFIX. We could then reopen it if someone else complains about this... I was more thinking about the bug title. But you're right, the scenario is fixed now. I updated the title to match the scenario Starting verification... Verified using I20061212-0010. |