Fix behavior of ${?} optional substitution overriding a field

The broken behavior was:
  foo=10
  foo=${?bar}
where 'foo' should be 10 if bar is undefined, but instead
ended up undefined. Now, foo will be 10 (which was the
documented behavior).

HOCON.md

@@ -443,7 +443,8 @@ is invalid and should generate an error.
 If a substitution with the `${?foo}` syntax is undefined:
 
  - if it is the value of an object field then the field should not
-   be created.
+   be created. If the field would have overridden a previously-set
+   value for the same field, then the previous value remains.
  - if it is an array element then the element should not be added.
  - if it is part of a value concatenation then it should become an
    empty string.

config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java

@@ -103,6 +103,13 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue {
         throw badMergeException();
     }
 
+    protected AbstractConfigValue mergedWithNonObject(AbstractConfigValue fallback) {
+        // falling back to a non-object doesn't merge anything, and also
+        // prohibits merging any objects that we fall back to later.
+        // so we have to switch to ignoresFallbacks mode.
+        return newCopy(true /* ignoresFallbacks */, origin);
+    }
+
     public AbstractConfigValue withOrigin(ConfigOrigin origin) {
         if (this.origin == origin)
             return this;
@@ -130,10 +137,7 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue {
                     return mergedWithObject((AbstractConfigObject) other);
                 }
             } else {
-                // falling back to a non-object doesn't merge anything, and also
-                // prohibits merging any objects that we fall back to later.
-                // so we have to switch to ignoresFallbacks mode.
-                return newCopy(true /* ignoresFallbacks */, origin);
+                return mergedWithNonObject((AbstractConfigValue) other);
             }
         }
     }

config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java

@@ -84,13 +84,10 @@ final class ConfigSubstitution extends AbstractConfigValue implements
                 ((AbstractConfigValue) fallback).ignoresFallbacks());
     }
 
-    @Override
-    protected AbstractConfigValue mergedWithObject(AbstractConfigObject fallback) {
+    protected AbstractConfigValue mergedLater(AbstractConfigValue fallback) {
         if (ignoresFallbacks)
             throw new ConfigException.BugOrBroken("should not be reached");
 
-        // if we turn out to be an object, and the fallback also does,
-        // then a merge may be required; delay until we resolve.
         List<AbstractConfigValue> newStack = new ArrayList<AbstractConfigValue>();
         newStack.add(this);
         newStack.add(fallback);
@@ -98,6 +95,23 @@ final class ConfigSubstitution extends AbstractConfigValue implements
                 fallback.ignoresFallbacks());
     }
 
+    @Override
+    protected AbstractConfigValue mergedWithObject(AbstractConfigObject fallback) {
+        // if we turn out to be an object, and the fallback also does,
+        // then a merge may be required; delay until we resolve.
+        return mergedLater(fallback);
+    }
+
+    @Override
+    protected AbstractConfigValue mergedWithNonObject(AbstractConfigValue fallback) {
+        // if the optional substitution ends up getting deleted (because it is
+        // not present), we'll have to use the fallback. So delay the merge.
+        if (pieces.size() == 1 && ((SubstitutionExpression) pieces.get(0)).optional())
+            return mergedLater(fallback);
+        else
+            return super.mergedWithNonObject(fallback);
+    }
+
     @Override
     public Collection<ConfigSubstitution> unmergedValues() {
         return Collections.singleton(this);

config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala

@@ -425,4 +425,49 @@ class ConfigSubstitutionTest extends TestUtils {
             resolve(obj)
         }
     }
+
+    @Test
+    def optionalOverrideNotProvided() {
+        val obj = parseObject("""{ a: 42, a : ${?NOT_HERE} }""")
+        val resolved = resolve(obj)
+        assertEquals(42, resolved.getInt("a"))
+    }
+
+    @Test
+    def optionalOverrideProvided() {
+        val obj = parseObject("""{ HERE : 43, a: 42, a : ${?HERE} }""")
+        val resolved = resolve(obj)
+        assertEquals(43, resolved.getInt("a"))
+    }
+
+    @Test
+    def optionalOverrideOfObjectNotProvided() {
+        val obj = parseObject("""{ a: { b : 42 }, a : ${?NOT_HERE} }""")
+        val resolved = resolve(obj)
+        assertEquals(42, resolved.getInt("a.b"))
+    }
+
+    @Test
+    def optionalOverrideOfObjectProvided() {
+        val obj = parseObject("""{ HERE : 43, a: { b : 42 }, a : ${?HERE} }""")
+        val resolved = resolve(obj)
+        assertEquals(43, resolved.getInt("a"))
+        assertFalse(resolved.hasPath("a.b"))
+    }
+
+    @Test
+    def optionalVanishesFromArray() {
+        import scala.collection.JavaConverters._
+        val obj = parseObject("""{ a : [ 1, 2, 3, ${?NOT_HERE} ] }""")
+        val resolved = resolve(obj)
+        assertEquals(Seq(1, 2, 3), resolved.getIntList("a").asScala)
+    }
+
+    @Test
+    def optionalUsedInArray() {
+        import scala.collection.JavaConverters._
+        val obj = parseObject("""{ HERE: 4, a : [ 1, 2, 3, ${?HERE} ] }""")
+        val resolved = resolve(obj)
+        assertEquals(Seq(1, 2, 3, 4), resolved.getIntList("a").asScala)
+    }
 }