Community
Participate
Working Groups
When converting a method the SourceTypeConverter converts annotations of the method itself but skips any parameter annotations. The BinaryTypeConverter does not convert any annotations of methods. Are there any hidden reasons why this is so, or should we just add the missing cases? Given that the model already contains all this information it looks strange that it is not converted.
At least the source type version would be required by bug 353472.
Created attachment 200637 [details] proposed change to SourceTypeConverter Here's a trivial change that would solve the issue for SourceTypeConverter. A change for BinaryTypeConverter would be slightly more involved, if needed.
Jay, can you please take a look? Thanks!
I think this needs to be updated. I don't think there is a particular reason for not doing it. If we do it, we should be consistent and do it for both converters.
I've seen bogus warnings from the reconciler when the support for null-annotations was installed. These warnings happened when a type with annotations entered compilation via the SourceTypeConverter. The patch from comment 2 indeed fixed the issue. However, I couldn't easily think of a test to challenge the BinaryTypeConverter in a similar way. What tests do we have to specifically test the BinaryTypeConverter?
(In reply to comment #5) > However, I couldn't easily think of a test to challenge the > BinaryTypeConverter in a similar way. > What tests do we have to specifically test the BinaryTypeConverter? We have a bunch of tests in TypeResolveTests that were added for bug 334783.
I'll prepare a more complete patch as this essentially blocks bug 186342.
Created attachment 204708 [details] test & fix I spent time on trying to find suitable tests. The SourceTypeConverter can be tested from ASTConverter15Test, e.g. I included corresponding tests on this behalf. However, for the BinaryTypeConverter I'm now convinced this issue cannot be triggered: Only two clients ever create BinaryTypeConverters: + CompletionEngine.complete(IType ...) + SelectionEngine.selectType(char[],IType) For neither scenario would we ever come close to asking for annotations in a method signature. I set a breakpoint to the constructor of BinaryTypeConverter and in all of AllJavaModelTests only two groups of tests triggered this breakpoint: SnippetCompletionTests (and _1_5) for complete and TypeResolveTests for selectType. Also these tests give no hint at how we could ever be interested in the missing annotations. I don't think we should implement behavior that can never be triggered. Besides, in the BinaryTypeConverter the implementation would be more involved because we don't have the infra structure in place as we have in SourceTypeConverter. Jay, can we release the patch as is? I think this is the best we can do here and bug 186342 will indeed benefit from this.
I'm going to release this, since it is needed for the scheduled review for bug 186342. Please reopen should you find reasons why BinaryTypeConverter should also be updated (cf. comment 8).
Released as commit de0226254a405a86138be24d23fd7ea72c01b855 for 3.8 M3.
(In reply to comment #8) > I don't think we should implement behavior that can never be triggered. That is not unreasonable. Patch looks good. Verified for 3.8 M3.
