Community
Participate
Working Groups
Implement javax.swing.Scrollable and do not use, e.g., the parameter 'direction'. You cannot avoid that warning (except disabling it) when you don't need the parameter. => The parameters should only be marked as redundant, if it is not used in any implementation AND the method declaration is editable within your projects (aka workspace).
This is only the case when the option 'Check overriding implementing or overriding method' is checked on Java -> Compiler -> Error/Warnings : Unnecesary code - Parameter is never read. Please reopen if thats not the case.
You are right, I have this option selected, because its text suggests to be selected. To be honest, it is unclear for me to tell the difference between selected and unselected option "Check implementing or overriding method". I would expect the "Parameter is never read" option to check automatically all necessary conditions, so false positives are impossible.
reopen
I also find the option 'Check overriding implementing or overriding method' unnecessary. Moving to jdt.core. Suggest to deprecate the option and ignore checking parameters from overridden methods by default.
The option is about 'overriding' not being 'overriden'. It means checking hierarchy up, not down. Checking down would be too expensive as subtype hierarchies are expensive to compute. I will keep the option as it is, you may remove it from the UI if you don't want it. Note by default the option matches your expectation. I suspect the issue really lies in the wording of the option in UI, which was misleading to the user.
(In reply to comment #5) > It means checking hierarchy up, not down. Checking down would be > too expensive as subtype hierarchies are expensive to compute. Do I understand you right, that in the following example: public class A { public void doSomething(Foo foo) { } } public class B extends A { public void doSomething(Foo foo) throws IOException { System.out.println(foo); } } in the method A.doSomething() the parameter 'foo' is marked as warning? If so, this makes the option completely useless and should be removed at all until the subclasses are checked as well.
False positives are really a problem and are the reason why we in our project disabled this option. What about limiting the analysis to constructors, and static, private or final methods? (or methods in local types)
moving to jdt.core to consider comment 7.
I was going to file a duplicate of this. FWIW, I agree ‘parameter never read’ should absolutely not warn on parameters to stub methods, overrides, or interface implementations. But I'd go further and say that it shouldn't warn on non-private parameters at all, since these may, without exception, be APIs, and removing the parameter would be a breaking change. Might I suggest wording the checkbox as ‘skip overrides’ or ‘suppress for overrides’? I think this less implies traversal of the caller graph. No particular need to also mention interface implementations. Of course, I'd prefer the proper default, without the potential for having the checkbox set wrong. And now I editorialize. I just imported a big third-party project into my workspace where I have aggressive warnings-as-errors settings configured for work, and it was a catastrophe. I'm submitting "quiesce warnings" patches. Very bad. Eclipse raises lots of false warnings in comparison to VC#; the comparison is frustrating moving between the two. I never had to suppress a csc compiler warning in order to both compile cleanly and satisfy binary compatibility. Moreover, I have never had to suppress a csc warning at all! Turn on all warnings as errors and try to compile this stub: public class MyComponent { private MyService _service; public void injectService(MyService service) { _service = service; } public void doSomethingWithService() { // TODO Unimplemented } } Either the API or the warning loses; not ideal. It might be worth considering this policy: The compiler should never complain about situations which can only be resolved with breaking changes. Such a policy may somewhat reduce the utility of certain warnings—in this case, the only consistent option is to restrict the warning to parameters of private methods. But a 20% infuriating warning has 0 utility, since it's turned off. 'Parameter is never read' and 'declared exception is not thrown' are the worst offenders. Few false positives also reduces the need for granular warning configuration with confusing wording that needs to be corrected. :) Thus ends my editorial for the day…
(In reply to comment #9) > I was going to file a duplicate of this. FWIW, I agree ‘parameter never read’ > should absolutely not warn on parameters to stub methods, overrides, or > interface implementations. But I'd go further and say that it shouldn't warn on > non-private parameters at all, since these may, without exception, be APIs, and > removing the parameter would be a breaking change. NO! If I have a self-contained application, I want to find all redundant parameters as well. If one is developing libraries, then switching off this option or adding another sub-option "check only private and package protected members" would be the best choice. But make it work also for public and protected members.
I don't think adding extra suboptions is going to address the real problem, that it doesn't recognize subhierarchies (being overriden). Scoping is only a workaround. What about rather documenting the code ? Or using an approach like C++ does ? Unused parameters are simply named '_'. Java doesn't allow this, but we can maybe come up with a way to tag a parameter as being unused but still legite. Annotations are no go, since they mandate Java5. Could we for instance say that if documented (javadoc), then no complaint is emitted by the compiler, since this looks like an API ?
Re: comment 7 > False positives are really a problem and are the reason why we in our project > disabled this option. This options is OFF by default. There is no need to disable it. I agree that with its current limitations, it should remain OFF (we do this as well).
If we can enable this check for constructors, privates, static and final methods, we already get some valuable results and detect bugs in our code. I don't see why you call this a workaround: Similar example is 'unused fields/methods'. There we limit ourselves also to private elements as we can't do full project dependency analysis. The option is still very valuable.
Just curious: Could someone with knowledge of the JDT internals please explain, why implementing the search in subclasses for public/protected methods is hard or not possible. Maybe it is an expensive operation?
(In reply to comment #14) > Just curious: Could someone with knowledge of the JDT internals please explain, > why implementing the search in subclasses for public/protected methods is hard > or not possible. Maybe it is an expensive operation? > It is due to be. While searching supertypes is something JDT must do anyway, searching subtypes is not. Hence JDT would do that for the sole benefit of the said warning. Moreover, there is no direct link from supertypes to subtypes. The search would then need to check all classes of all open projects for subtypes of the type being analyzed, or a specific index would have to be maintained. The net result is that the operation would be expensive. On a theoretical standpoint, the result would be deceptive as well: except for private methods, a third party could still use the said parameters, without Eclipse being able to be aware of that. I guess that this is the high cost/medium benefits ratio that governed the current choices. Philippe, more comments?
OK, then I would vote for applying it only to static methods, constructors and private or final instance methods.
Sounds like an improvement over the current state of affairs (which renders the warning useless), but I would caution that 'private' is the discriminator. A static or final API is just as constrained by binary compatibility as is a method override/implementation.
The suggestion from comment 11 ('Don't warn if parameter is in spec'ed in Javadoc') could be an option too but this still hides bugs when users by mistake didn't use a parameter, but should have. (I just recently discovered such a bug in my code where a constructor parameter wasn't assigned to the field)
The suggestion from comment 11 would look like that in the UI: Parameter is never read x Ignore overriding and implementing methods x Ignore parameters documented in Javadoc As I said in comment 18, this would be a good option and already help us. Private methods are normally not spec'ed in Javadoc so we have the most benefit there. There's still the possible that, being a good programmer, all constructors and static methods are fully Javadoc'ed but you forgot to use the passed argument. The UI for the full fledged proposal would be: Parameter is never read x Ignore overriding and implementing methods x Ignore overridable methods x Ignore parameters documented in Javadoc Both suggestions are superior to what we have now so I'm also perfectly fine with the first solution. It could be that for compatibility reasons it's not possible to turn 'Check' into 'Ignore': The solution would then be: Parameter is never read x Check overriding and implementing methods x Check overridable methods x Check parameters documented in Javadoc
Re: 'Check-->Ignore'. This is only a UI presentation issue I think. The option would remain as it is today (i.e. you simply reverse the logic in the UI). Maybe 'Check-->Include' is a smaller step, and doesn't change the logic. Re: 'x Ignore overridable methods'. Are you thinking of methods overriden somewhere (could be in 3rd party code) ? If so, this is definitely out of reach.
Re: 'x Check parameters documented in Javadoc' Do we need an option for real here ? If Javadoc support is enabled, shouldn't it be implicit ? Like it is for search for instance...
If the current option "Check overriding implementing or overriding method" does not work 100%, remove it. Because something that creates bogus does more harm than is useful.
re: "Check overriding implementing or overriding method" ... remove it It is still useful and should remain as such. It is common practice to inherit Javadoc when overriding, so it would be useful to complement the approach using Javadoc.
Created attachment 63604 [details] Proposed patch This patch requires API change (pref addition)
> Re: 'x Ignore overridable methods'. > Are you thinking of methods overriden somewhere (could be in 3rd party code) ? > If so, this is definitely out of reach. No 'overridable' means all methods that can potentially be overridden (non-private, non-final, non-static and not a constructor). The other thing would be 'Ignore overridden methods' what of course is out of reach.
Created attachment 63607 [details] Better patch Added regression tests and mentionned need to enable doc comment support in JavaCore pref addition documentation.
Created attachment 63623 [details] Better patch2 Tuned doc for new option, and tuned one existing test.
(In reply to comment #23) > re: "Check overriding implementing or overriding method" ... remove it > It is still useful and should remain as such. As said before: if it produces false-positives, it does more harm than it is useful. One should be able to rely on the results. Better have no warning/error in this case, than a wrong one.
Thomas, the overriding suboption is reducing the false hits. Removing it would simply increase the number of false hits. To be clear, this option means that when you override an existing method, you may be in a situation where the unused parameter is part of a mandated API, hence the option. There are some situations where some users are asking to still see these cases. By default, we don't show these. Removing the suboption is simply going to cause more grief than today.
Created attachment 63752 [details] Better patch3 Minor improvement. Changed the option name to be more explicit: COMPILER_PB_UNUSED_PARAMETER_INCLUDE_DOC_COMMENT_REFERENCE * COMPILER / Consider Reference in Doc Comment for Unused Parameter Check * When enabled, the compiler will consider doc comment references to parameters (i.e. @param clauses) for the unused * parameter check. Thus, documented parameters will be considered as mandated as per doc contract. * The severity of the unused parameter problem is controlled with option "org.eclipse.jdt.core.compiler.problem.unusedParameter". * Note: this option has no effect until the doc comment support is enabled according to the * option "org.eclipse.jdt.core.compiler.doc.comment.support". * - option id: "org.eclipse.jdt.core.compiler.problem.unusedParameterIncludeDocReference" * - possible values: { "enabled", "disabled" } * - default: "enabled"
Released for 3.3M7 (obtained PMC approval).
I tested the new option and it is good. But we got a test failure of a 'unused parameter removal quick fix' test, where the test wanted to test that also the corresponding Javadoc @param comment is removed when removing a parameter. To play it save and to stay 100% compatible, the new option should be disabled by default. This bug should also get the milestone set.
Our rule for suboptions of disabled options (like the master pref for unused parameter) is to align them as we'd recommend them to be when the master option gets enabled. This is why it got enabled by default. Which is backward compatible with the defaults.
Verified for 3.3M7 with I20070427-0010
*** Bug 60370 has been marked as a duplicate of this bug. ***
*** Bug 67068 has been marked as a duplicate of this bug. ***
*** Bug 157162 has been marked as a duplicate of this bug. ***
