Fix engine to properly handle hidden fields #647 #627

volivan239 · 2022-08-01T11:00:14Z

Description

Now if class has hidden fields, they can be recognized by engine and correct models are built.

Fixes #647

Type of Change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Automated Testing

Switched org.utbot.examples.objects.HiddenFieldExampleTest#testCheckSuccField to enabled, added org.utbot.examples.objects.HiddenFieldAccessModifiersTest

Manual Scenario

No manual scenario for testing this.

Checklist (remove irrelevant options):

This is the author self-check list

The change followed the style guidelines of the UTBot project
Self-review of the code is passed
The change contains enough commentaries, particularly in hard-to-understand areas
New documentation is provided or existed one is altered
No new warnings
New tests have been added
All tests pass locally with my changes

volivan239 · 2022-08-01T12:14:06Z

This PR is derived from #592. See this comment for more details.

volivan239 · 2022-08-01T12:27:49Z

Problems discovered in previous PR that were fixed but the fixes were not reviewed:

Changes in IdUtil.kt (naming and logic)
Naming problem in Traverser.kt:642

Damtev · 2022-08-02T10:48:16Z

utbot-framework-api/src/main/kotlin/org/utbot/framework/plugin/api/util/IdUtil.kt

-fun ClassId.findFieldOrNull(fieldName: String): Field? = jClass.findFieldOrNull(fieldName)
+fun ClassId.findFieldByIdOrNull(fieldId: FieldId): Field? {
+    if (isNotSubtypeOf(fieldId.declaringClass)) {
+        return null
+    }
+
+    return fieldId.safeField
+}


Isn't it a breaking change - the previous version looked for all fields (including inherited) but this version looks only for declared fields (so, skipping inherited)?

This version looks for inherited fields as well: if field in this is inherited, then this should be a subtype of field's declaring class, so this method will find it.

Damtev · 2022-08-02T10:49:42Z

utbot-framework-api/src/main/kotlin/org/utbot/framework/plugin/api/util/IdUtil.kt


 // TODO: maybe cache it somehow in the future
 val FieldId.field: Field
-    get() = declaringClass.jClass.declaredFields.firstOrNull { it.name == name }
-        ?: error("Field $name is not found in class ${declaringClass.jClass.name}")
+    get() = safeField ?: error("Field $name is not found in class ${declaringClass.jClass.name}")


I guess, "found" should be replaced with "declared"

utbot-framework/src/main/kotlin/org/utbot/engine/Hierarchy.kt

utbot-framework/src/main/kotlin/org/utbot/engine/Strings.kt

Damtev · 2022-08-02T10:54:17Z

utbot-framework/src/main/kotlin/org/utbot/engine/Traverser.kt

