Community
Participate
Working Groups
As discussed in PR 43276 searching for declaration of a default constructor should report no matches, which is the case. If I search for all occurrences it seems that declarations are reported as well. Consider the following example: package p; class A{ } class B extends A { } Searching for p.A() limited to "All occurrences" reports a match on class A and class B which is not correct. - A neither declares a default constuctor nor does it reference one - B only implicit reference constructor A() through its implicit constructor. Folloing the arguments in PR 43276 this shouldn't reported either. This results in a lot of work arounds in refactoring. Can you please comment on this semantic.
To be consistent, I agree we should not find them (or find them as well when searching for sole declarations).
I suspect the constructor locator gets fooled in all-occurrences mode due to the fact it is considering types & constructors for reference search, but then matches them as declarations as well where it shouldn't for default constructors.
If we find them all refactoring somehow needs an indication that they don't exist in source (e.g the are syntetic). Otherwise we have to build an AST again to check if the match has a representation in source.
This only occurs when the 2 types are in the same compilation unit since we keep track of possible matches per unit. Fixed so that references & all occurrences both find the single match, while declarations finds no matches.
Kent, which match is reported when searching for references ? If the match is reported on "class B" then refactoring somehow needs an easy way to figure out that this isn't a "real" match (easy means without parsing the source again).
Backup... We have always reported the explicit reference. Do you not want it anymore?
This defect was a consistency problem in between all-occurrence and declarations. We should find these references as we used to (would be a breaking change if not). Refactoring is only one particular use case. Furthermore, I could imagine situations where refactoring would need to turn an implicit constructor references into explicit ones (i.e. when adding a parameter to a superclass constructor which had no param before, the subtypes need to be fixed up).
But without any hint that the match is for a syntetic default constructor refactoring doesn't know what to do with the match. So currently we simply implement our own constructor reference finder using ASTs which IMO is somehow "stupid". What is your suggestion to distinguish between these two different kind of references.
Reopening I discussed this issue with Philippe on the phone and we agreed that refactoring should not implement their own search engine to work around this problem. Philippe suggested that we could extend the limitTo flag to specify if we want references to syntetic default constructors included in the result set. This doesn't break current clients and would allow refactoring to remove their work around code.
To be accurate, I claimed that the new search API would offer a mean to provide access to synthetic constructor declaration/reference at the best. The limitTo flag would not be an option for now (changing a soon obsolete API).
Correct, we agreed on considering this in the new search API only.
Post 3.0
Reopening
While converting to the new AST I again stumble over your nasty workaround code to handle this. Now that we have the new search engine couldn't we simple annotate the match object with information that it is an "implicit" constructor reference (e.g. there isn't any representation in code). Then refactoring could skip these. Currently we are building an AST to figure out if the have to consider the reference or not.
Should read sumbled of OUR nasty workaround...
Using 3.0 or HEAD, I now only find implicit reference constructor on B (which is different than results described in comment 1). Dirk, may you confirm this new behavior? Thx
I get the same behaviour. As outlined the fix shouldn't be to remove these matches (since this would be breaking). We should enrich the match object with additional state telling that the match is implicit.
Created attachment 13195 [details] Proposed implementation for additional implicit info Summary of this patch is: - Add a new specific match for constructor (ConstructorReferenceMatch) which will provide whether the found reference is on a synthetic constructor or not (API: isSynthetic()). - Returns this kind of match with isSynthetic field set instead of TypeDeclarationMatch when reference was found on a constructor.
Created attachment 13196 [details] UI is now able to distinguish implict constructor calls... With this patch (and some changes in JDT/UI), I was able to mark implicit constructors calls as potential matches...
Created attachment 13307 [details] Other proposal: without new match class but using existing one This second patch is an alternative to avoid creation of a match class ConstructorReferenceMatch. Instead, we use existing MethodReferenceMatch on which we add two additional information: isConstructor and isSynthetic... Of course this impact client code which should change the cast to get these extra information. We strongly prefer the second one as it is similar to bindings structure... Dirk, let me know if this second solution fit your need and I'll release it in HEAD.
The second patch is OK for me. As far as I understand it the patch will not impact existing code (since the dealt with Methode refs as well). And we don't have a special type in Java Model for constructors either.
Fixed using second patch has been released in HEAD. Dirk, you're assumption is correct as this implementation does not impact existing code (all JDT/UI tests pass). [jdt-core-internal] Test case testContructorReference10 added to JavaSearchTest.
Verified in I200409230100.
