search

vazexqi / CodingSpectator

Watches and analyzes code edits in the Eclipse IDE non-invasively
http://codingspectator.cs.illinois.edu
Other
20 stars 14 forks source link

ExtractMethodRefactoring does not store the method's parameters information in its descriptor #169

Closed Wanderer777 closed 13 years ago

Wanderer777 commented 13 years ago

ExtractMethodRefactoring neither adds to its descriptor nor reads from it (when initializing from a descriptor) the information about the parameters of the extracted method. As a result, the replayed ExtractMethodRefactoring may produce the code that is different from the code observed by a developer who performed this refactoring (e.g. when the default order of method parameters is changed in the refactoring's dialog box).

To fix this problem, ExtractMethodRefactoring should record/initialize its fParameterInfos field in a way that is similar to what ChangeSignatureProcessor does. In general, this is an Eclipse bug, but it would be good to fix it ourselves (if it is not very hard), such that we do not depend on when it gets fixed in Eclipse.

reprogrammer commented 13 years ago

I suggest to check that this bug exists in the latest version of Eclipse. If it still exists, we'll fix it and send our patch to Eclipse developers.

vazexqi commented 13 years ago

Nick: I tested it with Helios SR-2 and this bug is still there. Balaji: I tested with Indigo M5 and this bug is still there.

rajkuma1 commented 13 years ago

Refer line 1295 in class ChangeSignatureProcessor. The oldIndex is not updated. The list of parameters is in the actual order as the parameters appear on the method signature. To understand how the elements are always kept in the right order, you'll have to refer line 660 i.e. method moveUp under ChangeParametersControl. The code is a bit tricky so you might want to debug to get a better picture.

In fact, I checked the refactoring history file holds the oldIndex and not the new Index.

Again, while initializing from the refactoring history, if you refer the method createParameterInfoList, the list is populated in the order in the same order in which the parameters were stored.

So, we don't have to rely on the oldIndex at all.

vazexqi commented 13 years ago

Fixed in cd63360f1005d5c935e9ac15a51661a1fdfc90e2 .. 1566672ab81f80d7b3a3f145ec65f50d5d381064.

Please test it out more. We have already tested it under the following situations:

  1. No arguments
  2. Reorder arguments
  3. Rename arguments
  4. Test with different types: varargs (String...), arrays (String[]), collection (List), nested collections (List<List>)
vazexqi commented 13 years ago

We need to also update the existing Extract Method Refactoring tests with the new information.

rajkuma1 commented 13 years ago

Updated tests to check for parameters in the refactoring descriptor in 13e3c44dafac4dc5348e1b0e202bb002479224fd.

rajkuma1 commented 13 years ago

Added test for extract method refactoring with 1 parameter in 36fb4e8c7ee56b3c54354d55ba0eebe1cd16156e.

Wanderer777 commented 13 years ago

Added a test to CodingTracker that records/replays different variations of ExtractMethodRefactoring (no parameters, one parameter, multiple parameters, different combinations of reordering/renaming of parameters) in a453345f61b7afb181ba6dda0240e8ddc6338abe.

vazexqi commented 13 years ago

There is an additional issue for replaying in CodingTracker:

We do not have any information about the return type of the extracted method. Normally, the refactoring itself figures out the return type. But the problem is that during the replay many classes/methods are missing and, as a result, the refactoring sometimes uses "void" instead of some other type that was actually used when a developer performed this refactoring.

I'll see if we can store that information as well.

Wanderer777 commented 13 years ago

There might be different scenarios, in which a refactoring can not be correctly replayed. As a back-up measure, the recorder captures textual edits performed by a refactoring, not just the refactoring descriptor. At the same time, those textual edits are not sufficient to reproduce changes made by a refactoring, if the refactoring affects more than one file (or was applied without opening an editor). Therefore, it would be good to be able to replay as many refactorings as possible.

I think that the replaying strategy would be the following. First, it tries to replay a refactoring. If it fails for whatever reason, the replayer would apply the captured raw textual edits. This would work unless a refactoring that affects more than one file fails. But even in the latter case, I believe that it would be possible to look at the changes in one file to fix other affected files.

Extract method refactoring is an example of a refactoring that affects only a single file, which means that such refactoring can always be replaced by the recorded edits. Therefore, it is not critical if this refactoring can not be replayed correctly sometimes. In other words, if it would take a significant effort to keep track of the return type (by the way, I think that keeping just the return type might not be sufficient as the extracted method should know what to actually return, not just the type of the returned object), then we do not need to implement support for that.

reprogrammer commented 13 years ago

@Wanderer777: Can you remind us of why CodingTracker is not able to capture all the edits applied by the refactorings that affect multiple files? Does CodingTracker ignore the textual edits of such refactorings to save some space?

Wanderer777 commented 13 years ago

No, the current version of CodingTracker does not ignore any textual edits. The problem is that if a refactoring affects multiple files (or is performed without openning an editor), then CodingTracker misses textual edits made to files that are not openned in the currently active editor (i.e. CodingTracker listens to the document of the currently active editor and misses any changes to other parts of the code).

reprogrammer commented 13 years ago

As I suggested yesterday, you might be able to capture the Change object computed by the refactoring. The Change object is expected to modify both open and closed files. So, the replayer can later use the recorded Change object to simulate the refactoring. In addition, if CodingTracker records the AST changes created by refactorings, it might help CodingTracker in inferring the AST edit operations later.

Wanderer777 commented 13 years ago

This is no longer an issue since from now on CodingTracker reproduces effects of the refactorings rather than replaying them (see issue #203).

reprogrammer commented 13 years ago

@rajkuma1: Since @vazexqi and you have already fixed this bug of Eclipse, it would be nice to prepare a patch and submit it to Eclipse maintainers. @vazexqi has submitted patches to Eclipse before. So, he can help you with that.

vazexqi commented 13 years ago

Do this in a clean workspace.

  1. Grab a check out a clean version of JDT for Helios (use CVS). This was the version that we worked with.
  2. Incorporate the changes that we have made to enable properly capturing the refactoring. Do not re-format the code but follow the old coding convention. Do not include any other changes like our monitoring/logging code.
  3. Generate a patch against that. You should be able to do this easily from Eclipse. Right-click on the ExtractMethodRefactoring file and there should be "Patch" menu item.

After that you can report the bug.

  1. Go to https://bugs.eclipse.org/bugs/enter_bug.cgi
  2. Create an account (if you don't have one)
  3. File the bug and attach the patch.
reprogrammer commented 13 years ago

@rajkuma1: Could you please follow the steps that @vazexqi listed and share the final patch with us before submitting it to JDT maintainers?

reprogrammer commented 13 years ago

@rajkuma1, @vazexqi: You might find the following resources for contributing to Eclipse helpful.

  1. How to contribute to Eclipse projects…
  2. How to Fix a Bug in Eclipse
  3. Getting started with Eclipse plug-ins: contributing to Eclipse