fix edge attribute order confusion

Certain edge attributes are constructed in advance of their being seen in the
input because Graphviz knows it may need default values for them. Later, if seen
in the input, the values of these attributes are updated.

This all works fine unless the order in which these initially-defaulted edge
attributes appear in the input does not match the order in which the default
versions are constructed by Graphviz internally. In this case, the order in
which the attributes are seen in the input is used to construct a dictionary of
them, but the original copies are used to index into attribute values.

In the particular test case added in this commit,

  digraph {
    { rank=same; n1; n2 }

    n2 -> n1 [ headport=s, arrowhead=normal ]
  }

arrowhead was constructed with symbol ID 0 and headport was constructed with
symbol ID 1. But then the later parsing of these attributes resulted in a
dictionary where the headport value was in ID 0 and the arrowhead value was in
ID 1. Indexing into this dictionary with the initially constructed E_arrowhead
resulted in incorrectly returning the value "s". This caused a spurious error
'Arrow type "s" unknown' as well as incorrect graph output.

Fixes #1444. Note that this may just be one of several issues resulting from
using these initially constructed E_* symbols.

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8c2d875e3b1aaaaf753329388fd8f4ee4920317c..8ef996d9edff6941896581f3b10c59c412bf3d53 100644 (file)
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Repeated file read gives different results with libcgraph #1767
 - typo in cmd/gvpr/lib/clustg #1781
 - Segfault in dot #1783
+- Incorrect 'Arrow type "s" unknown' error #1444
 
 ## [2.44.1] - 2020-06-29
 

diff --git a/lib/common/arrows.c b/lib/common/arrows.c
index 6f4f340dafc80925fb5789bfb96e6aa8a387b111..8e59496ec361b2ecdbef96f0849311d7fbe1476a 100644 (file)
--- a/lib/common/arrows.c
+++ b/lib/common/arrows.c
@@ -211,10 +211,21 @@ void arrow_flags(Agedge_t * e, int *sflag, int *eflag)
            }
        }
     }
-    if (E_arrowhead && (*eflag == ARR_TYPE_NORM) && ((attr = agxget(e, E_arrowhead)))[0])
-       arrow_match_name(attr, eflag);
-    if (E_arrowtail && (*sflag == ARR_TYPE_NORM) && ((attr = agxget(e, E_arrowtail)))[0])
-       arrow_match_name(attr, sflag);
+    if (*eflag == ARR_TYPE_NORM) {
+       /* we cannot use the pre-constructed E_arrowhead here because the order in
+        * which edge attributes appear and are thus parsed into a dictionary mean
+        * E_arrowhead->id potentially points at a stale attribute value entry
+        */
+       Agsym_t *arrowhead = agfindedgeattr(agraphof(e), "arrowhead");
+       if (arrowhead != NULL && ((attr = agxget(e, arrowhead)))[0])
+               arrow_match_name(attr, eflag);
+    }
+    if (*sflag == ARR_TYPE_NORM) {
+       /* similar to above, we cannot use E_arrowtail here */
+       Agsym_t *arrowtail = agfindedgeattr(agraphof(e), "arrowtail");
+       if (arrowtail != NULL && ((attr = agxget(e, arrowtail)))[0])
+               arrow_match_name(attr, sflag);
+    }
     if (ED_conc_opp_flag(e)) {
        edge_t *f;
        int s0, e0;

diff --git a/rtest/1444-2.dot b/rtest/1444-2.dot
new file mode 100644 (file)
index 0000000..9e8c4cc
--- /dev/null
+++ b/rtest/1444-2.dot
@@ -0,0 +1,6 @@
+digraph {
+   { rank=same; n1; n2 }
+   
+   n2 -> n1 [ arrowhead=normal, headport=s ]
+}
+

diff --git a/rtest/1444.dot b/rtest/1444.dot
new file mode 100644 (file)
index 0000000..157cdb6
--- /dev/null
+++ b/rtest/1444.dot
@@ -0,0 +1,6 @@
+digraph {
+   { rank=same; n1; n2 }
+   
+   n2 -> n1 [ headport=s, arrowhead=normal ]
+}
+

diff --git a/rtest/test_regression.py b/rtest/test_regression.py
index 75f6757d0d1f5cf47dddd4ede2de581d7d02b434..5de3d80a686dea30c4c02a59ac6eb59c65effdfa 100644 (file)
--- a/rtest/test_regression.py
+++ b/rtest/test_regression.py
@@ -136,6 +136,42 @@ def test_1436():
     # has been reintroduced
     subprocess.check_call(['dot', '-Tsvg', '-o', os.devnull, input])
 
+def test_1444():
+    '''
+    specifying 'headport' as an edge attribute should work regardless of what
+    order attributes appear in
+    https://gitlab.com/graphviz/graphviz/-/issues/1444
+    '''
+
+    # locate the first of our associated tests
+    input1 = os.path.join(os.path.dirname(__file__), '1444.dot')
+    assert os.path.exists(input1), 'unexpectedly missing test case'
+
+    # ask Graphviz to process it
+    p = subprocess.Popen(['dot', '-Tsvg', input1], stdout=subprocess.PIPE,
+      stderr=subprocess.PIPE, universal_newlines=True)
+    stdout1, stderr = p.communicate()
+
+    assert p.returncode == 0, 'failed to process a headport edge'
+
+    assert stderr.strip() == '', 'emitted an error for a legal graph'
+
+    # now locate our second variant, that simply has the attributes swapped
+    input2 = os.path.join(os.path.dirname(__file__), '1444-2.dot')
+    assert os.path.exists(input2), 'unexpectedly missing test case'
+
+    # process it identically
+    p = subprocess.Popen(['dot', '-Tsvg', input2], stdout=subprocess.PIPE,
+      stderr=subprocess.PIPE, universal_newlines=True)
+    stdout2, stderr = p.communicate()
+
+    assert p.returncode == 0, 'failed to process a headport edge'
+
+    assert stderr.strip() == '', 'emitted an error for a legal graph'
+
+    assert stdout1 == stdout2, \
+      'swapping edge attributes altered the output graph'
+
 def test_1449():
     '''
     using the SVG color scheme should not cause warnings