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

fix: use alternative with stripped synthetic filename$package. #7000

Merged
merged 6 commits into from
Dec 9, 2024

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented Dec 3, 2024

fixes: #6852

Root cause of the problem

A toplevel object that is a companion for a top-level type has a synthetic parent - filename$package. This is not reflected by Metals symbol indexer (ScalaMtags). It would be problematic, since the the type definition might be after the object definition

Example

For this code:

// demo.scala
object Foo
type Foo = String

Symbols in compiler and what is in semanticDB is

_empty_/demo$package.Foo#
_empty_/demo$package.Foo.

Where Metals symbol indexer produces:

_empty_/demo$package.Foo#
_empty_/Foo.

Solution

  1. Add symbols with filename$package to alternatives in definition search
  2. When searching for docstring fallback to docs from found definition symbol

@kasiaMarek kasiaMarek force-pushed the i6852 branch 2 times, most recently from 018f323 to 86fdb34 Compare December 3, 2024 13:50
@@ -26,6 +29,27 @@ object DefinitionAlternatives {
Some(sym.owner -> sym.value.desc)
}

private def stripSyntheticPackageObjectFromObject(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to validate this? Could there be an option where we have both object with the $package prefix and without?

And if we really need to we should have something like:

case class Alternative(private val symbol: Symbol, validation: (loc: SymbolLocation) => Boolean = true) {
  def symbol(loc: SymbolLocation) = {
   if (validation(loc)) Some(symbol)
   else None
  }
}

this way we make sure that Validation is not ignored at any point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can skip it... One would have to specifically create an object / package object called something$package (where I think that $ is generally reserved for the compiler) and still this is only a fallback...

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe skip it then.

@kasiaMarek kasiaMarek requested a review from tgodzik December 6, 2024 09:10
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Nice idea! LGTM aside from the small nitpick

@@ -224,7 +224,7 @@ class SymbolIndexBucket(
if (!definitions.contains(symbol.value)) {
// Fallback 3: guess related symbols from the enclosing class.
DefinitionAlternatives(symbol)
.flatMap(alternative => query0(querySymbol, alternative))
.flatMap { case (alternative) => query0(querySymbol, alternative) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed now I think

@kasiaMarek kasiaMarek merged commit ac01eef into scalameta:main Dec 9, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

javadocs are suppressed within an object that shadows an opaque/alias type.
2 participants