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

  143 files   -     4    143 suites   - 4   29m 41s ⏱️ - 7m 22s
3 497 tests ±    0  3 442 ✅ +   39   54 💤 ± 0  1 ❌  - 39 
7 177 runs   - 2 135  7 070 ✅  - 2 072  106 💤  - 24  1 ❌  - 39 

For more details on these failures, see this check.

Results for commit aab5d60. ± 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?