Kotlin 2 #97
Kotlin 2 #97
Conversation
Run `gradle splotlessApply`, remove trailing whitesepace and file endings.
Also remove the in-repo copy of Semanticdb.java (it is autogenerated by protoc outside the source tree), and the unused krotoconfig.json, but keep the SemanticdbBuilders.kt that used to be autogenerated by Kroto+.
This was deprecated in Kotlin 1.9 already.
Squashed version of slycodemonkey@3701395.
This is from titooan@3ddcc1c
We don't want the output schema to change, otherwise we're not outputing SemanticDB anymore and downstream tooling like scip-java reject our output. We start with 24 failing tests to fix.
Its cast always fails
According to existing tests, exceptions in the semanticdb-kotlinc plugin should not propagate to the host. Down to 23 failing tests.
We want the symbol for a class to be package/package/Class#, instead this gave us `package/package/Class`#. Now we have just Class#, packages will be added back in a future commit. The display name should just be the simple class name. Still 23 failing tests.
This: - Adds special handling for packages, which are represented by FqName rather than PackageDescriptor. → This gives package/package/Class# instead of just Class#. - Removes FileKt# fake classes for toplevel methods and properties These don't appear in imports, so I don't think they are wanted. This changes the test expectations, but remains SemanticDB-compliant. → This gives kotlin/io/println(). instead of kotlin/io/ConsoleKt#println(). Down to 19 failing tests.
SemanticDB and SCIP don't disambiguate methods by adding their full signature, instead they sort overloaded methods by declaration order, the first one is `method().`, the next one is `method(+1).`, the next one `method(+2).` etc. Down to 14 failing tests.
Identify getters/setters using FirPropertyAccessorSymbol.isGetter and FirPropertyAccessorSymbol.isSetter instead of doing string comparisons. Still 14 failing tests.
Down to 13 failing tests.
Constructors should have the special `<init>`(). symbol. Down to 11 failing tests.
We are generally interested in the identifier for each item, but sometimes want some other token instead, such as the constructor, get or set keywords. Down to 5 failing tests.
We need to support the package directive and the imports so that: - `package a.b.c` has 3 symbol occurences, for `a/`, `a/b/` and `a/b/c/` - `import a.b.Klass` has 3 symbol occurences, for `a/`, `a/b/` and `a/b/Klass#` Still 5 failing tests.
Down to 4 failing tests.
We don't want all types to list Any as their supertype. Down to 3 failings tests.
Down to 2 failing tests.
Down to 1 failing test.
When we have a reference to a FirPropertySymbol, it is not obvious if it is used as a read or a write. Emit both just in case. No more failing tests!
Class overriddenSymbols were not pointing to actual symbols. Function overriddenSymbols were missing.
We don't want to call all anonymous objects <anonymous>, or we can't desambiguate between two anonymous objects from the same package. We emit anonymous objects just like regular classes, but take care to point at the `object` keyword for the class and primary constructor.
This was caught by Searchfox's scip-indexer, which would error out:
```
[ERROR scip_indexer] InvalidLocalSymbol("local 84getCurrentIndex().")
```
...cdb-kotlinc/src/test/kotlin/com/sourcegraph/semanticdb_kotlinc/test/SemanticdbSymbolsTest.kt
Show resolved
Hide resolved
|
@nicolas-guichard I've done some testing of this new implementation and I can confirm that it works in the wider scip-java project (I'm not finished yet). One thing that popped out is that using Kotlin 2.2. with this scip-kotlin PR crashes: In the past my understanding was that our usages of kotlin compiler APIs were prone to breaking with every minor version – this makes versioning strategy quite annoying. Questions for you:
|
Yes it's unfortunately still the case. The fix for that would be to rewrite the indexer using the Kotlin Analysis API, which “offers both source and binary backward compatibility for its stable parts”. But:
Since making this PR I have upgraded to Kotlin 2.2.0 in https://github.com/mozsearch/semanticdb-kotlinc/tree/kotlin-2.2.0, along with an extra bugfix for type operators (
I can't really tell about the broader community, since I'm not really a Kotlin dev either ;-)
We use semanticdb-kotlinc and semanticdb-javac as part of the Firefox build process (not through scip-java), so bumping the indexer plugin along with the Kotlin version isn't too much of an issue for us. I can see how this makes running |
|
Thanks for you responses, that's very useful!
Yeah, that's a deal breaker then. Shame.
Nice, hopefully we can bring those changes in soon (to the next minor).
Nice! How has your usage been so far? Gradle aggressively resists automatic injection of plugins or dependencies, so adding semanticdb plugins manually to the build is likely out future recommendation as I'm tired of fighting with Gradle. |
The initial implementation wasn't too hard: it was mainly a case of conditionally adding the compiler plugins to the build when the build is meant for indexing, and making sure that everything gets built even though we were not running the tests. The biggest issue we've had is that we lost Kotlin indexing when migrating to Kotlin 2 while I figured things out, and we didn't know what plans you had in #82. For a quick overview of our setup:
|
|
Thank you for this gargantuan amount of work @nicolas-guichard! |
This is based on https://github.com/slycodemonkey/scip-kotlin, which replaces the
AnalysisHandlerExtensionwith aFirAdditionalCheckersExtensionto collect declarations and expressions and anIrGenerationExtensionto analyze them (at the FIR level) after everything has been resolved.Fixes #82
Test plan
All existing tests pass (or were slightly adapted, eg. class and method signatures).
This is currently used in a test instance of Searchfox at https://kdab.searchfox.org/mozilla-central/source, see eg. https://kdab.searchfox.org/mozilla-central/source/mobile/android/android-components/components/browser/domains/src/main/java/mozilla/components/browser/domains/Domain.kt.
This means the output is accepted by
scip-java index-semanticdbthen Searchfox'sscip-indexeron the whole Firefox for Android code base.New tests were added based on my testing with Searchfox.