Fix withFallback() to avoid the mess where it has to be called from the bottom up.

Make ConfigObject have an includeFallbacks flag for whether it has
"ended" and will no longer merge anything.

src/main/java/com/typesafe/config/ConfigMergeable.java

@@ -21,35 +21,16 @@ public interface ConfigMergeable {
      * Config instances do anything in this method (they need to merge the
      * fallback keys into themselves). All other values just return the original
      * value, since they automatically override any fallback.
-     *
+     * 
      * The semantics of merging are described in
      * https://github.com/havocp/config/blob/master/HOCON.md
-     *
-     * Prefer <code>withFallbacks()</code>, listing all your fallbacks at once,
-     * over this method.
-     *
-     * <i>When using this method, there is an easy way to write a wrong
-     * loop.</i> Even if you don't loop, it's easy to do the equivalent wrong
-     * thing.
-     *
-     * <code>
-     *   // WRONG
-     *   for (ConfigMergeable fallback : fallbacks) {
-     *       // DO NOT DO THIS
-     *       result = result.withFallback(fallback);
-     *   }
-     * </code>
-     *
-     * This is wrong because when <code>result</code> is an object and
-     * <code>fallback</code> is a non-object,
-     * <code>result.withFallback(fallback)</code> returns an object. Then if
-     * there are more objects, they are merged into that object. But the correct
-     * semantics are that a non-object will block merging any more objects later
-     * in the list. To get it right, you need to iterate backward. Simplest
-     * solution: prefer <code>withFallbacks()</code> which is harder to get
-     * wrong, and merge all your fallbacks in one call to
-     * <code>withFallbacks()</code>.
-     *
+     * 
+     * Note that objects do not merge "across" non-objects; if you do
+     * <code>object.withFallback(nonObject).withFallback(otherObject)</code>,
+     * then <code>otherObject</code> will simply be ignored. This is an
+     * intentional part of how merging works. Both non-objects, and any object
+     * which has fallen back to a non-object, block subsequent fallbacks.
+     * 
      * @param other
      *            an object whose keys should be used if the keys are not
      *            present in this one

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

@@ -102,7 +102,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
         return ConfigValueType.OBJECT;
     }
 
-    protected abstract AbstractConfigObject newCopy(ResolveStatus status);
+    protected abstract AbstractConfigObject newCopy(ResolveStatus status,
+            boolean ignoresFallbacks);
 
     @Override
     public AbstractConfigObject withFallbacks(ConfigMergeable... others) {
@@ -113,7 +114,9 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
     public AbstractConfigObject withFallback(ConfigMergeable mergeable) {
         ConfigValue other = mergeable.toValue();
 
-        if (other instanceof Unmergeable) {
+        if (ignoresFallbacks()) {
+            return this;
+        } else if (other instanceof Unmergeable) {
             List<AbstractConfigValue> stack = new ArrayList<AbstractConfigValue>();
             stack.add(this);
             stack.addAll(((Unmergeable) other).unmergedValues());
@@ -143,12 +146,14 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
                         allResolved = false;
                 }
                 return new SimpleConfigObject(mergeOrigins(this, fallback),
-                        merged, ResolveStatus.fromBoolean(allResolved));
+                        merged, ResolveStatus.fromBoolean(allResolved),
+                        ignoresFallbacks());
             }
         } else {
-            // falling back to a non-object has no effect, we just override
-            // primitive values.
-            return this;
+            // 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(resolveStatus(), true /* ignoresFallbacks */);
         }
     }
 
@@ -209,7 +214,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
             }
         }
         if (changes == null) {
-            return newCopy(newResolveStatus);
+            return newCopy(newResolveStatus, ignoresFallbacks());
         } else {
             Map<String, AbstractConfigValue> modified = new HashMap<String, AbstractConfigValue>();
             for (String k : keySet()) {
@@ -219,8 +224,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
                     modified.put(k, peek(k));
                 }
             }
-            return new SimpleConfigObject(origin(), modified,
-                    newResolveStatus);
+            return new SimpleConfigObject(origin(), modified, newResolveStatus,
+                    ignoresFallbacks());
         }
     }
 
