Bug 157333 - calling delete on enum constant deletes entire Enum
Summary: calling delete on enum constant deletes entire Enum
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 blocker (vote)
Target Milestone: 3.2.2   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-09-14 11:29 EDT by Vikas Trivedi CLA
Modified: 2008-09-16 09:41 EDT (History)
1 user (show)

See Also:


Attachments
Proposed fix (1000 bytes, patch)
2006-09-14 20:23 EDT, Olivier Thomann CLA
no flags Details | Diff
Regression test (2.05 KB, patch)
2006-09-14 20:23 EDT, Olivier Thomann CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vikas Trivedi CLA 2006-09-14 11:29:14 EDT
We are using the ISourceManipulation.delete(..) api to delete an enumeration literal. However instead of just deleting the literal, it ends up deleting the entire Enum declaration. 

Debugging this issue ... I noticed that the problem appears to be in SourceField.findNode(..). The method is called from DeleteElementsOperation.deleteElement(..) and returns the parent of the actual node to handle fields. However in the case of enum constant declarations, it should not be returning the parent since the EnumConstantDeclaration is the right node itself.

public ASTNode findNode(org.eclipse.jdt.core.dom.CompilationUnit ast) {
	// For field declarations, a variable declaration fragment is returned
	// Return the FieldDeclaration instead
	ASTNode node = super.findNode(ast);
	if (node == null) return null;
	return node.getParent(); <<<=== Do not do this for enum constants.
}

Marking this defect as blocker since it is breaking our enum literal delete functionality and we don't appear to have any workaround short of using internal jdt calls to perform a Delete Refactoring as is done when deleting the elements from the Package Explorer.
Comment 1 Olivier Thomann CLA 2006-09-14 17:40:56 EDT
I'll investigate.
Comment 2 Olivier Thomann CLA 2006-09-14 20:23:31 EDT
Created attachment 50217 [details]
Proposed fix
Comment 3 Olivier Thomann CLA 2006-09-14 20:23:53 EDT
Created attachment 50218 [details]
Regression test
Comment 4 Olivier Thomann CLA 2006-09-14 20:45:32 EDT
Philippe,

This would be a good candidate for 3.2.2.
Comment 5 Olivier Thomann CLA 2006-09-14 22:10:06 EDT
Released for 3.3M2.
Regression test added in org.eclipse.jdt.core.tests.model.DeleteTests#testDeleteField5.
Comment 6 Vikas Trivedi CLA 2006-09-15 10:40:16 EDT
I'd like to request that this fix be pushed into 3.2.1.

We have identified atleast 2 components (and possibly a couple more) that are impacted by this. And in general anyone attempting to get the ast node for the enum literal is going to be adversely affected.

Without this fix, we have data loss (entire enum is deleted). 
Comment 7 Philipe Mulet CLA 2006-09-15 11:12:40 EDT
Our final contribution to 3.2.1 has already been made.
This defect was reported too late to be part of 3.2.1.

Good candidate for 3.2.2.
Comment 8 Philipe Mulet CLA 2006-09-15 11:15:37 EDT
reopening to address in 3.2 maintenance stream (-> 3.2.2)
Comment 9 Philipe Mulet CLA 2006-09-15 11:16:03 EDT
Note that we may be able to post an early preview patch
Comment 10 Vikas Trivedi CLA 2006-09-15 11:51:17 EDT
I noticed some other fixes are still going in ... (https://bugs.eclipse.org/bugs/show_bug.cgi?id=151743) so I'm wondering if we can squeeze this one in too? 

As I mentioned this is a bad break for us and without the fix in, we have to either pull out some functionality or switch to using jdt internal calls ... both highly undesirable options.
Comment 11 Philipe Mulet CLA 2006-09-18 05:00:54 EDT
The other bug you refer too was entered a long time ago, and planned carefully.
We are very selective in issues addressed for a rollup, and near the end of the fixing window we have to refrain from late changes. This one is tempting, but just too late. As one said: "Better is the enemy of good" (Voltaire).
Comment 12 Vikas Trivedi CLA 2006-09-18 13:03:35 EDT
Fair enough :).

In order to get things working again at my end, I will be making calls to jdt internal code (JavaDeleteProcessor & Delete Wizard). Is there any risk of these changing or being removed any time soon?
Comment 13 Philipe Mulet CLA 2006-09-19 04:03:44 EDT
There is no risk for these to disappear within 3.2 maintenance stream.
In 3.3 stream (and also in 3.2.2) we will address this very issue.
Comment 14 Olivier Thomann CLA 2006-09-28 11:06:29 EDT
Released for 3.2.2.
Regression test added in org.eclipse.jdt.core.tests.model.DeleteTests#testDeleteField5
Comment 15 Frederic Fusier CLA 2006-10-29 07:11:47 EST
Verified for 3.3 M2 using build I200609220010.
Comment 16 Maxime Daniel CLA 2007-01-16 01:50:00 EST
Verified for 3.2.2 using build M20070112-1200.
(Running the regression test upon v_684_R32x indeed.)