Comment 1 Srikanth Sankaran 2008-12-10 04:53:45 EST

> When I paste the package-info.java in the second source folder, I get:
> Java Model Exception: Java Model Status [package-info [in package-
> info.java [in
> my.foo [in src [in 252555]]]] does not exist]

1. I had some trouble reproducing this, but finally managed to. It turns out the problem does not show up for whatever reason if you in a step by step fashion:

	- create the second folder (test), 
	- the package my.foo inside it
	- the file package-info.java with the identical contents (as from bug #252555).

However you can consistently reproduce this if you 
	
	- create the second folder (test)
	- copy the package my.foo from the first folder (src)
	- paste it into the second folder.

2. The other difference I notice is that in the exception message I see 

Java Model Exception: Java Model Status [package-info [in package-info.java [in my.foo [in test [in P]]]] does not exist]. (cf Olivier's log which complains about src, not test)

3. A clean and full build does not throw up this exception, so there is something of incremental vs full compile behavior that is also influencing the outcome. 

Comment 2 Jerome Lanneluc 2008-12-10 12:11:30 EST

It looks like the incremental builder assumes that the ClassFile coming from "my/foo/package-info" corresponds to an IType.

Comment 3 Srikanth Sankaran 2008-12-10 22:58:31 EST

Hello Kent,

    If you haven't already invested some cycles on this, I would request you to pass the defect to me and suggest that you can be the reviewer and take my fix to commit.

    I spent some time analyzing this, since this originated as a follow up of bug 252555 which I closed. However, this appears to be an altogether different issue and I am interested in taking a crack at it.

    Here is a theory that could plausibly explain the scenario: When we want to create a problem marker for the duplicate package-info type, we attempt to query the source position on the JavaElement corresponding to the interface type "package-info". However this type is artificial being synthesized by the compiler and has no corresponding source representation. As a result, even after buildStructure operation on the concerned OpenableElementInfo, we don't find the "package-info" interface type in the java model cache.

    If this theory holds true, the JavaModelException with status set to ELEMENT_DOES_NOT_EXIST is legitimate (of course, it still shouldn't be exposed
for the case at hand)

Comment 4 Jerome Lanneluc 2008-12-11 02:00:43 EST

Sorry Srikanth, I had not realized that you started investigating this bug. I'm assigning it to you, and keep Kent on CC so that Kent can review your proposal.

Comment 5 Kent Johnson 2008-12-11 12:12:22 EST

Srikanth, its all yours.

And if you end up 'fixing' it in the builder, then also take a look at bug 214948.


I don't know who added support for the package-info but I know the full vs incremental build story for package-info.java does not work.

Comment 6 Srikanth Sankaran 2008-12-12 07:01:09 EST

(In reply to comment #3)

>     Here is a theory that could plausibly explain the scenario: When we want to
> create a problem marker for the duplicate package-info type, we attempt to
> query the source position on the JavaElement corresponding to the interface
> type "package-info". However this type is artificial being synthesized by the
> compiler and has no corresponding source representation. As a result, even
> after buildStructure operation on the concerned OpenableElementInfo, we don't
> find the "package-info" interface type in the java model cache.

    This theory is indeed correct and the current bug appears to be a side effect of the fix for bug# 99662. (also see bug# 99903) 
 
With the fix for bug # 99662

org.eclipse.jdt.internal.compiler.SourceElementNotifier.notifySourceElementRequestor(TypeDeclaration, boolean, TypeDeclaration)

begins with the following statement:

    if (CharOperation.equals(TypeConstants.PACKAGE_INFO_NAME, typeDeclaration.name)) return;

This if statement short circuits the code that populates the Java model cache with the "package-info" interface type. If this line is commented out, the
JME goes away (of course bringing back 99662 & 99903)

    So, it looks like the only way out is to special case the current JME scenario and make getNameRange swallow the exception and return some suitable value for package-info type. 

Comment 7 Srikanth Sankaran 2008-12-14 01:47:12 EST

Created attachment 120401 [details]
Proposed fix and test


   [ Kent, please review and help take this to commit - TIA ]

    - This patch introduces behavior in SourceType.getNameRange to intercept the JME just for the special case of package-info interface type and drop the exception and instead return a "suitable" source range.

    - The reason this problem doesn't show up in the full build case is that the source range for the Problem is derived directly out of the syntax tree (as opposed to querying getNameRange from an IType that findType returns.)

    - New test added org.eclipse.jdt.core.tests.builder.PackageInfoTest.test258145()

Comment 8 Kent Johnson 2008-12-15 15:10:37 EST

Created attachment 120507 [details]
Alternative patch

Srikanth, I'm wondering whether the builder should protect itself & check to see if the type exists before creating the problem marker.

This patch seems 'safer' to me than just catching the exception.

What do you think ?

Comment 9 Srikanth Sankaran 2008-12-15 22:48:10 EST

(In reply to comment #8)
> Created an attachment (id=120507) [details]
> Alternative patch
> Srikanth, I'm wondering whether the builder should protect itself & check to
> see if the type exists before creating the problem marker.
> This patch seems 'safer' to me than just catching the exception.
> What do you think ?

    This alternative patch will work, but I don't think it is necessarily safer (I would even argue if I were in a particularly argumentative frame of mind that it is downright dangerous :))

    For one thing, IType.exists (==> JavaElement.exists()) essentially does what the original patch was doing, i.e, catch the exception and drop it, but it does so on a very general basis and not just for the known case of package-info interface type. As such, the new patch has the potential to mask defects - in most cases, I would think our inability to find a type *where it is supposed to exist* in the Java model *should* result in a JME, no ?

    Also it leaves exposed other uses of IType("package-info").getNameRange() to JME  (- I don't know of any such uses and of course in both patches uses of IType("package-info").someOtherApi stand exposed to JME)

    That said, this is all perhaps overanalyzing and I am OK with either the original patch or the alternate, perhaps they can actually sensibly co-exist too.

    Does this make sense ?

Comment 10 Jerome Lanneluc 2008-12-16 04:43:34 EST

I agree with Srikanth's analysis that exists() is less safe than catching the JavaModelException only in the "package-info" case.

However, I would say that IType("package-info").getNameRange() throwing a JME is a feature since the type really doesn't exist so it should really throw a JME to be consistent with exists().

So I would be more in favor of catching the JME on the client side (namely the AbstractImageBuilder) since this is the client that makes an unusual usage of IType("package-info").getNameRange(). Of course the client should rethrow the JME if it is not the "package-info" case.

Comment 11 Srikanth Sankaran 2008-12-16 06:39:39 EST

(In reply to comment #10)
> I agree with Srikanth's analysis that exists() is less safe than catching the
> JavaModelException only in the "package-info" case.
> However, I would say that IType("package-info").getNameRange() throwing a JME
> is a feature since the type really doesn't exist so it should really throw a
> JME to be consistent with exists().
> So I would be more in favor of catching the JME on the client side (namely the
> AbstractImageBuilder) since this is the client that makes an unusual usage of
> IType("package-info").getNameRange(). Of course the client should rethrow the
> JME if it is not the "package-info" case.

Sounds good. Kent, please chime in with your thoughts and if there is agreement all around, I can rework the patch.

Comment 12 Srikanth Sankaran 2008-12-16 09:11:01 EST

Created attachment 120567 [details]
Modified patch 



To possibly prevent a day's delay, I have prepared a new patch anyway along the lines outlines in comment # 10.

Comment 13 Kent Johnson 2008-12-16 10:24:29 EST

Looks good to me.

Comment 14 Kent Johnson 2008-12-17 12:56:25 EST

Fix and test released for 3.5M5 for Srikanth

Comment 15 Kent Johnson 2009-01-27 10:54:33 EST

Verified for 3.5M5 using I20090126-1300