Kotlin: Handle properties better

2026-05-04 13:15:21 +02:00 · 2021-11-12 14:41:33 +00:00
parent 44bf35e623
commit e6e56238c5
8 changed files with 232 additions and 17 deletions
--- a/java/kotlin-extractor/src/main/kotlin/KotlinExtractorExtension.kt
+++ b/java/kotlin-extractor/src/main/kotlin/KotlinExtractorExtension.kt
@@ -937,18 +937,30 @@ class X {
        return id
    }

-    fun getPropertyLabel(p: IrProperty) : String {
+    fun getFieldLabel(p: IrField) : String {
        val parentId = useDeclarationParent(p.parent)
        val label = "@\"field;{$parentId};${p.name.asString()}\""
        return label
    }

-    fun useProperty(p: IrProperty): Label<out DbField> {
-        var label = getPropertyLabel(p)
+    fun useField(p: IrField): Label<out DbField> {
+        var label = getFieldLabel(p)
        val id: Label<DbField> = tw.getLabelFor(label)
        return id
    }

+    fun getPropertyLabel(p: IrProperty) : String {
+        val parentId = useDeclarationParent(p.parent)
+        val label = "@\"property;{$parentId};${p.name.asString()}\""
+        return label
+    }
+
+    fun useProperty(p: IrProperty): Label<out DbKt_property> {
+        var label = getPropertyLabel(p)
+        val id: Label<DbKt_property> = tw.getLabelFor(label)
+        return id
+    }
+
    private fun getEnumEntryLabel(ee: IrEnumEntry) : String {
        val parentId = useDeclarationParent(ee.parent)
        val label = "@\"field;{$parentId};${ee.name.asString()}\""
@@ -1216,12 +1228,15 @@ open class KotlinFileExtractor(
                    tw.writeExprs_assignexpr(assignmentId, type.javaResult.id, type.kotlinResult.id, stmtId, 0)
                    tw.writeHasLocation(assignmentId, declLocId)

+                    /*
+                    TODO
                    val lhsId = tw.getFreshIdLabel<DbVaraccess>()
                    val lhsType = useType(backingField.type)
                    tw.writeExprs_varaccess(lhsId, lhsType.javaResult.id, lhsType.kotlinResult.id, assignmentId, 0)
                    tw.writeHasLocation(lhsId, declLocId)
                    val vId = useProperty(decl) // todo: fix this. We should be assigning the field, and not the property
                    tw.writeVariableBinding(lhsId, vId)
+                    */

                    extractExpressionExpr(initializer.expression, obinitId, assignmentId, 1)
                }
@@ -1239,7 +1254,7 @@ open class KotlinFileExtractor(
        }
    }

-    fun extractFunction(f: IrFunction, parentId: Label<out DbReftype>) {
+    fun extractFunction(f: IrFunction, parentId: Label<out DbReftype>): Label<out DbCallable> {
        currentFunction = f

        f.typeParameters.map { extractTypeParameter(it) }
@@ -1274,24 +1289,57 @@ open class KotlinFileExtractor(
        }

        currentFunction = null
+        return id
+    }
+
+    fun extractField(f: IrField, parentId: Label<out DbReftype>): Label<out DbField> {
+        val id = useField(f)
+        val locId = tw.getLocation(f)
+        val type = useType(f.type)
+        tw.writeFields(id, f.name.asString(), type.javaResult.id, type.kotlinResult.id, parentId, id)
+        tw.writeHasLocation(id, locId)
+        return id
    }

    fun extractProperty(p: IrProperty, parentId: Label<out DbReftype>) {
+        val id = useProperty(p)
+        val locId = tw.getLocation(p)
+        tw.writeKtProperties(id, p.name.asString())
+        tw.writeHasLocation(id, locId)
+
        val bf = p.backingField
-        if(bf == null) {
-            logger.warnElement(Severity.ErrorSevere, "IrProperty without backing field", p)
+        val getter = p.getter
+        val setter = p.setter
+
+        if(getter != null) {
+            @Suppress("UNCHECKED_CAST")
+            val getterId = extractFunction(getter, parentId) as Label<out DbMethod>
+            tw.writeKtPropertyGetters(id, getterId)
        } else {
-            val id = useProperty(p)
-            val locId = tw.getLocation(p)
-            val type = useType(bf.type)
-            tw.writeFields(id, p.name.asString(), type.javaResult.id, type.kotlinResult.id, parentId, id)
-            tw.writeHasLocation(id, locId)
+            logger.warnElement(Severity.ErrorSevere, "IrProperty without a getter", p)
+        }
+
+        if(setter != null) {
+            if(!p.isVar) {
+                logger.warnElement(Severity.ErrorSevere, "!isVar property with a setter", p)
+            }
+            @Suppress("UNCHECKED_CAST")
+            val setterId = extractFunction(setter, parentId) as Label<out DbMethod>
+            tw.writeKtPropertySetters(id, setterId)
+        } else {
+            if(p.isVar) {
+                logger.warnElement(Severity.ErrorSevere, "isVar property without a setter", p)
+            }
+        }
+
+        if(bf != null) {
+            val fieldId = extractField(bf, parentId)
+            tw.writeKtPropertyBackingFields(id, fieldId)
        }
    }

    fun extractEnumEntry(ee: IrEnumEntry, parentId: Label<out DbReftype>) {
        val id = useEnumEntry(ee)
-        val locId = tw.getLocation(ee)
        val parent = ee.parent
        if(parent !is IrClass) {
            logger.warnElement(Severity.ErrorSevere, "Enum entry with unexpected parent: " + parent.javaClass, ee)
@@ -1300,6 +1348,7 @@ open class KotlinFileExtractor(
        } else {
            val type = useSimpleTypeClass(parent, emptyList(), false)
            tw.writeFields(id, ee.name.asString(), type.javaResult.id, type.kotlinResult.id, parentId, id)
+            val locId = tw.getLocation(ee)
            tw.writeHasLocation(id, locId)
        }
    }
--- a/java/kotlin-extractor/src/main/kotlin/utils/Logger.kt
+++ b/java/kotlin-extractor/src/main/kotlin/utils/Logger.kt
@@ -67,7 +67,7 @@ open class Logger(val logCounter: LogCounter, open val tw: TrapWriter) {
    fun trace(msg: String, exn: Exception) {
        trace(msg + " // " + exn)
    }
-    fun warn(severity: Severity, msg: String, locationString: String? = null, locationId: Label<DbLocation> = tw.unknownLocation) {
+    fun warn(severity: Severity, msg: String, locationString: String? = null, mkLocationId: () -> Label<DbLocation> = { tw.unknownLocation }) {
        val warningLoc = getWarningLocation()
        val warningLocStr = if(warningLoc == null) "<unknown location>" else warningLoc
        val suffix =
@@ -84,6 +84,8 @@ open class Logger(val logCounter: LogCounter, open val tw: TrapWriter) {
                }
            }
        val ts = timestamp()
+        // We don't actually make the location until after the `return` above
+        val locationId = mkLocationId()
        tw.writeDiagnostics(StarLabel(), "CodeQL Kotlin extractor", severity.sev, "", msg, "$ts $msg\n$suffix", locationId)
        val locStr = if (locationString == null) "" else "At " + locationString + ": "
        print("$ts Warning($warningLocStr): $locStr$msg\n$suffix")
@@ -118,7 +120,7 @@ class FileLogger(logCounter: LogCounter, override val tw: FileTrapWriter): Logge

    fun warnElement(severity: Severity, msg: String, element: IrElement) {
        val locationString = tw.getLocationString(element)
-        val locationId = tw.getLocation(element)
-        warn(severity, msg, locationString, locationId)
+        val mkLocationId = { tw.getLocation(element) }
+        warn(severity, msg, locationString, mkLocationId)
    }
 }
--- a/java/ql/lib/config/semmlecode.dbscheme
+++ b/java/ql/lib/config/semmlecode.dbscheme
@@ -948,7 +948,8 @@ javadocText(

 /** A program element that has a name. */
@element = @file | @package | @primitive | @class | @interface | @method | @constructor | @modifier | @param | @exception | @field |
-           @annotation | @boundedtype | @array | @localvar | @expr | @stmt | @import | @fielddecl | @kt_type | @kt_type_alias;
+           @annotation | @boundedtype | @array | @localvar | @expr | @stmt | @import | @fielddecl | @kt_type | @kt_type_alias |
+           @kt_property;

@modifiable = @member_modifiable| @param | @localvar ;

@@ -960,7 +961,7 @@ javadocText(
@locatable = @file | @class | @interface | @fielddecl | @field | @constructor | @method | @param | @exception
             | @boundedtype | @typebound | @array | @primitive
             | @import | @stmt | @expr | @whenbranch | @localvar | @javadoc | @javadocTag | @javadocText
-             | @xmllocatable | @ktcomment | @kt_type_alias;
+             | @xmllocatable | @ktcomment | @kt_type_alias | @kt_property;

@top = @element | @locatable | @folder;

@@ -1108,3 +1109,23 @@ ktExtensionFunctions(
  int typeid: @type ref,
  int kttypeid: @kt_type ref
 )
+
+ktProperties(
+  unique int id: @kt_property,
+  string nodeName: string ref
+)
+
+ktPropertyGetters(
+  unique int id: @kt_property ref,
+  int getter: @method ref
+)
+
+ktPropertySetters(
+  unique int id: @kt_property ref,
+  int setter: @method ref
+)
+
+ktPropertyBackingFields(
+  unique int id: @kt_property ref,
+  int backingField: @field ref
+)
--- a/java/ql/lib/semmle/code/Location.qll
+++ b/java/ql/lib/semmle/code/Location.qll
@@ -45,6 +45,8 @@ predicate hasName(Element e, string name) {
  modifiers(e, name)
  or
  kt_type_alias(e, name, _)
+  or
+  ktProperties(e, name)
 }

 /**
--- a/java/ql/lib/semmle/code/java/Member.qll
+++ b/java/ql/lib/semmle/code/java/Member.qll
@@ -686,6 +686,20 @@ class InstanceField extends Field {
  InstanceField() { not this.isStatic() }
 }

+/** A Kotlin property. */
+class Property extends Element, @kt_property {
+  /** Gets the getter method for this property, if any. */
+  Method getGetter() { ktPropertyGetters(this, result) }
+
+  /** Gets the setter method for this property, if any. */
+  Method getSetter() { ktPropertySetters(this, result) }
+
+  /** Gets the backing field for this property, if any. */
+  Field getBackingField() { ktPropertyBackingFields(this, result) }
+
+  override string getAPrimaryQlClass() { result = "Property" }
+}
+
 /** A Kotlin extension function. */
 class ExtensionMethod extends Method {
  Type extendedType;
--- a/java/ql/test/kotlin/library-tests/properties/properties.expected
+++ b/java/ql/test/kotlin/library-tests/properties/properties.expected
@@ -0,0 +1,20 @@
+| properties.kt:2:27:2:50 | constructorProp | properties.kt:2:27:2:50 | <get-constructorProp> | file://:0:0:0:0 | <none> | properties.kt:2:27:2:50 | constructorProp |
+| properties.kt:2:53:2:83 | mutableConstructorProp | properties.kt:2:53:2:83 | <get-mutableConstructorProp> | properties.kt:2:53:2:83 | <set-mutableConstructorProp> | properties.kt:2:53:2:83 | mutableConstructorProp |
+| properties.kt:3:5:3:25 | modifiableInt | properties.kt:3:5:3:25 | <get-modifiableInt> | properties.kt:3:5:3:25 | <set-modifiableInt> | properties.kt:3:5:3:25 | modifiableInt |
+| properties.kt:4:5:4:24 | immutableInt | properties.kt:4:5:4:24 | <get-immutableInt> | file://:0:0:0:0 | <none> | properties.kt:4:5:4:24 | immutableInt |
+| properties.kt:5:5:5:26 | typedProp | properties.kt:5:5:5:26 | <get-typedProp> | file://:0:0:0:0 | <none> | properties.kt:5:5:5:26 | typedProp |
+| properties.kt:6:5:6:38 | abstractTypeProp | properties.kt:6:14:6:38 | <get-abstractTypeProp> | file://:0:0:0:0 | <none> | file://:0:0:0:0 | <none> |
+| properties.kt:7:5:7:30 | initialisedInInit | properties.kt:7:5:7:30 | <get-initialisedInInit> | file://:0:0:0:0 | <none> | properties.kt:7:5:7:30 | initialisedInInit |
+| properties.kt:11:5:11:40 | useConstructorArg | properties.kt:11:5:11:40 | <get-useConstructorArg> | file://:0:0:0:0 | <none> | properties.kt:11:5:11:40 | useConstructorArg |
+| properties.kt:12:5:13:21 | five | properties.kt:13:13:13:21 | <get-five> | file://:0:0:0:0 | <none> | file://:0:0:0:0 | <none> |
+| properties.kt:14:5:15:21 | six | properties.kt:15:13:15:21 | <get-six> | file://:0:0:0:0 | <none> | file://:0:0:0:0 | <none> |
+| properties.kt:16:5:18:40 | getSet | properties.kt:17:13:17:33 | <get-getSet> | properties.kt:18:13:18:40 | <set-getSet> | file://:0:0:0:0 | <none> |
+| properties.kt:19:5:20:15 | defaultGetter | properties.kt:20:13:20:15 | <get-defaultGetter> | file://:0:0:0:0 | <none> | properties.kt:19:5:20:15 | defaultGetter |
+| properties.kt:21:5:22:15 | varDefaultGetter | properties.kt:22:13:22:15 | <get-varDefaultGetter> | properties.kt:21:5:22:15 | <set-varDefaultGetter> | properties.kt:21:5:22:15 | varDefaultGetter |
+| properties.kt:23:5:24:15 | varDefaultSetter | properties.kt:23:5:24:15 | <get-varDefaultSetter> | properties.kt:24:13:24:15 | <set-varDefaultSetter> | properties.kt:23:5:24:15 | varDefaultSetter |
+| properties.kt:25:5:27:15 | varDefaultGetterSetter | properties.kt:26:13:26:15 | <get-varDefaultGetterSetter> | properties.kt:27:13:27:15 | <set-varDefaultGetterSetter> | properties.kt:25:5:27:15 | varDefaultGetterSetter |
+| properties.kt:28:5:29:22 | overrideGetter | properties.kt:29:13:29:22 | <get-overrideGetter> | properties.kt:28:5:29:22 | <set-overrideGetter> | properties.kt:28:5:29:22 | overrideGetter |
+| properties.kt:30:5:31:29 | overrideGetterUseField | properties.kt:31:13:31:29 | <get-overrideGetterUseField> | properties.kt:30:5:31:29 | <set-overrideGetterUseField> | properties.kt:30:5:31:29 | overrideGetterUseField |
+| properties.kt:32:5:33:29 | useField | properties.kt:33:13:33:29 | <get-useField> | file://:0:0:0:0 | <none> | properties.kt:32:5:33:29 | useField |
+| properties.kt:34:5:34:36 | lateInitVar | properties.kt:34:14:34:36 | <get-lateInitVar> | properties.kt:34:14:34:36 | <set-lateInitVar> | properties.kt:34:5:34:36 | lateInitVar |
+| properties.kt:59:1:59:23 | constVal | properties.kt:59:7:59:23 | <get-constVal> | file://:0:0:0:0 | <none> | properties.kt:59:1:59:23 | constVal |
--- a/java/ql/test/kotlin/library-tests/properties/properties.kt
+++ b/java/ql/test/kotlin/library-tests/properties/properties.kt
@@ -0,0 +1,59 @@
+
+abstract class properties(val constructorProp: Int, var mutableConstructorProp: Int, extractorArg: Int) {
+    var modifiableInt = 1
+    val immutableInt = 2
+    val typedProp: Int = 3
+    abstract val abstractTypeProp: Int
+    val initialisedInInit: Int
+    init {
+        initialisedInInit = 4
+    }
+    val useConstructorArg = extractorArg
+    val five: Int
+            get() = 5
+    val six
+            get() = 6
+    var getSet
+            get() = modifiableInt
+            set(v) { modifiableInt = v }
+    val defaultGetter = 7
+            get
+    var varDefaultGetter = 8
+            get
+    var varDefaultSetter = 9
+            set
+    var varDefaultGetterSetter = 10
+            get
+            set
+    var overrideGetter = 11
+            get() = 12
+    var overrideGetterUseField = 13
+            get() = field + 1
+    val useField = 14
+            get() = field + 1
+    lateinit var lateInitVar: String
+    fun useProps(): Int {
+        return 0 +
+            constructorProp +
+            mutableConstructorProp +
+            modifiableInt +
+            immutableInt +
+            typedProp +
+            abstractTypeProp +
+            initialisedInInit +
+            useConstructorArg +
+            five +
+            six +
+            getSet +
+            defaultGetter +
+            varDefaultGetter +
+            varDefaultSetter +
+            varDefaultGetterSetter +
+            overrideGetter +
+            overrideGetterUseField +
+            useField +
+            constVal
+    }
+}
+
+const val constVal = 15
--- a/java/ql/test/kotlin/library-tests/properties/properties.ql
+++ b/java/ql/test/kotlin/library-tests/properties/properties.ql
@@ -0,0 +1,48 @@
+import java
+
+newtype TMaybeElement =
+  TElement(Element e) or
+  TNoElement()
+
+class MaybeElement extends TMaybeElement {
+  abstract string toString();
+  abstract Location getLocation();
+}
+
+class YesMaybeElement extends MaybeElement {
+  Element e;
+
+  YesMaybeElement() { this = TElement(e) }
+  override string toString() { result = e.toString() }
+  override Location getLocation() { result = e.getLocation() }
+}
+
+class NoMaybeElement extends MaybeElement {
+  NoMaybeElement() { this = TNoElement() }
+
+  override string toString() { result = "<none>" }
+  override Location getLocation() { none() }
+}
+
+MaybeElement getter(Property p) {
+  if exists(p.getGetter())
+  then result = TElement(p.getGetter())
+  else result = TNoElement()
+}
+
+MaybeElement setter(Property p) {
+  if exists(p.getSetter())
+  then result = TElement(p.getSetter())
+  else result = TNoElement()
+}
+
+MaybeElement backingField(Property p) {
+  if exists(p.getBackingField())
+  then result = TElement(p.getBackingField())
+  else result = TNoElement()
+}
+
+from Property p
+where p.fromSource()
+select p, getter(p), setter(p), backingField(p)
+