Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jdt.ls shouldn't modify disk file when handling newly created or renamed files #1438

Merged
merged 1 commit into from
May 9, 2020

Conversation

testforstephen
Copy link
Contributor

Signed-off-by: Jinbo Wang [email protected]

@testforstephen
Copy link
Contributor Author

This issue is found at redhat-developer/vscode-java#1403 (comment)

Update:
The root cause for the error "Cannot apply refactoring because 'x' file has changed in the meantime." is from eclipse.jdt.ls itself.

When renaming a package or applyEdit, vscode will send didOpen notifications for these sub files to the language server. Because these files are out-of-sync compilation units, the eclipse.jdt.ls will refreshLocal and then recreate the compilation unit (via pkg.createCompilationUnit() api). However, the underlying implementation of this API pkg.createCompilationUnit() actually will call unit.save() method to modify the disk file. That's why vscode detects conflicts happening.

See the code of DocumentLifeCycleHandler:

public ICompilationUnit handleOpen(DidOpenTextDocumentParams params) {
    String uri = params.getTextDocument().getUri();
    ICompilationUnit unit = resolveCompilationUnit(uri);
    if (unit == null || unit.getResource() == null || unit.getResource().isDerived()) {
        return unit;
    }
    try {
        // The open event can happen before the workspace element added event when a new file is added.
        // checks if the underlying resource exists and refreshes to sync the newly created file.
        if (!unit.getResource().isAccessible()) {
            try {
                unit.getResource().refreshLocal(IResource.DEPTH_ONE, new NullProgressMonitor());
                if (unit.getResource().exists()) {
                    IJavaElement parent = unit.getParent();
                    if (parent instanceof IPackageFragment) {
                        IPackageFragment pkg = (IPackageFragment) parent;
                        unit = pkg.createCompilationUnit(unit.getElementName(), unit.getSource(), true, new NullProgressMonitor());
                    }
                }
            } catch (CoreException e) {
                // ignored
            }
        }

// @snjeza, the extra logic is introduced to fix a previous bug. eclipse/eclipse.jdt.ls#486

@testforstephen testforstephen requested review from snjeza and fbricon and removed request for snjeza May 9, 2020 12:23
@testforstephen
Copy link
Contributor Author

Investigated the original issue redhat-developer/vscode-java#380 again. Actually the JDT builder won't report error, only didOpen a class that refers to the new file, there is diagnostics saying unresolved type. That's because didOpen handler uses reconcile to validate the document, and the reconcile process tries to find the unresolved type from the cached PackageFragment#getChildren(). However, the new file is created by disk file, not by JavaModelOperation, so the parent package fragment won't have the newly created CU in the children list. Manually updating the children list will fix the issue, too.

@testforstephen
Copy link
Contributor Author

test this please

@fbricon fbricon added this to the Mid May 2020 milestone May 9, 2020
@fbricon fbricon added the bug label May 9, 2020
@fbricon fbricon merged commit 31eebec into eclipse-jdtls:master May 9, 2020
@fbricon
Copy link
Contributor

fbricon commented May 9, 2020

Thanks @testforstephen

@testforstephen testforstephen deleted the jinbo_syncfile branch May 10, 2020 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants