Avoid crash when handling unbounded conditions during designspace splitting #2797
+65
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Conditions with unbounded values have a "minimum" or "maximum" value of
None.
These tests check that:
a) The public-facing split functions can receive None values without
crashing; and
b) That their internal helper functions correctly translate the None
values to math.inf and -math.inf to express them.
These tests are expected to fail, indicating where a fix is required.
For unbounded conditions, the previous code expects "minimum" or "maximum" to be entirely absent, whereas actually they will be consistently present with one having a value of None. This means that math.inf and -math.inf are never substituted in for the absent bound, and a crash occurs when the None value propagates. This commit corrects the behaviour by checking for a value of None, instead of checking for the presence of the keys, bringing the behaviour inline with the rest of the library.
anthrotype
reviewed
Sep 1, 2022
Some areas of the library check for both representations, and so doing this here too means we are less likely to break existing code. Despite this, flexibility introduces ambiguity, and so if typing gives us confidence that such an input is unlikely, we could re-review this; conditions with missing keys are not safe to use across the entire code- base.
As we make an effort to support conditions with "minimum" or "maximum" absent, as well as with "minimum" or "maximum" None, this commit confirms that the split functions can handle these to some degree also.
anthrotype
approved these changes
Sep 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Designspace documents can contain unbounded conditions, which omit one of "minimum" or "maximum". In fontTools, the absence of either is represented with None, both when designspace files are parsed, and when designspace objects are processed in varLib.
Problem
For unbounded conditions, the splitting functions expect "minimum" or "maximum" to be absent, instead of present with a value of None. This means that the None values are never substituted by an appropriate value, and so propagate further until the program crashes.
ufo2ft uses splitting as part of its build pipeline, and so unbounded conditions break the build process downstream.
Fix
This PR changes the splitting code to expect "minimum" and "maximum" to always be present, with potential values of None. This makes the behaviour consistent with the rest of the library, and fixes the crash.
In addition, a test has been added to check for regressions.
Caveat
At least one area of the existing code accounts for the values being None AND the keys being absent. This is probably overkill, as the codebase is already heavily dependent on both keys always being present. For this reason, checking for both cases was avoided here.
Future work
We could type the conditions in DesignSpaceDocument to avoid issues like this in the future. In addition, we could add tests downstream that compile projects with unbounded conditions, as this would have highlighted the issue sooner.