fix: look back in delayed merges for self-referential fields inside pieces

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

@@ -193,10 +193,12 @@ ResolveResult<? extends AbstractConfigValue> resolveSubstitutions(ResolveContext
             }
         }
 
-        // Right now there's no reason to pushParent here because the
-        // content of ConfigConcatenation should not need to replaceChild,
-        // but if it did we'd have to do this.
-        ResolveSource sourceWithParent = source; // .pushParent(this);
+        // Pieces can contain containers with unresolved substitutions (e.g. a
+        // delayed merge inside a list), and resolving those replaces them via
+        // ResolveSource.replaceWithinCurrentParent(); for the replacement to
+        // walk back up to the root, this concatenation must be on the parent
+        // chain.
+        ResolveSource sourceWithParent = source.pushParent(this);
         ResolveContext newContext = context;
 
         List<AbstractConfigValue> resolved = new ArrayList<AbstractConfigValue>(pieces.size());

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

@@ -81,6 +81,7 @@ static ResolveResult<? extends AbstractConfigValue> resolveSubstitutions(Replace
         ResolveContext newContext = context;
         int count = 0;
         AbstractConfigValue merged = null;
+        AbstractConfigValue wasMergeStackPiece = context.mergeStackPiece();
         for (AbstractConfigValue end : stack) {
             // Per the HOCON spec, a substitution hidden by a value that
             // cannot be merged with it is never evaluated. If merged already
@@ -146,6 +147,11 @@ && allKeysShadowed((SimpleConfigObject) end, (AbstractConfigObject) merged)) {
                     count += 1;
                     continue;
                 }
+
+                // mark the piece so self-referential fields directly inside
+                // it are dropped (looking back at the pieces below) when a
+                // lookup re-enters them while this piece is still resolving
+                newContext = newContext.withMergeStackPiece(end);
             }
 
             if (ConfigImpl.traceSubstitutionsEnabled()) {
@@ -157,7 +163,7 @@ && allKeysShadowed((SimpleConfigObject) end, (AbstractConfigObject) merged)) {
                         + " against " + sourceForEnd + " endWasRemoved=" + (source != sourceForEnd));
             ResolveResult<? extends AbstractConfigValue> result = newContext.resolve(end, sourceForEnd);
             AbstractConfigValue resolvedEnd = result.value;
