New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace Kotlin getters/setters with property access in codegen #496 #1002

Merged

volivan239 merged 9 commits into main from volivan239/fix_kotlin_property_access_rendering

Sep 27, 2022

Collaborator

volivan239 commented Sep 23, 2022 •

edited

Loading

Description

Now codegen properly treats Kotlin's autogenerated getters/setters and uses direct property access instead of them.

Kotlin properties are treated as fields. However, instead of FieldId.isAccessibleFrom which is now private, methods FieldId.canBeReadFrom and FieldId.canBeSetFrom should be used. First one behaves for Java code in the same way as isAccessibleFrom did before, but for Kotlin it also returns true if there is an accessible getter. Behavior of FieldId.canBeSetFrom hasn't been changed for Java, but for Kotlin it now also returns true if there is accessible setter/getter pair. See docs for more details.

These two methods guarantee that codegen produces code with field access where it is possible. However, some calls to getters/setters may be produced by AssembleModelGenerator, so this PR also adds replaceCgExecutableCallWithFieldAccessIfNeeded to replace such calls with direct accesses.

Fixes #496

Type of Change

Please delete options that are not relevant.

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

How Has This Been Tested?

Manual Scenario

Works correctly on examples from #496 and #948

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 requested a review from EgorkaKulikov

September 23, 2022 13:38

volivan239 marked this pull request as ready for review

September 23, 2022 13:38

volivan239 force-pushed the volivan239/fix_kotlin_property_access_rendering branch 2 times, most recently from b9fb58a to 31ddce3 Compare

September 24, 2022 16:58

EgorkaKulikov reviewed

View reviewed changes

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

-                                  +createCgExecutableCallFromUtExecutableCall(statementModel)
+                                  val call = createCgExecutableCallFromUtExecutableCall(statementModel)
+                                  val callOrAccess: CgStatement = replaceCgExecutableCallWithFieldAccessIfNeeded(call)
+                                  if (callOrAccess is CgExecutableCall)

Collaborator

EgorkaKulikov Sep 27, 2022

Improve this code fragment to make it more readable, please

Collaborator Author

volivan239 Sep 27, 2022

Should be clearer now. Also, changed return type of replaceCgExecutableCallWithFieldAccessIfNeeded to be nullable and added docs to it

EgorkaKulikov reviewed

View reviewed changes

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

                       val initExpr = if (isPrimitiveWrapperOrString(type)) {
                           cgLiteralForWrapper(params)
                       } else {
+                          // TODO: if instantiation chain could be a setter call, we need to replace it in Kotlin

Collaborator

EgorkaKulikov Sep 27, 2022

We have an instantiation call, now instantiation chain right now. Anyway, I can't imagine a situation when the top-level instantiation call is a setter.

Collaborator Author

volivan239 Sep 27, 2022

Thanks for advice, deleted the question

EgorkaKulikov reviewed

View reviewed changes

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

                       return cgCall
                   }
+                  private fun replaceCgExecutableCallWithFieldAccessIfNeeded(call: CgExecutableCall): CgStatement {
+                      if (call !is CgMethodCall || context.codegenLanguage != CodegenLanguage.KOTLIN)

Collaborator

EgorkaKulikov Sep 27, 2022

For me it seems better to write code-specific logic in a way like

when (context.codegenlangauge) {
is Java -> return call
is Kotlin -> do some specific logic
}

But I do not insist on it.

Collaborator Author

volivan239 Sep 27, 2022

Fixed here, didn't check for other places

EgorkaKulikov reviewed

View reviewed changes

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

+                      val caller = call.caller ?: return call
+                      caller.type.fieldThisIsSetterFor(call.executableId)?.let {
+                          return CgAssignment(caller[it], call.arguments.single())

Collaborator

EgorkaKulikov Sep 27, 2022

Why don't you require that arguments contains single item here as in line 285?

Collaborator Author

volivan239 Sep 27, 2022

I do -- at call.arguments.single()....

volivan239 added 8 commits

September 27, 2022 17:59


          Replace Kotlin getters/setters with property access in codegen


          Corrected behavior for static fields

3a93809


          Always use safe call (?.) in Kotlin codegen

4a46a78


          Fix calling wrong unary plus


          Fixed test crashing due to nullability problems

c8739aa


          Treat java-accessible fields as accessible properties in Kotlin

c8a8407


          Fix false-positive canBeSetIn result for private fields with getters …

2245a09

…in Kotlin


          Add comments

569e581

EgorkaKulikov reviewed

View reviewed changes

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

+                          return CgAssignment(caller[it], call.arguments.single())
+                      }
+                      caller.type.fieldThisIsGetterFor(call.executableId)?.let {
+                          require(call.arguments.isEmpty())

Collaborator

EgorkaKulikov Sep 27, 2022

require is more useful with a message with failure description

Collaborator Author

volivan239 Sep 27, 2022

Added message

EgorkaKulikov reviewed

View reviewed changes

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

+               */
+              internal fun ClassId.fieldThisIsGetterFor(methodId: MethodId): FieldId? =
+                  allDeclaredFieldIds.firstOrNull { !it.isStatic && it.getter == methodId }

Collaborator

EgorkaKulikov Sep 27, 2022

May be singleOrNull? Same for the second method.

Collaborator Author

volivan239 Sep 27, 2022

Fixed

EgorkaKulikov reviewed

View reviewed changes

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

+               * Returns field of [this], such that [methodId] is a setter for it (or null if methodId doesn't represent a setter)
+               */
+              internal fun ClassId.fieldThisIsSetterFor(methodId: MethodId): FieldId? =

Collaborator

EgorkaKulikov Sep 27, 2022

I suggest the name fieldThatIsSetWith.
Current name means that field is a setter isself, it seems strange.

Collaborator Author

volivan239 Sep 27, 2022

Renamed this and the method below

EgorkaKulikov reviewed

View reviewed changes

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

                       }
                       CodegenLanguage.KOTLIN -> {
                           """
-                          ${visibility by language}fun createInstance(className: String): kotlin.Any? {
+                          ${visibility by language}fun createInstance(className: String): kotlin.Any {

Collaborator

EgorkaKulikov Sep 27, 2022

To be honest, I don't understand this change, but can believe that is is possible :)

Collaborator Author

volivan239 Sep 27, 2022 •

edited

Loading

AFAIK, this method can't return null (at least, no compilation error is produced after this change), this may help to avoid unnecessary ?.-calls

EgorkaKulikov approved these changes

View reviewed changes


          Minor changes fixing issues found in review

38fb0d0

volivan239 force-pushed the volivan239/fix_kotlin_property_access_rendering branch from e01920b to 38fb0d0 Compare

September 27, 2022 15:21

volivan239 merged commit bcece73 into main

volivan239 deleted the volivan239/fix_kotlin_property_access_rendering branch

September 27, 2022 16:47

volivan239 mentioned this pull request

Calls to Kotlin autogenerated setter isn't changed to direct property access for tests from fuzzer #1241

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet