From 4e727ede4f5181c4650c4aa60427bef8b43d113d Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Tue, 4 Aug 2020 17:47:15 -0700 Subject: [PATCH] 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. --- CHANGELOG.md | 1 + lib/common/arrows.c | 19 +++++++++++++++---- rtest/1444-2.dot | 6 ++++++ rtest/1444.dot | 6 ++++++ rtest/test_regression.py | 36 ++++++++++++++++++++++++++++++++++++ 5 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 rtest/1444-2.dot create mode 100644 rtest/1444.dot diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c2d875e3..8ef996d9e 100644 --- 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 6f4f340da..8e59496ec 100644 --- 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 index 000000000..9e8c4cc5d --- /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 index 000000000..157cdb651 --- /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 75f6757d0..5de3d80a6 100644 --- 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 -- 2.40.0