Semantic Modifications in different Resources (LSP)

[OscarAblinger]

Hi, I hope this is the correct place to ask this after the forum shut down.
If not, I'll gladly move this somewhere else.

We're currently implementing some quickfix for our LSP server. Essentially, we just want to add a new line in another file.
After implementing, however, I've found, that it won't allow me to change any resources outside of the one in which the quickfix gets triggered.

After looking into the code a bit, I think the following code is the one that actually creates the changes:

xtext/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/editor/quickfix/DiagnosticResolution.java

Lines 122 to 144 in 57c1890

	public WorkspaceEdit apply() {
	try {
	EObject obj = getContextObject();
	if (obj == null) {
	return null;
	}
	WorkspaceEdit edit = new WorkspaceEdit();
	if (modification != null) {
	DiagnosticModificationContext modificationContext = factory.createModificationContext();
	ChangeConverter2 changeConverter = modificationContext.getConverterFactory().create(edit, access);
	IChangeSerializer serializer = modificationContext.getSerializer();
	serializer.addModification(obj, modification.apply(diagnostic, obj));
	serializer.applyModifications(changeConverter);
	} else {
	TextEditAcceptor textEditAcceptor = new TextEditAcceptor(edit, access);
	textEditAcceptor.accept(uri.toString(), document, textModification.apply(diagnostic, obj, document));
	}
	return edit;
	} catch (Exception exc) {
	log.error("Creation of WorkspaceEdit failed.", exc);
	return null;
	}
	}

In here I can see that, indeed, it only seems to care about the current resource.
Both in the TextEdit case, where it definitely only accepts TextEdits for the current resource, but even in the modification path, it seems to me, like only that one resource is looked at.
At least the way I read the ChangeSerializer code, it looks like it only cares about the resources of the objects directly given to it (so again only the original resource).

It does seem to correctly apply changes in other resources as long as they are handled by Xtext (e.g. namedElement.setName("foobar")), but not when I manually go into these other resources and change the AST.

So I guess my question is, how is it intended for quickfixes to create changes in other files/resources?

[cdietrich]

i dont know any details here but did you check what the rename refactoring does?

[OscarAblinger]

The rename service seems to work quite a bit differently, but ultimately prepares the changes first and then adds a modification for each resource.

The rename service has a pretty similar piece of code for the change serialization:

xtext/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/rename/RenameService2.java

Lines 162 to 178 in 2046e46

	protected void applyModifications(EObject element, WorkspaceEdit workspaceEdit,
	ServerRefactoringIssueAcceptor issueAcceptor, IRenameService2.Options options,
	ILanguageServerAccess.Context context) {
	IResourceServiceProvider services = serviceProviderRegistry
	.getResourceServiceProvider(element.eResource().getURI());
	IChangeSerializer changeSerializer = services.get(IChangeSerializer.class);
	RenameChange change = new RenameChange(options.getRenameParams().getNewName(), EcoreUtil.getURI(element));
	ResourceSet resourceSet = element.eResource().getResourceSet();
	RenameContext renameContext = new RenameContext(Lists.newArrayList(change), resourceSet, changeSerializer,
	issueAcceptor);
	IRenameStrategy2 renameStrategy = services.<IRenameStrategy2>get(IRenameStrategy2.class);
	renameStrategy.applyRename(renameContext);
	ChangeConverter2.Factory converterFactory = services.<ChangeConverter2.Factory>get(
	ChangeConverter2.Factory.class);
	ChangeConverter2 changeConverter = converterFactory.create(workspaceEdit, options.getLanguageServerAccess());
	changeSerializer.applyModifications(changeConverter);
	}

However the IChangeSerializer is actually passed to the RenameContext and then filled in the IRenameStrategy2#applyRename method:

xtext/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/refactoring/IRenameStrategy2.java

Lines 39 to 45 in 2046e46

	public void applyRename(RenameContext context) {
	for (RenameChange change : context.getChanges()) {
	if (canHandle(change)) {
	context.addModification(change, (EObject o) -> doRename(o, change, context));
	}
	}
	}

This essentially adds a modification for each resource.

xtext/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/refactoring/RenameContext.java

Lines 39 to 47 in 2046e46

	public void addModification(RenameChange change, IChangeSerializer.IModification<EObject> modification) {
	EObject target = resourceSet.getEObject(change.getTargetURI(), true);
	if (target != null) {
	EcoreUtil.resolveAll(target.eResource());
	changeSerializer.addModification(target, modification);
	} else {
	issues.add(RefactoringIssueAcceptor.Severity.ERROR, "Element cannot be found", change.getTargetURI());
	}
	}

So in that case, it has a clear code path on how modifications for multiple resources are added to the WorkspaceEdit.
I'm unsure how one should achieve that using the QuickFix API.

[OscarAblinger]

Okay, so I have now taken a closer look & walked through it step-by-step.

My specific example is similar to what I posted above, in that I only use the EObject to get to the resource set and then change something in a different resource.

I'm going a bit into detail, but I think it's important. So this is what's happening:

