Add support for classes with hidden fields #563 #592

volivan239 · 2022-07-26T14:17:45Z

Description

Now if class has hidden fields, they can be recognized by engine and correct code for instantiation of such classes will be generated.

Fixes #563

Type of Change

Please delete options that are not relevant.

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

Tested plugin on examples from #563 -- generates tests the way as expected.

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

CaelmBleidd · 2022-07-28T13:32:16Z

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

-        }
-    }
-    .firstOrNull()
+fun Class<*>.findDirectAccessedField(fieldName: String): Field? = generateSequence(this) { it.superclass }


Please, add documentation here. Especially about what 'directAccessed' means

Added documentation for this method

CaelmBleidd · 2022-07-28T13:33:19Z

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

@@ -855,7 +856,7 @@ open class FieldId(val declaringClass: ClassId, val name: String) {
        return result
    }

-    override fun toString() = declaringClass.findFieldOrNull(name).toString()
+    override fun toString() = declaringClass.fieldOrNull(this).toString()


Why not findField anymore?

Previously it was a method that was finding Field by its name. Now, when the FieldId is passed as argument, it is not really finding -- it's more like checking that this field belongs to this class and returning Field.

Renamed to fieldByIdOrNull for greater clarity

CaelmBleidd · 2022-07-28T13:33:55Z

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

+    if (this.isNotSubtypeOf(fieldId.declaringClass))
+        return null


Please, do not omit brackets for multiline constructions. Minor: redundant this

CaelmBleidd · 2022-07-28T13:37:23Z

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

+    return try {
+        fieldId.field
+    } catch (e: IllegalStateException) {
+        null


I believe, it is exception-dependent logic. Why you can't do it without throwing an exception? It is not a cheap operation after all. Moreover, please, add an empty line before the return

fixed, although it's a little bit copy-pasted now

CaelmBleidd · 2022-07-28T13:38:02Z

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

-        }
-    }
-    .firstOrNull()
+fun Class<*>.findDirectAccessedField(fieldName: String): Field? = generateSequence(this) { it.superclass }


Why it is not orNull anymore?

Renamed to findDirectAccessedFieldOrNull

CaelmBleidd · 2022-07-28T13:53:07Z

.../src/main/kotlin/org/utbot/framework/codegen/model/constructor/tree/CgVariableConstructor.kt