@@ -314,7 +319,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
 
     @Override
     public boolean equals(Object other) {
-        // note that "origin" is deliberately NOT part of equality
+        // note that "origin" is deliberately NOT part of equality.
+        // neither are other "extras" like ignoresFallbacks or resolve status.
         if (other instanceof ConfigObject) {
             // optimization to avoid unwrapped() for two ConfigObject,
             // which is what AbstractConfigValue does.
@@ -327,6 +333,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
     @Override
     public int hashCode() {
         // note that "origin" is deliberately NOT part of equality
+        // neither are other "extras" like ignoresFallbacks or resolve status.
         return mapHash(this);
     }
 

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

@@ -75,6 +75,13 @@ abstract class AbstractConfigValue implements ConfigValue {
         return this;
     }
 
+    // this is virtualized rather than a field because only some subclasses
+    // really need to store the boolean, and they may be able to pack it
+    // with another boolean to save space.
+    protected boolean ignoresFallbacks() {
+        return true;
+    }
+
     @Override
     public AbstractConfigValue withFallback(ConfigMergeable other) {
         return this;

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

@@ -27,16 +27,23 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements
 
     // earlier items in the stack win
     final private List<AbstractConfigValue> stack;
+    final private boolean ignoresFallbacks;
 
-    ConfigDelayedMerge(ConfigOrigin origin, List<AbstractConfigValue> stack) {
+    private ConfigDelayedMerge(ConfigOrigin origin,
+            List<AbstractConfigValue> stack, boolean ignoresFallbacks) {
         super(origin);
         this.stack = stack;
+        this.ignoresFallbacks = ignoresFallbacks;
         if (stack.isEmpty())
             throw new ConfigException.BugOrBroken(
                     "creating empty delayed merge value");
 
     }
 
+    ConfigDelayedMerge(ConfigOrigin origin, List<AbstractConfigValue> stack) {
+        this(origin, stack, false /* ignoresFallbacks */);
+    }
+
     @Override
     public ConfigValueType valueType() {
         throw new ConfigException.NotResolved(
@@ -85,14 +92,21 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements
         for (AbstractConfigValue o : stack) {
             newStack.add(o.relativized(prefix));
         }
-        return new ConfigDelayedMerge(origin(), newStack);
+        return new ConfigDelayedMerge(origin(), newStack, ignoresFallbacks);
+    }
+
+    @Override
+    protected boolean ignoresFallbacks() {
+        return ignoresFallbacks;
     }
 
     @Override
     public AbstractConfigValue withFallback(ConfigMergeable mergeable) {
         ConfigValue other = mergeable.toValue();
 
-        if (other instanceof AbstractConfigObject
+        if (ignoresFallbacks) {
+            return this;
+        } else if (other instanceof AbstractConfigObject
                 || other instanceof Unmergeable) {
             // if we turn out to be an object, and the fallback also does,
             // then a merge may be required; delay until we resolve.
@@ -103,11 +117,12 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements
             else
                 newStack.add((AbstractConfigValue) other);
             return new ConfigDelayedMerge(
-                    AbstractConfigObject.mergeOrigins(newStack), newStack);
+                    AbstractConfigObject.mergeOrigins(newStack), newStack,
+                    ignoresFallbacks);
         } else {
             // if the other is not an object, there won't be anything
             // to merge with, so we are it even if we are an object.
-            return this;
+            return new ConfigDelayedMerge(origin(), stack, true /* ignoresFallbacks */);
         }
     }
 

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

@@ -21,11 +21,19 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements
         Unmergeable {
 
     final private List<AbstractConfigValue> stack;
+    final private boolean ignoresFallbacks;
 
     ConfigDelayedMergeObject(ConfigOrigin origin,
             List<AbstractConfigValue> stack) {
+        this(origin, stack, false /* ignoresFallbacks */);
+    }
+
+    private ConfigDelayedMergeObject(ConfigOrigin origin,
+            List<AbstractConfigValue> stack, boolean ignoresFallbacks) {
         super(origin);
         this.stack = stack;
+        this.ignoresFallbacks = ignoresFallbacks;
+
         if (stack.isEmpty())
             throw new ConfigException.BugOrBroken(
                     "creating empty delayed merge object");
@@ -35,11 +43,12 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements
     }
 
     @Override
-    public ConfigDelayedMergeObject newCopy(ResolveStatus status) {
+    public ConfigDelayedMergeObject newCopy(ResolveStatus status,
+            boolean ignoresFallbacks) {
         if (status != resolveStatus())
             throw new ConfigException.BugOrBroken(
                     "attempt to create resolved ConfigDelayedMergeObject");
-        return new ConfigDelayedMergeObject(origin(), stack);
+        return new ConfigDelayedMergeObject(origin(), stack, ignoresFallbacks);
     }
 
     @Override
@@ -67,14 +76,22 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements
         for (AbstractConfigValue o : stack) {
             newStack.add(o.relativized(prefix));
         }
-        return new ConfigDelayedMergeObject(origin(), newStack);
+        return new ConfigDelayedMergeObject(origin(), newStack,
+                ignoresFallbacks);
+    }
+
+    @Override
+    protected boolean ignoresFallbacks() {
+        return ignoresFallbacks;
     }
 
     @Override
     public ConfigDelayedMergeObject withFallback(ConfigMergeable mergeable) {
         ConfigValue other = mergeable.toValue();
 
-        if (other instanceof AbstractConfigObject
+        if (ignoresFallbacks) {
+            return this;
+        } else if (other instanceof AbstractConfigObject
                 || other instanceof Unmergeable) {
             // since we are an object, and the fallback could be,
             // then a merge may be required; delay until we resolve.
@@ -85,12 +102,13 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements
             else
                 newStack.add((AbstractConfigValue) other);
             return new ConfigDelayedMergeObject(
-                    AbstractConfigObject.mergeOrigins(newStack),
-                    newStack);
+                    AbstractConfigObject.mergeOrigins(newStack), newStack,
+                    ignoresFallbacks);
         } else {
             // if the other is not an object, there won't be anything
-            // to merge with.
-            return this;
+            // to merge with but we do need to ignore any fallbacks
+            // that occur after it.
+            return newCopy(resolveStatus(), true /* ignoresFallbacks */);
         }
     }
 

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

@@ -10,7 +10,6 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
-import java.util.ListIterator;
 import java.util.Map;
 
 import com.typesafe.config.Config;
@@ -37,22 +36,11 @@ public class ConfigImpl {
 
     static <T extends ConfigMergeable> T merge(Class<T> klass, T first,
             List<? extends ConfigMergeable> stack) {
-        if (stack.isEmpty()) {
-            return first;
-        } else {
-            // to be consistent with the semantics of duplicate keys
-            // in the same file, we have to go backward like this.
-            // importantly, a primitive value always permanently
-            // hides a previous object value.
-            ListIterator<? extends ConfigMergeable> i = stack
-                    .listIterator(stack.size());
-            ConfigMergeable merged = i.previous();
-            while (i.hasPrevious()) {
-                merged = i.previous().withFallback(merged);
-            }
-            merged = first.withFallback(merged);
-            return klass.cast(merged);
+        ConfigMergeable merged = first;
+        for (ConfigMergeable fallback : stack) {
+            merged = merged.withFallback(fallback);
         }
+        return klass.cast(merged);
     }
 
     private interface NameSource {
@@ -428,7 +416,7 @@ public class ConfigImpl {
                     new SimpleConfigOrigin("env var " + key), entry.getValue()));
         }
         return new SimpleConfigObject(new SimpleConfigOrigin("env variables"),
-                m, ResolveStatus.RESOLVED);
+                m, ResolveStatus.RESOLVED, false /* ignoresFallbacks */);
     }
 
     /** For use ONLY by library internals, DO NOT TOUCH not guaranteed ABI */

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

@@ -28,17 +28,19 @@ final class ConfigSubstitution extends AbstractConfigValue implements
     // than one piece everything is stringified and concatenated
     final private List<Object> pieces;
     // the length of any prefixes added with relativized()
-    final int prefixLength;
+    final private int prefixLength;
+    final private boolean ignoresFallbacks;
 
     ConfigSubstitution(ConfigOrigin origin, List<Object> pieces) {
-        this(origin, pieces, 0);
+        this(origin, pieces, 0, false);
     }
 
     private ConfigSubstitution(ConfigOrigin origin, List<Object> pieces,
-            int prefixLength) {
+            int prefixLength, boolean ignoresFallbacks) {
         super(origin);
         this.pieces = pieces;
         this.prefixLength = prefixLength;
+        this.ignoresFallbacks = ignoresFallbacks;
     }
 
     @Override
@@ -58,7 +60,9 @@ final class ConfigSubstitution extends AbstractConfigValue implements
     public AbstractConfigValue withFallback(ConfigMergeable mergeable) {
         ConfigValue other = mergeable.toValue();
 
-        if (other instanceof AbstractConfigObject
+        if (ignoresFallbacks) {
+            return this;
+        } else if (other instanceof AbstractConfigObject
                 || other instanceof Unmergeable) {
             // if we turn out to be an object, and the fallback also does,
             // then a merge may be required; delay until we resolve.
@@ -73,7 +77,7 @@ final class ConfigSubstitution extends AbstractConfigValue implements
         } else {
             // if the other is not an object, there won't be anything
             // to merge with, so we are it even if we are an object.
-            return this;
+            return new ConfigSubstitution(origin(), pieces, prefixLength, true /* ignoresFallbacks */);
         }
     }
 
@@ -201,7 +205,7 @@ final class ConfigSubstitution extends AbstractConfigValue implements
             }
         }
         return new ConfigSubstitution(origin(), newPieces, prefixLength
-                + prefix.length());
+                + prefix.length(), ignoresFallbacks);
     }
 
     @Override

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

@@ -180,11 +180,12 @@ final class PropertiesParser {
                     .get(parentPath) : root;
 
             AbstractConfigObject o = new SimpleConfigObject(origin, scope,
-                    ResolveStatus.RESOLVED);
+                    ResolveStatus.RESOLVED, false /* ignoresFallbacks */);
             parent.put(scopePath.last(), o);
         }
 
         // return root config object
-        return new SimpleConfigObject(origin, root, ResolveStatus.RESOLVED);
+        return new SimpleConfigObject(origin, root, ResolveStatus.RESOLVED,
+                false /* ignoresFallbacks */);
     }
 }

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

