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

feature: Option to disable automatic updates of VFS #601

Open
wenkokke opened this issue Jan 1, 2025 · 11 comments
Open

feature: Option to disable automatic updates of VFS #601

wenkokke opened this issue Jan 1, 2025 · 11 comments

Comments

@wenkokke
Copy link

wenkokke commented Jan 1, 2025

I'm using lsp to implement a language server using incremental parsing. To update the parse tree, I need each partial document change together with the rope before applying that change. Specifically, I need this to convert positions to byte indexes into the document. This isn't possible with the current version of the library, since it automatically applies the document changes. To work around this, I'm maintaining a separate VFS, but since I'm doing all the same work, it would be much easier and less expensive to simply disable automatic updates of the VFS and perform them myself.

To make this a non-breaking change, I'd suggest a server option that enables/disables automatic VFS maintenance on a per operation basis—i.e., open, close, change—that are all enabled by default.

I'm happy to implement this.

@michaelpj
Copy link
Collaborator

Yes, there's something unsatisfying here. HLS also can't quite use the VFS as listed, since it wants to react to the changes in its own way also.

I'm not super keen on what you're proposing, since at that point it's unclear that what we're doing is even useful. It's very common to want to react to changes to the VFS, and at the moment there is no good way to do so other than polling it or adding your own handlers for the file change notifications. I wonder whether instead we need to offer an interface that's actually useful to implementations like yours and HLS's.

Here's one option. What we currently do for config changes is that a) we automatically apply them, so you can query the up-to-date version, but also b) we let you provide an "on changed" callback so you can react to changes yourself. We could do something like that for the VFS, i.e. let people give us a callback like onVFSChanged :: NormalizedUri -> VirtualFile -> DocumentChange -> VirtualFile -> m (). That would give people the path, the contents before and after, and also the change itself. Lots of details to get right, obviously.

I think that would work for HLS, and I think it might also work for you?

@wenkokke
Copy link
Author

wenkokke commented Jan 2, 2025

Yes, there's something unsatisfying here. HLS also can't quite use the VFS as listed, since it wants to react to the changes in its own way also.

I'm not super keen on what you're proposing, since at that point it's unclear that what we're doing is even useful. It's very common to want to react to changes to the VFS, and at the moment there is no good way to do so other than polling it or adding your own handlers for the file change notifications. I wonder whether instead we need to offer an interface that's actually useful to implementations like yours and HLS's.

Here's one option. What we currently do for config changes is that a) we automatically apply them, so you can query the up-to-date version, but also b) we let you provide an "on changed" callback so you can react to changes yourself. We could do something like that for the VFS, i.e. let people give us a callback like onVFSChanged :: NormalizedUri -> VirtualFile -> DocumentChange -> VirtualFile -> m (). That would give people the path, the contents before and after, and also the change itself. Lots of details to get right, obviously.

I think that would work for HLS, and I think it might also work for you?

I would need to be able to react to each individual TextDocumentContentChangePartial event, so as long as the DocumentChange does not bunch multiple TextDocumentContentChangePartial events, that would work for me. If it doesn't, I would still need to update the rope manually.

What I need, concretely, is to convert the ranges in each TextDocumentContentChangePartial from line/column pairs to byte indices, so if there's an easier way to do that, I'm also all ears.

@wenkokke
Copy link
Author

wenkokke commented Jan 2, 2025

Here's one option. What we currently do for config changes is that a) we automatically apply them, so you can query the up-to-date version, but also b) we let you provide an "on changed" callback so you can react to changes yourself. We could do something like that for the VFS, i.e. let people give us a callback like onVFSChanged :: NormalizedUri -> VirtualFile -> DocumentChange -> VirtualFile -> m (). That would give people the path, the contents before and after, and also the change itself. Lots of details to get right, obviously.

I think that would work for HLS, and I think it might also work for you?

Since the library essentially provides the functions to update the VFS, wouldn't it be easier to have the handlers for the document change events default to those functions, and—if a user overrides the handlers for those events—put the onus on the user to call those functions to update the VFS? It'd just be a single function call that the user would need to add, and it would empower them to choose when and how it happens, rather than giving that power to the library and providing additional and non-standard callbacks for the user to use.

@michaelpj
Copy link
Collaborator

I would need to be able to react to each individual TextDocumentContentChangePartial event, so as long as the DocumentChange does not bunch multiple TextDocumentContentChangePartial events, that would work for me. If it doesn't, I would still need to update the rope manually.

I'm a little confused. I'm assuming that lsp would still update the rope, it would just also tell you that the change was happening so that you can do your incremental update of the thing that depends on the rope - the parse tree.

Surely you must be handing multiple updates together already since that's what the LSP protocol can give us, and I assume you're currently handling the file change notifications yourself?

