Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 233278 - [surround with] "Surround With runnable" crash.
Summary: [surround with] "Surround With runnable" crash.
Status: RESOLVED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: 3.5 M1   Edit
Assignee: Markus Keller CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-21 13:05 EDT by Brian Miller CLA
Modified: 2008-07-22 13:50 EDT (History)
4 users (show)

See Also:


Attachments
patch (4.75 KB, patch)
2008-07-22 09:12 EDT, Benjamin Muskalla CLA
markus.kell.r: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Miller CLA 2008-05-21 13:05:04 EDT
Build ID: I20080330-1350 (3.4M6)

Steps To Reproduce:
1. Please select the text on lines 5-7.
2. On the "Surround With" menu, choose "runnable".  See crash below...

--------------- Bug.java ------------------
class Bug{{
	final Integer x=0, y=1;
	new Object(){
		void method(){
			if(x==y)//LINE 5
				return;
			toString();//LINE 7
		}};
}}
---------------------------------------------

java.lang.ArrayIndexOutOfBoundsException: 1
at org.eclipse.jdt.internal.corext.refactoring.code.flow.LocalFlowInfo.<init>(LocalFlowInfo.java:24)
at org.eclipse.jdt.internal.corext.refactoring.code.flow.FlowAnalyzer.endVisit(FlowAnalyzer.java:766)
at org.eclipse.jdt.core.dom.SimpleName.accept0(SimpleName.java:148)
at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2478)
at org.eclipse.jdt.core.dom.ASTNode.acceptChild(ASTNode.java:2525)
at org.eclipse.jdt.core.dom.InfixExpression.accept0(InfixExpression.java:367)
at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2478)
at org.eclipse.jdt.core.dom.ASTNode.acceptChild(ASTNode.java:2525)
at org.eclipse.jdt.core.dom.IfStatement.accept0(IfStatement.java:188)
at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2478)
at org.eclipse.jdt.internal.corext.refactoring.code.flow.InOutFlowAnalyzer.perform(InOutFlowAnalyzer.java:40)
at org.eclipse.jdt.internal.ui.text.correction.SurroundWith.getReads(SurroundWith.java:337)
at org.eclipse.jdt.internal.ui.text.correction.SurroundWith.getVariableDeclarationReadsInside(SurroundWith.java:277)
at org.eclipse.jdt.internal.ui.text.template.contentassist.SurroundWithTemplateProposal$SurroundWithTemplate.getVariableDeclarationReadsInside(SurroundWithTemplateProposal.java:79)
at org.eclipse.jdt.internal.ui.text.correction.SurroundWith.getRewrite(SurroundWith.java:221)
at org.eclipse.jdt.internal.ui.text.template.contentassist.SurroundWithTemplateProposal.createNewContext(SurroundWithTemplateProposal.java:244)
at org.eclipse.jdt.internal.ui.text.template.contentassist.SurroundWithTemplateProposal.apply(SurroundWithTemplateProposal.java:198)
Comment 1 Benno Baumgartner CLA 2008-05-27 10:48:46 EDT
No regression, this is also in 3.3.2
Comment 2 Benno Baumgartner CLA 2008-07-16 11:11:02 EDT
reproduced in I20080715-1015

Should fix in 3.5
Comment 3 Benjamin Muskalla CLA 2008-07-22 09:12:54 EDT
Created attachment 108057 [details]
patch

Here is a little patch
Comment 4 Markus Keller CLA 2008-07-22 13:21:51 EDT
Comment on attachment 108057 [details]
patch

Thanks for the patch. I released the test case, but not the actual fix.

While your fix fixed the reported problem, it's not at the right spot. The underlying problem is that LocalVariableIndex.perform(BodyDeclaration) does not follow the parent chain long enough if the affected declaration is inside an initializer. My fix in LocalVariableIndex also fixes the same problem for Extract Method and Inline Method.
Comment 5 Markus Keller CLA 2008-07-22 13:30:47 EDT
And also thanks to Brian for reporting. It's always amazing to see how code can look like if it's not taken from a CS 101 script ;-)
Comment 6 Benjamin Muskalla CLA 2008-07-22 13:31:48 EDT
I thought something like that but was not sure.
At least the testcases helped you :)

btw: are there any documents about the flow analysis stuff besides the code itself? It's a really interesting topic but not a little hard to have a good insight as a contributor.

Thanks!
Comment 7 Markus Keller CLA 2008-07-22 13:44:06 EDT
> btw: are there any documents about the flow analysis stuff besides the code
> itself? It's a really interesting topic but not a little hard to have a good
> insight as a contributor.

No, all we have is the code. I agree it's a hard topic, and the pitfalls are many. My way to cope with the complexities in refactoring code it is to
- have the type hierarchy and possible nesting hierarchy of all ASTNode subclasses in my head, and
- use search and the call hierarchy to see who else uses a helper method, and
- be suspicious whenever some code uses ASTNodes.getParent(ASTNode, ...): Usually, this is wrong, since it either misses possible parent types, or fails to stop at other node types (e.g. there's no enclosing method for a statement in an initializer, and walking further up would not be correct).
Comment 8 Benjamin Muskalla CLA 2008-07-22 13:50:21 EDT
> use search and the call hierarchy to see who else uses a helper method

That's how I do it at the moment ;)

Thanks anyway!