-
Notifications
You must be signed in to change notification settings - Fork 1.1k
improvement: Rework IndexedContext to reuse the previously calculated scopes #22898
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
Conversation
I wonder if this strategy helps #22430 (which I haven't returned to yet) Edit: I guess only in the sense of "moral support". |
I think I am actually doing it all wrong and might be possible that IndexedContext is not really needed 😅 |
3e69d9e
to
21aa3b8
Compare
d27ca2c
to
376f628
Compare
… scopes It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does. There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now.
@@ -48,6 +48,16 @@ case class Completion(label: String, description: String, symbols: List[Symbol]) | |||
|
|||
object Completion: | |||
|
|||
def scopeContext(pos: SourcePosition)(using Context): CompletionResult = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would get deduplicated with normal rawCompletions being invoked, but I will try to do it as a separate step afterwards.
@Test def `sealed-conflict` = | ||
check( | ||
""" | ||
|object A { | ||
| val e: Either[Int, String] = ??? | ||
| type Left = String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type val actually fine here, but val causes an error
@@ -655,7 +655,7 @@ class InlayHintsSuite extends BaseInlayHintsSuite { | |||
| val y/*: S<<scala/collection/Set#>>[Char<<scala/Char#>>]*/ = f | |||
| ??? | |||
| } | |||
| val x/*: AB<<scala/collection/AbstractMap#>>[Int<<scala/Int#>>, String<<scala/Predef.String#>>]*/ = test(Set/*[Int<<scala/Int#>>]*/(1), Set/*[Char<<scala/Char#>>]*/('a')) | |||
| val x/*: AB<<scala/collection/AbstractMap#>>[Int<<scala/Int#>>, String<<java/lang/String#>>]*/ = test(Set/*[Int<<scala/Int#>>]*/(1), Set/*[Char<<scala/Char#>>]*/('a')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was inconsistent previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks and questions, but overall looks really cool 🎉
presentation-compiler/src/main/dotty/tools/pc/IndexedContext.scala
Outdated
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/IndexedContext.scala
Outdated
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/IndexedContext.scala
Outdated
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/NamedArgCompletions.scala
Show resolved
Hide resolved
if !id.symbol.is(Synthetic) && !id.symbol.is(Implicit) => | ||
symbols + tree.symbol | ||
case sel: Select => | ||
indexedContext.lookupSym(sel.symbol) match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm questioning this right now. Shouldn't we actually just collect the symbol of the qualifier? (Sort of a side comment not connected to the PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing the +
method etc. to be reported as conflicting, which was not really a problem.
… conflciting constructors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
… scopes (scala#22898) It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does. There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now.
… scopes (scala#22898) It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does. There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now. [Cherry-picked 9d90ff5][modified]
… scopes (scala#22898) It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does. There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now.
… scopes (scala#22898) It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does. There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now. [Cherry-picked 9d90ff5][modified]
… scopes (scala#22898) It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does. There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now.
… scopes (scala#22898) It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does. There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now. [Cherry-picked 9d90ff5][modified]
It turns out the work being done in IndexedContext was already done in Completions, but better, since it doesn't try to read files as the separate logic does.
There is still some improvement to be done to not calculate it twice, but in order to keep this PR as simple as possible I will skip that for now.