-            if (canFieldBeDirectlySetByVariableAndFieldTypeRestrictions && fieldId.canBeSetIn(testClassPackageName)) {
-                // TODO: check if it is correct to use declaringClass of a field here
-                val fieldAccess = if (field.isStatic) CgStaticFieldAccess(fieldId) else CgFieldAccess(obj, fieldId)
+            if (field != null && field.type.id == variableForField.type && fieldId.canBeSetIn(testClassPackageName)) {


@Damtev or @EgorkaKulikov, please, look over these changes with code generation

There are no functional changes, but I am a bit confused with this TODO removing. Are you sure it is resolved?

CaelmBleidd · 2022-07-28T13:53:40Z

utbot-framework/src/main/kotlin/org/utbot/framework/codegen/model/util/DslUtil.kt

+    if (type.jClass.findDirectAccessedField(fieldId.name) != fieldId.field)
+        CgFieldAccess(CgTypeCast(fieldId.declaringClass, this), fieldId)
+    else
+        CgFieldAccess(this, fieldId)


CaelmBleidd · 2022-07-28T13:54:32Z

utbot-framework/src/test/kotlin/org/utbot/examples/UtModelTestCaseChecker.kt

@@ -167,8 +165,8 @@ internal abstract class UtModelTestCaseChecker(
    /**
     * Finds field model in [UtCompositeModel] and [UtAssembleModel]. For assemble model supports direct field access only.
     */
-    protected fun UtModel.findField(fieldName: String): UtModel =
-        findField(this.classId.jClass.findField(fieldName).fieldId)
+    protected fun UtModel.findField(fieldName: String, declarationClass: ClassId = this.classId): UtModel =


is declaration the right word?

replaced with declaring

CaelmBleidd · 2022-07-28T13:55:49Z

utbot-sample/src/main/java/org/utbot/examples/objects/HiddenFieldSuccClass.java

+    @Override
+    public String toString() {
+        return b + ". Super b: " + super.b;
+    }


Isn't it a forgotten method implementation?

This override really seems useless, removed it

CaelmBleidd · 2022-07-28T13:57:02Z

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

+            { o, _ -> o == null },
+            { _, r -> r == true },
+            { _, r -> r == false},
+            coverage = DoNotCalculate


added coverage here and to org.utbot.examples.objects.HiddenFieldExampleTest#testCheckSuccField

Damtev · 2022-07-29T14:24:30Z

.../src/main/kotlin/org/utbot/framework/codegen/model/constructor/tree/CgVariableConstructor.kt

-            if (canFieldBeDirectlySetByVariableAndFieldTypeRestrictions && fieldId.canBeSetIn(testClassPackageName)) {
-                // TODO: check if it is correct to use declaringClass of a field here
-                val fieldAccess = if (field.isStatic) CgStaticFieldAccess(fieldId) else CgFieldAccess(obj, fieldId)
+            if (field != null && field.type.id == variableForField.type && fieldId.canBeSetIn(testClassPackageName)) {


There are no functional changes, but I am a bit confused with this TODO removing. Are you sure it is resolved?

Damtev · 2022-07-29T14:30:25Z

utbot-framework/src/main/kotlin/org/utbot/framework/codegen/model/util/DslUtil.kt

-    CgFieldAccess(this, fieldId)
+    if (type.jClass.findDirectAccessedFieldOrNull(fieldId.name) != fieldId.field) {
+        CgFieldAccess(CgTypeCast(fieldId.declaringClass, this), fieldId)
+    } else {
+        CgFieldAccess(this, fieldId)
+    }


This functionality was just a shortcut for a CgFieldAccess constructor. I don't think we should use more complex logic with additional checks here.

Also, this logic does not always work - there is no check that fieldId.declaringClass is accessible, so there is possible situation with cast to a private class.

What is the best place to move this logic to? Before calling this there are checks that fieldId is accessible, aren't they enough?

Also, I believe that this logic should solve the removed TODO: it checks if we should use obj or declaring class as a caller. But maybe I misunderstood the TODO

Damtev · 2022-07-29T14:31:11Z

utbot-framework/src/main/kotlin/org/utbot/framework/codegen/model/visitor/CgKotlinRenderer.kt

+        if (element.caller is CgTypeCast) print("(")
        element.caller.accept(this)
+        if (element.caller is CgTypeCast) print(")")


If lhs is typecast, we need brackets for correct priority. Without this, the generated code will be like Succ as Super.f and not (Succ as Super).f.

This is a little bit of a hack, but I didn't find more general solution.

This is definitely a hack, which is also not total - it does not cover similar cases for a method invocation and an array access. We need to find another solution.

volivan239 · 2022-08-01T12:11:09Z

After discussion with @Damtev and @ArsenHD it was decided to merge into main only changes concerning engine (#627) so far. It was also decided to leave codegen untouched until @ArsenHD finishes his changes in codegen so we would have more rational ways of dealing with field hiding there.

volivan239 · 2022-09-07T13:44:36Z

Corresponding issue was finally solved in #763, closing this PR therefore.

volivan239 added 4 commits July 22, 2022 15:50

Fixed engine issues with field hiding

1e9587e

Fixed codegen issues with hidden fields

55bd35a

Fixed method/variable constructors & refactored findField methods

8e840ba

Minor codestyle improvements

66761f6

volivan239 requested review from CaelmBleidd and EgorkaKulikov July 26, 2022 14:19

volivan239 changed the title ~~Add support for classes with hidden fields~~ Add support for classes with hidden fields #563 Jul 26, 2022

volivan239 added 3 commits July 27, 2022 11:37

Fixed behavior to match tests

f6419ee

Fixed Hierarchy.chunkIdForField & minor changes

2583bd8

Fixed call of chunkIdForField in StringWrapper

fb6f19b

volivan239 marked this pull request as ready for review July 28, 2022 07:27

CaelmBleidd requested changes Jul 28, 2022

View reviewed changes

volivan239 added 2 commits July 29, 2022 15:10

Codestyle & minor changes

f080f49

renamed fieldById->findFieldById, deleted redundant stuff

eb7393b

volivan239 requested a review from CaelmBleidd July 29, 2022 14:26

Damtev requested changes Jul 29, 2022

View reviewed changes

volivan239 added 2 commits August 1, 2022 11:14

Removed incorrect assertions

94d8eca

Minor refactoring of IdUtils

78ed251

volivan239 removed the request for review from EgorkaKulikov August 1, 2022 11:49

This was referenced Aug 1, 2022

Fix engine to properly handle hidden fields #647 #627

Merged

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

Closed

volivan239 closed this Sep 7, 2022

Add support for classes with hidden fields #563 #592

Add support for classes with hidden fields #563 #592

Uh oh!

Conversation

volivan239 commented Jul 26, 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!

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!

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!

Damtev Jul 29, 2022 • edited by volivan239 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!

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!

Damtev Jul 29, 2022 • edited by volivan239 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!

Choose a reason for hiding this comment

Uh oh!

volivan239 commented Aug 1, 2022

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

volivan239 commented Jul 26, 2022 •

edited

Loading

Damtev Jul 29, 2022 •

edited by volivan239

Loading

Damtev Jul 29, 2022 •

edited by volivan239

Loading

volivan239 commented Sep 7, 2022 •

edited

Loading