From 16dd69fcfe894d56fb5869243e79d3dc271871d0 Mon Sep 17 00:00:00 2001 From: Francois Daoust Date: Thu, 16 Jan 2025 21:51:53 +0100 Subject: [PATCH] Warn/Report an error about duplicated CDDL keys/values Code now detects when a CDDL block contains anonymous types that would create duplicates. It dies when a key or a value is defined twice for the same CDDL type, preventing any reference to the term. It warns when the duplicate is for definitions that are not of the same type. For example, an error is returned for: ``` err = [ "dupl", "dupl" ] ``` ... while the following only returns an error because it remains possible to reference individual terms by making the link type explicit: ``` warn = [ ( dupl: "val" ), "dupl" ] ``` --- bikeshed/cddl.py | 145 ++-- bikeshed/refs/utils.py | 7 - tests/cddl001.html | 150 +++- tests/cddl002.html | 24 +- tests/cddl003.bs | 4 + tests/cddl003.console.txt | 7 +- tests/cddl003.html | 97 ++- tests/cddl004.html | 6 +- tests/cddl005.bs | 24 + tests/cddl005.console.txt | 2 + tests/cddl005.html | 1409 +++++++++++++++++++++++++++++++++++++ 11 files changed, 1747 insertions(+), 128 deletions(-) create mode 100644 tests/cddl005.bs create mode 100644 tests/cddl005.console.txt create mode 100644 tests/cddl005.html diff --git a/bikeshed/cddl.py b/bikeshed/cddl.py index eb45cd016a..9286387e67 100644 --- a/bikeshed/cddl.py +++ b/bikeshed/cddl.py @@ -33,10 +33,11 @@ def serializeValue(self, prefix: str, value: str, suffix: str, node: cddlparser. # Cannot easily link member key back to a definition return name else: - # Name of created type should not include the quotes + # Create a key with and without quotes as linking text + lts = [value, name] return '{}'.format( h.escapeAttr(forName), - h.escapeAttr(value), + h.escapeAttr("|".join(lts)), name, ) elif isinstance(parent, cddlparser.ast.Operator) and parent.controller == node: @@ -49,9 +50,10 @@ def serializeValue(self, prefix: str, value: str, suffix: str, node: cddlparser. if forName is None: return name else: + lts = [value, name] return '{}'.format( h.escapeAttr(forName), - h.escapeAttr(name), + h.escapeAttr("|".join(lts)), name, ) @@ -88,9 +90,14 @@ def serializeName(self, name: str, node: cddlparser.ast.CDDLNode) -> str: # Cannot easily link member key back to a definition return name else: + lts = [] + if name[0] == '"': + lts = [name[1:-1], name] + else: + lts = [name, '"' + name + '"'] return '{}'.format( h.escapeAttr(forName), - h.escapeAttr(name), + h.escapeAttr("|".join(lts)), name, ) elif isinstance(parent, cddlparser.ast.GenericParameters): @@ -162,66 +169,96 @@ def markupCDDLBlock(pre: t.ElementT, doc: t.SpecT) -> set[t.ElementT]: Convert blocks into "dfn" or links. """ localDfns = set() - forcedDfns = [] - for x in (h.treeAttr(pre, "data-dfn-force") or "").split(): - x = x.strip() - if x.endswith(""): - x = x[:-11] - forcedDfns.append(x) + + # A CDDL definition may create duplicates, e.g. argh = [ "dupl", "dupl" ] + # To detect these duplicates, let's maintain a list of actual definitions + # contained in CDDL blocks. + cddlDfns = set() + + def recordDfns(el: t.ElementT) -> bool: + cddlType = "cddl-" + (el.get("data-cddl-type") or "") + for cddlText in (el.get("data-lt") or "").split("|"): + linkFors = t.cast("list[str|None]", config.splitForValues(el.get("data-cddl-for", ""))) or [None] + for linkFor in linkFors: + dfnText = cddlType + ">" + (linkFor or "") + "/" + cddlText + if dfnText in cddlDfns: + forText = "" if linkFor is None else f' defined in type "{linkFor}"' + m.die( + f"CDDL {cddlType[5:]} {cddlText}{forText} creates a duplicate and cannot be referenced.\nPlease create additional CDDL types to disambiguate.", + ) + return False + cddlDfns.add(dfnText) + if cddlType != "cddl-parameter": + warned = False + for cddlText in (el.get("data-lt") or "").split("|"): + linkFors = t.cast("list[str|None]", config.splitForValues(el.get("data-cddl-for", ""))) or [None] + for linkFor in linkFors: + dfnText = (linkFor or "") + "/" + cddlText + if dfnText in cddlDfns and not warned: + warned = True + forText = "" if linkFor is None else f' defined in type "{linkFor}"' + m.warn( + f"CDDL {cddlType[5:]} {cddlText}{forText} creates a duplicate with another CDDL definition.\nLink type needs to be specified to reference the term.\nConsider creating additional CDDL types to disambiguate.", + ) + cddlDfns.add(dfnText) + return True + for el in h.findAll("cddl", pre): # Prefix CDDL types with "cddl-" to avoid confusion with other # types (notably CSS ones such as "value") cddlType = "cddl-" + (el.get("data-cddl-type") or "") assert isinstance(cddlType, str) url = None - forceDfn = False ref = None cddlText = None - for cddlText in (el.get("data-lt") or "").split("|"): - if cddlType == "interface" and cddlText in forcedDfns: - forceDfn = True - linkFors = t.cast("list[str|None]", config.splitForValues(el.get("data-cddl-for", ""))) or [None] - for linkFor in linkFors: - ref = doc.refs.getRef( - cddlType, - cddlText, - linkFor=linkFor, - status="local", - el=el, - error=False, - ) + # Record the dfns that the term would create and check whether + # it creates a duplicate. If it does, let's not link the term. + if not recordDfns(el): + el.tag = "span" + else: + for cddlText in (el.get("data-lt") or "").split("|"): + linkFors = t.cast("list[str|None]", config.splitForValues(el.get("data-cddl-for", ""))) or [None] + for linkFor in linkFors: + ref = doc.refs.getRef( + cddlType, + cddlText, + linkFor=linkFor, + status="local", + el=el, + error=True, + ) + if ref: + url = ref.url + break if ref: - url = ref.url break - if ref: - break - if url is None or forceDfn: - el.tag = "dfn" - el.set("data-dfn-type", cddlType) - del el.attrib["data-cddl-type"] - if el.get("data-cddl-for"): - el.set("data-dfn-for", el.get("data-cddl-for") or "") - del el.attrib["data-cddl-for"] - else: - # Copy over the auto-generated linking text to the manual dfn. - dfn = h.find(url, doc) - # How in the hell does this work, the url is not a selector??? - assert dfn is not None - lts = combineCDDLLinkingTexts(el.get("data-lt"), dfn.get("data-lt")) - dfn.set("data-lt", lts) - localDfns.add(dfn) - - # Reset the element to be a link to the manual dfn. - el.tag = "a" - el.set("data-link-type", cddlType) - el.set("data-lt", cddlText) - del el.attrib["data-cddl-type"] - if el.get("data-cddl-for"): - el.set("data-link-for", el.get("data-cddl-for") or "") - del el.attrib["data-cddl-for"] - if el.get("id"): - # ID was defensively added by the Marker. - del el.attrib["id"] + if url is None: + el.tag = "dfn" + el.set("data-dfn-type", cddlType) + del el.attrib["data-cddl-type"] + if el.get("data-cddl-for"): + el.set("data-dfn-for", el.get("data-cddl-for") or "") + del el.attrib["data-cddl-for"] + else: + # Copy over the auto-generated linking text to the manual dfn. + dfn = h.find(url, doc) + # How in the hell does this work, the url is not a selector??? + assert dfn is not None + lts = combineCDDLLinkingTexts(el.get("data-lt"), dfn.get("data-lt")) + dfn.set("data-lt", lts) + localDfns.add(dfn) + + # Reset the element to be a link to the manual dfn. + el.tag = "a" + el.set("data-link-type", cddlType) + el.set("data-lt", cddlText) + del el.attrib["data-cddl-type"] + if el.get("data-cddl-for"): + el.set("data-link-for", el.get("data-cddl-for") or "") + del el.attrib["data-cddl-for"] + if el.get("id"): + # ID was defensively added by the Marker. + del el.attrib["id"] return localDfns diff --git a/bikeshed/refs/utils.py b/bikeshed/refs/utils.py index d0f29a7460..4b2f7ae782 100644 --- a/bikeshed/refs/utils.py +++ b/bikeshed/refs/utils.py @@ -181,13 +181,6 @@ def linkTextVariations(str: str, linkType: str | None) -> t.Generator[str, None, if str[0] != '"': yield '"' + str + '"' - if config.linkTypeIn(linkType, "cddl-key") or config.linkTypeIn(linkType, "cddl-value"): - # Allow linking to a cddl-key or cddl-value with or without quotes - if str[0] == '"': - yield str[1:-1] - if str[0] != '"': - yield '"' + str + '"' - if t.TYPE_CHECKING: U = t.TypeVar("U", bound="t.MutableMapping|t.MutableSequence") diff --git a/tests/cddl001.html b/tests/cddl001.html index da46e7d2bd..efedf3e5b6 100644 --- a/tests/cddl001.html +++ b/tests/cddl001.html @@ -602,15 +602,15 @@

Table of Contents

1. The definitions

- The attire enumeration lets you wear a "bow tie", a "necktie", an "Internet attire" or "swimwear". -

The basecolors construct defines a set of base colors, including black and white.

-

The extended-color construct extends basecolors with orange.

+ The attire enumeration lets you wear a "bow tie", a "necktie", an "Internet attire" or "swimwear". +

The basecolors construct defines a set of base colors, including black and white.

+

The extended-color construct extends basecolors with orange.

2. The CDDL

attire = "bow tie" / "necktie" / "Internet attire"
 attireBlock = (
-    "bow tie" /
-    "necktie" /
-    "Internet attire"
+    "bow tie" /
+    "necktie" /
+    "Internet attire"
 )
 attireGroup = (
     attire //
@@ -621,21 +621,21 @@ 
2. address = { delivery }
 
 delivery = (
-    street: tstr,
-    ? number: uint,
-    city // po-box: uint,
-    city // foo: tstr / uint // per-pickup: true
+    street: tstr,
+    ? number: uint,
+    city // po-box: uint,
+    city // foo: tstr / uint // per-pickup: true
 )
 
 city = (
-    name: tstr,
-    zip-code: uint
+    name: tstr,
+    zip-code: uint
 )
 
 attire /= "swimwear"
 
 delivery //= (
-    lat: float, long: float, drone-type: tstr
+    lat: float, long: float, drone-type: tstr
 )
 
 device-address = byte
@@ -645,13 +645,13 @@ 
2. terminal-color = &basecolors
 
 basecolors = (
-    black: 0,  red: 1,  green: 2,  yellow: 3,
-    blue: 4,  magenta: 5,  cyan: 6,  white: 7,
+    black: 0,  red: 1,  green: 2,  yellow: 3,
+    blue: 4,  magenta: 5,  cyan: 6,  white: 7,
 )
 
 extended-color = &(
     basecolors,
-    orange: 8,  pink: 9,  purple: 10,  brown: 11,
+    orange: 8,  pink: 9,  purple: 10,  brown: 11,
 )
 
 my_breakfast = #6.55799(breakfast)   ; cbor-any is too general!
@@ -664,8 +664,8 @@ 
2. solid = tstr
 
 apartment = {
- kitchen: size,
- * bedroom: size,
+ kitchen: size,
+ * bedroom: size,
 }
 size = float ; in m2
 
@@ -673,28 +673,28 @@ 
2. one-or-two-people = [1*2 person]
 at-least-two-people = [2* person]
 person = (
-   name: tstr,
-   age: uint,
+   name: tstr,
+   age: uint,
 )
 
 located-samples = {
- sample-point: int,
- samples: [+ float],
+ sample-point: int,
+ samples: [+ float],
  * equipment-type => equipment-tolerances,
 }
-equipment-type = [name: tstr, manufacturer: tstr]
+equipment-type = [name: tstr, manufacturer: tstr]
 equipment-tolerances = [+ [float, float]]
 
 PersonalData = {
-  ? displayName: tstr,
+  ? displayName: tstr,
   NameComponents,
-  ? age: uint,
+  ? age: uint,
   * tstr => any
 }
 
 NameComponents = (
-  ? firstName: tstr,
-  ? familyName: tstr,
+  ? firstName: tstr,
+  ? familyName: tstr,
 )
 
 square-roots = {* x => y}
@@ -702,32 +702,39 @@ 
2. y = float
 
 extensible-map-example = {
-  ? "optional-key": int,
+  ? "optional-key": int,
   * tstr => any
 }
 
 tcpflagbytes = bstr .bits flags
 flags = &(
-  fin: 8,
-  syn: 9,
-  rst: 10,
-  psh: 11,
-  ack: 12,
-  urg: 13,
-  ece: 14,
-  cwr: 15,
-  ns: 0,
+  fin: 8,
+  syn: 9,
+  rst: 10,
+  psh: 11,
+  ack: 12,
+  urg: 13,
+  ece: 14,
+  cwr: 15,
+  ns: 0,
 ) / (4..7) ; data offset bits
 
 rwxbits = uint .bits rwx
-rwx = &(r: 2, w: 1, x: 0)
+rwx = &(r: 2, w: 1, x: 0)

Index

Terms defined by this specification

+
  • + Internet attire + +
  • "kitchen", in § 2
  • kitchen, in § 2 +
  • "lat", in § 2
  • lat, in § 2
  • liquid, in § 2
  • located-samples, in § 2 +
  • "long", in § 2
  • long, in § 2 +
  • "magenta", in § 2
  • magenta, in § 2 +
  • "manufacturer", in § 2
  • manufacturer, in § 2
  • max-byte, in § 2
  • milk, in § 2
  • my_breakfast, in § 2 +
  • + "name" +
  • name +
  • + necktie + +
  • "ns", in § 2
  • ns, in § 2 +
  • "number", in § 2
  • number, in § 2
  • one-or-two-people, in § 2 +
  • "optional-key", in § 2
  • optional-key, in § 2 +
  • "orange", in § 1
  • orange, in § 1 +
  • "per-pickup", in § 2
  • per-pickup, in § 2
  • person, in § 2
  • PersonalData, in § 2 +
  • "pink", in § 2
  • pink, in § 2 +
  • "po-box", in § 2
  • po-box, in § 2
  • porridge, in § 2
  • protocol, in § 2 +
  • "psh", in § 2
  • psh, in § 2 +
  • "purple", in § 2
  • purple, in § 2 +
  • "r", in § 2
  • r, in § 2 +
  • "red", in § 2
  • red, in § 2 +
  • "rst", in § 2
  • rst, in § 2
  • rwx, in § 2
  • rwxbits, in § 2 +
  • "sample-point", in § 2
  • sample-point, in § 2 +
  • "samples", in § 2
  • samples, in § 2
  • size, in § 2
  • solid, in § 2
  • square-roots, in § 2 +
  • "street", in § 2
  • street, in § 2
  • "swimwear", in § 1 +
  • swimwear, in § 1 +
  • "syn", in § 2
  • syn, in § 2
  • tcpflagbytes, in § 2
  • terminal-color, in § 2
  • unlimited-people, in § 2 +
  • "urg", in § 2
  • urg, in § 2 +
  • "w", in § 2
  • w, in § 2
  • water, in § 2 +
  • "white", in § 1
  • white, in § 1 +
  • "x", in § 2
  • x
      @@ -842,7 +914,9 @@

      cddl-key for rwx, in § 2

  • y, in § 2 +
  • "yellow", in § 2
  • yellow, in § 2 +
  • "zip-code", in § 2
  • zip-code, in § 2

    CDDL Index

    diff --git a/tests/cddl002.html b/tests/cddl002.html index 3a14c50a84..496be696e0 100644 --- a/tests/cddl002.html +++ b/tests/cddl002.html @@ -619,14 +619,14 @@

    Table of Contents

    request-id = uint agent-capability = &( - receive-audio: 1 - receive-video: 2 - receive-presentation: 3 - control-presentation: 4 - receive-remote-playback: 5 - control-remote-playback: 6 - receive-streaming: 7 - send-streaming: 8 + receive-audio: 1 + receive-video: 2 + receive-presentation: 3 + control-presentation: 4 + receive-remote-playback: 5 + control-remote-playback: 6 + receive-streaming: 7 + send-streaming: 8 ) @@ -636,15 +636,23 @@

    agent-capability, in § Unnumbered section
  • agent-info, in § Unnumbered section
  • agent-info-response, in § Unnumbered section +
  • "control-presentation", in § Unnumbered section
  • control-presentation, in § Unnumbered section +
  • "control-remote-playback", in § Unnumbered section
  • control-remote-playback, in § Unnumbered section +
  • "receive-audio", in § Unnumbered section
  • receive-audio, in § Unnumbered section +
  • "receive-presentation", in § Unnumbered section
  • receive-presentation, in § Unnumbered section +
  • "receive-remote-playback", in § Unnumbered section
  • receive-remote-playback, in § Unnumbered section +
  • "receive-streaming", in § Unnumbered section
  • receive-streaming, in § Unnumbered section +
  • "receive-video", in § Unnumbered section
  • receive-video, in § Unnumbered section
  • request-id, in § Unnumbered section
  • response, in § Unnumbered section +
  • "send-streaming", in § Unnumbered section
  • send-streaming, in § Unnumbered section

    CDDL Index

    diff --git a/tests/cddl003.bs b/tests/cddl003.bs index e47e404409..ffbd78d1f9 100644 --- a/tests/cddl003.bs +++ b/tests/cddl003.bs @@ -56,6 +56,8 @@ nested = { key2: "nested" } } + +warn = [ (dupl: "val"), "dupl" ] Linking to the CDDL {#links} @@ -77,3 +79,5 @@ If there is no ambiguity, a key or value may be referenced directly, for example Otherwise, full path must be specified, as in `{^anon1/"v1"^}`, `{^anon2/key/"v2"^}`, `{^nested/key1/key2/"nested"^}`: {^anon1/"v1"^}, {^anon2/key/"v2"^}, {^nested/key1/key2/"nested"^}. In particular, partial paths such as `{^key/"unique"^}` or `{^key2/"nested"^}` do not work: {^key/"unique"^}, {^key2/"nested"^} (Bikeshed reports `No 'cddl' refs found` messages in such cases). + +The {^warn^} type defines both a key and a value "dupl". Bikeshed reports a warning to the console to suggest creating additional CDDL types, because the shorthand `{^warn/dupl^}` can no longer be used. It remains possible to reference either construct by expliciting the link type, i.e., `dupl` and `dupl`: dupl, dupl. diff --git a/tests/cddl003.console.txt b/tests/cddl003.console.txt index f12b56d7bb..e0cd0ef649 100644 --- a/tests/cddl003.console.txt +++ b/tests/cddl003.console.txt @@ -1,6 +1,9 @@ -LINE 79: No 'cddl' refs found for '"unique"'. +WARNING: CDDL value dupl defined in type "warn" creates a duplicate with another CDDL definition. +Link type needs to be specified to reference the term. +Consider creating additional CDDL types to disambiguate. +LINE 81: No 'cddl' refs found for '"unique"'. {^key/"unique"^} -LINE 79: No 'cddl' refs found for '"nested"'. +LINE 81: No 'cddl' refs found for '"nested"'. {^key2/"nested"^} LINE 17: Unexported dfn that's not referenced locally - did you mean to export it? local end diff --git a/tests/cddl003.html b/tests/cddl003.html index 097c614cf2..f02f89bf6b 100644 --- a/tests/cddl003.html +++ b/tests/cddl003.html @@ -609,38 +609,40 @@

    Table of Contents

    1. The CDDL

    CDDL specified for the local end:

    agent-capability = &(
-  receive-audio: 1
-  receive-video: 2
-  receive-presentation: 3
-  control-presentation: 4
-  receive-remote-playback: 5
-  control-remote-playback: 6
-  receive-streaming: 7
-  send-streaming: 8
+  receive-audio: 1
+  receive-video: 2
+  receive-presentation: 3
+  control-presentation: 4
+  receive-remote-playback: 5
+  control-remote-playback: 6
+  receive-streaming: 7
+  send-streaming: 8
 )

    CDDL specified for the remote end:

    webExtension.ExtensionBase64Encoded = {
-  type: "base64",
-  value: text,
+  type: "base64",
+  value: text,
 }

    CDDL specified for all modules:

    barewords = {
-  bare: [+ float],
-  "word": int,
+  bare: [+ float],
+  "word": int,
 }
 
-anon1 = [("v1" / "v2")]
+anon1 = [("v1" / "v2")]
 anon2 = {
-  key: "v1" / "v2" / "unique"
+  key: "v1" / "v2" / "unique"
 }
 
 nested = {
-  key1: {
-    key2: "nested"
+  key1: {
+    key2: "nested"
   }
 }
+
+warn = [ (dupl: "val"), "dupl" ]

    The agent-capability production gives information about agent capabilities.

    @@ -659,6 +661,7 @@