Clean up code and semantics around self-referential handling

The basic idea in this patch is to unify on the "replacer"
(modifying the tree in which we lookup substitutions)
mechanism for detecting cycles.

Drop the "traverse" method, instead keeping a trace of expressions
we've passed through for nice error messages.

There is now only one internal checked exception possible,
NotPossibleToResolve which is thrown when a cycle is detected;
this checked exception is always caught by ConfigReference
so cannot "escape" from the library. The exception exists
because we want to get the traceString debug info out
of the spot that detected the cycle, if we didn't want that
debug info we could just return null as usual for undefined.

As part of simplifying this (which should also simplify the
spec), resolutions which require double-traverse of the same
reference are no longer supported:
   a=1, b=${a}, a=${b}

Also, cycles now always throw UnresolvedSubstitution rather
than BadValue. This was needed for consistency since
conceptually a single a=${a} is going to "look back" earlier
in the merge stack, discover there is no earlier value of a,
and fail; it should be the same exception as a=${a},a={b:1},
and in both cases referring to these cycles via ${?} should
hide the exception.

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

@@ -244,12 +244,12 @@ public abstract class ConfigException extends RuntimeException {
     public static class UnresolvedSubstitution extends Parse {
         private static final long serialVersionUID = 1L;
 
-        public UnresolvedSubstitution(ConfigOrigin origin, String expression, Throwable cause) {
-            super(origin, "Could not resolve substitution to a value: " + expression, cause);
+        public UnresolvedSubstitution(ConfigOrigin origin, String detail, Throwable cause) {
+            super(origin, "Could not resolve substitution to a value: " + detail, cause);
         }
 
-        public UnresolvedSubstitution(ConfigOrigin origin, String expression) {
-            this(origin, expression, null);
+        public UnresolvedSubstitution(ConfigOrigin origin, String detail) {
+            this(origin, detail, null);
         }
     }
 

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

@@ -85,20 +85,22 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements Confi
      * resolver != null.
      * 
      * @throws NotPossibleToResolve
+     *             if context is not null and resolution fails
      */
     protected AbstractConfigValue peekPath(Path path, ResolveContext context) throws NotPossibleToResolve {
         return peekPath(this, path, context);
     }
 
     /**
-     * Looks up the path and throws public API exceptions (ConfigException).
-     * Doesn't do any resolution, will throw if any is needed.
+     * Looks up the path. Doesn't do any resolution, will throw if any is
+     * needed.
      */
-    AbstractConfigValue peekPathWithExternalExceptions(Path path) {
+    AbstractConfigValue peekPath(Path path) {
         try {
             return peekPath(this, path, null);
         } catch (NotPossibleToResolve e) {
-            throw e.exportException(origin(), path.render());
+            throw new ConfigException.BugOrBroken(
+                    "NotPossibleToResolve happened though we had no ResolveContext in peekPath");
         }
     }
 

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

@@ -33,53 +33,29 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue, Seria
     }
 
     /**
-     * This exception means that a value is inherently not resolveable, for
-     * example because there's a cycle in the substitutions. That's different
-     * from a ConfigException.NotResolved which just means it hasn't been
-     * resolved. This is a checked exception since it's internal to the library
-     * and we want to be sure we handle it before passing it out to public API.
+     * This exception means that a value is inherently not resolveable, at the
+     * moment the only known cause is a cycle of substitutions. This is a
+     * checked exception since it's internal to the library and we want to be
+     * sure we handle it before passing it out to public API. This is only
+     * supposed to be thrown by the target of a cyclic reference and it's
+     * supposed to be caught by the ConfigReference looking up that reference,
+     * so it should be impossible for an outermost resolve() to throw this.
+     *
+     * Contrast with ConfigException.NotResolved which just means nobody called
+     * resolve().
      */
     static class NotPossibleToResolve extends Exception {
         private static final long serialVersionUID = 1L;
 
-        ConfigOrigin origin;
-        String path;
+        final private String traceString;
 
-        NotPossibleToResolve(String message) {
-            super(message);
-            this.origin = null;
-            this.path = null;
+        NotPossibleToResolve(ResolveContext context) {
+            super("was not possible to resolve");
+            this.traceString = context.traceString();
         }
 
-        // use this constructor ONLY if you know the right origin and path
-        // to describe the problem.
-        NotPossibleToResolve(ConfigOrigin origin, String path, String message) {
-            this(origin, path, message, null);
-        }
-
-        NotPossibleToResolve(ConfigOrigin origin, String path, String message, Throwable cause) {
-            super(message, cause);
-            this.origin = origin;
-            this.path = path;
-        }
-
-        ConfigException exportException(ConfigOrigin outerOrigin, String outerPath) {
-            ConfigOrigin o = origin != null ? origin : outerOrigin;
-            String p = path != null ? path : outerPath;
-            if (p == null)
-                path = "";
-            if (o != null)
-                return new ConfigException.BadValue(o, p, getMessage(), this);
-            else
-                return new ConfigException.BadValue(p, getMessage(), this);
-        }
-    }
-
-    static final class SelfReferential extends NotPossibleToResolve {
-        private static final long serialVersionUID = 1L;
-
-        SelfReferential(ConfigOrigin origin, String path) {
-            super(origin, path, "Substitution ${" + path + "} is part of a cycle of substitutions");
+        String traceString() {
+            return traceString;
         }
     }
 

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

@@ -102,20 +102,13 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab
         return Collections.singleton(this);
     }
 
-    private static ResolveReplacer undefinedReplacer = new ResolveReplacer() {
-        @Override
-        protected AbstractConfigValue makeReplacement() throws Undefined {
-            throw new Undefined();
-        }
-    };
-
     @Override
     AbstractConfigValue resolveSubstitutions(ResolveContext context) throws NotPossibleToResolve {
         List<AbstractConfigValue> resolved = new ArrayList<AbstractConfigValue>(pieces.size());
         // if you have "foo = ${?foo}bar" then we will
         // self-referentially look up foo and we need to
         // get undefined, rather than "bar"
-        context.replace(this, undefinedReplacer);
+        context.source().replace(this, ResolveReplacer.cycleResolveReplacer);
         try {
             for (AbstractConfigValue p : pieces) {
                 // to concat into a string we have to do a full resolve,
@@ -138,7 +131,7 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab
                 }
             }
         } finally {
-            context.unreplace(this);
+            context.source().unreplace(this);
         }
 
         // now need to concat everything

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

@@ -12,7 +12,6 @@ import java.util.List;
 import com.typesafe.config.ConfigException;
 import com.typesafe.config.ConfigOrigin;
 import com.typesafe.config.ConfigValueType;
-import com.typesafe.config.impl.ResolveReplacer.Undefined;
 
 /**
  * The issue here is that we want to first merge our stack of config files, and
@@ -91,7 +90,7 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl
                 // ConfigDelayedMerge with a value that is only
                 // the remainder of the stack below this one.
 
-                context.replace((AbstractConfigValue) replaceable,
+                context.source().replace((AbstractConfigValue) replaceable,
                         replaceable.makeReplacer(count + 1));
                 replaced = true;
             }
@@ -101,7 +100,7 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl
                 resolved = context.resolve(v);
             } finally {
                 if (replaced)
-                    context.unreplace((AbstractConfigValue) replaceable);
+                    context.source().unreplace((AbstractConfigValue) replaceable);
             }
 
             if (resolved != null) {
@@ -120,20 +119,21 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl
     public ResolveReplacer makeReplacer(final int skipping) {
         return new ResolveReplacer() {
             @Override
-            protected AbstractConfigValue makeReplacement() throws Undefined {
-                return ConfigDelayedMerge.makeReplacement(stack, skipping);
+            protected AbstractConfigValue makeReplacement(ResolveContext context)
+                    throws NotPossibleToResolve {
+                return ConfigDelayedMerge.makeReplacement(context, stack, skipping);
             }
         };
     }
 
     // static method also used by ConfigDelayedMergeObject
-    static AbstractConfigValue makeReplacement(List<AbstractConfigValue> stack, int skipping)
-            throws Undefined {
+    static AbstractConfigValue makeReplacement(ResolveContext context,
+            List<AbstractConfigValue> stack, int skipping) throws NotPossibleToResolve {
 
         List<AbstractConfigValue> subStack = stack.subList(skipping, stack.size());
 
         if (subStack.isEmpty()) {
-            throw new ResolveReplacer.Undefined();
+            throw new NotPossibleToResolve(context);
         } else {
             // generate a new merge stack from only the remaining items
             AbstractConfigValue merged = null;

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

@@ -75,8 +75,9 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unm
     public ResolveReplacer makeReplacer(final int skipping) {
         return new ResolveReplacer() {
             @Override
-            protected AbstractConfigValue makeReplacement() throws Undefined {
-                return ConfigDelayedMerge.makeReplacement(stack, skipping);
+            protected AbstractConfigValue makeReplacement(ResolveContext context)
+                    throws NotPossibleToResolve {
+                return ConfigDelayedMerge.makeReplacement(context, stack, skipping);
             }
         };
     }

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

@@ -103,14 +103,33 @@ final class ConfigReference extends AbstractConfigValue implements Unmergeable {
         return Collections.singleton(this);
     }
 
+    // ConfigReference should be a firewall against NotPossibleToResolve going
+    // further up the stack; it should convert everything to ConfigException.
+    // This way it's impossible for NotPossibleToResolve to "escape" since
+    // any failure to resolve has to start with a ConfigReference.
     @Override
-    AbstractConfigValue resolveSubstitutions(ResolveContext context) throws NotPossibleToResolve {
-        AbstractConfigValue v = context.source().lookupSubst(context, this, expr, prefixLength);
+    AbstractConfigValue resolveSubstitutions(ResolveContext context) {
+        context.source().replace(this, ResolveReplacer.cycleResolveReplacer);
+        try {
+            AbstractConfigValue v;
+            try {
+                v = context.source().lookupSubst(context, expr, prefixLength);
+            } catch (NotPossibleToResolve e) {
+                if (expr.optional())
+                    v = null;
+                else
+                    throw new ConfigException.UnresolvedSubstitution(origin(), expr
+                            + " was part of a cycle of substitutions involving " + e.traceString(),
+                            e);
+            }
 
-        if (v == null && !expr.optional()) {
-            throw new ConfigException.UnresolvedSubstitution(origin(), expr.toString());
+            if (v == null && !expr.optional()) {
+                throw new ConfigException.UnresolvedSubstitution(origin(), expr.toString());
+            }
+            return v;
+        } finally {
+            context.source().unreplace(this);
         }
-        return v;
     }
 
     @Override

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

@@ -1,15 +1,11 @@
 package com.typesafe.config.impl;
 
-import java.util.Collections;
-import java.util.LinkedList;
-import java.util.Set;
-import java.util.LinkedHashSet;
-import java.util.concurrent.Callable;
+import java.util.List;
+import java.util.ArrayList;
 
 import com.typesafe.config.ConfigException;
 import com.typesafe.config.ConfigResolveOptions;
 import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve;
-import com.typesafe.config.impl.AbstractConfigValue.SelfReferential;
 
 final class ResolveContext {
     // this is unfortunately mutable so should only be shared among
@@ -20,12 +16,6 @@ final class ResolveContext {
     // ResolveContext in the same traversal.
     final private ResolveMemos memos;
 
-    // Resolves that we have already begun (for cycle detection).
-    // SubstitutionResolver separately memoizes completed resolves.
-    // this set is unfortunately mutable and the user of ResolveContext
-    // has to be sure it's only shared between ResolveContext that
-    // are in the same traversal.
-    final private LinkedList<Set<MemoKey>> traversedStack;
     final private ConfigResolveOptions options;
     // the current path restriction, used to ensure lazy
     // resolution and avoid gratuitous cycles. without this,
@@ -34,83 +24,25 @@ final class ResolveContext {
     // CAN BE NULL for a full resolve.
     final private Path restrictToChild;
 
-    ResolveContext(ResolveSource source, ResolveMemos memos,
-            LinkedList<Set<MemoKey>> traversedStack, ConfigResolveOptions options,
-            Path restrictToChild) {
+    // another mutable unfortunate. This is
+    // used to make nice error messages when
+    // resolution fails.
+    final private List<SubstitutionExpression> expressionTrace;
+
+    ResolveContext(ResolveSource source, ResolveMemos memos, ConfigResolveOptions options,
+            Path restrictToChild, List<SubstitutionExpression> expressionTrace) {
         this.source = source;
         this.memos = memos;
-        this.traversedStack = traversedStack;
         this.options = options;
         this.restrictToChild = restrictToChild;
+        this.expressionTrace = expressionTrace;
     }
 
     ResolveContext(AbstractConfigObject root, ConfigResolveOptions options, Path restrictToChild) {
         // LinkedHashSet keeps the traversal order which is at least useful
         // in error messages if nothing else
-        this(new ResolveSource(root), new ResolveMemos(), new LinkedList<Set<MemoKey>>(
-                Collections.singletonList(new LinkedHashSet<MemoKey>())), options, restrictToChild);
-    }
-
-    private void traverse(ConfigReference value, SubstitutionExpression via)
-            throws SelfReferential {
-        Set<MemoKey> traversed = traversedStack.peekFirst();
-
-        MemoKey key = new MemoKey(value, restrictToChild);
-        if (traversed.contains(key)) {
-            throw new SelfReferential(value.origin(), via.path().render());
-        }
-
-        traversed.add(key);
-    }
-
-    private void untraverse(ConfigReference value) {
-        Set<MemoKey> traversed = traversedStack.peekFirst();
-
-        MemoKey key = new MemoKey(value, restrictToChild);
-        if (!traversed.remove(key))
-            throw new ConfigException.BugOrBroken(
-                    "untraverse() did not find the untraversed substitution " + value);
-    }
-
-    // this just exists to fix the "throws Exception" on Callable
-    interface Resolver extends Callable<AbstractConfigValue> {
-        @Override
-        AbstractConfigValue call() throws NotPossibleToResolve;
-    }
-
-    AbstractConfigValue traversing(ConfigReference value, SubstitutionExpression subst,
-            Resolver callable) throws NotPossibleToResolve {
-        try {
-            traverse(value, subst);
-        } catch (SelfReferential e) {
-            if (subst.optional())
-                return null;
-            else
-                throw e;
-        }
-
-        try {
-            return callable.call();
-        } finally {
-            untraverse(value);
-        }
-    }
-
-    void replace(AbstractConfigValue value, ResolveReplacer replacer) {
-        source.replace(value, replacer);
-
-        // we have to reset the cycle detection because with the
-        // replacement, a cycle may not exist anymore.
-        traversedStack.addFirst(new LinkedHashSet<MemoKey>());
-    }
-
-    void unreplace(AbstractConfigValue value) {
-        source.unreplace(value);
-
-        Set<MemoKey> oldTraversed = traversedStack.removeFirst();
-        if (!oldTraversed.isEmpty())
-            throw new ConfigException.BugOrBroken(
-                    "unreplace() with stuff still in the traverse set: " + oldTraversed);
+        this(new ResolveSource(root), new ResolveMemos(), options, restrictToChild,
+                new ArrayList<SubstitutionExpression>());
     }
 
     ResolveSource source() {
@@ -133,15 +65,34 @@ final class ResolveContext {
         if (restrictTo == restrictToChild)
             return this;
         else
-            return new ResolveContext(source, memos, traversedStack, options, restrictTo);
+            return new ResolveContext(source, memos, options, restrictTo, expressionTrace);
     }
 
     ResolveContext unrestricted() {
         return restrict(null);
     }
 
-    AbstractConfigValue resolve(AbstractConfigValue original)
-            throws NotPossibleToResolve {
+    void trace(SubstitutionExpression expr) {
+        expressionTrace.add(expr);
+    }
+
+    void untrace() {
+        expressionTrace.remove(expressionTrace.size() - 1);
+    }
+
+    String traceString() {
+        String separator = ", ";
+        StringBuilder sb = new StringBuilder();
+        for (SubstitutionExpression expr : expressionTrace) {
+            sb.append(expr.toString());
+            sb.append(separator);
+        }
+        if (sb.length() > 0)
+            sb.setLength(sb.length() - separator.length());
+        return sb.toString();
+    }
+
+    AbstractConfigValue resolve(AbstractConfigValue original) throws NotPossibleToResolve {
 
         // a fully-resolved (no restrictToChild) object can satisfy a
         // request for a restricted object, so always check that first.
@@ -190,20 +141,20 @@ final class ResolveContext {
     }
 
     static AbstractConfigValue resolve(AbstractConfigValue value, AbstractConfigObject root,
-            ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve {
-        ResolveContext context = new ResolveContext(root, options, restrictToChildOrNull);
-
-        return context.resolve(value);
-    }
-
-    static AbstractConfigValue resolveWithExternalExceptions(AbstractConfigValue value,
-            AbstractConfigObject root, ConfigResolveOptions options) {
+            ConfigResolveOptions options, Path restrictToChildOrNull) {
         ResolveContext context = new ResolveContext(root, options, null /* restrictToChild */);
 
         try {
             return context.resolve(value);
         } catch (NotPossibleToResolve e) {
-            throw e.exportException(value.origin(), null);
+            // ConfigReference was supposed to catch NotPossibleToResolve
+            throw new ConfigException.BugOrBroken(
+                    "NotPossibleToResolve was thrown from an outermost resolve", e);
         }
     }
+
+    static AbstractConfigValue resolve(AbstractConfigValue value, AbstractConfigObject root,
+            ConfigResolveOptions options) {
+        return resolve(value, root, options, null);
+    }
 }

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

@@ -1,15 +1,9 @@
 package com.typesafe.config.impl;
 
+import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve;
+
 /** Callback that generates a replacement to use for resolving a substitution. */
 abstract class ResolveReplacer {
-    static final class Undefined extends Exception {
-        private static final long serialVersionUID = 1L;
-
-        Undefined() {
-            super("No replacement, substitution will resolve to undefined");
-        }
-    }
-
     // this is a "lazy val" in essence (we only want to
     // make the replacement one time). Making it volatile
     // is good enough for thread safety as long as this
@@ -17,11 +11,20 @@ abstract class ResolveReplacer {
     // twice has no side effects, which it should not...
     private volatile AbstractConfigValue replacement = null;
 
-    final AbstractConfigValue replace() throws Undefined {
+    final AbstractConfigValue replace(ResolveContext context) throws NotPossibleToResolve {
         if (replacement == null)
-            replacement = makeReplacement();
+            replacement = makeReplacement(context);
         return replacement;
     }
 
-    protected abstract AbstractConfigValue makeReplacement() throws Undefined;
+    protected abstract AbstractConfigValue makeReplacement(ResolveContext context)
+            throws NotPossibleToResolve;
+
+    static final ResolveReplacer cycleResolveReplacer = new ResolveReplacer() {
+        @Override
+        protected AbstractConfigValue makeReplacement(ResolveContext context)
+                throws NotPossibleToResolve {
+            throw new NotPossibleToResolve(context);
+        }
+    };
 }

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

@@ -1,12 +1,10 @@
 package com.typesafe.config.impl;
 
 import java.util.IdentityHashMap;
-import java.util.LinkedList;
 import java.util.Map;
 
 import com.typesafe.config.ConfigException;
 import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve;
-import com.typesafe.config.impl.ResolveReplacer.Undefined;
 
 /**
  * This class is the source for values for a substitution like ${foo}.
@@ -18,83 +16,79 @@ final class ResolveSource {
     // traversed node and therefore avoid circular dependencies.
     // We implement it with this somewhat hacky "patch a replacement"
     // mechanism instead of actually transforming the tree.
-    final private Map<AbstractConfigValue, LinkedList<ResolveReplacer>> replacements;
+    final private Map<AbstractConfigValue, ResolveReplacer> replacements;
 
     ResolveSource(AbstractConfigObject root) {
         this.root = root;
-        this.replacements = new IdentityHashMap<AbstractConfigValue, LinkedList<ResolveReplacer>>();
+        this.replacements = new IdentityHashMap<AbstractConfigValue, ResolveReplacer>();
     }
 
-    static private AbstractConfigValue findInObject(final AbstractConfigObject obj,
-            final ResolveContext context, ConfigReference traversed,
-            final SubstitutionExpression subst) throws NotPossibleToResolve {
-        return context.traversing(traversed, subst, new ResolveContext.Resolver() {
-            @Override
-            public AbstractConfigValue call() throws NotPossibleToResolve {
-                return obj.peekPath(subst.path(), context);
-            }
-        });
+    static private AbstractConfigValue findInObject(AbstractConfigObject obj,
+            ResolveContext context, SubstitutionExpression subst)
+            throws NotPossibleToResolve {
+        return obj.peekPath(subst.path(), context);
     }
 
-    AbstractConfigValue lookupSubst(final ResolveContext context, ConfigReference traversed,
-            final SubstitutionExpression subst, int prefixLength) throws NotPossibleToResolve {
-        // First we look up the full path, which means relative to the
-        // included file if we were not a root file
-        AbstractConfigValue result = findInObject(root, context, traversed, subst);
+    AbstractConfigValue lookupSubst(ResolveContext context, SubstitutionExpression subst,
+            int prefixLength) throws NotPossibleToResolve {
+        context.trace(subst);
+        try {
+            // First we look up the full path, which means relative to the
+            // included file if we were not a root file
+            AbstractConfigValue result = findInObject(root, context, subst);
 
-        if (result == null) {
-            // Then we want to check relative to the root file. We don't
-            // want the prefix we were included at to be used when looking
-            // up env variables either.
-            SubstitutionExpression unprefixed = subst
-                    .changePath(subst.path().subPath(prefixLength));
+            if (result == null) {
+                // Then we want to check relative to the root file. We don't
+                // want the prefix we were included at to be used when looking
+                // up env variables either.
+                SubstitutionExpression unprefixed = subst.changePath(subst.path().subPath(
+                        prefixLength));
 
-            if (result == null && prefixLength > 0) {
-                result = findInObject(root, context, traversed, unprefixed);
-            }
+                // replace the debug trace path
+                context.untrace();
+                context.trace(unprefixed);
 
-            if (result == null && context.options().getUseSystemEnvironment()) {
-                result = findInObject(ConfigImpl.envVariablesAsConfigObject(), context, traversed,
-                        unprefixed);
-            }
-        }
-
-        if (result != null) {
-            final AbstractConfigValue unresolved = result;
-            result = context.traversing(traversed, subst, new ResolveContext.Resolver() {
-                @Override
-                public AbstractConfigValue call() throws NotPossibleToResolve {
-                    return context.resolve(unresolved);
+                if (result == null && prefixLength > 0) {
+                    result = findInObject(root, context, unprefixed);
                 }
-            });
-        }
 
-        return result;
+                if (result == null && context.options().getUseSystemEnvironment()) {
+                    result = findInObject(ConfigImpl.envVariablesAsConfigObject(), context,
+                            unprefixed);
+                }
+            }
+
+            if (result != null) {
+                result = context.resolve(result);
+            }
+
+            return result;
+        } finally {
+            context.untrace();
+        }
     }
 
     void replace(AbstractConfigValue value, ResolveReplacer replacer) {
-        LinkedList<ResolveReplacer> stack = replacements.get(value);
-        if (stack == null) {
-            stack = new LinkedList<ResolveReplacer>();
-            replacements.put(value, stack);
-        }
-        stack.addFirst(replacer);
+        ResolveReplacer old = replacements.put(value, replacer);
+        if (old != null)
+            throw new ConfigException.BugOrBroken("should not have replaced the same value twice: "
+                    + value);
     }
 
     void unreplace(AbstractConfigValue value) {
-        LinkedList<ResolveReplacer> stack = replacements.get(value);
-        if (stack == null)
+        ResolveReplacer replacer = replacements.remove(value);
+        if (replacer == null)
             throw new ConfigException.BugOrBroken("unreplace() without replace(): " + value);
-
-        stack.removeFirst();
     }
 
-    private AbstractConfigValue replacement(AbstractConfigValue value) throws Undefined {
-        LinkedList<ResolveReplacer> stack = replacements.get(value);
-        if (stack == null || stack.isEmpty())
+    private AbstractConfigValue replacement(ResolveContext context, AbstractConfigValue value)
+            throws NotPossibleToResolve {
+        ResolveReplacer replacer = replacements.get(value);
+        if (replacer == null) {
             return value;
-        else
-            return stack.peek().replace();
+        } else {
+            return replacer.replace(context);
+        }
     }
 
     /**
@@ -104,13 +98,8 @@ final class ResolveSource {
     AbstractConfigValue resolveCheckingReplacement(ResolveContext context,
             AbstractConfigValue original) throws NotPossibleToResolve {
         AbstractConfigValue replacement;
-        boolean forceUndefined = false;
-        try {
-            replacement = replacement(original);
-        } catch (Undefined e) {
-            replacement = original;
-            forceUndefined = true;
-        }
+
+        replacement = replacement(context, original);
 
         if (replacement != original) {
             // start over, checking if replacement was memoized
@@ -118,10 +107,7 @@ final class ResolveSource {
         } else {
             AbstractConfigValue resolved;
 
-            if (forceUndefined)
-                resolved = null;
-            else
-                resolved = original.resolveSubstitutions(context);
+            resolved = original.resolveSubstitutions(context);
 
             return resolved;
         }

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

@@ -22,7 +22,6 @@ import com.typesafe.config.ConfigOrigin;
 import com.typesafe.config.ConfigResolveOptions;
 import com.typesafe.config.ConfigValue;
 import com.typesafe.config.ConfigValueType;
-import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve;
 
 /**
  * One thing to keep in mind in the future: as Collection-like APIs are added
@@ -57,8 +56,7 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
 
     @Override
     public SimpleConfig resolve(ConfigResolveOptions options) {
-        AbstractConfigValue resolved = ResolveContext.resolveWithExternalExceptions(object,
-                object, options);
+        AbstractConfigValue resolved = ResolveContext.resolve(object, object, options);
 
         if (resolved == object)
             return this;
@@ -72,9 +70,7 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
         Path path = Path.newPath(pathExpression);
         ConfigValue peeked;
         try {
-            peeked = object.peekPath(path, null);
-        } catch (NotPossibleToResolve e) {
-            throw e.exportException(origin(), pathExpression);
+            peeked = object.peekPath(path);
         } catch (ConfigException.NotResolved e) {
             throw ConfigImpl.improveNotResolved(pathExpression, e);
         }
@@ -669,7 +665,7 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
     }
 
     private AbstractConfigValue peekPath(Path path) {
-        return root().peekPathWithExternalExceptions(path);
+        return root().peekPath(path);
     }
 
     private static void addProblem(List<ConfigException.ValidationProblem> accumulator, Path path,

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

@@ -31,7 +31,7 @@ class ConfParserTest extends TestUtils {
         // interpolating arrays into strings
         tree match {
             case obj: AbstractConfigObject =>
-                ResolveContext.resolveWithExternalExceptions(tree, obj, ConfigResolveOptions.noSystem())
+                ResolveContext.resolve(tree, obj, ConfigResolveOptions.noSystem())
             case _ =>
                 tree
         }

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

@@ -15,20 +15,20 @@ class ConfigSubstitutionTest extends TestUtils {
 
     private def resolveWithoutFallbacks(v: AbstractConfigObject) = {
         val options = ConfigResolveOptions.noSystem()
-        ResolveContext.resolveWithExternalExceptions(v, v, options).asInstanceOf[AbstractConfigObject].toConfig
+        ResolveContext.resolve(v, v, options).asInstanceOf[AbstractConfigObject].toConfig
     }
     private def resolveWithoutFallbacks(s: ConfigSubstitution, root: AbstractConfigObject) = {
         val options = ConfigResolveOptions.noSystem()
-        ResolveContext.resolveWithExternalExceptions(s, root, options)
+        ResolveContext.resolve(s, root, options)
     }
 
     private def resolve(v: AbstractConfigObject) = {
         val options = ConfigResolveOptions.defaults()
-        ResolveContext.resolveWithExternalExceptions(v, v, options).asInstanceOf[AbstractConfigObject].toConfig
+        ResolveContext.resolve(v, v, options).asInstanceOf[AbstractConfigObject].toConfig
     }
     private def resolve(s: ConfigSubstitution, root: AbstractConfigObject) = {
         val options = ConfigResolveOptions.defaults()
-        ResolveContext.resolveWithExternalExceptions(s, root, options)
+        ResolveContext.resolve(s, root, options)
     }
 
     private val simpleObject = {
@@ -97,10 +97,12 @@ class ConfigSubstitutionTest extends TestUtils {
 
     @Test
     def resolveMissingThrows() {
-        intercept[ConfigException.UnresolvedSubstitution] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             val s = subst("bar.missing")
             val v = resolveWithoutFallbacks(s, simpleObject)
         }
+        assertTrue("wrong exception: " + e.getMessage,
+            !e.getMessage.contains("cycle"))
     }
 
     @Test
@@ -218,10 +220,11 @@ class ConfigSubstitutionTest extends TestUtils {
     @Test
     def throwOnCycles() {
         val s = subst("foo")
-        val e = intercept[ConfigException.BadValue] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             val v = resolveWithoutFallbacks(s, substCycleObject)
         }
         assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle"))
+        assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("${foo}, ${bar}, ${a.b.c}, ${foo}"))
     }
 
     @Test
@@ -229,7 +232,7 @@ class ConfigSubstitutionTest extends TestUtils {
         // we look up ${?foo}, but the cycle has hard
         // non-optional links in it so still has to throw.
         val s = subst("foo", optional = true)
-        val e = intercept[ConfigException.BadValue] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             val v = resolveWithoutFallbacks(s, substCycleObject)
         }
         assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle"))
@@ -256,7 +259,7 @@ class ConfigSubstitutionTest extends TestUtils {
     @Test
     def throwOnTwoKeyCycle() {
         val obj = parseObject("""a:${b},b:${a}""")
-        val e = intercept[ConfigException.BadValue] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             resolve(obj)
         }
         assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle"))
@@ -265,7 +268,7 @@ class ConfigSubstitutionTest extends TestUtils {
     @Test
     def throwOnFourKeyCycle() {
         val obj = parseObject("""a:${b},b:${c},c:${d},d:${a}""")
-        val e = intercept[ConfigException.BadValue] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             resolve(obj)
         }
         assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle"))
@@ -1023,7 +1026,7 @@ class ConfigSubstitutionTest extends TestUtils {
     @Test
     def substSelfReferenceUndefined() {
         val obj = parseObject("""a=${a}""")
-        val e = intercept[ConfigException.BadValue] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             resolve(obj)
         }
         assertTrue("wrong exception: " + e.getMessage, e.getMessage.contains("cycle"))
@@ -1039,15 +1042,19 @@ class ConfigSubstitutionTest extends TestUtils {
     @Test
     def substSelfReferenceIndirect() {
         val obj = parseObject("""a=1, b=${a}, a=${b}""")
-        val resolved = resolve(obj)
-        assertEquals(1, resolved.getInt("a"))
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
+            resolve(obj)
+        }
+        assertTrue("wrong exception: " + e.getMessage, e.getMessage.contains("cycle"))
     }
 
     @Test
     def substSelfReferenceDoubleIndirect() {
         val obj = parseObject("""a=1, b=${c}, c=${a}, a=${b}""")
-        val resolved = resolve(obj)
-        assertEquals(1, resolved.getInt("a"))
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
+            resolve(obj)
+        }
+        assertTrue("wrong exception: " + e.getMessage, e.getMessage.contains("cycle"))
     }
 
     @Test
@@ -1201,7 +1208,7 @@ class ConfigSubstitutionTest extends TestUtils {
     @Test
     def substInChildFieldNotASelfReference3() {
         // checking that having bar.foo earlier in the merge
-        // stack doesn't break the behavior
+        // stack doesn't break the behavior.
         val obj = parseObject("""
          bar : { foo : 43 }
          bar : { foo : 42,

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

@@ -20,11 +20,11 @@ import com.typesafe.config.ConfigMergeable
 class ConfigTest extends TestUtils {
 
     private def resolveNoSystem(v: AbstractConfigValue, root: AbstractConfigObject) = {
-        ResolveContext.resolveWithExternalExceptions(v, root, ConfigResolveOptions.noSystem())
+        ResolveContext.resolve(v, root, ConfigResolveOptions.noSystem())
     }
 
     private def resolveNoSystem(v: SimpleConfig, root: SimpleConfig) = {
-        ResolveContext.resolveWithExternalExceptions(v.root, root.root,
+        ResolveContext.resolve(v.root, root.root,
             ConfigResolveOptions.noSystem()).asInstanceOf[AbstractConfigObject].toConfig
     }
 
@@ -346,10 +346,10 @@ class ConfigTest extends TestUtils {
         // the point here is that we should not try to evaluate a substitution
         // that's been overridden, and thus not end up with a cycle as long
         // as we override the problematic link in the cycle.
-        val e = intercept[ConfigException.BadValue] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             val v = resolveNoSystem(subst("foo"), cycleObject)
         }
-        assertTrue(e.getMessage().contains("cycle"))
+        assertTrue("wrong exception: " + e.getMessage, e.getMessage().contains("cycle"))
 
         val fixUpCycle = parseObject(""" { "a" : { "b" : { "c" : 57 } } } """)
         val merged = mergeUnresolved(fixUpCycle, cycleObject)
@@ -362,10 +362,10 @@ class ConfigTest extends TestUtils {
         // the point here is that if our eventual value will be an object, then
         // we have to evaluate the substitution to see if it's an object to merge,
         // so we don't avoid the cycle.
-        val e = intercept[ConfigException.BadValue] {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
             val v = resolveNoSystem(subst("foo"), cycleObject)
         }
-        assertTrue(e.getMessage().contains("cycle"))
+        assertTrue("wrong exception: " + e.getMessage, e.getMessage().contains("cycle"))
 
         val fixUpCycle = parseObject(""" { "a" : { "b" : { "c" : { "q" : "u" } } } } """)
         val merged = mergeUnresolved(fixUpCycle, cycleObject)

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

@@ -34,7 +34,7 @@ class EquivalentsTest extends TestUtils {
                 // for purposes of these tests, substitutions are only
                 // against the same file's root, and without looking at
                 // system prop or env variable fallbacks.
-                ResolveContext.resolveWithExternalExceptions(v, v, ConfigResolveOptions.noSystem())
+                ResolveContext.resolve(v, v, ConfigResolveOptions.noSystem())
             case v =>
                 v
         }