Community
Participate
Working Groups
A method invocation to a deprecated method currently causes a problem with a range corresponding to the MethodInvocation AST node. This is in contrast to field accesses where the problem only covers the range of the field name. This causes problems if the method invocation has several problems, such as in the following case: /** @deprecated Use Bar1 instead */ public class Bar { /** @deprecated Use bar1 instead */ public void bar() {} /** @deprecated Use field1 instead */ public int field; } class Foo { private final Bar b; Foo(Bar bar) { b = bar; } public void foo() { new Bar().bar(); // Hover over this line b.field(); // Hover over this line } } The method invocation line marked above hides three warnings, which are not easily accessible ie. during quick fix or hovering. In general, problem ranges should only cover the minimum if possible, such as Identifiers or Simple Names (and no "new" keyword as for deprecated constructors. Use Java->Editor->Hovers->Expand vertical ruler icons to quickly see the problem ranges.
I'll look at this.
So you would expect the problem range to cover only: new Bar().bar() ^^^ for the problem related to the deprecated constructor and: new Bar().bar() ^^^ for the problem related to the deprecated method invocation. Am I right? This should then be generalized for all errors/warnings related to method invocation and constructor calls.
Yes, this is exactly what I meant.
Martin, Could this have a side-effect on quick fix?
no, not yet. No tests yet for this feature (working on it). So it would be great if you can change this soon!
I plan to do it today or tomorrow. I will change positions for all method invocations and constructor calls to be consistent.
My changes are ready to be released, but a lot of the quickfix tests are broken. I believe they make assumption on the problem's positions. Martin, How much time do you need to fix this? Philippe, I cannot release this as long as quick fix is not fixed.
These tests seem to be impacted by the fix for this PR: org.eclipse.jdt.ui.tests.quickfix.UnresolvedMethodsQuickFixTest org.eclipse.jdt.ui.tests.quickfix.LocalCorrectionsQuickFixTest org.eclipse.jdt.ui.tests.quickfix.TypeMismatchQuickFixTests org.eclipse.jdt.ui.tests.quickfix.ModifierCorrectionsQuickFixTest org.eclipse.jdt.ui.tests.quickfix.ChangeNonStaticToStaticTest org.eclipse.jdt.ui.tests.quickfix.CleanUpTest I'll run the refactoring tests to check if they are also broken.
Created attachment 36821 [details] Different patches for jdt/core projects Apply on HEAD and you should be able to reproduce the failures. Quickfix should not rely on the problem's positions.
Refactoring tests are not affected by this fix.
Opened bug 133046 to track down the fix for quickfix feature.
Regression test will be org.eclipse.jdt.core.tests.compiler.regression.DeprecatedTest.test012
Created attachment 36841 [details] Simplified patch to apply on the org.eclipse.jdt.core project Use this patch if you apply on HEAD. The previous patch contained other changes that have been released.
I think this patch is doing too much. There shouldn't be any failing tests in quick fixes e.g. on mismatching parameters, or static access warnings. This should all stay the same. It also results in very strange behaviour in cases like public class E { public void foo(Iterator iter) { String str, str2; str= iter.next(); } } See screenshot.
Created attachment 36959 [details] screenshot
I don't see what is strange. import java.util.Iterator; public class X { public void foo(Iterator iter) { String str, str2; str= iter.next(); } } Before the patch, it would underline the whole expression ("iter.next()"). Now it simply underlines "next". This is right. What is weird in this error? Quickfix needs to be changed. The patch is changing all method invocations to reduce the lenght of the underline red line. I won't change simply the deprecated case. I prefer to keep all problems consistent. I put a patch in bug 133046 to fix the quickfix for the failing failures. I don't like to make a change without changing other error report that are also using method invocations. This would make our compiler inconsistent when reporting errors. I can update the title of the bug report if you prefer.
> What is weird in this error? The error is that the _expression_ isn't assigment compatible. So highlighting the expression was exactly the right thing to do. Other examples that I think got worse with the path: - Indirect access warnings: Only the method name is not enough, highlight the access. - raw access: rawList.put(""). The problem is the raw type access, not just the method
I would tend to agree with Martin. My main problem is when method invocation span over multiple lines, like in anonymous type allocations etc...
Actually, any time the problem is truly related to the method invocation, I think highlighting the selector is an improvement. The type mismatch is not specific to a method invocation, as pointed out by Martin; but in case the problem would be a visibility issue, a missing method (see ProblemReporter#invalidMethod/invalidConstructor), then I would expect selector to be highlighted only to reduce clutter. Similar for unchecked method invocation, issues with inference for generic methods.
OK, I'll try to get the changes in this direction. Martin, This might still involved changes in the quick fix.
To be honset, I'm a bit scared of such a change in the last minute. We obviously have good tests for these scenarios, but as there are so many, we also might have missed a case. We really thought of just having the deprecation warning fixed, but do not really want to have such a massive change.
Here is a list of affected methods in ProblemReporter and the corresponding IProblem ids. Let me know if you think about other cases. cannotDireclyInvokeAbstractMethod: IProblem.DirectInvocationOfAbstractMethod errorNoMethodFor: IProblem.NoMessageSendOnArrayType, IProblem.NoMessageSendOnBaseType invalidMethod: IProblem.UndefinedMethod, IProblem.ParameterMismatch, IProblem.NotVisibleMethod, IProblem.AmbiguousMethod, IProblem.InheritedMethodHidesEnclosingName, IProblem.InstanceMethodDuringConstructorInvocation, IProblem.StaticMethodRequested, IProblem.GenericMethodTypeArgumentMismatch, IProblem.NonGenericMethod, IProblem.IncorrectArityForParameterizedMethod, IProblem.ParameterizedMethodArgumentTypeMismatch, IProblem.ParameterizedMethodArgumentTypeMismatch invalidConstructor: IProblem.UndefinedConstructorInDefaultConstructor, IProblem.UndefinedConstructorInImplicitConstructorCall, IProblem.UndefinedConstructor IProblem.NotVisibleConstructorInDefaultConstructor, IProblem.NotVisibleConstructorInImplicitConstructorCall, IProblem.NotVisibleConstructor, IProblem.AmbiguousConstructorInDefaultConstructor, IProblem.AmbiguousConstructorInImplicitConstructorCall, IProblem.AmbiguousConstructor IProblem.GenericConstructorTypeArgumentMismatch, IProblem.NonGenericConstructor, IProblem.IncorrectArityForParameterizedConstructor, IProblem.ParameterizedConstructorArgumentTypeMismatch, IProblem.TypeArgumentsForRawGenericConstructor javadocErrorNoMethodFor: IProblem.JavadocNoMessageSendOnArrayType, IProblem.JavadocNoMessageSendOnBaseType javadocInvalidMethod: Same for invalid method, but for javadoc mustUseAStaticMethod: IProblem.StaticMethodRequested javadocInvalidConstructor: same for invalid constructor but for javadoc noSuchEnclosingInstance: IProblem.EnclosingInstanceInConstructorCall, IProblem.MissingEnclosingInstanceForConstructorCall IProblem.MissingEnclosingInstance, IProblem.IncorrectEnclosingInstanceReference deprecatedMethod: IProblem.UsingDeprecatedConstructor, IProblem.UsingDeprecatedMethod
Close as LATER. Will investigate post 3.2.
As of now 'LATER' and 'REMIND' resolutions are no longer supported. Please reopen this bug if it is still valid for you.
