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

Adjust owner in Interactive.contextOfPath causing crash in ImplicitSearch #19875

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

rochala
Copy link
Contributor

@rochala rochala commented Mar 4, 2024

Interactive provided us with the method contextOfPath which should return enclosing ctx for given position. It was working fine until given loop detection was improved some time ago.

It started crashing as the context owner was set to original context owner, instead of the real owner. This PR changes this and sets context to its outer context owner.

Fixes scalameta/metals#6193

@rochala rochala requested a review from smarter March 5, 2024 08:04
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM! I just now realized that maybe we could use the context returned by contextOfPath to determine which names can be printed without a qualifying path (so like ShortenedTypePrinter, but could be re-used for compiler error messages), does that make any sense?

@smarter
Copy link
Member

smarter commented Mar 5, 2024

Also it seems like this should be backported to 3.4.1. (I assume the given loop change isn't in 3.3 ?)

@smarter smarter added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Mar 5, 2024
@rochala
Copy link
Contributor Author

rochala commented Mar 5, 2024

LGTM! I just now realized that maybe we could use the context returned by contextOfPath to determine which names can be printed without a qualifying path (so like ShortenedTypePrinter, but could be re-used for compiler error messages), does that make any sense?

This is actually a good idea. I think current ShortenedTypePrinter does similar stuff using IndexedContext, but it also handles symbols that are about to be added e.g. if we already have Foo in scope, and want to complete other Foo from symbol index, it would prepend prefixes until the conflict is resolved.

If we approach this problem in error messages, we can shorten types to current scope.
This will be great improvement. I'm curious how it will handle errors coming from other source files, e.g.

// fileA.scala
package test.api
class Foo()
def test(x: Foo): Unit = ???
// fileB.scala
package test.newApi
class Foo(x: Int)
test.api.test(Foo(5)) // it should report 
  // type mismatch;
  // found:    newApi.Foo
  // required: api.Foo
  // test.api.test(Foo(5))
  //               ^^^^^^

@rochala rochala merged commit 9cc9107 into scala:main Mar 5, 2024
19 checks passed
Kordyjan pushed a commit to dotty-staging/dotty that referenced this pull request Mar 12, 2024
…tSearch` (scala#19875)

`Interactive` provided us with the method `contextOfPath` which should
return enclosing ctx for given position. It was working fine until given
loop detection was improved some time ago.

It started crashing as the context owner was set to original context
owner, instead of the real owner. This PR changes this and sets context
to its outer context owner.

Fixes scalameta/metals#6193
@Kordyjan Kordyjan added backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Mar 12, 2024
Kordyjan added a commit that referenced this pull request Mar 12, 2024
@Kordyjan Kordyjan added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Mar 19, 2024
@Kordyjan Kordyjan added this to the 3.4.1 milestone Mar 19, 2024
WojciechMazur pushed a commit that referenced this pull request Jul 2, 2024
…tSearch` (#19875)

`Interactive` provided us with the method `contextOfPath` which should
return enclosing ctx for given position. It was working fine until given
loop detection was improved some time ago.

It started crashing as the context owner was set to original context
owner, instead of the real owner. This PR changes this and sets context
to its outer context owner.

Fixes scalameta/metals#6193
[Cherry-picked 9cc9107]
WojciechMazur added a commit that referenced this pull request Jul 3, 2024
…n `ImplicitSearch`" to LTS (#20977)

Backports #19875 to the LTS branch.

PR submitted by the release tooling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion crashes with a Conversion requiring a using parameter
3 participants