YANGTOOLS-838: detect grouping identifier shadowing

RFC7950 sections 5.5 and 6.2.1 explicitly forbid identifier
shadowing. Update grouping support to detect and report this
violation.

This patch also fixes typedef support to correctly detect
forward shadowing, when a nested conflicting typedef shadows
a sibling which occurs later in the model text.

Change-Id: I7c67c93c5f13b24657454e13d8b329338f98d7b4
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/yang/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/SchemaUtilsTest.java b/yang/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/SchemaUtilsTest.java
index 2067575a73bd490ccf33540373cf9c5fa60c5a6d..065cea61a16faf8dd611508a5411a7accb1d9deb 100644 (file)
--- a/yang/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/SchemaUtilsTest.java
+++ b/yang/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/SchemaUtilsTest.java
@@ -29,11 +29,11 @@ public class SchemaUtilsTest {
     public void test() throws Exception {
         final SchemaContext schemaContext = YangParserTestUtils.parseYangResource("/schema-utils-test/foo.yang");
         assertTrue(SchemaUtils.findDataParentSchemaOnPath(schemaContext,
-                SchemaPath.create(true, qN("my-name"), qN("my-name"))) instanceof ContainerSchemaNode);
+                SchemaPath.create(true, qN("my-name"), qN("my-name-a"))) instanceof ContainerSchemaNode);
         assertTrue(SchemaUtils.findDataParentSchemaOnPath(schemaContext,
-                SchemaPath.create(true, qN("my-name-2"), qN("my-name"))) instanceof NotificationDefinition);
+                SchemaPath.create(true, qN("my-name-2"), qN("my-name-b"))) instanceof NotificationDefinition);
         assertTrue(SchemaUtils.findDataParentSchemaOnPath(schemaContext,
-                SchemaPath.create(true, qN("my-name-2"), qN("my-name-2"))) instanceof ActionDefinition);
+                SchemaPath.create(true, qN("my-name-2"), qN("my-name-2-b"))) instanceof ActionDefinition);
     }
 
     @Test
@@ -42,11 +42,11 @@ public class SchemaUtilsTest {
                 .parseYangResource("/schema-utils-test/name-conflicts.yang");
         // test my-name conflicts
         assertEquals(8, SchemaUtils.findParentSchemaNodesOnPath(schemaContext,
-                SchemaPath.create(true, qN("my-name"), qN("my-name"), qN("my-name"))).size());
+                SchemaPath.create(true, qN("my-name"), qN("my-name-nested"), qN("my-name-nested2"))).size());
 
         // test target container
         final Collection<SchemaNode> target = SchemaUtils.findParentSchemaNodesOnPath(schemaContext,
-                SchemaPath.create(true, qN("my-name-2"), qN("my-name-2"), qN("target")));
+                SchemaPath.create(true, qN("my-name-2"), qN("my-name-nested"), qN("target")));
         assertEquals(1, target.size());
         assertTrue(target.iterator().next() instanceof ContainerSchemaNode);
 

