From 25d4a35154d2f6cca1b0027ce274f052cfacaf23 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Tue, 26 Jan 2021 19:15:50 +0100 Subject: [PATCH] Fix detection of reused substatements There is a thinko here, as the comparison check is hidden by eager copy. This renders complete statement reuse inoperative. Fix this by explicitly specifying the contract of asEffectiveChildOf() and properly wrap it. JIRA: YANGTOOLS-1212 Change-Id: I9c3c3bc73e57854f1c126a5d6862b485c8659d34 Signed-off-by: Robert Varga --- .../reactor/InferredStatementContext.java | 87 ++++++++++++------- .../parser/stmt/reactor/ReactorStmtCtx.java | 13 +++ .../stmt/reactor/ReplicaStatementContext.java | 3 +- .../stmt/reactor/StatementContextBase.java | 2 +- .../yangtools/yang/stmt/YT1212Test.java | 30 +++++++ .../test/resources/bugs/YT1212/container.yang | 22 +++++ 6 files changed, 125 insertions(+), 32 deletions(-) create mode 100644 yang/yang-parser-rfc7950/src/test/resources/bugs/YT1212/container.yang diff --git a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/InferredStatementContext.java b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/InferredStatementContext.java index 183f4d79bf..f99a46b03f 100644 --- a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/InferredStatementContext.java +++ b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/InferredStatementContext.java @@ -20,13 +20,13 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; import java.util.stream.Stream; import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; +import org.opendaylight.yangtools.concepts.Immutable; import org.opendaylight.yangtools.yang.common.QName; import org.opendaylight.yangtools.yang.common.QNameModule; import org.opendaylight.yangtools.yang.model.api.SchemaPath; @@ -55,6 +55,32 @@ import org.slf4j.LoggerFactory; */ final class InferredStatementContext, E extends EffectiveStatement> extends StatementContextBase implements OnDemandSchemaTreeStorageNode { + // An effective copy view, with enough information to decide what to do next + private static final class EffectiveCopy implements Immutable { + // Original statement + private final ReactorStmtCtx orig; + // Effective view, if the statement is to be reused it equals to orig + private final ReactorStmtCtx copy; + + EffectiveCopy(final ReactorStmtCtx orig, final ReactorStmtCtx copy) { + this.orig = requireNonNull(orig); + this.copy = requireNonNull(copy); + } + + boolean isReused() { + return orig == copy; + } + + ReactorStmtCtx toChildContext(final @NonNull InferredStatementContext parent) { + return isReused() ? orig.replicaAsChildOf(parent) : copy; + } + + ReactorStmtCtx toReusedChild(final @NonNull InferredStatementContext parent) { + verify(isReused(), "Attempted to discard copy %s", copy); + return orig.replicaAsChildOf(parent); + } + } + private static final Logger LOG = LoggerFactory.getLogger(InferredStatementContext.class); // Sentinel object for 'substatements' @@ -210,35 +236,37 @@ final class InferredStatementContext, E extend } // We can reuse this statement let's see if all the statements agree - final List, Mutable>> declared = prototype.streamDeclared() + final List declCopy = prototype.streamDeclared() .filter(StmtContext::isSupportedByFeatures) .map(sub -> effectiveCopy((ReactorStmtCtx) sub)) .filter(Objects::nonNull) .collect(Collectors.toUnmodifiableList()); - final List, Mutable>> effective = prototype.streamEffective() + final List effCopy = prototype.streamEffective() .map(sub -> effectiveCopy((ReactorStmtCtx) sub)) .filter(Objects::nonNull) .collect(Collectors.toUnmodifiableList()); - // We no longer need the prototype's substatements, but we may need to retain ours - prototype.decRef(); - if (haveRef()) { - substatements = Streams.concat(declared.stream(), effective.stream()) - .map(Entry::getValue) - .collect(ImmutableList.toImmutableList()); - } else { - // This should immediately get swept anyway. Should we use a poison object? - substatements = List.of(); - } - - if (allReused(declared) && allReused(effective)) { + if (allReused(declCopy) && allReused(effCopy)) { LOG.debug("Reusing after substatement check: {}", origEffective); + // FIXME: can we skip this if !haveRef()? + substatements = reusePrototypeReplicas(Streams.concat(declCopy.stream(), effCopy.stream()) + .map(copy -> copy.toReusedChild(this))); + prototype.decRef(); return origEffective; } - // Values are the effective copies, hence this efficienly deals with recursion. - return factory.createEffective(this, declared.stream().map(Entry::getValue), - effective.stream().map(Entry::getValue)); + final List> declared = declCopy.stream() + .map(copy -> copy.toChildContext(this)) + .collect(ImmutableList.toImmutableList()); + final List> effective = effCopy.stream() + .map(copy -> copy.toChildContext(this)) + .collect(ImmutableList.toImmutableList()); + substatements = declared.isEmpty() ? effective + : Streams.concat(declared.stream(), effective.stream()).collect(ImmutableList.toImmutableList()); + prototype.decRef(); + + // Values are the effective copies, hence this efficiently deals with recursion. + return factory.createEffective(this, declared.stream(), effective.stream()); } private @NonNull E tryToReuseSubstatements(final StatementFactory factory, @@ -262,9 +290,13 @@ final class InferredStatementContext, E extend } private List> reusePrototypeReplicas() { - return Streams.concat( + return reusePrototypeReplicas(Streams.concat( prototype.streamDeclared().filter(StmtContext::isSupportedByFeatures), - prototype.streamEffective()) + prototype.streamEffective())); + } + + private List> reusePrototypeReplicas(final Stream> stream) { + return stream .map(stmt -> { final ReplicaStatementContext ret = ((ReactorStmtCtx) stmt).replicaAsChildOf(this); ret.buildEffective(); @@ -294,13 +326,8 @@ final class InferredStatementContext, E extend return true; } - private static boolean allReused(final List, Mutable>> entries) { - for (Entry, Mutable> entry : entries) { - if (entry.getKey() != entry.getValue()) { - return false; - } - } - return true; + private static boolean allReused(final List entries) { + return entries.stream().allMatch(EffectiveCopy::isReused); } @Override @@ -498,14 +525,14 @@ final class InferredStatementContext, E extend YangStmtMapping.TYPEDEF, YangStmtMapping.USES); - private Map.Entry, Mutable> effectiveCopy(final ReactorStmtCtx stmt) { + private EffectiveCopy effectiveCopy(final ReactorStmtCtx stmt) { // FIXME: YANGTOOLS-652: formerly known as "isReusedByUses" if (REUSED_DEF_SET.contains(stmt.definition().getPublicView())) { - return Map.entry(stmt, stmt.replicaAsChildOf(this)); + return new EffectiveCopy(stmt, stmt); } final ReactorStmtCtx effective = stmt.asEffectiveChildOf(this, childCopyType, targetModule); - return effective == null ? null : Map.entry(stmt, effective); + return effective == null ? null : new EffectiveCopy(stmt, effective); } private void copySubstatement(final Mutable substatement, final Collection> buffer, diff --git a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReactorStmtCtx.java b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReactorStmtCtx.java index 402e0d2c18..a070ecb82a 100644 --- a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReactorStmtCtx.java +++ b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReactorStmtCtx.java @@ -313,6 +313,19 @@ abstract class ReactorStmtCtx, E extends Effec // definition().onNamespaceElementAdded(this, type, key, value); } + /** + * Return the effective statement view of a copy operation. This method may return one of: + *
    + *
  • {@code this}, when the effective view did not change
    • + *
  • an InferredStatementContext, when there is a need for inference-equivalent copy
    • + *
  • {@code null}, when the statement failed to materialize
    • + *
+ * + * @param parent Proposed new parent + * @param type Copy operation type + * @param targetModule New target module + * @return {@link ReactorStmtCtx} holding effective view + */ abstract @Nullable ReactorStmtCtx asEffectiveChildOf(StatementContextBase parent, CopyType type, QNameModule targetModule); diff --git a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReplicaStatementContext.java b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReplicaStatementContext.java index 68f750ce4b..6e4daf1255 100644 --- a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReplicaStatementContext.java +++ b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/ReplicaStatementContext.java @@ -120,7 +120,8 @@ final class ReplicaStatementContext, E extends @Override ReactorStmtCtx asEffectiveChildOf(final StatementContextBase newParent, final CopyType type, final QNameModule targetModule) { - return source.asEffectiveChildOf(newParent, type, targetModule); + final ReactorStmtCtx ret = source.asEffectiveChildOf(newParent, type, targetModule); + return ret == null ? null : this; } @Override diff --git a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java index d305f77879..84823f6531 100644 --- a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java +++ b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java @@ -682,7 +682,7 @@ public abstract class StatementContextBase, E } parent.ensureCompletedPhase(copy); - return canReuseCurrent(copy) ? replicaAsChildOf(parent) : copy; + return canReuseCurrent(copy) ? this : copy; } private boolean canReuseCurrent(final ReactorStmtCtx copy) { diff --git a/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT1212Test.java b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT1212Test.java index ba93e183c8..a27de9aec3 100644 --- a/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT1212Test.java +++ b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT1212Test.java @@ -7,17 +7,22 @@ */ package org.opendaylight.yangtools.yang.stmt; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; import java.net.URI; +import java.util.List; +import java.util.stream.Collectors; import org.junit.Test; import org.opendaylight.yangtools.yang.common.QNameModule; import org.opendaylight.yangtools.yang.model.api.stmt.AnyxmlEffectiveStatement; +import org.opendaylight.yangtools.yang.model.api.stmt.ContainerEffectiveStatement; import org.opendaylight.yangtools.yang.model.api.stmt.GroupingEffectiveStatement; import org.opendaylight.yangtools.yang.model.api.stmt.LeafEffectiveStatement; import org.opendaylight.yangtools.yang.model.api.stmt.ModuleEffectiveStatement; +import org.opendaylight.yangtools.yang.model.api.stmt.NotificationEffectiveStatement; public class YT1212Test { @Test @@ -61,4 +66,29 @@ public class YT1212Test { // The 'type' is not context-independent, but it being copy-insensitive and statements get reused assertSame(foo.effectiveSubstatements(), grpFoo.effectiveSubstatements()); } + + @Test + public void testContainerStatementReuse() throws Exception { + final ModuleEffectiveStatement module = StmtTestUtils.parseYangSource("/bugs/YT1212/container.yang") + .getModuleStatements() + .get(QNameModule.create(URI.create("foo"))); + assertNotNull(module); + + final NotificationEffectiveStatement notif = + module.findFirstEffectiveSubstatement(NotificationEffectiveStatement.class).orElseThrow(); + final List groupings = notif.effectiveSubstatements().stream() + .filter(GroupingEffectiveStatement.class::isInstance).map(GroupingEffectiveStatement.class::cast) + .collect(Collectors.toList()); + assertEquals(2, groupings.size()); + final GroupingEffectiveStatement grp = groupings.get(0); + assertEquals("grp", grp.argument().getLocalName()); + final GroupingEffectiveStatement barGrp = groupings.get(1); + assertEquals("bar", barGrp.argument().getLocalName()); + final ContainerEffectiveStatement bar = notif.findFirstEffectiveSubstatement(ContainerEffectiveStatement.class) + .orElseThrow(); + + // Container needs to be reused + assertSame(bar.findFirstEffectiveSubstatement(ContainerEffectiveStatement.class).orElseThrow(), + barGrp.findFirstEffectiveSubstatement(ContainerEffectiveStatement.class).orElseThrow()); + } } diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT1212/container.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT1212/container.yang new file mode 100644 index 0000000000..e797b445e1 --- /dev/null +++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT1212/container.yang @@ -0,0 +1,22 @@ +module foo { + namespace foo; + prefix foo; + + notification foo { + grouping grp { + container baz { + description "desc"; + reference "ref"; + } + } + + grouping bar { + uses grp; + } + + container bar { + uses bar; + } + } +} + -- 2.36.6