Bug 110771 - Fup on bug 102305 - align all ICompilationUnit#getContents implementations on a 'never null' behavior
Summary: Fup on bug 102305 - align all ICompilationUnit#getContents implementations on...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.1.1   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 3.3 M4   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-09-27 05:43 EDT by Maxime Daniel CLA
Modified: 2006-12-13 04:45 EST (History)
4 users (show)

See Also:


Attachments
Suggested fix (1.93 KB, patch)
2006-11-14 05:02 EST, Maxime Daniel CLA
no flags Details | Diff
Proposed fix (883 bytes, patch)
2006-12-11 14:52 EST, Olivier Thomann CLA
no flags Details | Diff
Other proposed fix (832 bytes, patch)
2006-12-12 04:08 EST, Frederic Fusier CLA
no flags Details | Diff
New proposed patch (2.09 KB, patch)
2006-12-12 12:00 EST, Frederic Fusier CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Daniel CLA 2005-09-27 05:43:26 EDT
Bug 102305 establishes that ICompilationUnit#getContents is never expected to
return null. While the case that caused the said bug has been addressed in
3.1.1, at least two implementors seem to be able to break this contract.
I suggest that a systematic search of these potential breakages be run, and that
these get changed.
Comment 1 Maxime Daniel CLA 2006-11-13 06:30:26 EST
Potential breakages:
- org.eclipse.jdt.internal.core.CompilationUnit: anonymous class into buildStructure();
- org.eclipse.jdt.internal.core.search.matching.PossibleMatch.
Comment 2 Maxime Daniel CLA 2006-11-14 05:01:29 EST
No test case of our base gets the said methods to really return null.
Some calling points merely try to access .length on the result without testing for null. Will hence return CharOperation.NO_CHAR in the two identified instances where we would return null.
Comment 3 Maxime Daniel CLA 2006-11-14 05:02:15 EST
Created attachment 53801 [details]
Suggested fix
Comment 4 Maxime Daniel CLA 2006-11-14 11:44:50 EST
Released for 3.3 M4.

Also amended the javadoc of ICompilationUnit#getContents to warn implementors.
Comment 5 Maxime Daniel CLA 2006-11-14 11:45:34 EST
Forgot to change the state...
Comment 6 Olivier Thomann CLA 2006-12-11 14:49:08 EST
Shouldn't org.eclipse.jdt.internal.core.ClassFileWorkingCopy also change its getContents() implementation ?
It can potentially return null.
Comment 7 Olivier Thomann CLA 2006-12-11 14:52:27 EST
Created attachment 55428 [details]
Proposed fix
Comment 8 Olivier Thomann CLA 2006-12-11 14:53:02 EST
Frederic,

Would you take care of releasing it?
Comment 9 Frederic Fusier CLA 2006-12-12 04:08:22 EST
Created attachment 55468 [details]
Other proposed fix

Looking at implementation in Buffer, getCharacters() returns always a non-null array as soon as the buffer is opened. getContents() on ClassFileWorkingCopy calls getBuffer() and this method returns an opened buffer or null, I think it's better to test whether the buffer is null and return an empty array in this case...
Comment 10 Frederic Fusier CLA 2006-12-12 04:09:00 EST
Jerome, can you double-check?
Thanks
Comment 11 Frederic Fusier CLA 2006-12-12 09:33:50 EST
Comment on attachment 55468 [details]
Other proposed fix

Jerome rightly highlighted the fact that IBuffer might be implemented by clients and so we has to rely on spec for getCharacters() method return. As it may be null, we also need to return empty char array as previous Olivier's patch did.
I'll put a new one merging the two changes...
Comment 12 Frederic Fusier CLA 2006-12-12 12:00:29 EST
Created attachment 55510 [details]
New proposed patch

Merge patches + fix same potential issue (IBuffer.getCharacters() may return null) in initial suggested patch...
Comment 13 Frederic Fusier CLA 2006-12-12 12:17:05 EST
All JDT/Core tests pass with this patch.
Released for 3.3 M4 in HEAD.
Comment 14 Frederic Fusier CLA 2006-12-13 04:45:42 EST
We will not contribute again for the additional patch. However, as there's no specific test case for this bug, just consider it as verified for 3.3 M4 using HEAD stream...