Java LSP server: Associate a single source file with an available workspace with no client workspace folders in order to obtain the configured options

[sid-srini]

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

1. When a single-source file is opened in a workspace with no workspace folders, the global workspace options need to be made available to the operations done on that file.
   • In #7382, SingleFileOptionsQueryImpl introduced search for a single source file's parent in open workspaces' folders.
   • However, when setConfiguration() is invoked with null workDirectory, the options for the workspace are not made available to single-source files which are outside any workspace folders.
2. The netbeans IDE works around this by inserting 2 entries in the attributes.xml, one for the file and subsequently another one for its parent.
   • Example ~/Library/Application Support/NetBeans/dev/var/attributes.xml

   <?xml version="1.0" encoding="UTF-8"?>
   <!DOCTYPE attributes PUBLIC "-//NetBeans//DTD DefaultAttributes 1.0//EN" "http://www.netbeans.org/dtds/attributes-1_0.dtd">
   <attributes version="1.0">
       <fileobject name="|path|to|single|file">
           <attr name="single_file_vm_options" stringvalue="--enable-preview --source 23"/>
       </fileobject>
       <fileobject name="|path|to|single|file|Test.java">
           <attr name="single_file_vm_options" stringvalue="--enable-preview --source 23"/>
       </fileobject>
   </attributes>
   

3. However, the Java VS Code extension cannot achieve the same since the Java LSP server cannot apply this.

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.

• Example Test.java:

  public static void main(String... args) {
      System.out.println("Hello");
  }
  

• This can be reproduced in the IDE as well if the artificial entry added for the file's parent is removed from the attributes.xml as below:

  <?xml version="1.0" encoding="UTF-8"?>
  <!DOCTYPE attributes PUBLIC "-//NetBeans//DTD DefaultAttributes 1.0//EN" "http://www.netbeans.org/dtds/attributes-1_0.dtd">
  <attributes version="1.0">
      <fileobject name="|path|to|single|file|Test.java">
          <attr name="single_file_vm_options" stringvalue="--enable-preview --source 23"/>
      </fileobject>
  </attributes>
  

  • Visible error in the editor even though the file runs successfully:

Fix

1. Fixed SingleFileOptionsQueryImpl.optionsFor():
   • to: associate a single-source file's parent with a workspace having no client workspace folders,
   • when: it has no associated workspace folder.
2. Updated the associated unit tests in SingleFileOptionsQueryImplTest.

[sid-srini]

Hi @mbien. Please review this PR and/or assign other reviewers, as per your convenience. Thanks.

[mbien]

@sid-srini I pinged a few who might be able to review this.

[lahodaj]

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?

[sid-srini]

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).

• 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.
• 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.
• 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.

Please let me know what you think.

Thank you.

[lahodaj]

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.

[sid-srini]

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.

• In this sense, there is no extra conceptual distinction between multiple workspaces which do not have any folders associated with it. They may be viewed as instances of the same global settings.
  • The workspaces may exist concurrently or at different points of time.
  • Nonetheless, they are supposed to share the same configuration persisted on disk.
• Thus if someone opens a file in different such UI windows, at the same time, or across different points of time, the runtime configuration loaded is expected to be the same.

The contrast scenario for this is to associate the single-file with a workspace having one-or-more folders.

• In this case, my conception seems to be that this will hold and persist its unique configuration across different points of time.
• A workspace with folders may have a configuration or setting, that is different from the global settings.
  • In a very rare case, a workspace without folders may also have different settings than the global.
  • However, a workspace having folders is definitely not global.
• So, irrespective of the backend being shared across multiple UI windows, we can clearly avoid associating the non-global configurations of workspaces having folders.

Does that make sense?

[lahodaj]

(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.

[sid-srini]

Understood. It makes sense to keep this simple. I'll revise the patch and add to this PR. Thank you very much @lahodaj.

[sid-srini]

I've pushed the patch incorporating the suggestion above by @lahodaj. Please review. Thanks.

[mbien]

don't forget to squash before merge

[sid-srini]

don't forget to squash before merge

Thanks @mbien. I've squashed the commits.

[sid-srini]

Requesting a merge of this PR. Thank you.

[sid-srini]

Would someone please re-open this PR and merge it for NB24? It got closed due to an accidental deletion of the source branch but the branch is restored now. Thank you.

[sid-srini]

Requesting a merge of this PR. Thank you.

[sid-srini]

@mbien Requesting a merge of this PR. Thank you.

[sid-srini]

don't forget to squash before merge

@mbien Requesting a merge of this PR. Thank you.

[mbien]

Hi @sid-srini, I am not involved in the VSCode extension. I saw this PR and only mentioned squashing as a reminder since we had some accidental merges around that time. (thanks for squashing!)

But since the last CI run was a while ago, it would be good to rebase it on top of latest master (again) for a fresh CI run - since a lot happened including the repo-wide spec bump. I am sure one of the reviewers will be able to merge once the time is right - not sure why its still open - this repo has two schedules which might be a reason.

cc @lahodaj @dbalek @dbalek

[sid-srini]

But since the last CI run was a while ago, it would be good to rebase it on top of latest master (again) for a fresh CI run - since a lot happened including the repo-wide spec bump. I am sure one of the reviewers will be able to merge once the time is right - not sure why its still open - this repo has two schedules.

Thanks @mbien. I've pushed the commit rebased against the latest master. If possible, kindly trigger the CI build.

[mbien]

@sid-srini thanks! CI is already running.

[sid-srini]

@lahodaj @dbalek The CI checks passed successfully after rebase. Please merge this PR if its ok. Thank you.

[sid-srini]

@lahodaj @dbalek - I've rebased against the latest master. Kindly trigger the CI build and merge the PR if ok. Thank you.

[lahodaj]

Unless there are objections, I'll integrate sometime soon.

[mbien]

@sid-srini could you rebase this PR since a lot happened in the last ~3 weeks. thanks!

[sid-srini]

@sid-srini could you rebase this PR since a lot happened in the last ~3 weeks. thanks!

Sure. I've rebased onto the current master. Thanks @lahodaj and @mbien. Please trigger the CI and proceed if fine.