Bug 265142

Summary:

Compiler fails to warn on unused constructors of private classes.

Product:

[Eclipse Project] JDT

Reporter:

Srikanth Sankaran <srikanth_sankaran>

Component:

Core

Assignee:

Srikanth Sankaran <srikanth_sankaran>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

kent_johnson, philippe_mulet

Version:

3.5

Target Milestone:

3.5 M6

Hardware:

OS:

Windows XP

Whiteboard:

Attachments:

Description	Flags
Proposed patch + tests	none
Patch with review comments incorporated	kent_johnson: iplog+

Description Srikanth Sankaran

2009-02-17 07:53:10 EST

Build ID: I20090217-0800

Steps To Reproduce:
Basically the fix for bug#264991 suppressed the emission
of unused constructor message for unused constructors of
private classes. These diagnostics were introduced by the
fix for bug#201912. These need to be reintroduced in the
appropriate manner.

Test case: Look at org.eclipse.jdt.core.tests.compiler.regression.ProblemConstructorTest.test004()

We should warn about the constructor of X.M#M(int) being unused. We don't.

More information:

Comment 1 Srikanth Sankaran

2009-02-18 07:34:23 EST

Created attachment 126014 [details]
Proposed patch + tests

Kent, can you please review this patch and help take it to commit. Thanks.

Comment 2 Kent Johnson

2009-02-18 11:09:58 EST

In ConstructorDeclaration.analyseCode(), you're finding the super constructor by asking the scope.

MethodBinding superCtor = this.scope.getConstructor(superClass,
  Binding.NO_PARAMETERS, this.constructorCall);
if (superCtor == null || !superCtor.isValidBinding())
	break checkUnused;

Why not just ask the superclass directly?

if (superClass.getExactConstructor(Binding.NO_PARAMETERS) == null)
	break checkUnused;

Comment 3 Kent Johnson

2009-02-18 11:19:17 EST

Ok - so I now see why - we need to check visibility. That can still be done explicitly vs. the scope call which has side affects.


I have another question, if a private type has only 1 constructor - why would we ever complain that it is unused ? Does it make sense that the user remove the type's only constructor ?

Comment 4 Srikanth Sankaran

2009-02-19 06:09:13 EST

Created attachment 126152 [details]
Patch with review comments incorporated

Comment 5 Srikanth Sankaran

2009-02-19 06:22:20 EST

(In reply to comment #3)
> Ok - so I now see why - we need to check visibility. That can still be done
> explicitly vs. the scope call which has side affects.

    I don't think this call has any adverse side effects (I picked up that
usage from ExplicitConstructorCall.resolve), but I agree it is needlessly heavyweight for the current needs. The patch posted now addresses this
concern.

> I have another question, if a private type has only 1 constructor - why would
> we ever complain that it is unused ? Does it make sense that the user remove
> the type's only constructor ?

    As long as an element is not explicitly used and (cannot be used in future
in unknown ways (by virtue of being private)) it should be ok to warn I think.

    The litmus test should really be whether each warning is "actionable on
an independant basis" and whether the action of deleting the seemingly unused
item causes any ill effects (such as program build failure or run time
behavior difference)

    Am I missing something ??

Comment 6 Kent Johnson

2009-02-19 11:39:00 EST

We call scope methods to resolve explicit references in the AST. When you're checking to see if an inherited method exists, we usually just go to the bindings themselves since we do care to report problems, record references, etc.

"Am I missing something ??"

No - I was just wondering since some users define a 'default' constructor for no reason, so warning that it is unused seems unneccessary.



BTW - The comments are backwards for ProblemConstructorTest test008 & test009

Comment 7 Kent Johnson

2009-02-19 13:54:39 EST

Fix and test released for 3.5M6

Comment 8 Srikanth Sankaran

2009-02-19 23:08:40 EST

(In reply to comment #6)

> BTW - The comments are backwards for ProblemConstructorTest test008 & test009

Thanks Kent!

Comment 9 Frederic Fusier

2009-03-10 10:05:09 EDT

Verified for 3.5M6 using I20090310-0100.