Open sid-srini opened 1 month ago
Hi @mbien. Please review this PR and/or assign other reviewers, as per your convenience. Thanks.
@sid-srini I pinged a few who might be able to review this.
Overall, this makes sense to me. But I am thinking, what if I open a file from outside of the workspace. So, what if, just before the new code here, we would check if there's exactly one workspace, and if there is, we would use the single workspace. (Keeping the pre-existing code computing the workspace folders, as that is needed as a default working directory.) Having one workspace should be the typical thing.
What do you think?
Thank you @lahodaj for reviewing the PR.
So, what if, just before the new code here, we would check if there's exactly one workspace, and if there is, we would use the single workspace. (Keeping the pre-existing code computing the workspace folders, as that is needed as a default working directory.) Having one workspace should be the typical thing.
Yes, having one workspace is the typical use-case.
What do you think?
When I was adding this new code, my concern with this approach of defaulting to the single open workspace was that it may lead to a security/cross-talk issue if someone opens a file external to the regular workspace (with workspace folders).
Do you think that in case no suitable workspace is found, then this method should introduce adding a new Workspace
?
WorkspaceServiceImpl
and maybe WorkspaceService
and the scope of that seems very wide.Please let me know what you think.
Thank you.
Thank you @lahodaj for reviewing the PR.
So, what if, just before the new code here, we would check if there's exactly one workspace, and if there is, we would use the single workspace. (Keeping the pre-existing code computing the workspace folders, as that is needed as a default working directory.) Having one workspace should be the typical thing.
Yes, having one workspace is the typical use-case.
Note that the current default is that for every workspace/VS Code UI window, there is a separate backend started. I.e. this is not about the number of windows open, but rather whether the backend is shared by multiple windows, which is not the default.
What do you think?
When I was adding this new code, my concern with this approach of defaulting to the single open workspace was that it may lead to a security/cross-talk issue if someone opens a file external to the regular workspace (with workspace folders).
* The workspace arguments and VM options might contain credentials etc. which should not be sent to the workspace-external program, _even when its a trusted file_.
Isn't that a problem with this patch as well? If the backend is shared among multiple windows, and I have multiple windows with workspaces without folders, this patch will pick one of the UI windows/settings/clients semi-"randomly", no?
* Additionally, opening the same file when another workspace is open would lead to different runtime behaviour. * This would also be outside the user's control.
I suspect the same will be with this patch - if the backend is shared among windows, then one of then will be selected, no? And if the backend is not shared, then there's only one workspace, and all should work consistently?
* On the other hand, by using only the _global_ workspace, i.e. without any workspace folders, the user would have greater control. * It would be clear that only the global run configuration is in effect.
Do you think that in case no suitable workspace is found, then this method should introduce adding a new
Workspace
?* I think this might be harder to achieve since it would require changing `WorkspaceServiceImpl` and maybe `WorkspaceService` and the scope of that seems very wide.
The Workspace is basically just a part of the mirror that mirrors the UI Window. I don't think we can meaningfully create a new one.
Please let me know what you think.
Thank you.
Thanks @lahodaj for the explanation of the concrete implementation context.
The key differentiation in the patch is that a workspace with no folders is chosen to associate the single-file with.
The contrast scenario for this is to associate the single-file with a workspace having one-or-more folders.
Does that make sense?
(I might have diverged a bit in my previous comment, so sorry for that.)
I guess I don't see the problem with "leaking" some settings to a file that the user explicitly ran in the same window as the settings are defined. Not quite clear to me what's the problem with that - the user wants to run the file, and we read the relevant settings in the given window.
With the current proposed patch, if I open a file A
while having no workspace folders, things will work one way (running the file will get the settings), and once I add a folder to the workspace (that does not contain A
), things will work differently (running the file will not get the settings). Not quite sure if this is user-friendly.
Also note that with the current patch, (I believe) I can have a file open in window 1 and ran from there, but settings may be taken from window 2, just because window 2's workspace has no folders. Whether the settings are global or not does not seem all too relevant to me.
All in all, I wonder if we should simply permit out-of-workspace files with settings only when there's a single workspace (as then we can reliably say which workspace to use and we know the backend is attached to exactly one workspace/window), and keep the current behavior if we there are multiple workspaces.
Understood. It makes sense to keep this simple. I'll revise the patch and add to this PR. Thank you very much @lahodaj.
I've pushed the patch incorporating the suggestion above by @lahodaj. Please review. Thanks.
don't forget to squash before merge
don't forget to squash before merge
Thanks @mbien. I've squashed the commits.
Requesting a merge of this PR. Thank you.
Java LSP server: Associate a single source file with an available workspace with no client workspace folders in order to obtain the configured options
Issue
SingleFileOptionsQueryImpl
introduced search for a single source file's parent in open workspaces' folders.setConfiguration()
is invoked withnull
workDirectory
, the options for the workspace are not made available to single-source files which are outside any workspace folders.Effect
Errors such as those resolved by vmOptions such as
--enable-preview
or a--source <version>
continue to be flagged even when the single-source file can be run successfully.Fix
SingleFileOptionsQueryImpl.optionsFor()
:SingleFileOptionsQueryImplTest
.