From: Robert Varga Date: Fri, 28 Jan 2022 23:06:59 +0000 (+0100) Subject: Enforce explicit generator linkage X-Git-Tag: v9.0.0~57 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;ds=sidebyside;h=3131e455f634c58b81443b6ddfa495e448c2a7bf;p=mdsal.git Enforce explicit generator linkage We are keeping the original pointer as a simple nullable field. This can result in us confusing an unresolved generator with an original, leading to potential badness. Refactor the AbstractExplicitGenerator to track incremental resolution, catching invalid accesses. JIRA: MDSAL-718 Change-Id: Ie67fa4d08d0887f301948e3d03d846ed9ee1d628 Signed-off-by: Robert Varga (cherry picked from commit 40dd3e7e4793c0f04d8a4635f9d2d368e8e5214c) --- diff --git a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractCompositeGenerator.java b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractCompositeGenerator.java index 13776a5284..1828410a19 100644 --- a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractCompositeGenerator.java +++ b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractCompositeGenerator.java @@ -17,6 +17,7 @@ import java.util.List; import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; +import org.opendaylight.mdsal.binding.generator.impl.reactor.OriginalLink.Partial; import org.opendaylight.mdsal.binding.model.api.Enumeration; import org.opendaylight.mdsal.binding.model.api.GeneratedTransferObject; import org.opendaylight.mdsal.binding.model.api.GeneratedType; @@ -65,6 +66,10 @@ abstract class AbstractCompositeGenerator> ex private List augments = List.of(); private List groupings; + // Performance optimization: if this is true, we have ascertained our original definition as well that of all our + // children + private boolean originalsResolved; + AbstractCompositeGenerator(final T statement) { super(statement); children = createChildren(statement); @@ -159,16 +164,36 @@ abstract class AbstractCompositeGenerator> ex augments.add(requireNonNull(augment)); } + @Override + long linkOriginalGenerator() { + if (originalsResolved) { + return 0; + } + + long remaining = super.linkOriginalGenerator(); + if (remaining == 0) { + for (Generator child : children) { + if (child instanceof AbstractExplicitGenerator) { + remaining += ((AbstractExplicitGenerator) child).linkOriginalGenerator(); + } + } + if (remaining == 0) { + originalsResolved = true; + } + } + return remaining; + } + @Override final AbstractCompositeGenerator getOriginal() { return (AbstractCompositeGenerator) super.getOriginal(); } - final @NonNull AbstractExplicitGenerator getOriginalChild(final QName childQName) { + final @NonNull OriginalLink getOriginalChild(final QName childQName) { // First try groupings/augments ... final AbstractExplicitGenerator found = findInferredGenerator(childQName); if (found != null) { - return found; + return new Partial(found); } // ... no luck, we really need to start looking at our origin @@ -177,7 +202,7 @@ abstract class AbstractCompositeGenerator> ex final QName prevQName = childQName.bindTo(prev.getQName().getModule()); return verifyNotNull(prev.findSchemaTreeGenerator(prevQName), - "Failed to find child %s (proxy for %s) in %s", prevQName, childQName, prev).getOriginal(); + "Failed to find child %s (proxy for %s) in %s", prevQName, childQName, prev).originalLink(); } @Override diff --git a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractExplicitGenerator.java b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractExplicitGenerator.java index 3e282c827c..b3e542f6ee 100644 --- a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractExplicitGenerator.java +++ b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractExplicitGenerator.java @@ -8,12 +8,15 @@ package org.opendaylight.mdsal.binding.generator.impl.reactor; import static com.google.common.base.Verify.verify; +import static com.google.common.base.Verify.verifyNotNull; import static java.util.Objects.requireNonNull; import com.google.common.base.MoreObjects.ToStringHelper; import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; import org.opendaylight.mdsal.binding.generator.impl.reactor.CollisionDomain.Member; +import org.opendaylight.mdsal.binding.generator.impl.reactor.OriginalLink.Complete; +import org.opendaylight.mdsal.binding.generator.impl.reactor.OriginalLink.Partial; import org.opendaylight.mdsal.binding.model.api.MethodSignature.ValueMechanics; import org.opendaylight.mdsal.binding.model.api.Type; import org.opendaylight.mdsal.binding.model.api.TypeMemberComment; @@ -40,12 +43,17 @@ public abstract class AbstractExplicitGenerator prev; + /** + * Field tracking previous incarnation (along reverse of 'uses' and 'augment' axis) of this statement. This field + * can either be one of: + *
    + *
  • {@code null} when not resolved, i.e. access is not legal, or
    • + *
  • {@code this} object if this is the original definition, or
    • + *
  • an {@link AbstractExplicitGenerator} pointing to the original definition, or
    • + *
  • a {@link Partial} link pointing to a generator closer to the original definition
    • + *
