Bugzilla – Full Text Bug Listing
| Summary: | [inline] returned value is not cast | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Deepak Azad <deepakazad> | ||||||
| Component: | UI | Assignee: | Deepak Azad <deepakazad> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | markus.kell.r | ||||||
| Version: | 3.7 | ||||||||
| Target Milestone: | 3.7 M6 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
The following also does not work
---------------------------------------------------------------------
package p;
class A {
void x() {
int a= foo().intValue();
System.out.println(foo().intValue());
}
private Integer foo() {
return 1 + 1;
}
}
---------------------------------------------------------------------
The code is not very smart..see CallInliner.needsExplicitCast(RefactoringStatus)
> The code is not very smart..see
> CallInliner.needsExplicitCast(RefactoringStatus)
Yes, it should use ASTNodes#getExplicitCast(Expression, Expression).
(In reply to comment #2) > Yes, it should use ASTNodes#getExplicitCast(Expression, Expression). Yup, but even this is not perfect. See bug 337815. > > Yes, it should use ASTNodes#getExplicitCast(Expression, Expression).
> Yup, but even this is not perfect. See bug 337815.
Nobody's perfect, but a least it's correct ;-)
[BTW: Unfortunately, the verb "cast" is irregular and the past is also "cast"]
Created attachment 189588 [details] fix+tests Initially I just used ASTNodes#getExplicitCast(Expression, Expression) in CallInliner.needsExplicitCast(RefactoringStatus). This caused InlineMethodTests.testParameterizedType1() and InlineMethodTests.testParameterizedType2() to fail. My change resulted in an unnecessary cast in these cases. The problem is in Bindings.isSuperType(ITypeBinding, ITypeBinding, boolean), which does not really handle type arguments. My changes in Bindings.isSuperType(..) fix the failures in testParameterizedType1() and testParameterizedType2() and ALLJDTTests suite is green with this. But I am not sure if the changes are perfect, and also if the failures were legitimate, something like bug 337815 where we need the cast in some cases. Bummer! We've opened Pandora's box.
InlineMethodTests.testParameterizedType1() can easily be translated into an equivalently failing test for Inline Local Variable:
Vector<? extends Number> inline= new Vector<Integer>();
Vector<? extends Number> var= inline;
Let me think a bit more on how to best proceed, but the changes in Bindings#isSuperType() are not OK.
This isn't about supertypes in the classical sense any more. I think we shouldn't touch the legacy isSuperType(.., .., true) any more and let it die (i.e. replace references whenever we need to fix something). These changes only make sense for TypeRules#canAssign(..), so the fixes should also go there.
This area is very complicated since generics cam into play, so any new code should be backed by references to the JLS3 that tell why the code is correct.
E.g. I don't buy the recursive isSuperType(..) calls on the type arguments (because e.g. a List<Integer> is not assignable to a List<Number>).
The changes in CallInliner are mostly fine. Only "parent.getNodeType() == ASTNode.METHOD_INVOCATION" is redundant here and should be removed:
ASTNode parent= fTargetNode.getParent();
if (parent.getNodeType() == ASTNode.METHOD_INVOCATION &&
fTargetNode.getLocationInParent() == MethodInvocation.ARGUMENTS_PROPERTY) {
I've fixed the problems in TypeRules#canAssign(..) with bug 338271. You can release your patch with the fix in CallInliner but without releasing Bindings. Created attachment 189875 [details] final fix + tests (In reply to comment #7) > I've fixed the problems in TypeRules#canAssign(..) with bug 338271. Thanks Markus! > You can release your patch with the fix in CallInliner but without releasing > Bindings. Done, with few additional tests. Fixed in HEAD. |
HEAD -Inline foo() - Expected: long much = (long) (1+1) * Integer.MAX_VALUE; - Actual: long much = (1+1) * Integer.MAX_VALUE; ------------------------------------------------------------------ package p; class A { void x() { long much = foo() * Integer.MAX_VALUE; } private long foo() { return 1 + 1; } } ------------------------------------------------------------------ See also InlineTempTests#test36, InlineConstantTests#test32.