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: correctly import companion object for apply method #4567

Merged
merged 2 commits into from Nov 7, 2022

Conversation

jkciesluk
Copy link
Member

@jkciesluk jkciesluk commented Oct 25, 2022

fixes #4066

Previously, when choosing apply method completion we would import incorrect symbol, now we import companion object.

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.

Thanks for quickly fixing this! I have a single suggestion, maybe it can be a bit simpler this way.

Comment on lines 280 to 282
val forImportSymbol = v match
case w: CompletionValue.Workspace => w.applyOwner
case _ => sym
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's enough to check it here? And not add a new field?

Suggested change
val forImportSymbol = v match
case w: CompletionValue.Workspace => w.applyOwner
case _ => sym
val forImportSymbol =if (v.symbol.isMethod) v.symbol.owner else v.symbol

it would also work for expressions within string interpolation.

Copy link
Member Author

@jkciesluk jkciesluk Oct 25, 2022

Choose a reason for hiding this comment

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

Unfortunately we can't, because for example in ListBuffer(), owner of the apply method is trait IterableFactory. We have to somehow remember from which object this method came from

Copy link
Member Author

Choose a reason for hiding this comment

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

I will see what we can do in case of string interpolation completions

Copy link
Member

Choose a reason for hiding this comment

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

It seems suspicious that we are getting apply method symbols as Workspace completion.
In theory, workspace completion can be created only from a type or companion object symbol as they comes from classpath search.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that was added by me when I added apply completions

) extends Symbolic:
override def isFromWorkspace: Boolean = true
val applyOwner: Symbol = maybeApplyOwner.getOrElse(symbol)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could have importSymbol with a default of symbol and put it into the main Symbolic trait?

|}""".stripMargin,
"""|import scala.collection.mutable.ListBuffer
|object Main {
| val a = s"${ListBuffer($0)}""
Copy link
Member Author

Choose a reason for hiding this comment

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

There is still an issue in case like

val a = s"$ListBuff@@"

For now we don't provide apply completion there, because shouldAddSnippet flag is false in string interpolations (there is a problem with adding {}). I will work on that later

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.

LGTM!

@tgodzik tgodzik merged commit d3b8472 into scalameta:main Nov 7, 2022
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.

Scala 3: Invalid import inserted when apply method is completed
3 participants