-
Notifications
You must be signed in to change notification settings - Fork 319
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
Outline compiler #6114
Outline compiler #6114
Conversation
dd965c4
to
e5982ab
Compare
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.
Thanks for working on this!
Let's also add a user setting to enable the new logic.
mtags-interfaces/src/main/java/scala/meta/pc/CompilerFiles.java
Outdated
Show resolved
Hide resolved
val richCompilationCache: TrieMap[String, RichCompilationUnit] = | ||
TrieMap.empty[String, RichCompilationUnit] | ||
|
||
// for those paths units were |
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.
We should add a bit more to the comment here
34ac473
to
f3967a7
Compare
|
||
def successfulCompilation(): Unit = { | ||
wasSuccessfullyCompiled.set(true) | ||
changedDocuments.clear() |
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.
Note: that this doesn't account for possible changes before compilation end is reported, it may not 100% reflect the state of files that was during compilation.
.asJava | ||
Some( | ||
OutlineFiles( | ||
allFiles, |
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.
We may want to cap the number of files for initial compilation, so it won't take too long.
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.
We can do a high number like 300 or something
6675382
to
d1060a8
Compare
Failing Scala3 NIGHTLY test is a test-only problem, so it shouldn't block this PR. |
metals/src/main/scala/scala/meta/internal/metals/InteractiveSemanticdbs.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/OutlineFilesProvider.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/OutlineFilesProvider.scala
Outdated
Show resolved
Hide resolved
8c58a4c
to
5cd84e8
Compare
else false | ||
} | ||
|
||
// TODO the same in autoimports TEST |
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.
Do we check auto imports? Is the TODO still relevant?
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 added tests, this seems to be already handled
5cd84e8
to
56d4ce5
Compare
dd3c21e
to
1499749
Compare
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 minor comments, but I also tested it on Metals codebase this workflow seems to break:
- Add non compiling method to the MD5 object in mtagsShared
- Change the name of MD5 to MD55
- Try to auto import MD55 in another package.
- We get completions for MD5 aside from MD55 and something like
MD55.<local>
Could you take a look if you can reproduce it?
@@ -101,6 +104,10 @@ class Compilers( | |||
|
|||
import compilerConfiguration._ | |||
|
|||
val plugins = new CompilerPlugins() |
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.
val plugins = new CompilerPlugins() |
I don't think it's used here now.
s"no build target found for $path. Using presentation compiler with project's scala-library version: ${scalaVersion}" | ||
) | ||
Some(fallbackCompiler) | ||
case None => Some(fallbackCompiler) |
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.
Why remove this?
} provider.didChange(path) | ||
} | ||
|
||
def getOutlineFiles(id: String): Optional[JOutlineFiles] = |
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.
What is this id? Should we use BuildTargetIdentifier here? Or at least add an alias.
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.
It is BuildTargetIdentifier
, I'm constructing it here.
def getOutlineFiles(id: String): Optional[JOutlineFiles] =
getOutlineFiles(buildTargetId(id))
def getOutlineFiles(
buildTargetId: Option[BuildTargetIdentifier]
)
It's just that it comes as a string from pc (can't be BuildTargetIdentifier
, because pc interface doesn't depend on bsp4j
) and it's less boilerplate to map it here instead of each place in Compilers
.
b1bb0e7
to
0656479
Compare
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.
Let's wait for the next release and merge it right after.
Great work!
1f7ab2f
to
a15c8af
Compare
a15c8af
to
5a327c4
Compare
connected to: #917