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

Add implementation for BSP workspace/reload #852

Closed
wants to merge 13 commits into from

Conversation

Gedochao
Copy link
Contributor

@Gedochao Gedochao commented Apr 5, 2022

bsp_reload_2

The reload generally works now, but there are some limitations on IntelliJ:

  • if there are sources directly in the primary workspace directory (first input arg passed to the scala-cli setup-ide command), they will not be included in the new build after the reload if the project hash has changed.
  • The reason for this is the old problem with intellij-scala creating an empty *-root module. Even though we work around it on BSP initialization (as per changes in Adjust BSP project base directory to scala-cli content root to fix IntelliJ import #780), it becomes harder when handling a workspace/reload request with changed sources. If the source directories change on reload, the build target ids also change (the bsp project has a new hash). As a result, some targets need to be deleted, while others are created and intellij-scala creates the *-root module in between, as it's their hack for there always being a module with the primary workspace source directory.

image

  • Now, this is tolerable if the sources are hidden in subdirectories (screenshot above), as then this only causes an empty, harmless module to be added to the IntelliJ project structure. It gets worse if the workspace root directory also contains sources, as then we have a duplicate, and IntelliJ immediately gets rid of it just as we reload. This is because IntelliJ never allows for 2 modules to have overlapping source directories (and in all likelihood, the pre-reload build & the post-reload build share the root workspace directory). Thus, even though we reload correctly, we miss any sources in the root directory. (screenshot below)

image

  • There may be some workaround for this, but I think it should be handled outside of the scope of this PR. Currently if a user runs into this, they have to either fix the project structure manually (by adding the correct sources directory to the BSP module and getting rid of the *-root module) or get rid of the .idea directory and reimport the project. Quite annoying 😞
  • This issue is absent from Metals, of course.


val inputArgs = inputs.elements.collect { case d: Inputs.OnDisk => d.path.toString }

val ideInputs = IdeInputs(args =
options.shared.validateInputArgs(args)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't a lot of overhead, but the args are effectively validated twice here (once for the IdeInputs instance, and once for the Inputs instance). I intend to try and refactor this in a future PR.

message: String,
errorCode: Int = JsonRpcErrorCodes.InternalError
): ResponseError =
new ResponseError(errorCode, message, new Object())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though we return a valid error response to the workspace/reload request, it doesn't seem like IntelliJ is handling it properly, as no error message is shown in the UI. We may consider sending a BSP logMessage request, so the IntelliJ user has at least a slight chance of noticing that something went wrong (it'd show in the BSP build log, not just the BSP logs in the IntelliJ directory). showMessage isn't an option either, as IntelliJ ignores that as well. TBD, I'm open to suggestions (although I'd rather do it in a separate PR as well)

@Gedochao
Copy link
Contributor Author

Gedochao commented Apr 7, 2022

We're going ahead with a reworked version of this in #858, closing this.

@Gedochao Gedochao closed this Apr 7, 2022
alexarchambault added a commit that referenced this pull request Apr 7, 2022
Add support for BSP workspace/reload (reworked version of #852)
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

Successfully merging this pull request may close these issues.

1 participant