Bug 251356 - Fix for bug 146768 breaks JDT Refactoring and its test
Summary: Fix for bug 146768 breaks JDT Refactoring and its test
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.5   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 3.5 M3   Edit
Assignee: Kent Johnson CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-20 04:46 EDT by Dani Megert CLA
Modified: 2008-10-28 09:03 EDT (History)
2 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2008-10-20 04:46:56 EDT 
N20081017-2000.

With the changes made to fix bug 146768 it is now possible to inline invalid selections/methods. This is also surfaced by a broken test, see:

http://download.eclipse.org/eclipse/downloads/drops/N20081017-2000/testresults/html/org.eclipse.jdt.ui.tests.refactoring_linux.gtk.x86_6.0.html
Comment 1 Jerome Lanneluc CLA 2008-10-20 04:56:46 EDT 
Can you please paste the test case in this bug?
Comment 2 Dani Megert CLA 2008-10-20 05:00:58 EDT 
Sure,it's: 
org.eclipse.jdt.ui.tests.refactoring.InlineMethodTests.testMultipleMethods
Comment 3 Jerome Lanneluc CLA 2008-10-20 06:39:03 EDT 
Sorry, I meant the source code that doesn't compile (I don't understand how the test works)
Comment 4 Dani Megert CLA 2008-10-20 10:31:19 EDT 
Here's a test case:

package test;

public class TestMultipleMethods {
	void bar() {
		toInline("");
	}

	public void toInline(String d) {
		System.out.println(d);
	}

	public void toInline(String d) {
		System.out.println("");
	}
}


Trying to inline 'toInline' inside bar() should fail but doesn't anymore.


Also note that the first duplicate method doesn't get an error anymore which I think is wrong too.
Comment 5 Dani Megert CLA 2008-10-20 10:49:35 EDT 
FYI: I've disabled the test in HEAD.
Comment 6 Kent Johnson CLA 2008-10-21 10:02:52 EDT 
Dani, the whole point of bug 146760 was to make duplicate fields & methos behave like other duplicate elements.

We no longer complain against the first duplicate field/method, so it means less secondary errors because the first field/method is now available.

I do not see this as a bug with 146760, but your test needs to be updated or just deleted.
Comment 7 Dani Megert CLA 2008-10-21 10:15:40 EDT 
Kent, it's not just the test:
1. when the user tries to inline code it now works on stuff that has errors and
   the result is not predictable. It should not allow the refactoring and for that
   we need to get an error reported (reconcile plus binding)
2. as a user it is just wrong that the first dup one isn't flagged with an error. 
   What if I open a large file? I have to scroll down to notice that one of my 
   methods is duplicated.
Comment 8 Philipe Mulet CLA 2008-10-21 12:13:27 EDT 
I agree for errors on each dup field/method. All needed for original bug was a way to avoid secondary issues when referring to a dup method/field (i.e. leaving a binding behind)
Comment 9 Kent Johnson CLA 2008-10-21 14:04:58 EDT 
Dani, we released a new patch for bug 146760 which reports a duplicate error against each field/method.

BUT we keep the first binding around so any valid references to it are compiled with an error.


I think your test will still fail if you assume that this won't compile :

        void bar() {
                toInline("");
        }
Comment 10 Kent Johnson CLA 2008-10-21 14:05:53 EDT 
Sorry that should have said

BUT we keep the first binding around so any valid references to it are compiled
withOUT an error.
Comment 11 Dani Megert CLA 2008-10-22 08:48:45 EDT 
Kent, your fix does the trick because we check for errors on the methods. You can mark this bug FIXED. Thanks.
Comment 12 Kent Johnson CLA 2008-10-22 10:19:26 EDT 
Fixed with the new patch for bug 146760
Comment 13 David Audel CLA 2008-10-28 09:03:57 EDT 
Verified for 3.5M3 using I20081026-2000 build.