Merge pull request #1389 from WebFuzzing/refactor-set-from-gene

Refactor set from gene

Author: arcuri82

core/src/main/kotlin/org/evomaster/core/problem/externalservice/httpws/service/HarvestActualHttpWsResponseHandler.kt

@@ -532,10 +532,11 @@ class HarvestActualHttpWsResponseHandler {
         try {
             val template = getACopyOfItsActualResponseIfExist(geneToMutate, probability)?.responseBody ?: return false
 
-            val v = ParamUtil.getValueGene(geneToMutate)
-            val t = ParamUtil.getValueGene(template)
+            val v = geneToMutate.getLeafGene()
+            val t = template.getLeafGene()
             if (v::class.java == t::class.java) {
                 v.copyValueFrom(t)
+                v.forceNewTaints()
                 return true
             } else if (v is StringGene) {
                 // add template as part of specialization

core/src/main/kotlin/org/evomaster/core/problem/rest/data/RestCallAction.kt

@@ -191,7 +191,9 @@ class RestCallAction(
                     For example, they could be a ChoiceGene when dealing with "examples" or Regex when having patterns
                     only defined on some endpoints
                  */
-                parameters[i].primaryGene().copyValueFrom(k.primaryGene())
+                val g = parameters[i].primaryGene()
+                g.copyValueFrom(k.primaryGene())
+                g.forceNewTaints()
             }
         }
     }

core/src/main/kotlin/org/evomaster/core/problem/util/BindingBuilder.kt

@@ -109,14 +109,14 @@ object BindingBuilder {
     }
 
     private fun bindValues(p: Pair<Gene,Gene>, doBuildBindingGene: Boolean){
-        val ok = p.first.setFromDifferentGene(p.second)
+        val ok = p.first.copyValueFrom(p.second)
         if (ok && doBuildBindingGene){
             p.first.addBindingGene(p.second)
             p.second.addBindingGene(p.first)
         }
 
-        val first = ParamUtil.getValueGene(p.first)
-        val second = ParamUtil.getValueGene(p.second)
+        val first = p.first.getLeafGene()
+        val second = p.second.getLeafGene()
         if(ok && !doBuildBindingGene && first is StringGene && TaintInputName.isTaintInput(first.value)){
             //do not use same tainted value in non-bound genes
             if(second is StringGene){

core/src/main/kotlin/org/evomaster/core/problem/util/ParamUtil.kt

@@ -16,7 +16,6 @@ import org.evomaster.core.search.gene.numeric.LongGene
 import org.evomaster.core.search.gene.wrapper.CustomMutationRateGene
 import org.evomaster.core.search.gene.wrapper.OptionalGene
 import org.evomaster.core.search.gene.string.StringGene
-import org.evomaster.core.search.gene.utils.GeneUtils
 
 /**
  * this class used to handle binding values among params
@@ -100,7 +99,7 @@ class ParamUtil {
          */
         fun compareGenesWithValue(geneA: Gene, geneB: Gene): Boolean {
             val geneAWithGeneBType = geneB.copy()
-            geneAWithGeneBType.setFromDifferentGene(geneA)
+            geneAWithGeneBType.copyValueFrom(geneA)
             return when (geneB) {
                 is StringGene -> geneB.value == (geneAWithGeneBType as StringGene).value
                 is IntegerGene -> geneB.value == (geneAWithGeneBType as IntegerGene).value
@@ -275,7 +274,7 @@ class ParamUtil {
 
         fun generateParamId(list: Array<String>): String = list.joinToString(separator)
 
-        @Deprecated(message = "Rather use GeneUtils.getWrappedValueGene(gene)",
+        @Deprecated(message = "Rather use getLeafGene()",
             replaceWith = ReplaceWith("GeneUtils.getWrappedValueGene(gene)"))
         fun getValueGene(gene: Gene): Gene {
            return gene.getLeafGene()!!

core/src/main/kotlin/org/evomaster/core/search/Individual.kt

@@ -527,7 +527,7 @@ abstract class Individual(
                       y !=x && !y.hasAnyBindingRelationship(x)
                }
            }){
-               errors.add("Taint id ${d.key} has duplicate genes that are not related}")
+               errors.add("Taint id ${d.key} has duplicate genes that are not related")
            }
         }
         return errors

core/src/main/kotlin/org/evomaster/core/search/action/Action.kt

@@ -79,11 +79,7 @@ abstract class Action(children: List<StructuralElement>) : ActionComponent(
 
     fun forceNewTaints(){
         seeTopGenes().forEach { g ->
-            g.flatView().forEach { r ->
-                if(r is TaintableGene && !r.isDependentTaint()){
-                    r.forceNewTaintId()
-                }
-            }
+            g.forceNewTaints()
         }
     }
 

core/src/main/kotlin/org/evomaster/core/search/gene/BooleanGene.kt

@@ -33,7 +33,7 @@ class BooleanGene(
         this.value = value.toBoolean()
     }
 
-    override fun setValueBasedOn(value: String) : Boolean{
+    override fun unsafeSetFromStringValue(value: String) : Boolean{
         try{
             this.value = value.toBoolean()
             return true
@@ -62,20 +62,6 @@ class BooleanGene(
         return value.toString()
     }
 
-    override fun copyValueFrom(other: Gene): Boolean {
-        if (other !is BooleanGene) {
-            throw IllegalArgumentException("Invalid gene type ${other.javaClass}")
-        }
-        val current = this.value
-        this.value = other.value
-        if (!isLocallyValid()){
-            this.value = current
-            return false
-        }
-
-        return true
-    }
-
     override fun containsSameValueAs(other: Gene): Boolean {
         if (other !is BooleanGene) {
             throw IllegalArgumentException("Invalid gene type ${other.javaClass}")
@@ -84,10 +70,10 @@ class BooleanGene(
     }
 
 
-    override fun setValueBasedOn(gene: Gene): Boolean {
-        if (gene is SeededGene<*>){
-            return this.setValueBasedOn(gene.getPhenotype()as Gene)
-        }
+    override fun unsafeCopyValueFrom(other: Gene): Boolean {
+
+        val gene = other.getPhenotype()
+
         if (gene !is BooleanGene){
             LoggingUtil.uniqueWarn(log, "Do not support to bind boolean gene with the type: ${gene::class.java.simpleName}")
             return false

core/src/main/kotlin/org/evomaster/core/search/gene/Gene.kt

@@ -15,15 +15,9 @@ import org.evomaster.core.Lazy
 import org.evomaster.core.problem.enterprise.EnterpriseIndividual
 import org.evomaster.core.problem.enterprise.SampleType
 import org.evomaster.core.search.RootElement
-import org.evomaster.core.search.gene.sql.SqlAutoIncrementGene
-import org.evomaster.core.search.gene.sql.SqlPrimaryKeyGene
+import org.evomaster.core.search.gene.interfaces.TaintableGene
 import org.evomaster.core.search.gene.utils.GeneUtils
-import org.evomaster.core.search.gene.wrapper.CustomMutationRateGene
-import org.evomaster.core.search.gene.wrapper.FlexibleGene
-import org.evomaster.core.search.gene.wrapper.NullableGene
-import org.evomaster.core.search.gene.wrapper.OptionalGene
-import org.evomaster.core.search.gene.wrapper.SelectableWrapperGene
-import org.evomaster.core.search.gene.wrapper.WrapperGene
+import org.evomaster.core.search.gene.interfaces.WrapperGene
 import org.evomaster.core.search.service.SearchGlobalState
 import org.evomaster.core.search.service.monitor.ProcessMonitorExcludeField
 
@@ -349,6 +343,17 @@ abstract class Gene(
         return this
     }
 
+    /**
+     * Return the gene used for the phenotype.
+     * Most of the time this would be `this` gene.
+     * Note, it is different from the concept of "wrapper", as the wrapper itself can impact
+     * the phenotype.
+     *
+     * This is mainly used to handle very special cases such as [StringGene] and [SeededGene]
+     */
+    open fun getPhenotype() : Gene{
+        return this
+    }
 
 
     protected fun matchingClass(klass: Class<*>, strict: Boolean): Boolean {
@@ -906,19 +911,22 @@ abstract class Gene(
      * sync [bindingGenes] based on [this]
      */
     fun syncBindingGenesBasedOnThis(all: MutableSet<Gene> = mutableSetOf()) {
-        if (bindingGenes.isEmpty()) return
+        if (bindingGenes.isEmpty()){
+            return
+        }
         all.add(this)
         bindingGenes.filterNot { all.contains(it) }.forEach { b ->
             all.add(b)
-            if (!b.setValueBasedOn(this))
-                LoggingUtil.uniqueWarn(
-                    log,
-                    "fail to bind the gene (${b.name} with the type ${b::class.java.simpleName}) based on this gene (${this.name} with ${this::class.java.simpleName})"
-                )
+            if (!b.copyValueFrom(this)) {
+                LoggingUtil.uniqueWarn(log, "fail to bind the gene (${b.name} with the type ${b::class.java.simpleName})" +
+                            " based on this gene (${this.name} with ${this::class.java.simpleName})")
+            }
             b.syncBindingGenesBasedOnThis(all)
         }
 
-        children.filterNot { all.contains(it) }.forEach { it.syncBindingGenesBasedOnThis(all) }
+        children.filterNot { all.contains(it) }.forEach {
+            it.syncBindingGenesBasedOnThis(all)
+        }
     }
 
     /**
@@ -1110,62 +1118,52 @@ abstract class Gene(
      * The type of genes can be different.
      * However, there is no check if constraints are kept satisfied.
      * So, this method should not be called directly.
-     * Rather use [setFromDifferentGene], which internally it calls this method,
+     * Rather use [copyValueFrom], which internally it calls this method,
      * and then revert in case of constraint violations.
      *
-     * @return whether the binding performs successfully.
+     * @return whether the value is copied based on [other] successfully.
+     * This is based only on the gene type.
+     * _WARNING_:
+     * - `true` might still leave the gene in an inconsistent state (ie violated constraints)
+     * - `false` might still apply partial updates (eg, think of an objects with several fields)
+     *
+     * Do not call directly outside this package. Call [copyValueFrom]
      *
      * TODO unfortunately, Kotlin has major design flows that do not allow package-level and true protected-level
      * scope, like in Java :(
      * This is a case in which is much worse than Java.
      * But it could be simulated with Detekt and a rule like @PackagePrivate
      */
-    @Deprecated("Do not call directly outside this package. Call setFromDifferentGene")
-    //TODO remove deprecated once we integrate @PackagePrivate
-    internal abstract fun setValueBasedOn(gene: Gene): Boolean
+    abstract fun unsafeCopyValueFrom(other: Gene): Boolean
 
 
-    /**
-     * copy value based on [other]
-     * in some case, the [other] might not satisfy constraints of [this gene],
-     * then copying will not be performed successfully
-     *
-     * FIXME unclear if side-effects or not
-     *
-     * @return whether the value is copied based on [other] successfully
-     */
-    abstract fun copyValueFrom(other: Gene): Boolean
-
+    fun forceNewTaints(){
+        flatView().forEach { r ->
+            if(r is TaintableGene && !r.isDependentTaint()){
+                r.forceNewTaintId()
+            }
+        }
+    }
 
     /**
-     * Update current value of this gene, base on other gene.
-     * This is not [copyValueFrom], as the gene could be different.
-     * FIXME that comment seems wrong
+     * Update current value of this gene, base on [other] gene.
      * If for any reason the update fails, there is not going to be any side-effects.
+     * A successful update must guarantee that the gene remains valid (ie, no violated constraints).
      *
      * @return if the update was successful
      */
-    fun setFromDifferentGene(gene: Gene, undoIfUpdateFails: Boolean = true): Boolean {
-
-        //FIXME current implementation leads to infinite loops. must fix copyValueFrom
-        //return updateValueOnlyIfValid( { setValueBasedOn(gene) } , undoIfUpdateFails)
-        //TODO update once fixed
-        return setValueBasedOn(gene)
+    fun copyValueFrom(gene: Gene): Boolean {
+        return updateValueOnlyIfValid( { unsafeCopyValueFrom(gene) }, true)
     }
 
-    /*
-        FIXME: looks like redundancies and inconsistencies between copyValueFrom and setFromDifferentGene.
-        TODO once fixed, update
-     */
-
     /**
      * Given a string value, apply it to the current state of this gene (and possibly recursively to its children).
      * If it fails for any reason, return false.
      * This method guarantees the validity of the resulting gene, ie, changes are reverted if any constraints
      * is violated.
      */
     fun setFromStringValue(value: String, undoIfUpdateFails: Boolean = true): Boolean {
-        return updateValueOnlyIfValid({ setValueBasedOn(value) }, undoIfUpdateFails)
+        return updateValueOnlyIfValid({ unsafeSetFromStringValue(value) }, undoIfUpdateFails)
     }
 
     /**
@@ -1181,33 +1179,42 @@ abstract class Gene(
      * TODO @PackagePrivate
      */
     @Deprecated("Do not call directly outside this package. Call setFromStringValue")
-    internal open fun setValueBasedOn(value: String): Boolean {
+    internal open fun unsafeSetFromStringValue(value: String): Boolean {
         //TODO in future this should be abstract, to force each gene to handle it.
         //few implementations can be based on AbstractParser class for Postman
         throw IllegalStateException("setValueBasedOn() is not implemented for gene ${this::class.simpleName}")
     }
 
 
     /**
-     * here `valid` means that 1) [updateValue] performs correctly, ie, returns true AND 2) isLocallyValid is true
+     * here `valid` means that 1) [updateValue] performs correctly, ie, returns true AND 2) [isGloballyValid] is true
      *
      * @param updateValue lambda performs update of value of the gene
-     * @param undoIfUpdateFails represents whether it needs to undo the value update if [undoIfUpdateFails] returns false
+     * @param undoIfUpdateFails represents whether it needs to undo the value update if [updateValue] returns false
      *
-     * @return if the value is updated with [updateValue]
+     * @return if the value is updated with [updateValue]. note if for any reason the current gene was not valid,
+     *         the validity of the update will not be checked.
      */
     fun updateValueOnlyIfValid(updateValue: () -> Boolean, undoIfUpdateFails: Boolean): Boolean {
+
+        if(!undoIfUpdateFails) {
+            return updateValue()
+        }
+
+        val currentlyValid = isGloballyValid()
+
         val current = copy()
         val ok = updateValue()
-        if (!ok && !undoIfUpdateFails) return false
 
-        if (!ok || !isLocallyValid()) {
-            val success = copyValueFrom(current)
-            assert(success)
+        if (!ok || (currentlyValid && !isGloballyValid())) {
+            //revert back
+            val success = unsafeCopyValueFrom(current)
+            //reversion should always work... if fails, it is a bug
+            // FIXME put back once all are implemented, eg, TaintedMapGene currently missing
+            //assert(success)
             return false
         }
         return true
-
     }
 
     /**