Add packagePrefix to SourceItem

[lukaszwawrzyk]

A source set in Intellij has a property called packagePrefix so that if the source root is not located at root of package namespace it is know which package it belongs to.
For example:
for file src/test/Test.scala with content

package test
class Test

If the source set directory is /src/test, we should set package prefix to test, otherwise Intellij would think that test.Test class is actually _root_.Test.

Related PRs for integration:
scalacenter/bloop#1183
scalameta/metals#1470
JetBrains/intellij-scala#503

[jastice]

Thanks, looks good! Can you also add it to the specification and amend the generators?

[lukaszwawrzyk]

Thanks, looks good! Can you also add it to the specification and amend the generators?

I added the comment in specification.md, also updated the generators.

Would you be able to release this so I can proceed with other PRs?

[jvican]

Thanks for the contribution! I don't fully understand the motivation of this change.

If the source set directory is /src/test, we should set package prefix to test, otherwise Intellij would think that test.Test class is actually root.Test.

Why is the source set directory /src/test and why does IntelliJ think that? The root should be /src and if it's /src/test then IntelliJ shouldn't resolve that symbol to _root_. Looks like an issue that should be fixed on the IntelliJ side, either in the IntelliJ's project configuration or the presentation compiler.

I think we should keep the fields in SourceItem to a minimum and I'm concerned of adding a new language-specific field just to make the IntelliJ integration easier. Have you considered other ways of fixing this issue?

[lukaszwawrzyk]

Thanks for the contribution! I don't fully understand the motivation of this change.

If the source set directory is /src/test, we should set package prefix to test, otherwise Intellij would think that test.Test class is actually root.Test.

Why is the source set directory /src/test and why does IntelliJ think that? The root should be /src and if it's /src/test then IntelliJ shouldn't resolve that symbol to _root_. Looks like an issue that should be fixed on the IntelliJ side, either in the IntelliJ's project configuration or the presentation compiler.

I think we should keep the fields in SourceItem to a minimum and I'm concerned of adding a new language-specific field just to make the IntelliJ integration easier. Have you considered other ways of fixing this issue?

@jvican Thanks for looking at this!

This is due to pants model, that we can put BUILD files in any package to customize the build (e.g. dependencies). For example we can have following files:

src/main/bsp/BUILD
src/main/bsp/A.scala (with package 'bsp')
src/main/bsp/foo/BUILD
src/main/bsp/foo/B.scala (with package 'bsp.foo'

Generally, if there is a build file on some level, the parent directory is mapped to a module and to a source root of this module. For such inner modules, that have mismatch of package structure and source root, the package prefix is the way to say how it should be interpreted.
If we want to support pants through bsp, I think this kind of concept will be required. It should be enough to know the package prefix per BuildTarget (not source item) but still. Same problem would appear for bazel integration as well as other tools that use similar concept.
Do you see some other reasonable place to put it?

[jvican]

Thanks for the detailed response.

If we want to support pants through bsp, I think this kind of concept will be required. It should be enough to know the package prefix per BuildTarget (not source item) but still.

I think adding this information to the build target (scala and java build targets) instead of every source item is what makes the most sense. Then, build tools such as pants and bazel can export the right package prefix based on the source roots of the build target.

[jastice]

I think adding this information to the build target (scala and java build targets) instead of every source item is what makes the most sense.

I'm not sure. Wouldn't the prefix depend on the individual source root? Given that we can have sources in arbitrary places for any target.

But also, I realized it shouldn't really be part of SourceItem as it's a JVM-specific feature. So if we need it on every source item, it could be a kind of extended JvmSourceItem? OR some other way of communicating this information?

[jvican]

I'm not sure. Wouldn't the prefix depend on the individual source root? Given that we can have sources in arbitrary places for any target.

If I've understood correctly, a package only has one individual source root so this should be fine.

[jastice]

If I've understood correctly, a package only has one individual source root so this should be fine.

A package may have multiple source roots. Perhaps this is not relevant to this use case, but we should consider it.

[lukaszwawrzyk]

If I've understood correctly, a package only has one individual source root so this should be fine.

A package may have multiple source roots. Perhaps this is not relevant to this use case, but we should consider it.

Indeed, we can have individual files with different package prefix in the same directory with pants. So to respect the model, it should be at the level of SourceItem, perhaps handled with dataKind and data field like in BuildTarget?
On the other hand, this is not possible to be represented in Intellij so I don't personally worry about this now, at some point during the mapping it will still be required to just pick one package prefix per directory.

For now I am amending PRs with the version where I put this into ScalaBuildTarget. If we should go with SourceItem and it is acceptable to make it extensible in the way I mentioned, let me know I will update again.

[lukaszwawrzyk]

@jastice @jvican Do you think we should go with just ScalaBuildTarget or extend SourceItem or maybe something else?

[jvican]

I would like to keep the source item abstraction as simple as possible and would like to avoid adding repeated, language-specific information to it that bloats up the abstraction, so I prefer adding this information to the scala build target.

The case where a target has multiple source roots can be implemented by adding a list of source roots to a target and then having the tool consuming the information mapping every source to a source root based on the prefix. This is what makes the most sense to me. Most of the time, a build target only has one source root but when it does have more the tool consuming this information should be in charge of doing the work of mapping sources and source roots.

[lukaszwawrzyk]

I would like to keep the source item abstraction as simple as possible and would like to avoid adding repeated, language-specific information to it that bloats up the abstraction, so I prefer adding this information to the scala build target.

The case where a target has multiple source roots can be implemented by adding a list of source roots to a target and then having the tool consuming the information mapping every source to a source root based on the prefix. This is what makes the most sense to me. Most of the time, a build target only has one source root but when it does have more the tool consuming this information should be in charge of doing the work of mapping sources and source roots.

Ok, cool! In that case, do you think we can proceed with merging these and releasing bsp so I can go further with the remaining 3 PRs?

[jastice]

I would like to keep the source item abstraction as simple as possible and would like to avoid adding repeated, language-specific information to it that bloats up the abstraction, so I prefer adding this information to the scala build target.

The case where a target has multiple source roots can be implemented by adding a list of source roots to a target and then having the tool consuming the information mapping every source to a source root based on the prefix. This is what makes the most sense to me. Most of the time, a build target only has one source root but when it does have more the tool consuming this information should be in charge of doing the work of mapping sources and source roots.

I agree, this idea sounds very practical. Can you sketch how this would look? a kind of mapping
sourceRoot -> packagePrefix?

[olafurpg]

I agree with previous comments that it's undesirable to put this new field under SourceItem. I find it unsatisfying to add this to ScalaBuildTarget however since this is a "buildTargets/sources" related feature.

Would it make sense to update SourcesItem instead?

export interface SourcesItem {
  target: BuildTargetIdentifer;
  /** The text documents or and directories that belong to this build target. */
  sources: SourceItem[];
+  /** The root directories from where source files should be relativized.
+   * 
+   * Example: ["file://Users/name/dev/metals/src/main/scala"]
+   */
+  roots?: Uri[]; 
}

The contract would be that when relativizing a source file, the first root who owns that file would be used as a parent.

The BSP spec currently doesn't talk about "packages", only URIs. It would be the job of a client like IntelliJ to infer the package name by relativizing a given source item to the root URI.

[olafurpg]

I took a stab at implementing this change in Metals and the following logic seems to be a sufficient approximation for how Pants detects "source roots"

  def approximateSourceRoot(dir: Path): Option[Path] = {
    val pattern = FileSystems.getDefault.getPathMatcher(
      "glob:**/{main,test,tests,src,3rdparty,3rd_party,thirdparty,third_party}/{resources,scala,java,jvm,proto,python,protobuf,py}"
    )
    def loop(d: Path): Option[Path] = {
      if (pattern.matches(d)) Some(d)
      else {
        Option(d.getParent()) match {
          case Some(parent) => loop(parent)
          case None => None
        }
      }
    }
    loop(dir)
  }

approximateSourceRoot(
  Paths.get("foobar/src/main/scala/com/foobar/").toAbsolutePath()
)
res0: Option[Path] = Some(/Users/lgeirsson/dev/metals/foobar/src/main/scala)

This method attempts to reproduce the Python logic here
https://github.com/pantsbuild/pants/blob/cf26b534d562745b6e3add9a6b5185bd6fa3e7ab/src/python/pants/source/source_root.py#L140

Down the road, this method should not be necessary once this issue pantsbuild/pants#9266 is fixed in upstream Pants.

[jvican]

Would it make sense to update SourcesItem instead?

Yes, I like this solution even better.

[jastice]

Would it make sense to update SourcesItem instead?

I agree, I like this solution most so far

[olafurpg]

LGTM 👍 Thank you @lukaszwawrzyk for updating the PR

[jvican]

Perfect 👌 Thanks for the contribution @lukaszwawrzyk

[jastice]

Great, thanks for the update!

[olafurpg]

I'll work on getting a release out now