Since the library essentially provides the functions to update the VFS, wouldn't it be easier to have the handlers for the document change events default to those functions, and—if a user overrides the handlers for those events—put the onus on the user to call those functions to update the VFS?

I have thought about this, basically pushing more towards an "assemble your own language server" design. But I would like us to provide some level of abstraction over the raw protocol, even if it's optional. I'd like people to be able to work at the level of "this file changed in this way" rather than "I got this notification from the client" 🤷

Overall I guess what I'm saying is that the right design isn't obvious to me here. I'll think about it, and please do make suggestions. Maybe "just turn off the VFS updating" works for you (although I'm still not quite clear how it helps) and we should just do that, but I don't know that it helps anyone else.

@wenkokke
Copy link
Author

wenkokke commented Jan 3, 2025

I would need to be able to react to each individual TextDocumentContentChangePartial event, so as long as the DocumentChange does not bunch multiple TextDocumentContentChangePartial events, that would work for me. If it doesn't, I would still need to update the rope manually.

I'm a little confused. I'm assuming that lsp would still update the rope, it would just also tell you that the change was happening so that you can do your incremental update of the thing that depends on the rope - the parse tree.

Surely you must be handing multiple updates together already since that's what the LSP protocol can give us, and I assume you're currently handling the file change notifications yourself?

I am, but the issue is that I must do some computation after applying each individual TextDocumentContentChangePartial to the rope, so if the library performs these updates in a batch, and only provides a callback for each batch—rather than for each individual update—I'm still in trouble.

@michaelpj
Copy link
Collaborator

Why can't you just do your computation sequentially starting from the initial rope for each update?

@wenkokke
Copy link
Author

wenkokke commented Jan 3, 2025

Why can't you just do your computation sequentially starting from the initial rope for each update?

That's what I'm currently doing. However, in the process of doing so, I have to apply each update to the rope, which means I'm already doing the work that the VFS is doing. So I'd either need a callback with the rope and the edit between each TextDocumentContentChangePartial or I might as well do all the work myself.

@wenkokke
Copy link
Author

wenkokke commented Jan 8, 2025

@michaelpj Would a possible alternative be for the LSP library to add byte ranges to the partial changes as its updating the VFS? That'd solve my issue entirely, would require much more work, and is required to interface with tree-sitter, which I assume would make it a common use case.

@wenkokke
Copy link
Author

wenkokke commented Jan 8, 2025

Reading through the code I'm also keenly aware that I'm probably applying these editing the wrong way, so having the lsp library take these things out of the consumer's hands might be a good idea, i.e., I am applying them in sequence, assuming their respective ranges are influenced by one another, instead of applying them bottom up.

@michaelpj
Copy link
Collaborator

Would a possible alternative be for the LSP library to add byte ranges to the partial changes as its updating the VFS

I guess I'm still not quite clear on what you need here. I think you're saying that you want to know for each change, what that change looks like at the byte level, i.e. byte range i-j changed to have content c? Which isn't all that cheap to compute either, since you need to know the document encoding and have the whole document available.

If that's the sort of thing that users need, then yeah, I would think we could provide that. I had been thinking we would use our existing DocumentChange type, but if we're providing our own interface for notifying changes then we can do whatever we want, and include byte ranges if that's helpful.

Reading through the code I'm also keenly aware that I'm probably applying these editing the wrong way, so having the lsp library take these things out of the consumer's hands might be a good idea, i.e., I am applying them in sequence, assuming their respective ranges are influenced by one another, instead of applying them bottom up.

That I think we can definitely do right. I would assume that whatever incremental parsing tool you're using will also want to receive the updates in order. But might also benefit from batching them? I don't know, you tell me!

@wenkokke
Copy link
Author

wenkokke commented Jan 14, 2025

Would a possible alternative be for the LSP library to add byte ranges to the partial changes as its updating the VFS

I guess I'm still not quite clear on what you need here. I think you're saying that you want to know for each change, what that change looks like at the byte level, i.e. byte range i-j changed to have content c? Which isn't all that cheap to compute either, since you need to know the document encoding and have the whole document available.

Yes, the byte level ranges are exactly what I need. It’s pretty much free while you’re doing the document update, since you’re already splitting the document in all the relevant places.

Reading through the code I'm also keenly aware that I'm probably applying these editing the wrong way, so having the lsp library take these things out of the consumer's hands might be a good idea, i.e., I am applying them in sequence, assuming their respective ranges are influenced by one another, instead of applying them bottom up.

That I think we can definitely do right. I would assume that whatever incremental parsing tool you're using will also want to receive the updates in order. But might also benefit from batching them? I don't know, you tell me!

The parser I’m using won’t re-parse until I ask it to, but does need to be notified about all intermediate changes. If there’s no dependence between the ranges of those changes, I suppose the order doesn’t matter all that much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants