Fixes corruption of e4xmi files when class is renamed

[p-O-q]

The corruption happens in a setup with a top level Maven project and plug-in projects in sub directories (eg bundles). If the name of a Java handler class is renamed in the Java file

• the e4xmi file is found twice with
  • full workspace path
  • plug-in project path

This leads to 2 changes in one e4xmi file with the same text, offset and length.
If the old and new names are of different length the corruption occurs.

This fix skips the file if any of the paths (workspace or project) are already included for renaming.

[github-actions]

Test Results

  147 files  ±0    147 suites  ±0   34m 54s ⏱️ - 2m 9s
3 497 tests ±0  3 438 ✅ +35   54 💤 ±0  5 ❌  - 35 
9 312 runs  ±0  9 177 ✅ +35  130 💤 ±0  5 ❌  - 35 

For more details on these failures, see this check.

Results for commit d848780. ± Comparison against base commit 9d2e85f.

♻️ This comment has been updated with latest results.

[p-O-q]

I'm not sure that I understand why the commit got "unverified"...

[merks]

Each push does a build and because you're not contributed before, someone needs to approve the workflows which I've just done and did the first time too.

[p-O-q]

@merks Thank you for your approval. Is there anything I can do to help?

[merks]

Might it be better to check something like the org.eclipse.core.resources.IResource.getLocationURI() being the same? Can you give an example of the two different IFile instances that you're seeing in this scenario? I don't really have a good mental picture of how this arises...

[p-O-q]

@merks Thank you for your effort!

I try to explain using screenshots of a debugger session.

The Eclipse instance the pictures are taken from is the instance that has started a platform ide.
The platform ide instance has opened the provided/attached Maven project and is processing the rename operation.
The class SaveHandler is renamed to SavHandler.

The

return false;

in line 66 (part of my fix) is commented out. So we see the original behavior.

The renaming operation is stopped at the breakpoint in line 146 where the changes are created.

Both pictures show the value of the variable "changes" created in line 59:

final Map<IFile, TextFileChange> changes = new HashMap<>();

The first picture highlights the first key value of type IFile: it is the project path of the e4xmi file.
The second picture highlights the second key value of type IFile: it is the workspace path of the e4xmi file.

The content of both ReplaceEdit's are equal (text, offset, length).

Later when the ReplaceEdit's are applied the first ReplaceEdit changes the e4xmi file

• from offset 3902 the next 82 characters will be replaced with 'bundleclass://de...handlers.SavHandler'

Now the e4xmi file is changed correctly.

At this point the second ReplaceEdit does exactly the same:

• from offset 3902 the next 82 characters will be replaced with 'bundleclass://de...handlers.SavHandler'

The string to replace has already been replaced and is now shorter (81 chars).
But 82 chars were replaced.
Now the e4xmi file is corrupted.

That also means both paths (workspace and project) are resolved correctly. In this case it does not matter
which of the paths are used for renaming. So in my fix the first path (and ReplaceEdit) that comes in for an e3xmi file will be used.

Using IFile (IResource).getLocationURI() returns the absolute path. For both e4xmi files the absolute has to be the same. So, yes, comparing two URI's is smarter. Thank you :) I'll update today.

Pls don't hesitate to ask for more information. Thank you in advance.

[p-O-q]

