scala / scala-dev

Scala 2 team issues. Not for user-facing bugs or directly actionable user-facing improvements. For build/test/infra and for longer-term planning and idea tracking. Our bug tracker is at https://github.com/scala/bug/issues
Apache License 2.0
130 stars 15 forks source link

Use BSP for IntelliJ import rather than our custom `intellij` tasks? #778

Open retronym opened 3 years ago

retronym commented 3 years ago

Remaining issues:


IntelliJ could never correctly import our project as a standard SBT project. IIRC that was due to an assumption that scala-library was a third party dependency, whereas in our project we have it as an internal sub-project.

Instead, we have some custom SBT tasks that fill in template IntelliJ config from ./src/intellij/*.SAMPLE and combine it with result of the update task to fill in the classpath.

SBT 1.4.x introduced support for the Build Server Protocol (BSP), which is an alternative means to import the project structure into IntelliJ (or Metals, VSCode, ...).

We should try to migrate to this as the standard way to use our project in an IDE of the users choice.

Status

The import was successful for me. In the SBT shell I left running, I saw a notification that a new client has connected.

Here's what I saw in the Build->Sync output in IntelliJ:

Processing workspace/buildTargets
Processing buildTarget/sources
compiling 533 Scala sources and 33 Java sources to /Users/jz/code/scala-worktrees/scala-bsp-intellij/build/quick/classes/library ...
/Users/jz/code/scala-worktrees/scala-bsp-intellij/src/library/scala/reflect/package.scala:61:27: @nowarn annotation does not suppress any warnings
    if (!m.isAccessible: @nowarn("cat=deprecation")) {
                          ^
one warning found
compiling 160 Scala sources and 3 Java sources to /Users/jz/code/scala-worktrees/scala-bsp-intellij/build/quick/classes/reflect ...
/Users/jz/code/scala-worktrees/scala-bsp-intellij/src/reflect/scala/reflect/internal/util/AbstractFileClassLoader.scala:114:43: @nowarn annotation does not suppress any warnings
    case null => super.getPackage(name): @nowarn("cat=deprecation")
                                          ^
one warning found
compiling 326 Scala sources and 5 Java sources to /Users/jz/code/scala-worktrees/scala-bsp-intellij/build/quick/classes/compiler ...
/Users/jz/code/scala-worktrees/scala-bsp-intellij/src/compiler/scala/tools/nsc/ast/TreeBrowsers.scala:218:76: @nowarn annotation does not suppress any warnings
      val menuKey = Toolkit.getDefaultToolkit().getMenuShortcutKeyMask(): @nowarn("cat=deprecation") // deprecated since JDK 10, replacement only available in 10+
                                                                           ^
one warning found
compiling 61 Scala sources and 1 Java source to /Users/jz/code/scala-worktrees/scala-bsp-intellij/test/benchmarks/target/scala-2.13/classes ...
41 deprecations (since 2.13.0)
1 deprecation (since 2.13.2)
10 deprecations (since 2.13.3)
52 deprecations in total; re-run with -deprecation for details
four warnings found
Processing buildTarget/dependencySources
Updating 
Resolved  dependencies
Updating 
Resolved  dependencies
Fetching artifacts of 
Fetched artifacts of 
Updating 
Resolved  dependencies
Fetching artifacts of 
Fetched artifacts of 
Fetching artifacts of 
Fetched artifacts of 
Fetching artifacts of 
Fetched artifacts of 
Fetching artifacts of 
Fetched artifacts of 
Fetching artifacts of 
Fetched artifacts of 
Fetching artifacts of 
Fetched artifacts of 
Processing buildTarget/scalacOptions

Notes:

lrytz commented 3 years ago

I've been using IntelliJ (including JUnit tests) in this mode for a few months and it's overall working well.

I had to start sbt in a terminal before doing an import / build refresh in IntelliJ, at least in the past, maybe that's fixed now.

I think the main drawback is the long delay until the IDE is ready when the build changes (common when switching branches - not switching betweeen 2.12 and 2.13, but even switching between 2.13 branches). And losing code-assist in the build itself.

Jasper-M commented 3 years ago

Sorry if this message is redundant, but if you can use bloop instead of the sbt server you should still get code-assist in the build.

retronym commented 3 years ago

Sorry if this message is redundant, but if you can use bloop instead of the sbt server you should still get code-assist in the build.

I'm working on adding to SBT's BSP server too. WIP: https://github.com/retronym/sbt/pull/3

retronym commented 3 years ago

I think the main drawback is the long delay until the IDE is ready when the build changes

I think the problem here is that evaluating bench / Jmh / bspBuildTargetSourcesItem actually runs JMH source code generator. This is what triggers compilation of library. IntelliJ would be perfectly happy knowing that bench/target/scala-2.13/src_managed will be a root directory for generated sources, but the BSP server instead eagerly generates the sources and provides the full list of them.

I guess it does this so that builds the directly override the behaviour of managedSources are correctly represented, rather than relying on the convention that they should only return files within one of managedSourceDirectories. I guess for many types of generated sources the user wants them eagerly generated so they can code against the APIs. I feel like this should be a separate BSP operation though that isn't on the critical path to getting the project loaded.

We could probably override:

    bspBuildTargetSourcesItem := {
      val id = bspTargetIdentifier.value
      val dirs = unmanagedSourceDirectories.value
      val managed = managedSources.value
      val items = (dirs.toVector map { dir =>
        SourceItem(dir.toURI, SourceItemKind.Directory, generated = false)
      }) ++
        (managed.toVector map { x =>
          SourceItem(x.toURI, SourceItemKind.File, generated = true)
        })
      SourcesItem(id, items)
    }

In the scala build to be:

val dirs = unmanagedSourceDirectories.value ++ managedSourceDirectories.value
val items = (dirs.toVector map { dir =>
    SourceItem(dir.toURI, SourceItemKind.Directory, generated = false)
  })
  SourcesItem(id, items)

Or, we might be able to tag the bench / Jmh scope with the no-ide tag so it isn't imported at all. We should still be able to edit the code.

retronym commented 3 years ago

I've just noticed another problem: the "Refresh All BSP Targets" action doesn't pick up changes to the build definitions that occurred during the lifetime of the BSP server process: https://youtrack.jetbrains.com/issue/SCL-19230 [fixed]

lrytz commented 3 years ago

On current 2.13.x, importing / refreshing the BSP build still trigger compilation of the compiler (and library, reflect). I managed to narrow it down to the buildTarget/resources call, which runs bspBuildTargetResources file:/Users/luc/scala/scala13/#repl/Compile [... a lot more ...] in sbt. The contender is bspBuildTargetResources file:/Users/luc/scala/scala13/#scaladoc/Compile, it can be reproduced by just calling scaladoc/resources in sbt, which triggers compiler/compile.

I'm out of time for today, but it should hopefully be an easy fix.

lrytz commented 3 years ago

https://github.com/scala/scala/pull/9749

lrytz commented 3 years ago

updated the issue description with a list of remaining issues

jastice commented 2 years ago

https://youtrack.jetbrains.com/issue/SCL-19300 has been fixed in the meantime