Duplicate idents generated when ODDs try to change/replace/delete attributes and reference them in an outdated class

[ebeshero]

While addressing PR https://github.com/TEIC/Stylesheets/pull/681 we discovered a problem in ODD processing. When a user has an ODD that references attributes that are no longer defined in that class, this generates duplicate definitions.

When modifying an attribute class, the application of mode="replace", mode="change", mode="delete" for an attribute not already in the class being modified generates duplicate attDefs, which are pulled from where that attribute already exists somewhere else.

This is not an issue for attempting to change attributes that were never defined in the first place.

[bwbohl]

I encountered the very same problem, thanks for reporting!

[ebeshero]

We have isolated more conditions that cause the problem: It arises:

• When we attempt to remove or change an existing attribute from an attribute class that simply does not contain it,
• But the attribute does exist in a different class
• And that attribute must be redefined on an element spec with mode='delete' (or maybe on mode='change')

We think we've located the problem and corrected it here by adding some conditional processing to prevent copying the attribute definition from its classSpec, and copying only the change to the elementSpec: https://github.com/TEIC/Stylesheets/commit/1d2ea124d270967e55221611d46be12c856b60c7

@joeytakeda feel free to elaborate here!

[joeytakeda]

Just to add: this is an error in the Stylesheets, but not a new one. The recent change of @calendar into its own class revealed the problem since @calendar was no longer part of att.datable BUT was customized in the old attribute class (e.g. <attDef ident="calendar" mode="delete"/> in att.calendar) AND the attribute was defined in the elementSpec (for deprecation purposes).

A simpler example, though, might be something like:

<schemaSpec ident="T" prefix="T_" start="space">
       <moduleRef key="tei"/>
       <moduleRef key="transcr" include="space"/>
       <classSpec type="atts" ident="att.dimensions" module="tei">
             <attList>
                 <attDef ident="resp" mode="change"/>
             </attList>
        </classSpec>
</schemaSpec>

Here, we're trying to remove the @resp attribute from att.dimensions, which does not contain an @resp (but att.global.responsibility does). As well, the space element currently customizes the definition of @resp to provide a customized definition. So when this is processed through the current stylesheets, you get something like (trimmed for brevity):

 <elementSpec module="transcr" ident="space">
       <!--[...]-->
          <classes>
            <memberOf key="att.global"/>
            <memberOf key="att.typed"/>
            <memberOf key="model.global.edit"/>
            <memberOf key="att.dimensions"/>
          </classes>
          <content>
            <alternate minOccurs="0" maxOccurs="unbounded">
              <classRef key="model.descLike"/>
              <classRef key="model.certLike"/>
            </alternate>
          </content>
          <attList>
            <attDef ident="dim" usage="rec">
              <!--[...]-->
            </attDef>
            <attDef ident="resp" mode="change"> 
              <desc versionDate="2013-12-10" xml:lang="en">(responsible party) indicates the
                individual responsible for identifying and measuring the space</desc>
            </attDef>
          </attList>
        </elementSpec>
        <classSpec module="tei" type="atts"  rend="change"
          ident="att.dimensions">
          <desc versionDate="2007-08-05" xml:lang="en">provides attributes for 
            describing the size of physical objects.</desc>
          <!--[...]-->
          <classes>
            <memberOf key="att.ranging"/>
          </classes>
          <attList>
            <!--  ⬇ THIS SHOUDLN'T BE HERE  ⬇    -->
            <attDef ident="resp" mode="delete"/>
            <attDef ident="unit" usage="opt">
                      <!--[...]-->
            </attDef>
            <attDef ident="quantity">
                      <!--[...]-->
            </attDef>
            <attDef ident="extent" usage="opt">
                     <!--[...]-->
            </attDef>
            <attDef ident="precision">
                     <!--[...]-->
            </attDef>
            <attDef ident="scope" usage="opt">
                     <!--[...]-->
            </attDef>
          </attList>
        </classSpec>

Which then produces duplicate @resp definitions in the RNG (since it is indeed defined twice — once in the element itself and once in att.dimensions even though the definition in att.dimensions isn't meant to be retained).

We (aka @sydb, @ebeshero, and I) believe that the issue derives from how class attributes are processed and merged in oddtoodd — the current behaviour is to 1) investigate all new attributes added to a class and copy them, then 2) investigate any attributes that are defined on a class in the customization and if they don't exist in the source, then just copy them, and then 3) step through each of the attributes for the class as defined in the source and modify (or just copy) per the customization. We ended our (lengthy) meeting thinking that step 2 is wrong: if a customization tries to customize an attribute in a class that doesn't actually exist in that class, then the ODD processor should (at the very least) just ignore it (but better would be to have some sort of warning about it imo)

[lb42]

Might it be helpful to flag up obvious incoherencies such as trying to modify an object that doesnt exist(in a class e.g.) as a pre-processor step? Something like this was done by the "sanity checker" written for an earlier version of Roma by ArnoMittelbach, back in the late middle ages.