Bug in overridden vallists

[JanelleJenstad]

Have a look at the element spec for tag (https://tei-c.org/release/doc/tei-p5-doc/en/html/ref-tag.html). There should be a custom vallist, but it's not displaying. We think there's an error in the spec file. Bug occurred between 3.6 (which has the values) and 4.0.0 (which doesn't have the values). vallist should have mode=add

[JanelleJenstad]

There are 21 element specs with the same problem.

[martindholmes]

Other possible victim elements from a quick XPath:
castItem, constitution, derivation, desc, dimensions, domain, factuality, fw, gram, graph, interaction, iType, move, node, oRef, preparedness, purpose, tag, tech, usg, xr

[ebeshero]

@JanelleJenstad @martindholmes @sydb This seems weirdly relevant to our discussion of TEIC/Stylesheets#272 yesterday, over behavior of mode attributes related to vallists.

[martindholmes]

@ebeshero Yes, I think @sydb may have assumed that mode=add was not needed, but I think it is. Whether it should be or not is a different question. I think this might warrant an interim release, assuming it affects schemas as well as the web guidelines.

[sydb]

Yes, schemas affected. Yes, I think an interim release would be appropriate. I will try to investigate further later this weekend.

[martindholmes]

This does raise an interesting question about the @mode issue, though. We have two competing assumptions:

• @mode is inherited
• if there is no @mode, then "add" is assumed

In this case, it appears that the first is in effect, so @mode="change" is assumed on the <valList>, and since there is no original valList, nothing happens. That's the generous assumption, though; most likely there's also a bug in the Stylesheets whereby a <valList> with no @mode just gets ignored in this context.

I'm coming around to the idea that we should insist on @mode on all structures at any level that are expected to cause something to happen. That should be easy to enforce, and much easier to write processing for.

[martindholmes]

Working on this in branch issue-2128-valLists.

[martindholmes]

PR #2129 is live -- please review and merge.

[sydb]

I disagree — having what I think it is supposed to be, and what I vaguely recall Sebastian implying, is that it is inherited; if there is nothing to inherit, it is "add". This should be trivially easy to do in XSLT 2+:

<xsl:variable name="mode" value="( ancestor::*[@mode][1]/@mode, 'add' )[1]"/>

but was probably quite a pain to do in XSLT 1. Thus I strongly suspect there is just a bug(s) in the Stylesheets.
Furthermore, though, while we’re talking @mode, I am continually at least confused, if not annoyed, that of the two elements that essentially contain a sequence of items that themselves bear @mode (<classes> and <attList>), the former has a somewhat confusing @mode that cannot match that of its children, and the latter does not have @mode.
BTW, I am unconvinced that adding type=add to an <attDef> that is overriding is the correct solution to this problem. (Although I see you have just done that. :-)

[martindholmes]

@sydb Adding @mode="add", which is what I've done in the PR, simply brings these elements into line with 25 others which use the same mechanism when overriding @type. There is certainly a much larger discussion to be had about whether this should be necessary or not, but that should be had in the Stylesheets, and any action on that is likely to take a while; meanwhile, I think we need a quick fix here, and it's certainly not wrong to add @mode="add", since it's already widely used for exactly this in the Guidelines source already.

[lb42]

Where does the idea that the default value for @mode is (in old money) #INHERITED come from? Sfaicr, it is always ADD.

[lb42]

p.s, yes please fix this asap -- it also affects anyone trying to document their own ODD properly

[martindholmes]

@lb42 No idea where the inheritance idea comes from, other than I seem to recall it being mentioned in a discussion the Stylesheets Working Group was having when looking at TEIC/Stylesheets#272. Maybe I'm misremembering. That would be excellent -- if that's not a thing, then it's much easier to describe what should happen in cascading @mode contexts.

[sydb]

I have just tested and get the same results using the more semantically correct (IMHO) "change", rather than "add". Since that is what at least some of those <valList>s used to have before someone (namely me) messed up and deleted the @mode attrs while fixing #386, I am inclined to use "change", not "add".
I have a checkout of @martindholmes’s issue-2128-valLists branch with that change made waiting to be checked in (if others agree) or deleted (if others disagree).

Tests I ran

To compare the two I checked out two different local copies of the issue-2128-valLists branch, and changed the 22 files that had new mode=add to mode=change (using an Emacs macro, as I only wanted to change the very same line @martindholmes had changed, not any other cases of mode=add). I than (roughly simultaneously) ran make clean validate html-web test exemplars in both directories.
I then did a side-by-side visual scan of the output of the two make commands, and they looked the same. I also did a diff, but since every single timestamp and every single filepath was different, that output also had to be scanned by eye. Still, looked like no significant changes at all.
Last, I did a diff comparing the entirety of the two Exemplars/ directories, and the only differences were the timestamps.

[martindholmes]

I think if there was no valList in the class (which there isn't), and you provide one in the elementSpec, then it should be "add".

[sydb]

Hmmm … Makes some sense. But why should you, the ODD writer, need to figure out whether or not there was a <valList> in the class you are overriding? (And if the class changes from there is no <valList> to there is one, then ODDs that have "change" still work, but ODDs that have "add" should explode, no?)

[sydb]

IIRC, "change" is not supposed to throw an error when there is nothing to change, it just acts as "add". But at the moment (worrying about taxes) I am not sure.

[lb42]

Detection and reporting of error conditions is not one of the odd processing tool chains strengths

[martindholmes]

I think "add" is strictly correct in this context, but if a valList were to be added to the type attribute in the class (which it never will be), the processing should raise an error or a warning suggesting changing it to "change". I still think the PR as it stands is the correct fix.

[sydb]

I sought guidance from the Guidelines, but am still uncertain, I’m afraid. The last bit of TDbuild says that if you (the processor) are working on some declaration with mode=change and there is already an existing declaration for the same thing:

process identifiable children according to their modes; process unidentifiable children in replace mode; retain existing children where no replacement or change is provided

Which begs the question (already being toyed with in this discussion) about what is the default value of @mode, no?

[martindholmes]

If there were an ancestor valList, surely the required value to replace it would be "replace". I don't see how "change" makes any sense, unless you have some child valItems that you're going to keep and some new ones you're going to add. This is unlikely to happen with @type.

[martindholmes]

In any case, I think this is an issue for another ticket. The immediate urgency is to fix the missing things, and it makes more sense to fix them using the same attribute value that's been used to create the same output for all their colleague elements in the same situation which are not broken.

[sydb]

What makes you think "add" has been used for their colleague elements in the same situation? As I alluded to, above, I think the value "change" is what had been used, at least in some cases. (Could be wrong, and do not have the time to research it right now. Maybe tonight …)

My instinct is the harm in using the wrong one is pretty minimal, so I do not wan to delay this for long over the "change" v "add" issue … we can change them from one to the other without much sequelæ.

[martindholmes]

//elementSpec[classes/memberOf[@key='att.typed']][attList/attDef[@ident = 'type'][valList[@mode='add']]]

[sydb]

But that only tells us about overrides of att.typed (by far the most common, of course).

[sydb]

Interestingly, if I checkout 8995b48 (the commit before the change that I think caused the problem), build p5subset.xml, and run that XPath on it (for each hitting spitting out concat('in ', ancestor::*[@ident]/@ident, ' my mode is ', @mode, ' inside a mode=',ancestor::*[@mode][1]/@mode )), the result is:

in correspAction my mode is add inside a mode=change
in abbr my mode is add inside a mode=change
in list my mode is add inside a mode=change
in title my mode is add inside a mode=change
in recording my mode is add inside a mode=change

Which is only 5 of the 22. Hmmm …

[martindholmes]

Yes, I was only looking for att.typed overrides because virtually every commit linked to issue #386 was on att.typed items.

[martindholmes]

I worry a little that a lengthy discussion of the history will give Council members the impression that there's a debate to be had about whether or how this should be fixed in the short term. There isn't. This is bad and it should be fixed as soon as possible. People's ODDs and schemas are broken, and the Guidelines are currently missing lots of key info. Please let's fix this quickly, and then go back to the thorny issue of what @mode values mean and how they should be processed after that's done. :-)

[martinascholger]

@martindholmes I'm just trying to catch up on what I missed over the weekend. In which cases are ODDs and schemas broken? Do you have an example? I tried to reproduce one, but I did not succeed.

[martindholmes]

@martinascholger If you go here:

https://tei-c.org/release/doc/tei-p5-doc/en/html/ref-tag.html

you'll see that the valList for @type is missing. It's also missing from the schema:

https://tei-c.org/release/xml/tei/custom/schema/relaxng/tei_all.rng

(Lines 19752ff). There are a bunch of other elements also missing their valLists. Up to 3.6.0, these elements had their own locally-defined type attributes. Then they were added to the att.typed class, and the attribute was overridden at the elementSpec level, but no mode attribute was supplied on the valList, so the values have just been missing ever since.

[martinascholger]

Yes, but that does not automatically mean that the ODDs and schemas are broken, but that they do not validate the values correctly, or? I just wonder why we did not notice it for a while. There is no question that we need to fix this.

[martindholmes]

They're broken in the sense that where previously people would have got suggested values popping up in Oxygen, suddenly those values would disappear; and if any of these valLists were closed, encoders would suddenly have been able to add bad values that would have been caught by validation before, but would now be allowed. I didn't mean the schemas were technically invalid or anything like that; just that they would cause problems for projects using them. That's how @JanelleJenstad and I found the problem in the first place; we couldn't understand why the values for tag/@type were not showing up in our documentation or in Oxygen, causing encoders to make stuff up because they had no guidance.

[JanelleJenstad]

@martindholmes sums it up nicely. After we noticed the problem on Friday, Martin and I were speculating last week about why people hadn't noticed. I only noticed because I was writing documentation for one of our own projects and wanted to double-check what values are allowed on tag/@type. Martin had the values in his head, as would most people who write a lot of documentation or know the guidelines inside and out. So it's mostly non-expert users of the guidelines who would go looking for values; if you don't find them, you assume there aren't any and start making up your own. (In a way, I'm the perfect reader for the Guidelines right now -- not experienced enough to be independent of them, but experienced enough to know how to tell you when I think something is missing/wrong/unclear.)

[martinascholger]

Thanks @JanelleJenstad and @martindholmes !

[JanelleJenstad]

@npcole offered to do the point release.

[JanelleJenstad]

Thanks everyone! I'm closing this ticket now.