+ */ + private Object prev; AbstractExplicitGenerator(final T statement) { this.statement = requireNonNull(statement); @@ -75,20 +83,90 @@ public abstract class AbstractExplicitGenerator previous() { - return prev; + final var local = prev(); + if (local == this) { + return null; + } else if (local instanceof Partial) { + return ((Partial) local).previous(); + } else { + return verifyExplicit(local); + } } + /** + * Return the original incarnation of this generator, or self if this is the original generator. + * + * @return Original incarnation of this generator + */ @NonNull AbstractExplicitGenerator getOriginal() { - return prev == null ? this : prev.getOriginal(); + final var local = prev(); + return local == this ? this : verifyExplicit(local).getOriginal(); + } + + /** + * Return the link towards the original generator. + * + * @return Link towards the original generator. + */ + final @NonNull OriginalLink originalLink() { + final var local = prev; + if (local == null) { + return new Partial(this); + } else if (local == this) { + return new Complete(this); + } else if (local instanceof Partial) { + return (Partial) local; + } else { + return verifyExplicit(local).originalLink(); + } + } + + private static @NonNull AbstractExplicitGenerator verifyExplicit(final Object prev) { + verify(prev instanceof AbstractExplicitGenerator, "Unexpected previous %s", prev); + return (AbstractExplicitGenerator) prev; + } + + private @NonNull Object prev() { + return verifyNotNull(prev, "Generator %s does not have linkage to previous instance resolved", this); } @Nullable AbstractExplicitGenerator findSchemaTreeGenerator(final QName qname) { diff --git a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractTypeObjectGenerator.java b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractTypeObjectGenerator.java index 9d8526aad0..002ab608e0 100644 --- a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractTypeObjectGenerator.java +++ b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/AbstractTypeObjectGenerator.java @@ -281,6 +281,11 @@ abstract class AbstractTypeObjectGenerator> e private final TypeEffectiveStatement type; + // FIXME: these fields should be better-controlled with explicit sequencing guards. It it currently stands, we are + // expending two (or more) additional fields to express downstream linking. If we had the concept of + // resolution step (an enum), we could just get by with a simple queue of Step/Callback pairs, which would + // trigger as needed. See how we manage baseGen/inferred fields. + /** * The generator corresponding to our YANG base type. It produces the superclass of our encapsulated type. If it is * {@code null}, this generator is the root of the hierarchy. diff --git a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/GeneratorReactor.java b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/GeneratorReactor.java index db6080c312..8eec089c17 100644 --- a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/GeneratorReactor.java +++ b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/GeneratorReactor.java @@ -137,8 +137,18 @@ public final class GeneratorReactor extends GeneratorContext implements Mutable } // Step 1c: ... finally establish linkage along the reverse uses/augment axis. This is needed to route generated - // type manifestations (isAddedByUses/isAugmenting) to their type generation sites. - linkOriginalGenerator(children); + // type manifestations (isAddedByUses/isAugmenting) to their type generation sites. Since generator + // tree iteration order does not match dependencies, we may need to perform multiple passes. + long unlinkedOriginals = Long.MAX_VALUE; + do { + long remaining = 0; + for (ModuleGenerator module : children) { + remaining += module.linkOriginalGenerator(); + } + verify(remaining < unlinkedOriginals, "Failed to make progress on linking of remaining %s originals", + remaining); + unlinkedOriginals = remaining; + } while (unlinkedOriginals != 0); /* * Step 2: link typedef statements, so that typedef's 'type' axis is fully established @@ -333,17 +343,6 @@ public final class GeneratorReactor extends GeneratorContext implements Mutable } } - private static void linkOriginalGenerator(final Iterable parent) { - for (Generator child : parent) { - if (child instanceof AbstractExplicitGenerator) { - ((AbstractExplicitGenerator) child).linkOriginalGenerator(); - } - if (child instanceof AbstractCompositeGenerator) { - linkOriginalGenerator(child); - } - } - } - private void linkDependencies(final Iterable parent) { for (Generator child : parent) { if (child instanceof AbstractDependentGenerator) { diff --git a/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/OriginalLink.java b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/OriginalLink.java new file mode 100644 index 0000000000..e51fe4b936 --- /dev/null +++ b/binding/mdsal-binding-generator/src/main/java/org/opendaylight/mdsal/binding/generator/impl/reactor/OriginalLink.java @@ -0,0 +1,89 @@ +/* + * Copyright (c) 2022 PANTHEON.tech, s.r.o. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.mdsal.binding.generator.impl.reactor; + +import static java.util.Objects.requireNonNull; + +import com.google.common.base.MoreObjects; +import com.google.common.base.MoreObjects.ToStringHelper; +import org.eclipse.jdt.annotation.NonNull; +import org.eclipse.jdt.annotation.Nullable; + +/** + * Link to the original definition of an {@link AbstractExplicitGenerator}. + */ +// FIXME: sealed when we have JDK17+ +abstract class OriginalLink { + static final class Complete extends OriginalLink { + private final @NonNull AbstractExplicitGenerator original; + + Complete(final AbstractExplicitGenerator original) { + this.original = requireNonNull(original); + } + + @Override + AbstractExplicitGenerator previous() { + return original; + } + + @Override + @NonNull AbstractExplicitGenerator original() { + return original; + } + + @Override + ToStringHelper addToStringAttributes(final ToStringHelper helper) { + return helper.add("original", original); + } + } + + static final class Partial extends OriginalLink { + private final @NonNull AbstractExplicitGenerator previous; + private AbstractExplicitGenerator original; + + Partial(final AbstractExplicitGenerator previous) { + this.previous = requireNonNull(previous); + } + + @Override + AbstractExplicitGenerator previous() { + return previous; + } + + @Override + AbstractExplicitGenerator original() { + if (original == null) { + final var link = previous.originalLink(); + if (link instanceof Complete || link.previous() != previous) { + original = link.original(); + } + } + return original; + } + + @Override + ToStringHelper addToStringAttributes(final ToStringHelper helper) { + return helper.add("previous", previous).add("original", original); + } + } + + private OriginalLink() { + // Hidden on purpose + } + + abstract @NonNull AbstractExplicitGenerator previous(); + + abstract @Nullable AbstractExplicitGenerator original(); + + abstract ToStringHelper addToStringAttributes(ToStringHelper helper); + + @Override + public final String toString() { + return addToStringAttributes(MoreObjects.toStringHelper(this).omitNullValues()).toString(); + } +}