+            else -> {
+                val id = field.fieldId
+                val jField = id.field
+                jField.let { it.withAccessibility { it.get(null) } }


Please, do not write this in one line

Damtev · 2022-08-02T11:08:14Z

utbot-framework/src/test/kotlin/org/utbot/examples/objects/HiddenFieldAccessModifiersTest.kt

+internal class HiddenFieldAccessModifiersTest : UtValueTestCaseChecker(testClass = HiddenFieldAccessModifiersExample::class) {
+    @Test
+    fun testCheckSuperFieldEqualsOne() {
+        // TODO: currently, codegen can't handle tests with field hiding


Create an issue and mention it here, please

Damtev · 2022-08-02T11:08:23Z

utbot-framework/src/test/kotlin/org/utbot/examples/objects/HiddenFieldExampleTest.kt

-            { o, r -> o.a != 1 && o.b != 2.0 && (o as HiddenFieldSuperClass).b != 3 && r == 4 },
-            coverage = DoNotCalculate
-        )
+        // TODO: currently, codegen can't handle tests with field hiding


Same about issue

dtim

Generally OK, but a fix in UtAbstractStringBuilderWrapper.value() needs correction.

As you fix only the engine part, I think it could be useful to create two separate issues, one for the engine, and another for the codegen ("children" of #563 and linked to it). That way you could reference (and close) the relevant issue and keep another issue open, and then close both the remaining issue and the parent issue with a second PR.

dtim · 2022-08-02T08:32:32Z

utbot-core/src/main/kotlin/org/utbot/common/KClassUtil.kt

-    }
-    .firstOrNull()
+/**
+ * Returns the Field that would be used by compiler when you try to direct access [fieldName] from code, i.e.


"directly access", "from the code"

dtim · 2022-08-02T08:39:36Z

utbot-core/src/main/kotlin/org/utbot/common/KClassUtil.kt

-    .firstOrNull()
+/**
+ * Returns the Field that would be used by compiler when you try to direct access [fieldName] from code, i.e.
+ * when you write `${this.name}.$fieldName`.


I think that ${this.name}.$fieldName expression may be hard to understand and a bit misguiding (as fieldName is not a variable). Maybe use something simpler, like obj.fieldName?

This is a valuable comment, but I found out that this function is not even needed in this PR, so this is to be fixed in #592. Same to the comment above

dtim · 2022-08-02T10:47:40Z

utbot-framework-api/src/main/kotlin/org/utbot/framework/plugin/api/util/IdUtil.kt

@@ -303,11 +310,12 @@ fun ClassId.defaultValueModel(): UtModel = when (this) {
 }

 // FieldId utils
+val FieldId.safeField: Field?
+    get() = declaringClass.jClass.declaredFields.firstOrNull { it.name == name }


JFYI: theoretically it is possible to have ClassId but not jClass, and to get ClassNotFoundException. We currently have this problem in other jClass-related code (e.g., ClassId.canonicalName, ClassId.simpleName), and it would be a very rare case (at least while lambdas are filtered out), so I think it's OK.

dtim · 2022-08-02T14:39:33Z

utbot-framework/src/main/kotlin/org/utbot/engine/Strings.kt

@@ -313,7 +313,7 @@ sealed class UtAbstractStringBuilderWrapper(className: String) : BaseOverriddenW

        val arrayValuesChunkId = typeRegistry.arrayChunkId(charArrayType)

-        val valuesFieldChunkId = hierarchy.chunkIdForField(utStringClass.type, overriddenClass.valueField)
+        val valuesFieldChunkId = hierarchy.chunkIdForField(utStringClass.type, utStringClass.valueField)


Replacement of overriddenClass.valueField with utStringClass.valueField seems incorrect, as we are dealing with UtStringBuilder/UtStringBuffer and not UtString. At the same time, the first argument utStringClass.type seems suspicious.

fixed, now overriddenClass is used in both places

dtim · 2022-08-02T14:42:37Z

utbot-framework/src/main/kotlin/org/utbot/engine/Traverser.kt

        when (field.signature) {
            SECURITY_FIELD_SIGNATURE -> SecurityManager()
            FIELD_FILTER_MAP_FIELD_SIGNATURE -> mapOf(Reflection::class to arrayOf("fieldFilterMap", "methodFilterMap"))
            METHOD_FILTER_MAP_FIELD_SIGNATURE -> emptyMap<Class<*>, Array<String>>()
-            else -> declaringClass.id.jClass.findField(field.name).let { it.withAccessibility { it.get(null) } }
+            else -> {
+                val id = field.fieldId


This code seems not very clear, too many ids. Maybe rename id to a more descriptive name?

replaced with fieldId

dtim · 2022-08-02T14:51:43Z

utbot-framework-api/src/main/kotlin/org/utbot/framework/plugin/api/util/IdUtil.kt


 // TODO: maybe cache it somehow in the future
 val FieldId.field: Field
-    get() = declaringClass.jClass.declaredFields.firstOrNull { it.name == name }
-        ?: error("Field $name is not found in class ${declaringClass.jClass.name}")
+    get() = safeField ?: error("Field $name is not found in class ${declaringClass.jClass.name}")


If we have a ClassId.jClass, maybe it would be consistent to rename FieldId.field to FieldId.jField?

dtim

LGTM

volivan239 mentioned this pull request Aug 1, 2022

Add support for classes with hidden fields #563 #592

Closed

7 tasks

volivan239 requested review from Damtev and dtim August 1, 2022 12:28

volivan239 marked this pull request as ready for review August 1, 2022 12:28

volivan239 changed the title ~~Fixed engine dealing with field hiding~~ Fix engine to properly handle hidden fields Aug 1, 2022

Damtev requested changes Aug 2, 2022

View reviewed changes

volivan239 mentioned this pull request Aug 2, 2022

Errors in test generation when there is variable hiding in src code #563

Closed

dtim requested changes Aug 2, 2022

View reviewed changes

volivan239 changed the title ~~Fix engine to properly handle hidden fields~~ Fix engine to properly handle hidden fields #647 Aug 2, 2022

volivan239 requested review from dtim and Damtev August 2, 2022 16:38

dtim approved these changes Aug 3, 2022

View reviewed changes

Damtev approved these changes Aug 3, 2022

View reviewed changes

Fix engine to properly handle hidden fields

96a72a7

volivan239 force-pushed the volivan239/fix_field_hiding_engine_only branch from 1dae4a8 to 96a72a7 Compare August 3, 2022 12:48

volivan239 merged commit affa753 into main Aug 3, 2022

volivan239 deleted the volivan239/fix_field_hiding_engine_only branch August 3, 2022 15:02

denis-fokin pushed a commit that referenced this pull request Aug 18, 2022

Fix engine to properly handle hidden fields #647 (#627)

20fe567

Fix engine to properly handle hidden fields #647 #627

Fix engine to properly handle hidden fields #647 #627

Uh oh!

Conversation

volivan239 commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

How Has This Been Tested?

Automated Testing

Manual Scenario

Checklist (remove irrelevant options):

Uh oh!

volivan239 commented Aug 1, 2022

Uh oh!

volivan239 commented Aug 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

volivan239 Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

volivan239 Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

volivan239 commented Aug 1, 2022 •

edited

Loading

volivan239 Aug 2, 2022 •

edited

Loading

volivan239 Aug 2, 2022 •

edited

Loading