-            newContext = result.context;
+            newContext = result.context.withMergeStackPiece(wasMergeStackPiece);
 
             if (resolvedEnd != null) {
                 if (merged == null) {

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

@@ -27,8 +27,14 @@ final class ResolveContext {
 
     final private Set<AbstractConfigValue> cycleMarkers;
 
+    // the delayed merge stack piece currently being resolved, or null. A
+    // self-referential field directly inside the piece is dropped in favor of
+    // earlier pieces when a lookup re-enters it while the piece resolves.
+    final private AbstractConfigValue mergeStackPiece;
+
     ResolveContext(ResolveMemos memos, ConfigResolveOptions options, Path restrictToChild,
-            List<AbstractConfigValue> resolveStack, Set<AbstractConfigValue> cycleMarkers) {
+            List<AbstractConfigValue> resolveStack, Set<AbstractConfigValue> cycleMarkers,
+            AbstractConfigValue mergeStackPiece) {
         this.memos = memos;
         this.options = options;
         this.restrictToChild = restrictToChild;
@@ -38,6 +44,7 @@ final class ResolveContext {
         // up shared between multiple ResolveContext.
         this.resolveStack = resolveStack;
         this.cycleMarkers = cycleMarkers;
+        this.mergeStackPiece = mergeStackPiece;
     }
 
     private static Set<AbstractConfigValue> newCycleMarkers() {
@@ -47,7 +54,8 @@ private static Set<AbstractConfigValue> newCycleMarkers() {
     ResolveContext(ConfigResolveOptions options, Path restrictToChild) {
         // LinkedHashSet keeps the traversal order which is at least useful
         // in error messages if nothing else
-        this(new ResolveMemos(), options, restrictToChild, new ArrayList<AbstractConfigValue>(), newCycleMarkers());
+        this(new ResolveMemos(), options, restrictToChild, new ArrayList<AbstractConfigValue>(), newCycleMarkers(),
+                null);
         if (ConfigImpl.traceSubstitutionsEnabled())
             ConfigImpl.trace(depth(), "ResolveContext restrict to child " + restrictToChild);
     }
@@ -60,7 +68,7 @@ ResolveContext addCycleMarker(AbstractConfigValue value) {
         Set<AbstractConfigValue> copy = newCycleMarkers();
         copy.addAll(cycleMarkers);
         copy.add(value);
-        return new ResolveContext(memos, options, restrictToChild, resolveStack, copy);
+        return new ResolveContext(memos, options, restrictToChild, resolveStack, copy, mergeStackPiece);
     }
 
     ResolveContext removeCycleMarker(AbstractConfigValue value) {
@@ -70,12 +78,12 @@ ResolveContext removeCycleMarker(AbstractConfigValue value) {
         Set<AbstractConfigValue> copy = newCycleMarkers();
         copy.addAll(cycleMarkers);
         copy.remove(value);
-        return new ResolveContext(memos, options, restrictToChild, resolveStack, copy);
+        return new ResolveContext(memos, options, restrictToChild, resolveStack, copy, mergeStackPiece);
     }
 
     private ResolveContext memoize(MemoKey key, AbstractConfigValue value) {
         ResolveMemos changed = memos.put(key, value);
-        return new ResolveContext(changed, options, restrictToChild, resolveStack, cycleMarkers);
+        return new ResolveContext(changed, options, restrictToChild, resolveStack, cycleMarkers, mergeStackPiece);
     }
 
     ConfigResolveOptions options() {
@@ -95,13 +103,35 @@ ResolveContext restrict(Path restrictTo) {
         if (restrictTo == restrictToChild)
             return this;
         else
-            return new ResolveContext(memos, options, restrictTo, resolveStack, cycleMarkers);
+            return new ResolveContext(memos, options, restrictTo, resolveStack, cycleMarkers, mergeStackPiece);
     }
 
     ResolveContext unrestricted() {
         return restrict(null);
     }
 
+    ResolveContext withMergeStackPiece(AbstractConfigValue mergeStackPiece) {
+        if (mergeStackPiece == this.mergeStackPiece)
+            return this;
+        else
+            return new ResolveContext(memos, options, restrictToChild, resolveStack, cycleMarkers, mergeStackPiece);
+    }
+
+    AbstractConfigValue mergeStackPiece() {
+        return mergeStackPiece;
+    }
+
+    // identity check whether the value is already being resolved somewhere
+    // else on the resolve stack; the current resolution itself is always on
+    // top of the stack, so it is not counted
+    boolean isResolvingElsewhere(AbstractConfigValue value) {
+        for (int i = 0; i < resolveStack.size() - 1; i++) {
+            if (resolveStack.get(i) == value)
+                return true;
+        }
+        return false;
+    }
+
     String traceString() {
         String separator = ", ";
         StringBuilder sb = new StringBuilder();
@@ -121,15 +151,15 @@ private ResolveContext pushTrace(AbstractConfigValue value) {
             ConfigImpl.trace(depth(), "pushing trace " + value);
         List<AbstractConfigValue> copy = new ArrayList<AbstractConfigValue>(resolveStack);
         copy.add(value);
-        return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers);
+        return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers, mergeStackPiece);
     }
 
     ResolveContext popTrace() {
         List<AbstractConfigValue> copy = new ArrayList<AbstractConfigValue>(resolveStack);
         AbstractConfigValue old = copy.remove(resolveStack.size() - 1);
         if (ConfigImpl.traceSubstitutionsEnabled())
             ConfigImpl.trace(depth() - 1, "popped trace " + old);
-        return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers);
+        return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers, mergeStackPiece);
     }
 
     int depth() {
@@ -189,6 +219,12 @@ private ResolveResult<? extends AbstractConfigValue> realResolve(AbstractConfigV
 
             ResolveContext withMemo = result.context;
 
+            // a resolution re-entered while the same value is still being
+            // resolved further out depends on that in-flight state (for
+            // example a self-referential field looking back inside a merge
+            // stack), so its result must not be cached
+            boolean reentering = mergeStackPiece != null && isResolvingElsewhere(original);
+
             if (resolved == null || resolved.resolveStatus() == ResolveStatus.RESOLVED) {
                 // if the resolved object is fully resolved by resolving
                 // only the restrictToChildOrNull, then it can be cached
@@ -197,7 +233,8 @@ private ResolveResult<? extends AbstractConfigValue> realResolve(AbstractConfigV
                 if (ConfigImpl.traceSubstitutionsEnabled())
                     ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved);
 
-                withMemo = withMemo.memoize(fullKey, resolved);
+                if (!reentering)
+                    withMemo = withMemo.memoize(fullKey, resolved);
             } else {
                 // if we have an unresolved object then either we did a
                 // partial resolve restricted to a certain child, or we are
@@ -210,12 +247,14 @@ private ResolveResult<? extends AbstractConfigValue> realResolve(AbstractConfigV
                     if (ConfigImpl.traceSubstitutionsEnabled())
                         ConfigImpl.trace(depth(), "caching " + restrictedKey + " result " + resolved);
 
-                    withMemo = withMemo.memoize(restrictedKey, resolved);
+                    if (!reentering)
+                        withMemo = withMemo.memoize(restrictedKey, resolved);
                 } else if (options().getAllowUnresolved()) {
                     if (ConfigImpl.traceSubstitutionsEnabled())
                         ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved);
 
-                    withMemo = withMemo.memoize(fullKey, resolved);
+                    if (!reentering)
+                        withMemo = withMemo.memoize(fullKey, resolved);
                 } else {
                     throw new ConfigException.BugOrBroken(
                             "resolveSubstitutions() did not give us a resolved object");

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

@@ -435,15 +435,25 @@ private SimpleConfigObject modifyMayThrow(Modifier modifier) throws Exception {
         }
     }
 
+    // HOCON.md: a self-referential field has a substitution, or a value
+    // concatenation containing a substitution, as its value
+    private static boolean potentiallySelfReferential(AbstractConfigValue v) {
+        return v instanceof ConfigReference || v instanceof ConfigConcatenation;
+    }
+
     private static final class ResolveModifier implements Modifier {
 
         final Path originalRestrict;
+        // Allow a self-referential leaf to look back within a merge piece,
+        // including when nested in one of its object fields.
+        final boolean dropInFlightChildren;
         ResolveContext context;
         final ResolveSource source;
 
-        ResolveModifier(ResolveContext context, ResolveSource source) {
+        ResolveModifier(ResolveContext context, ResolveSource source, boolean dropInFlightChildren) {
             this.context = context;
             this.source = source;
+            this.dropInFlightChildren = dropInFlightChildren;
             originalRestrict = context.restrictToChild();
         }
 
@@ -458,7 +468,12 @@ public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v
                         context = result.context.unrestricted().restrict(originalRestrict);
                         return result.value;
                     } else {
-                        // we don't want to resolve the leaf child.
+                        // Drop an in-flight self-referential leaf to look back through the merge.
+                        // Object and array cycles remain unbreakable under HOCON.md.
+                        if (dropInFlightChildren && potentiallySelfReferential(v)
+                                && context.isResolvingElsewhere(v)) {
+                            return null;
+                        }
                         return v;
                     }
                 } else {
@@ -484,7 +499,12 @@ ResolveResult<? extends AbstractConfigObject> resolveSubstitutions(ResolveContex
         final ResolveSource sourceWithParent = source.pushParent(this);
 
         try {
-            ResolveModifier modifier = new ResolveModifier(context, sourceWithParent);
+            boolean inMergeStackPiece = context.mergeStackPiece() == this;
+            for (ResolveSource.Node<Container> node = sourceWithParent.pathFromRoot;
+                    !inMergeStackPiece && node != null; node = node.tail()) {
+                inMergeStackPiece = node.head() == context.mergeStackPiece();
+            }
+            ResolveModifier modifier = new ResolveModifier(context, sourceWithParent, inMergeStackPiece);
 
             AbstractConfigValue value = modifyMayThrow(modifier);
             return ResolveResult.make(modifier.context, value).asObjectResult();

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

@@ -656,4 +656,49 @@ class ConcatenationTest extends TestUtils {
             parseConfig("""foo = [1], bar = [2], x = ${foo}"  "${bar}""").resolve()
         }
     }
+
+    // https://github.com/lightbend/config/issues/751
+    // A delayed merge (a duplicate key whose later value is a substitution)
+    // inside a list that is a piece of a value concatenation used to throw
+    // BugOrBroken "SimpleConfigObject.replaceChild did not find ...": the
+    // concatenation was not on the substitution source's parent chain, so the
+    // resolver walked past it and could not find the list in the parent.
+    @Test
+    def delayedMergeInConcatenatedListPiece(): Unit = {
+        val conf = parseConfig("""
+            |tail = [ 2 ]
+            |front = [
+            |  {
+            |    foo = 1
+            |    foo = ${?bar}
+            |  }
+            |] ${tail}
+            |""".stripMargin).resolve()
+
+        val front = conf.getList("front")
+        assertEquals(2, front.size())
+        assertEquals(1, front.get(0).asInstanceOf[AbstractConfigObject].toConfig.getInt("foo"))
+        assertEquals(2, front.get(1).unwrapped())
+    }
+
+    // Same shape as delayedMergeInConcatenatedListPiece, but the substitution
+    // is strict and defined, so it takes part in the delayed merge.
+    @Test
+    def definedSubstitutionInConcatenatedListPiece(): Unit = {
+        val conf = parseConfig("""
+            |bar = 7
+            |tail = [ 2 ]
+            |front = [
+            |  {
+            |    foo = 1
+            |    foo = ${bar}
+            |  }
+            |] ${tail}
+            |""".stripMargin).resolve()
+
+        val front = conf.getList("front")
+        assertEquals(2, front.size())
+        assertEquals(7, front.get(0).asInstanceOf[AbstractConfigObject].toConfig.getInt("foo"))
+        assertEquals(2, front.get(1).unwrapped())
+    }
 }

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

@@ -318,6 +318,64 @@ class ConfigSubstitutionTest extends TestUtils {
         assertEquals(42, resolved.getInt("a.cycle"))
     }
 
+    // issue #728: a self-referential list concatenation in a field of an
+    // object merged over a ${d} substitution must look back at the earlier
+    // definition of the field instead of reporting a cycle
+    @Test
+    def selfReferentialListConcatenationThroughMerge() {
+        val resolved = resolve(parseObject("""d { x = [] }, c : ${d}, c { x : ${c.x}[1, 2] }"""))
+        assertEquals(Seq(1, 2), resolved.getList("c.x").asScala.map(_.unwrapped()))
+    }
+
+    @Test
+    def selfReferentialListConcatenationThroughMergeSingleElement() {
+        val resolved = resolve(parseObject("""d { x = [] }, c : ${d}, c { x : ${c.x}[1] }"""))
+        assertEquals(Seq(1), resolved.getList("c.x").asScala.map(_.unwrapped()))
+    }
+
+    @Test
+    def substitutionThroughMergeStillLooksForward() {
+        // a sibling field of a merge piece refers to a key the piece itself
+        // defines; it must keep seeing the piece's final value
+        val resolved = resolve(parseObject("""d { x = 1 }, c : ${d}, c { x = 2, y = ${c.x} }"""))
+        assertEquals(2, resolved.getInt("c.y"))
+    }
+
+    @Test
+    def throwOnSelfReferentialCycleInsideObject() {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
+            resolve(parseObject("""a { b = ${a.c}, c = ${a.b} }"""))
+        }
+        assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle"))
+    }
+
+    @Test
+    def throwOnTrivialRootCycle() {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
+            resolve(parseObject("""a = ${a}"""))
+        }
+        assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle"))
+        assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("${a}"))
+    }
+
+    @Test
+    def throwOnSelfReferentialArrayInsideMergePiece() {
+        // an array with a substitution inside it is never self-referential,
+        // even when the field it is on was merged over a substitution
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
+            resolve(parseObject("""d { x = [1] }, c : ${d}, c { x = [${c.x}, 2] }"""))
+        }
+        assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle"))
+    }
+
+    @Test
+    def throwOnSelfReferentialObjectInsideMergePiece() {
+        val e = intercept[ConfigException.UnresolvedSubstitution] {
+            resolve(parseObject("""d { x = 1 }, c : ${d}, c { x = { i = ${c.x} } }"""))
+        }
+        assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle"))
+    }
+
     @Test
     def ignoreHiddenUndefinedSubst() {
         // if a substitution is overridden then it shouldn't matter that it's undefined
@@ -1613,4 +1671,16 @@ class ConfigSubstitutionTest extends TestUtils {
         assertNotSame(shared, cleared)
         assertSame(cleared, cleared.withFallbacksNotIgnored())
     }
+    @Test
+    def reviewNestedSelfReferenceThroughMerge(): Unit = {
+        val resolved = resolve(parseObject("""d { x { y = [0] } }, c = ${d}, c { x { y = ${c.x.y}[1] } }"""))
+        assertEquals(Seq(0, 1), resolved.getIntList("c.x.y").asScala.toSeq)
+    }
+
+    @Test
+    def reviewNestedOptionalSelfReferenceThroughMerge(): Unit = {
+        val resolved = resolve(parseObject("""d { x { y = [0] } }, c = ${d}, c { x { y = ${?c.x.y}[1] } }"""))
+        assertEquals(Seq(0, 1), resolved.getIntList("c.x.y").asScala.toSeq)
+    }
+
 }