@@ -20,21 +20,23 @@ final class SimpleConfigObject extends AbstractConfigObject {
     // this map should never be modified - assume immutable
     final private Map<String, AbstractConfigValue> value;
     final private boolean resolved;
+    final private boolean ignoresFallbacks;
 
     SimpleConfigObject(ConfigOrigin origin,
-            Map<String, AbstractConfigValue> value, ResolveStatus status) {
+            Map<String, AbstractConfigValue> value, ResolveStatus status,
+            boolean ignoresFallbacks) {
         super(origin);
         if (value == null)
             throw new ConfigException.BugOrBroken(
                     "creating config object with null map");
         this.value = value;
         this.resolved = status == ResolveStatus.RESOLVED;
+        this.ignoresFallbacks = ignoresFallbacks;
     }
 
     SimpleConfigObject(ConfigOrigin origin,
             Map<String, AbstractConfigValue> value) {
-        this(origin, value, ResolveStatus.fromValues(value
-                .values()));
+        this(origin, value, ResolveStatus.fromValues(value.values()), false /* ignoresFallbacks */);
     }
 
     @Override
@@ -43,9 +45,10 @@ final class SimpleConfigObject extends AbstractConfigObject {
     }
 
     @Override
-    public SimpleConfigObject newCopy(ResolveStatus newStatus) {
-        return new SimpleConfigObject(origin(), value,
-                newStatus);
+    public SimpleConfigObject newCopy(ResolveStatus newStatus,
+            boolean ignoresFallbacks) {
+        return new SimpleConfigObject(origin(), value, newStatus,
+                ignoresFallbacks);
     }
 
     @Override
@@ -53,6 +56,11 @@ final class SimpleConfigObject extends AbstractConfigObject {
         return ResolveStatus.fromBoolean(resolved);
     }
 
+    @Override
+    protected boolean ignoresFallbacks() {
+        return ignoresFallbacks;
+    }
+
     @Override
     public Map<String, Object> unwrapped() {
         Map<String, Object> m = new HashMap<String, Object>();

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

@@ -327,7 +327,8 @@ class ConfigValueTest extends TestUtils {
 
         // ConfigDelayedMergeObject
         val emptyObj = SimpleConfigObject.empty()
-        val dmo = new ConfigDelayedMergeObject(fakeOrigin(), List[AbstractConfigValue](emptyObj, subst("a"), subst("b")).asJava)
+        val dmo = new ConfigDelayedMergeObject(fakeOrigin(),
+            List[AbstractConfigValue](emptyObj, subst("a"), subst("b")).asJava)
         assertEquals(ConfigValueType.OBJECT, dmo.valueType())
         unresolved { dmo.unwrapped() }
         unresolved { dmo.containsKey(null) }