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().")
```
| displayName = "x" | ||
| language = Language.KOTLIN | ||
| documentation { | ||
| message = | ||
| "```kotlin\npublic val x: kotlin.String\n```\n\n----\n\n\nhello world\n test content\n" | ||
| "```kotlin\npublic get(): String\n```\n\n----\n\n\nhello world\n test content\n" |
There was a problem hiding this comment.
Hm, interesting – I'd expected this to be kotlin.String still as it seems to be a separate class in Kotlin: https://github.com/JetBrains/kotlin/blob/whyoleg/dokka2-sync-stdlib/libraries/stdlib/jvm/builtins/String.kt#L12
There was a problem hiding this comment.
The builtin String class and kotlin.String class are one and the same. From the file you link to (which is kotlin.String):
All string literals in Kotlin programs, such as
"abc", are implemented as instances of this class.
If you go to https://searchfox.org/mozilla-central/source/mobile/android/android-components/components/browser/domains/src/main/java/mozilla/components/browser/domains/Domain.kt and look at the builtin String class references (there's no import kotlin.String), you'll see that they get recorded as references to the kotlin/String# SCIP symbol.
There was a problem hiding this comment.
Sorry, I should've clarified – in Java and Scala we fully qualify even the classes available from predef, e.g. in Scala String = scala/Predef.String#, in Java String = java/lang/String#
Especially in a mixed-language codebase (e.g. Java + Kotlin) those fully qualified names allow for cross-language referencing.
BUT I'm only now noticing that this is a signature documentation output, where using short names is totally fine (I see in other tests kotlin/Boolean# which is correct behaviour).
Sorry if I'm being slow/dense – not only do I not work with Kotlin, I haven't touched this project, so the review is going to be slow, as this is more of a rewrite (a necessary one) :D
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.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.