MathInfo: subtracting list and None attributes gives invalid results

Say you have two Info objects, a and b; in a, the postscriptFamilyBlues is some list of integers, whereas in b object, that attribute is None (not an empty list).
When subtracting a - b, the resulting object will have the attribute set to 0, because of this code here:

fontMath/Lib/fontMath/mathInfo.py

Lines 62 to 73 in a07bcc3

    
           elif a is not None and b is None: 
        
               #v = a 
        
               if func == add: 
        
                   v = a 
        
               else: 
        
                   v = 0 
        
           elif b is not None and a is None: 
        
               #v = b 
        
               if func is add: 
        
                   v = b 
        
               else: 
        
                   v = 0

but that's invalid, because the type of postscriptFamilyBlues attribute must be an Optional[list], not int.

I don't understand the rationale of special-casing a subtrahend value of None. For the "add" function it simply sets the value to self (i.e. adding None doesn't do anything). Why for subtraction it would be any different?

The UFO fontinfo.plist spec says the file is optional and not all fields are required (and doesn't explicitly require any one of them), so I must conclude that all info values, including those that are typed as "list" can be omitted, thus set to None.
In defcon, these list-attributes are always parsed and written out as lists; so this issue never occurs becase both a and b in the code snippet above are always non-None and processed as lists. If they have different length, _processMathOneNumberList returns None (not 0), which is valid value for an Optional[list] attribute.
In ufoLib2, we treat all info attributes as optional, including these list-attributes, and thus if they are not set in the fontinfo.plist they are None (or conversely if they are None, they won't be written out to fontinfo.plist, and if all info attributes are None, no fontinfo.plist is written, which makes more sense to me).

somehow related to #136

Here's some test files: FontMathIncompleteInfoTest.zip

I have no recollection of why I did this, but I can't think of a reason for the behavior. My opinion is that it is a mistake. Feel free to fix it.

I think @LettError has a better understanding of this as he was the last to touch that code in #136. I hope he can shed some light.

@anthrotype can you shed some light on why this specifically is an issue?

It relies on defcon's handling of font info list attributes (e.g. postscript*), which will take an assignment of v = 0 and ignore it. ufoLib2 doesn't, causing pipeline explosions.

Edit: so doing defcon.Font().info.postscriptFamilyBlues = 0 explodes, too, but the quoted code above results in [] on defcon Info object attributes (presumably correct?) and 0 in ufoLib2 Info object attributes.

The outcome of these operations is undefined but I'm hesitant to just change it to what you need without understanding the repercussions for the all other users of this code. Is there anything keeping your code from doing some reality checks on your data before making a mathInfo object?

What I wanted to undestand is why should addition be treated differently than subtraction when one of the terms is undefined? That's what you did in #137.

doing some reality checks on your data before making a mathInfo object?

As I sad, a None value for any info object should be valid, since all the info attributes are optional, including the whole fontinfo.plist file. None signals that an attribute is missing from fontinfo.plist, which is not the same thing as an empty plist array or python list.
The current code doesn't handle this case, and ends up setting an invalid value (0) to an info attribute which can only be a list (or missing).

BTW, if undefined is really bad, then blowing up would be a better option.

I have this patch that doesn't affect the current behaviour, but handles the case when a number-list attribute is not defined. Setting v = None is equivalent to the operation doing nothing. This is also what's happening when the two MathInfo being subtracted are built from defcon.Info objects, and thus always have not None number-list attributes (an empty list is always returned); when two number-list attributes have different lengths, then the _processMathOneNumberList function currently returns None. I'm doing the same below.

diff --git a/Lib/fontMath/mathInfo.py b/Lib/fontMath/mathInfo.py
index 39e7c37..5c2af17 100644
--- a/Lib/fontMath/mathInfo.py
+++ b/Lib/fontMath/mathInfo.py
@@ -64,13 +64,13 @@ class MathInfo(object):
                 if func == add:
                     v = a
                 else:
-                    v = 0
+                    v = None if attr in _numberListAttrs else 0
             elif b is not None and a is None:
                 #v = b
                 if func is add:
                     v = b
                 else:
-                    v = 0
+                    v = None if attr in _numberListAttrs else 0
             if v is not None:
                 setattr(copiedInfo, attr, v)
         # special attributes
@@ -421,6 +421,12 @@ _infoAttrs = dict(
     # postscriptWeightName=unicode
 )
 
+_numberListAttrs = {
+    attr
+    for attr, (formatter, _) in _infoAttrs.items()
+    if formatter is _numberListFormatter
+}
+
 _postscriptWeightNameOptions = {
     100 : "Thin",
     200 : "Extra-light",

But I don't feel like committing this code before understanding why subtraction should be handled differently from addition.

Treating add and sub differently sounds like a mistake to me.

If blowing up is not an option, then these are reasonable options imo:

treat None as zero-ish (can be well defined, even for lists of numbers):
- a + None == a
- None + b == b
- a - None == a
- None - b == -b
or treat None as undefined:
- a + None == None
- None + b == None
- a - None == None
- None - b == None

a - None == 0 seems wrong and useless, and is clearly a bug when a is a list/tuple.

The solution in #176 is good, but only if the awkward a - None == 0 really can't be reconsidered.

I prefer the second option of treating None as undefined and not doing anything unless both operands are well defined.
My previous self raised the same question when this a - None = 0 change was first introduced, but even as I re-read the explanation today, it still isn't clear to me why.
See discussion here #136 (comment)

quoting @LettError reply from there:

in mutatormath systems (example afdko tests) only the default master has a complete set of font.info values. If a specific attribute is missing in a non-default master, the value of the default master is used. Mutatormath processes the objects differently from varlib and its not possible to change either one. These changes will restore that behavior in varlib.model. It also prevents duplication of data.

I don't understand the explanation either, but I just had a mini-epiphany: in FontMath-world, __sub__ is pretty much only used to calculate deltas ("how far to travel from A to B"), and in that particular context, a value of zero is the same as "this delta has no effect on this field".

This also explains the use for a + None == a.

I would think that a __sub__ a result of None (if there's a missing operand) should always have the same effect as zero. However, given the insecurities about changing anything at all about fontmath possibly causing problems down the line, I think your conservative solution (#176) is best for now.

	elif a is not None and b is None:
	#v = a
	if func == add:
	v = a
	else:
	v = 0
	elif b is not None and a is None:
	#v = b
	if func is add:
	v = b
	else:
	v = 0