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

bugfix: in semanticdb make synthetic apply disambiguator consistent w/ Scala 2 implicit #17341

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented Apr 25, 2023

Previously:
For the following code:

case class Foo(a: Int, b: Int)

object Foo {
  def apply(a: Int): Foo = Foo(a, 1)
 }

In SemanticDB the synthetic apply in Scala 3 would get () disambiguator:

_empty_/Foo.apply(). => method apply(a: Int, b: Int): Foo
_empty_/Foo.apply(+1). => method apply(a: Int): Foo

where, in Scala 2 implementation the synthetic apply gets (+1) disambiguator:

_empty_/Foo.apply(). => method apply(a: Int): Foo
_empty_/Foo.apply(+1). => method apply(a: Int, b: Int): Foo

Now:
We make sure that for overloaded methods synthetic ones are added at the end to fix the incompatibility w/ Scala 2 implementation.

connected to: scalameta/metals#4769

@anatoliykmetyuk
Copy link
Contributor

You can use partition instead of sort to make it linear.

@kasiaMarek kasiaMarek force-pushed the semanticdb-apply-disambiguator branch from 373f0e7 to d194619 Compare May 8, 2023 08:58
@bishabosha bishabosha force-pushed the semanticdb-apply-disambiguator branch from d194619 to 2f65f35 Compare July 14, 2023 10:33
Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

awesome, thanks a lot! sorry for the delay

@bishabosha bishabosha enabled auto-merge (squash) July 14, 2023 10:36
@bishabosha bishabosha disabled auto-merge July 14, 2023 10:36
@bishabosha bishabosha enabled auto-merge July 14, 2023 10:38
@filipwiech
Copy link

Hello @kasiaMarek, gentle ping. 😉 Is there anything left to do here? Would be great to get this in time for 3.3.2 if possible (no pressure of course). 🙂

@bishabosha
Copy link
Member

bishabosha commented Sep 8, 2023

needs a rebase, otherwise I'm not sure why it didn't automerge before.

Edit: im doing the rebase, there was probably another semanticdb change that was merged at the same time as the CI was running and caused the conflict

@bishabosha bishabosha force-pushed the semanticdb-apply-disambiguator branch from 2f65f35 to bef7a4a Compare September 8, 2023 12:05
@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Sep 8, 2023
@bishabosha bishabosha modified the milestone: 3.3.2 Sep 8, 2023
@kasiaMarek
Copy link
Contributor Author

@bishabosha, tests failed, though it doesn't seem connected to the fix

@bishabosha
Copy link
Member

bishabosha commented Sep 12, 2023

Thanks, I restarted the CI because as you say it looked like some unrelated network error

@bishabosha bishabosha merged commit a37dac6 into scala:main Sep 12, 2023
14 checks passed
@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 10, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
@tgodzik
Copy link
Contributor

tgodzik commented Dec 27, 2023

@Kordyjan was this backported or will be added in 3.3.3 ?

@tgodzik
Copy link
Contributor

tgodzik commented Dec 27, 2023

Ach nvm. I see it's in the queue

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.

6 participants