1. In the DiagnosticResolution, one modification is added with the original EObject as reference.
2. Then the serializer is asked to apply the modifications with a ChangeConverter2.
3. The serializer takes the reference EObject for all modifications and in beginRecordChanges creates an updater (RecordingResourceUpdater) for each of their resources. It also checks that the resource is in the same resource set, which doesn't matter for us now
4. The serializer applies all modifications in order (aka executes the Quickfix lambda code). With DiagnosticResolution it's always just one.
5. Then it calls endRecordChanges, where the rest of the steps happen.
6. It collects all snapshots of all updaters and creates a delta from them. This delta will be empty in our example.
7. Then it creates a new local list of updaters, where not just the ones from the IModification, but also creates and adds RelatedResourceUpdaters for all related resources of all objects in any snapshots (snapshots->objects->incomingReferences->SourceEObject->ResourceUri).
8. Then it lets all updaters apply their changes to the ChangeConverter2.
9. The RelatedXtextResourceUpdater then checks if it's affected by the reference update. In our case it will never be affected, because the delta is empty.

So I think the cleanest way to add other resources into this flow, would be to somehow add them before step 3.

We could add them simply explicitely, perhaps through a list of changing Resources.
But I don't think there is any way to collect them automatically, so you'd have to specify that when calling DiagnosticResolutionAcceptor#accept.
That's alternative (1)

Alternative (2) would be to add another parameter to DiagnosticResolutionAcceptor#accept that allows you to add more modifications.
So, in a quickfix you could do something like

@QuickFix("ErrorCode")
public void addEntryInOtherFile(DiagnosticResolutionAcceptor acceptor) {
  acceptor.accept("Add name to list file", (obj, resourceModifier) -> {
    for (Resource resource : obj.eResource().getResourceSet().getResources()) {
      if ("list.lang".equals(resource.getLanguageName()) {
                    resourceModifier.modifyResource(resource, () -> {
          IMyList myList = (IMyList)resource.getContents().getFirst();

                                 myList.add(((INamedElement) obj).getName());
                               });
      }
    }
  });
}

and then something like this:

// ChangeSerializer
protected <T extends Notifier> void apply(Notifier context, IModification<T> modification) {
  modification.modify((T) context, this::beginRecordChanges);
}

It's not super pretty and you have to know about it, otherwise it's pretty easy to accidentally make changes without specifying the correct resource set.
It's also annoying when you already have implemented functions in some library of yours, where you simply change other resources without a callback for the "resourceModifier".

 Alternative (3) would be to simply extend DiagnosticResolutionAcceptor#accept to allow multiple modifications and also allow you to specify where they happen (to with EObject ideally).
That's a bit hard, however, because with the current declarative quickfix interface from AbstractDeclarativeIdeQuickfixProvider, we only get the DiagnosticResolutionAcceptor, which does not have the EObject that the quickfix is applied to.
In theory we can provide one at that point (as the interface IQuickFixProvider does have it), but that one would be in the original resource set & not in a separate one, so changes to it would be dangerous.

 Alternative (4) is the simplest, which is just to add no-op modifications for all resources in the resourceSet first.
I think that should also work.
The downside is that you'll have a lot of unnecessary resources being recorded.
However, this is always a newLiveScopeResourceSet, so they should normally be rather bare anyways.
In our case we had to add more to it, but by default, I think it's entirely empty. So only the resource of the current EObject is added.

---

Lastly, I'd like to also quickly address textual changes.
I've not really done much research, but looking at the code in DiagnosticResolution#apply, it's pretty clear to me, that that API does not support changes in different files at all.

TextEditAcceptor textEditAcceptor = new TextEditAcceptor(edit, access);
// note: this accepts a List<TextEdit> for a given document
// TextEdits do NOT have information about which resource/document they are in
textEditAcceptor.accept(uri.toString(), document, textModification.apply(diagnostic, obj, document));

IMO we could easily expand this API as well.
Much easier than the semantic changes path.
You probably just have to return a map of uris to TextEdits.

Do you have any opinions on any of this?

Personally I'll probably overwrite this for our use case as described in alternative (2) or (4).

[cdietrich]

What I don’t understand is. Why is ever more than 1 issue fixed ? Unfortunately I have no example to debug myself and no time to create it .

So is the problem that the converter does go into the text edit mode but does not produce. Proper messages .

[cdietrich]

(i'd expect org.eclipse.xtext.ide.server.rename.ChangeConverter2._handleReplacements(IEmfResourceChange) to pick the correct uri / document and produce text edit for each separate document

[cdietrich]

same for org.eclipse.xtext.ide.server.rename.ChangeConverter2._handleReplacements(ITextDocumentChange)

[rubenporras]

Sorry, I do not know. (We do not use rename in our product because it does not perform well and it was never a priority for us to fix the use case)

This isn't about rename. It's about QuickFixes. Rename was only mentioned as another code piece with similar functionality.

I see. Still I do not know the answer and do not have time to dig on the topic.