diff --git a/yang/yang-data-impl/src/test/resources/schema-utils-test/foo.yang b/yang/yang-data-impl/src/test/resources/schema-utils-test/foo.yang
index 4311cd2ffdcf922e80a81c06410ce503e5799417..abd079b1a4640f2dab8c66281e1250fa9cb1fd77 100644 (file)
--- a/yang/yang-data-impl/src/test/resources/schema-utils-test/foo.yang
+++ b/yang/yang-data-impl/src/test/resources/schema-utils-test/foo.yang
@@ -20,20 +20,20 @@ module foo {
     }
 
     notification my-name {
-        grouping my-name {
+        grouping my-name-a {
         }
-        container my-name {
+        container my-name-a {
         }
     }
 
     container my-name-2 {
-        grouping my-name {
+        grouping my-name-b {
         }
-        notification my-name {
+        notification my-name-b {
         }
-        grouping my-name-2 {
+        grouping my-name-2-b {
         }
-        action my-name-2 {
+        action my-name-2-b {
         }
     }
 }

diff --git a/yang/yang-data-impl/src/test/resources/schema-utils-test/name-conflicts.yang b/yang/yang-data-impl/src/test/resources/schema-utils-test/name-conflicts.yang
index e0209c768f1e813f61963631ec6aec4ef0c97310..bc51627301113cf0c94d7bfa4e189268c2e2a793 100644 (file)
--- a/yang/yang-data-impl/src/test/resources/schema-utils-test/name-conflicts.yang
+++ b/yang/yang-data-impl/src/test/resources/schema-utils-test/name-conflicts.yang
@@ -3,61 +3,61 @@ module name-conflicts {
     prefix nc;
 
     grouping my-name {
-        grouping my-name {
-            grouping my-name {
+        grouping my-name-nested {
+            grouping my-name-nested2 {
             }
-            container my-name {
+            container my-name-nested2 {
             }
         }
-        container my-name {
-            grouping my-name {
+        container my-name-nested {
+            grouping my-name-nested2 {
             }
-            container my-name {
+            container my-name-nested2 {
             }
         }
     }
 
     container my-name {
-        grouping my-name {
-            grouping my-name {
+        grouping my-name-nested {
+            grouping my-name-nested2 {
             }
-            container my-name {
+            container my-name-nested2 {
             }
         }
-        container my-name {
-            grouping my-name {
+        container my-name-nested {
+            grouping my-name-nested2 {
             }
-            container my-name {
+            container my-name-nested2 {
             }
         }
     }
 
     grouping my-name-2 {
-        grouping my-name-2 {
-            grouping my-name-2 {
+        grouping my-name-nested {
+            grouping my-name-nested2 {
             }
-            container my-name-2 {
+            container my-name-nested2 {
             }
         }
         container my-name-2 {
-            grouping my-name-2 {
+            grouping my-name-nested2 {
             }
-            container my-name-2 {
+            container my-name-nested2 {
             }
         }
     }
 
     container my-name-2 {
-        grouping my-name-2 {
-            grouping my-name-2 {
+        grouping my-name-nested {
+            grouping my-name-nested2 {
             }
             container target {
             }
         }
-        container my-name-2 {
-            grouping my-name-2 {
+        container my-name-nested {
+            grouping my-name-nested2 {
             }
-            container my-name-2 {
+            container my-name-nested2 {
             }
         }
     }

diff --git a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/grouping/AbstractGroupingStatementSupport.java b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/grouping/AbstractGroupingStatementSupport.java
index b6e838e92a5daf994c793a04eeb133794203a337..57e0829ab823b404add07a75c902beb592c65366 100644 (file)
--- a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/grouping/AbstractGroupingStatementSupport.java
+++ b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/grouping/AbstractGroupingStatementSupport.java
@@ -16,6 +16,7 @@ import org.opendaylight.yangtools.yang.parser.spi.meta.AbstractQNameStatementSup
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext.Mutable;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContextUtils;
+import org.opendaylight.yangtools.yang.parser.spi.source.SourceException;
 
 abstract class AbstractGroupingStatementSupport
         extends AbstractQNameStatementSupport<GroupingStatement, EffectiveStatement<QName, GroupingStatement>> {
@@ -31,6 +32,16 @@ abstract class AbstractGroupingStatementSupport
 
     @Override
     public final GroupingStatement createDeclared(final StmtContext<QName, GroupingStatement, ?> ctx) {
+        // Shadowing check: make sure grandparent does not see a conflicting definition. This is required to ensure
+        // that a grouping in child scope does not shadow a grouping in parent scope which occurs later in the text.
+        final StmtContext<?, ?, ?> parent = ctx.getParentContext();
+        if (parent != null) {
+            final StmtContext<?, ?, ?> grandParent = parent.getParentContext();
+            if (grandParent != null) {
+                checkConflict(grandParent, ctx);
+            }
+        }
+
         return new GroupingStatementImpl(ctx);
     }
 
@@ -45,8 +56,21 @@ abstract class AbstractGroupingStatementSupport
             EffectiveStatement<QName, GroupingStatement>> stmt) {
         super.onFullDefinitionDeclared(stmt);
 
-        if (stmt != null && stmt.getParentContext() != null) {
-            stmt.getParentContext().addContext(GroupingNamespace.class, stmt.getStatementArgument(), stmt);
+        if (stmt != null) {
+            final Mutable<?, ?, ?> parent = stmt.getParentContext();
+            if (parent != null) {
+                // Shadowing check: make sure we do not trample on pre-existing definitions. This catches sibling
+                // declarations and parent declarations which have already been declared.
+                checkConflict(parent, stmt);
+                parent.addContext(GroupingNamespace.class, stmt.getStatementArgument(), stmt);
+            }
         }
     }
+
+    private static void checkConflict(final StmtContext<?, ?, ?> parent, final StmtContext<QName, ?, ?> stmt) {
+        final QName arg = stmt.getStatementArgument();
+        final StmtContext<?, ?, ?> existing = parent.getFromNamespace(GroupingNamespace.class, arg);
+        SourceException.throwIf(existing != null, stmt.getStatementSourceReference(), "Duplicate name for grouping %s",
+                arg);
+    }
 }
\ No newline at end of file

diff --git a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/typedef/TypedefStatementSupport.java b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/typedef/TypedefStatementSupport.java
index f8b0882daaa95c0e90fc3b46d2cc62ba7729d62b..2a5c73eba6f55eaec22b15c3b9411a58fa77683d 100644 (file)
--- a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/typedef/TypedefStatementSupport.java
+++ b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/typedef/TypedefStatementSupport.java
@@ -10,11 +10,11 @@ package org.opendaylight.yangtools.yang.parser.rfc7950.stmt.typedef;
 import org.opendaylight.yangtools.yang.common.QName;
 import org.opendaylight.yangtools.yang.model.api.YangStmtMapping;
 import org.opendaylight.yangtools.yang.model.api.meta.EffectiveStatement;
-import org.opendaylight.yangtools.yang.model.api.stmt.TypedefEffectiveStatement;
 import org.opendaylight.yangtools.yang.model.api.stmt.TypedefStatement;
 import org.opendaylight.yangtools.yang.parser.spi.TypeNamespace;
 import org.opendaylight.yangtools.yang.parser.spi.meta.AbstractQNameStatementSupport;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext;
+import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext.Mutable;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContextUtils;
 import org.opendaylight.yangtools.yang.parser.spi.meta.SubstatementValidator;
 import org.opendaylight.yangtools.yang.parser.spi.source.SourceException;
@@ -47,6 +47,16 @@ public final class TypedefStatementSupport extends
 
     @Override
     public TypedefStatement createDeclared(final StmtContext<QName, TypedefStatement, ?> ctx) {
+        // Shadowing check: make sure grandparent does not see a conflicting definition. This is required to ensure
+        // that a typedef in child scope does not shadow a typedef in parent scope which occurs later in the text.
+        final StmtContext<?, ?, ?> parent = ctx.getParentContext();
+        if (parent != null) {
+            final StmtContext<?, ?, ?> grandParent = parent.getParentContext();
+            if (grandParent != null) {
+                checkConflict(grandParent, ctx);
+            }
+        }
+
         return new TypedefStatementImpl(ctx);
     }
 
@@ -57,17 +67,18 @@ public final class TypedefStatementSupport extends
     }
 
     @Override
-    public void onFullDefinitionDeclared(final StmtContext.Mutable<QName, TypedefStatement,
+    public void onFullDefinitionDeclared(final Mutable<QName, TypedefStatement,
             EffectiveStatement<QName, TypedefStatement>> stmt) {
         super.onFullDefinitionDeclared(stmt);
 
-        if (stmt != null && stmt.getParentContext() != null) {
-            final StmtContext<?, TypedefStatement, TypedefEffectiveStatement> existing = stmt.getParentContext()
-                    .getFromNamespace(TypeNamespace.class, stmt.getStatementArgument());
-            SourceException.throwIf(existing != null, stmt.getStatementSourceReference(),
-                    "Duplicate name for typedef %s", stmt.getStatementArgument());
-
-            stmt.getParentContext().addContext(TypeNamespace.class, stmt.getStatementArgument(), stmt);
+        if (stmt != null) {
+            final Mutable<?, ?, ?> parent = stmt.getParentContext();
+            if (parent != null) {
+                // Shadowing check: make sure we do not trample on pre-existing definitions. This catches sibling
+                // declarations and parent declarations which have already been declared.
+                checkConflict(parent, stmt);
+                parent.addContext(TypeNamespace.class, stmt.getStatementArgument(), stmt);
+            }
         }
     }
 
@@ -75,4 +86,12 @@ public final class TypedefStatementSupport extends
     protected SubstatementValidator getSubstatementValidator() {
         return SUBSTATEMENT_VALIDATOR;
     }
+
+    private static void checkConflict(final StmtContext<?, ?, ?> parent, final StmtContext<QName, ?, ?> stmt) {
+        final QName arg = stmt.getStatementArgument();
+        final StmtContext<?, ?, ?> existing = parent.getFromNamespace(TypeNamespace.class, arg);
+        // RFC7950 sections 5.5 and 6.2.1: identifiers must not be shadowed
+        SourceException.throwIf(existing != null, stmt.getStatementSourceReference(), "Duplicate name for typedef %s",
+                arg);
+    }
 }
\ No newline at end of file

diff --git a/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT838Test.java b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT838Test.java
new file mode 100644 (file)
index 0000000..386bde2
--- /dev/null
+++ b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT838Test.java
@@ -0,0 +1,61 @@
+/*
+ * Copyright (c) 2017 Pantheon Technologies, 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.yangtools.yang.stmt;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import org.junit.Test;
+import org.opendaylight.yangtools.yang.parser.spi.meta.ReactorException;
+import org.opendaylight.yangtools.yang.parser.spi.source.SourceException;
+
+public class YT838Test {
+    @Test
+    public void testGroupingShadowing() throws Exception {
+        testGrouping("grouping.yang");
+    }
+
+    @Test
+    public void testGroupingPostShadowing() throws Exception {
+        testGrouping("grouping-post.yang");
+    }
+
+    @Test
+    public void testTypedefShadowing() throws Exception {
+        testTypedef("typedef.yang");
+    }
+
+    @Test
+    public void testTypedefPostShadowing() throws Exception {
+        testTypedef("typedef-post.yang");
+    }
+
+    private static void testGrouping(final String model) throws Exception {
+        try {
+            StmtTestUtils.parseYangSource("/bugs/YT838/" + model);
+            fail("Expected failure due to grouping identifier shadowing");
+        } catch (ReactorException e) {
+            final Throwable cause = e.getCause();
+            assertTrue(cause instanceof SourceException);
+            assertTrue(cause.getMessage().startsWith(
+                "Duplicate name for grouping (grouping?revision=2017-12-20)foo [at "));
+        }
+    }
+
+    private static void testTypedef(final String model) throws Exception {
+        try {
+            StmtTestUtils.parseYangSource("/bugs/YT838/" + model);
+            fail("Expected failure due to type identifier shadowing");
+        } catch (ReactorException e) {
+            final Throwable cause = e.getCause();
+            assertTrue(cause instanceof SourceException);
+            assertTrue(cause.getMessage().startsWith(
+                "Duplicate name for typedef (typedef?revision=2017-12-20)foo [at "));
+        }
+    }
+}

diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/grouping-post.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/grouping-post.yang
new file mode 100644 (file)
index 0000000..316cee7
--- /dev/null
+++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/grouping-post.yang
@@ -0,0 +1,18 @@
+module grouping {
+    namespace grouping;
+    prefix grp;
+
+    revision "2017-12-20";
+
+    container top {
+        container nested {
+            grouping foo {
+
+            }
+        }
+
+        grouping foo {
+
+        }
+    }
+}

diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/grouping.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/grouping.yang
new file mode 100644 (file)
index 0000000..9f5526a
--- /dev/null
+++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/grouping.yang
@@ -0,0 +1,18 @@
+module grouping {
+    namespace grouping;
+    prefix grp;
+
+    revision "2017-12-20";
+
+    container top {
+        grouping foo {
+
+        }
+
+        container nested {
+            grouping foo {
+
+            }
+        }
+    }
+}

diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/typedef-post.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/typedef-post.yang
new file mode 100644 (file)
index 0000000..f82295b
--- /dev/null
+++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/typedef-post.yang
@@ -0,0 +1,18 @@
+module typedef {
+    namespace typedef;
+    prefix tdef;
+
+    revision "2017-12-20";
+
+    container top {
+        container nested {
+            typedef foo {
+                type string;
+            }
+        }
+
+        typedef foo {
+            type string;
+        }
+    }
+}

diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/typedef.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/typedef.yang
new file mode 100644 (file)
index 0000000..70fb9f7
--- /dev/null
+++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT838/typedef.yang
@@ -0,0 +1,18 @@
+module typedef {
+    namespace typedef;
+    prefix tdef;
+
+    revision "2017-12-20";
+
+    container top {
+        typedef foo {
+            type string;
+        }
+
+        container nested {
+            typedef foo {
+                type string;
+            }
+        }
+    }
+}