@merks The code looks much better now (don't know why I used a match var). Thank you.

Could I explain the situation in a bit more detail?

[merks]

It seems it's possible for getLocationURI() to return null, so one might use Objects.equals but then I"m not sure we'd want null to match null.

Probably better to call file.getLocationURI once before line 121, check for null, returning false in that case, and use the non-null URI on the left side of equals test.

[merks]

I think it should be more like this:

			private boolean isFileAlreadyIncluded(IFile file) {
				URI locationURI = file.getLocationURI();
				return locationURI != null
						&& changes.entrySet().stream().anyMatch(e -> locationURI.equals(e.getKey().getLocationURI()));
			}

[p-O-q]

If the call file.getLocationURI returns null (and the file is not omitted) the issue may occur again.
Is file.getLocationURI the right method to call?
There seem to be a lot of possibilities the return value may be null.

In contrast file.getFullPath() never returns null. The code doesn't look that nice b/c the project or the workspace path may come first and one is a substring of the other. However, there are no null values that need to be taken into account.

What do you think?

[merks]

I tried to determine when getLocationURI returns null, but there are many layers involved. I've never seen it actually return null. It might happen for example if the project doesn't exist:

	public URI getLocationURI() {
		IProject project = getProject();
		if (project != null && !project.exists()) {
			return null;
		}

So that's not really likely to happen here in this situation; arguably it cannot happen here. I think trying to write to such a file would fail anyway...

Your testing seems to be focused on "does this IFile represent the same underlying file as this other IFile" so I think the locationURI of the IFile is the best semantic match for that. Two "is prefix" tests seems confusing to me. Also, moving the getLocationURI out of the loop is a performance enhancement...

[p-O-q]

Ok, afais returning false if getLocationURI returns null is the current/old behavior. You are right with the best semantic match and - for sure - with moving getLocationURI out of the loop.
Thank you.

[merks]

It looks correct to me.

[merks]

@laeubi @HannesWell

Would at least one of you like to have a quick look?

[merks]

I guess it's obvious that you missed adding the import.

[merks]

Others are likely to complain that final isn't needed. It's implicitly final and if it weren't the lamba would complain that it's not final.

[p-O-q]

Yes, my bad. Take your time to review...

[p-O-q]

Others are likely to complain that final isn't needed. It's implicitly final and if it weren't the lamba would complain that it's not final.

You are right. In the projects I've worked on we usually enabled the setting to add final wherever possible.
So a var not being final is something that attracts attention.
If you like I'll remove final. What fits better?

[p-O-q]

@merks Pls let us wait until tomorrow.
I found something I'd like to analyze that could be related to my changes.
I'll get back to you.

[p-O-q]

@merks The fix does not work for project renaming. Project renaming expects a series of changes for the same e4xmi file.

The fix should ensure that all TextEdit instances are applied to one e4xmi file only once.
That means for a physical e4xmi file all the TextEdit (ReplaceEdit) instance have to be unique (text, length, offset).

Maybe even the type of the TextFileChange 'change' has to be considered.

What do you think?

[merks]

I think it would be good to share a reproducible sample. It sounds like the think gathering the resources to change should be more careful about dealing with changes to the same underlying file system file. Like the Search view has "Show Only Most Nested Match"for a similar purpose.

[p-O-q]

@merks Pls take a look at issue #2298. There is a sample project attached and a short description how to reproduce.
Sorry, I missed to tell you.

It sounds like the think gathering the resources to change should be more careful about dealing with changes to the same underlying file system file

If I'm right the tuple (physical file, TextFileChange, ReplaceEdit) has to be unique before creating the CompositeChange result. I'm not sure about the TextFileChange. Without considering the type of TextFileChange the attributes (file, text, offset, length) have to be unique.

The way the refactoring process works

• first gather all changes (file, text, offset, length)
• second apply all gathered changes

implies that none of the changes may be applied twice.

Does that sound reasonable?

[merks]

I think so. FYI I’m at OCX this weekend so my time is limited next week. Perhaps have a look at how search filters “duplicate” search results.

[p-O-q]

Using the four attributes (file, text, offset, length) to find present changes seems to work.
The renaming of a class works and now renaming a plug-in project results in the same amount of changes as before.

@laeubi @HannesWell what do you think?

(The renaming of a RCP plug-in project does not work. But that's another story.)

[p-O-q]

@merks

I’m at OCX

Have a good time

[laeubi]

It also happens to me almost always, maybe because of hierarchical projects or similar. So thanks for spotting the root cause here!

[p-O-q]

@merks The PR overview says "Changes requested". Are you still requesting changes or haven't you had a chance to look yet (b/c of OCX)?

[merks]

I’m not sure how to clear my review from my phone.

Please remove final keyword from all locations.

I likely don’t have time to review until next week.

@laeubi might have time sooner. I trust his judgment so if he approves, that’s sufficient.

[p-O-q]

@merks Ok, I'll remove the final keyword from locations and push again.

[p-O-q]

@merks the "presentFileLocation" variable is not "effectively final". So I didn't remove the final keyword b/c I want it to be final.

("effectively final" not me rule! ;)

[p-O-q]

@laeubi Could you pls trigger the build again? I don't know how to trigger a build without pushing.
I don't think removing a final keyword would break the build.
Do you have time to review this PR?