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

Extensions can reverse order of TextDocumentChangeEvent.contentChanges causing other extensions documents to become out of sync #88310

Closed
bmewburn opened this issue Jan 8, 2020 · 2 comments
Assignees
Labels
api bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Milestone

Comments

@bmewburn
Copy link

bmewburn commented Jan 8, 2020

Version: 1.41.1
Commit: 26076a4
Date: 2019-12-18T15:04:31.999Z
Electron: 6.1.5
Chrome: 76.0.3809.146
Node.js: 12.4.0
V8: 7.6.303.31-electron.0
OS: Linux x64 4.19.0-6-amd64

Steps to Reproduce:

  1. Install https://marketplace.visualstudio.com/items?itemName=austenc.laravel-blade-spacer
  2. Install another extension that has TextDocumentSyncKind.Incremental support. eg https://marketplace.visualstudio.com/items?itemName=bmewburn.vscode-intelephense-client
  3. Turn server trace to verbose.
  4. Edit the document then undo those changes.
  5. Spurious errors can be observed indicating document is out of sync if diagnostics are supported.
  6. Try same edits without the extension in step 1 and compare server traces and observe that changes sent to the server are reversed.

See https://github.com/austenc/vscode-blade-spacer/blob/ae88ae1e82719c7b9fc088b93903285ac61b283a/src/extension.ts#L35 . Expected result would be that an extension doing this would not be able to affect other extensions.

According to the LSP spec the order is important.
https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#textDocument_didChange

/**
	 * The actual content changes. The content changes describe single state changes
	 * to the document. So if there are two content changes c1 and c2 for a document
	 * in state S then c1 move the document to S' and c2 to S''.
	 */
server trace
**REVERSED**
[Trace - 9:07:06 am] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///home/ben/Projects/laravel-test/app/User.php",
        "version": 19
    },
    "contentChanges": [
        {
            "range": {
                "start": {
                    "line": 13,
                    "character": 31
                },
                "end": {
                    "line": 13,
                    "character": 32
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 13,
                    "character": 32
                },
                "end": {
                    "line": 13,
                    "character": 33
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 13,
                    "character": 33
                },
                "end": {
                    "line": 13,
                    "character": 34
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 13,
                    "character": 34
                },
                "end": {
                    "line": 13,
                    "character": 35
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 13,
                    "character": 35
                },
                "end": {
                    "line": 13,
                    "character": 36
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 13,
                    "character": 36
                },
                "end": {
                    "line": 13,
                    "character": 37
                }
            },
            "rangeLength": 1,
            "text": ""
        }
    ]
}

**NOT REVERSED**
[Trace - 9:11:25 am] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///home/ben/Projects/laravel-test/app/User.php",
        "version": 25
    },
    "contentChanges": [
        {
            "range": {
                "start": {
                    "line": 15,
                    "character": 36
                },
                "end": {
                    "line": 15,
                    "character": 37
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 15,
                    "character": 35
                },
                "end": {
                    "line": 15,
                    "character": 36
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 15,
                    "character": 34
                },
                "end": {
                    "line": 15,
                    "character": 35
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 15,
                    "character": 33
                },
                "end": {
                    "line": 15,
                    "character": 34
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 15,
                    "character": 32
                },
                "end": {
                    "line": 15,
                    "character": 33
                }
            },
            "rangeLength": 1,
            "text": ""
        },
        {
            "range": {
                "start": {
                    "line": 15,
                    "character": 31
                },
                "end": {
                    "line": 15,
                    "character": 32
                }
            },
            "rangeLength": 1,
            "text": ""
        }
    ]
}

Does this issue occur when all extensions are disabled?: Extensions are needed to observe error.

@dbaeumer
Copy link
Member

dbaeumer commented Jan 9, 2020

@bmewburn good catch.

@jrieken very interesting case. Can we freeze the array to avoid that extensions mess with the event?

@dbaeumer dbaeumer added the api label Jan 9, 2020
@dbaeumer dbaeumer removed their assignment Jan 9, 2020
@dbaeumer
Copy link
Member

dbaeumer commented Jan 9, 2020

@alexdima FYI.

@jrieken jrieken added the bug Issue identified by VS Code Team member as probable bug label Jan 9, 2020
@jrieken jrieken added this to the January 2020 milestone Jan 9, 2020
@mjbvz mjbvz added the verified Verification succeeded label Jan 30, 2020
mjbvz added a commit that referenced this issue Jan 30, 2020
Follow up on #88310

Callers should never mutate these events (and after #88310, this will now produce an error since the result should be frozen)
@vscodebot vscodebot bot locked and limited conversation to collaborators Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants