Add typing to otfautohint #1719

simoncozens · 2023-10-18T21:07:02Z

Description

otfautohint is a complex piece of code, and it's easy to get lost; it's even easier to make mistakes. Having typing information available makes it easier to catch such mistakes, and allows editors with Python type inferencing (Visual Code with pylance, Sublime Text with pyright, etc.) to provide type hints for variables and methods. This in turn helps to find typos, dead code, and so on.

Checklist:

I have followed the Contribution Guidelines
~~I have added test code and data to prove that my code functions correctly~~ (N/A)
I have verified that new and existing tests pass locally with my changes
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~ (N/A)

skef · 2023-10-18T21:16:05Z

As you're probably realizing this has a lot of formatting related problems (flake8), some of which are due to edits for line length that you've removed.

josh-hadley · 2023-10-18T21:17:23Z

@simoncozens thanks for this contribution. It looks like there are a number of linter failures that are blocking the tests from running. I recommend that you run flake8 using the config from this repo, fix all reported issues, and resubmit. Once we get the linter + tests green we will have a closer review and see about getting it merged.

simoncozens · 2023-10-18T21:18:37Z

Yeah, sorry, I was assuming the code formatting was black/PEP8 so my editor runs black on save to keep things tidy. I'll try and put things back into your preferred style.

skef · 2023-10-18T21:23:08Z

I think I would prefer a submission with the substantive changes and then (perhaps) a different submission with the large amount of stylistic changes. Or perhaps just skip the latter? Best to either mesh one's contributions with the existing style or talk in advance about what you want to change and why.

Edit: Ninja-ed.

skef · 2023-10-18T21:36:20Z

@josh-hadley Merging this would presumably require additional pytype github checks, so that notations like # pytype: disable=attribute-error won't just rot.

simoncozens · 2023-10-18T22:05:06Z

To be honest, it doesn't currently work with pytype. I started working with pytype, and as I added annotations I found myself in situations where a method returned an instance of its own class, which needs the Self type, and pytype doesn't currently support that. So I switched to mypy. In a couple of cases mypy doesn't like the Self return value, but it does a better job than pytype (and continues running, whereas pytype just stops and refuses to go on).

simoncozens · 2023-10-19T09:07:29Z

Coverage tests are sad because apparently you can't use weakref.ReferenceType as a generic in Python 3.8.

skef

Some preliminary review feedback

skef · 2023-10-19T12:30:00Z

python/afdko/otfautohint/glyphData.py

 from fontTools.pens.basePen import BasePen

 import logging
+
+# from .hintstate import glyphHintState


May not need this commented import?

skef · 2023-10-19T12:42:57Z

python/afdko/otfautohint/glyphData.py

    """Returns True if a and b are not close enough to be considered equal"""
    return abs(a - b) >= factor


-class pt(tuple):


I did make pt a tuple consciously, so that they are immutable and reusable and for certain other benefits. Can you talk about the reasons for your converting them to standard objects?

skef · 2023-10-19T12:46:24Z

python/afdko/otfautohint/glyphData.py

@@ -224,14 +238,14 @@ def __mul__(self, other):
        Returns a new pt object with this object's coordinates multiplied by
        a scalar value
        """
-        if not isinstance(other, numbers.Number):
+        if not isinstance(other, (int, float)):


I guess I'm OK with this, although it goes a bit against the spirit of the interface. In theory, at least, someone might want to call these operations with some other number-like parameter.

numbers.Number includes complex, which I think you should probably exclude.

Fair enough.

skef · 2023-10-19T12:59:46Z

python/afdko/otfautohint/glyphData.py

@@ -1407,7 +1439,7 @@ def toStems(self, data):
        for i in range(len(data) // 2):
            low = high + data[i * 2]
            high = low + data[i * 2 + 1]
-            sl.append(stem(low, high))
+            sl.append(stem(low, high))  # pytype: disable=wrong-arg-count


What's the thinking about keeping these relative to pytype not working with this code (at least for now)

python/afdko/otfautohint/otfFont.py

python/afdko/otfautohint/hintstate.py

skef · 2023-10-19T14:19:56Z

python/afdko/otfautohint/hinter.py

@@ -428,7 +462,7 @@ def handleOverlap(self):
        else:
            return self.isMulti

-    def addSegment(self, fr, to, loc, pe1, pe2, typ, desc, mid=False):
+    def addSegment(self, fr, to, loc, pe1: Optional[pathElement], pe2: Optional[pathElement], typ, desc, mid=False) -> None:


This has flake8 problems too, doesn't it?

skef · 2023-10-19T14:21:45Z

python/afdko/otfautohint/hinter.py

        """Return point cp3 adjusted relative to cp2 by CPFrac"""
        return (cp3 - cp2) * (1.0 - self.CPfrac) + cp2

-    def CPTo(self, cp0, cp1):
+    def CPTo(self, cp0, cp1) -> Any:


What is the purpose of the Any return type notations? Does it help compared with just leaving them off?

Python typing is progressive, and basically, I haven't finished adding all the annotations. But if you add any type notation, a method gets checked as best it can be, but with no type notation at all you don't get any checking or inferencing.

skef · 2023-10-19T14:29:57Z

python/afdko/otfautohint/hinter.py

                    if seg0.isBBox():
                        if seg0.isGBBox():
                            pbs = self.glyph.getBounds(None)
                        else:
+                            # I don't believe this can be reached, because


This is probably just so obscure a corner case that none of our tests exercise it and no one has hit it yet, so we didn't catch the bug. I'll take a look at some point.

seg0.isBBox() and not seg0.isGBBox() implies that the stems for seg0 were generated from the bounding box of the segment itself because nothing else worked. Only very unusual splines will have that problem.

skef · 2023-10-19T14:50:47Z

One reason we ported psautohint to python was the hope that it could be modified by the broader community, in comparison to the C code which was only ever worked on by a small group of Adobe-internal folks while squinting hard and crossing their fingers.

This PR is both an external contribution and an effort to make further contributions easier, so in that sense it's what we want and were hoping for.

Still, there is a basic concern about keeping these annotations from rotting. Some users will have environments that will make good use of the typing. Others will just be editing the files and running the tests and may have no idea what this stuff is or, if they do, how to change the annotations as the code changes.

Obviously some sort of verifier that can be run as part of the GitHub checks would be ideal, but it sounds like pytype can't do that and mypy might or might not be able to.

Do you have thoughts on this? Basically, how to maintain types in Python assuming a wide variety of users with different levels of experience and tooling?

Also: Keep in mind that we haven't been doing this internally, so it make take a while just to digest the implications of typed Python on our end.

anthrotype · 2023-10-19T14:59:25Z

FWIW annotations are pretty useless unless they are actually checked by a typechecker

anthrotype · 2023-10-19T14:59:58Z

ok, well.. self-documentation sure, but they then tend to rot as @skef noted.

simoncozens · 2023-10-19T16:51:33Z

Obviously some sort of verifier that can be run as part of the GitHub checks would be ideal, but it sounds like pytype can't do that and mypy might or might not be able to.

We can certainly get mypy set up in the CI, and add a disable comment to turn off the Self checks.

simoncozens added 7 commits October 18, 2023 09:46

Fix typo

b2de9cf

Wait, this whole method is dead

1448a92

Typo

fb4e2c5

Assertions to avoid unbound/None access

fc4a454

ABC annotations only make sense if we're an ABC

57fbdce

More assertions to calm type checkers

b7e51f6

Dead code?

10700c2

simoncozens force-pushed the fixups-and-types branch from e8c89f7 to f82dd2b Compare October 19, 2023 08:48

simoncozens added 16 commits October 19, 2023 09:59

Hopefully uncontroversial typings

bf643ad

Import the types

d078bfd

Make point into an ordinary class, not a tuple, because reasons

b7d0ece

Typing requires types.

1e37190

Uncontroversial typings

b729bb1

Some more typings

49f35c2

Really void functions

f8ca0b5

Better typings

c46b555

Mainly assertions

dac1ad0

More type hints

48551a5

Move number upstairs, others

2c24afa

Complex numbers would be a mistake here

cb04597

More type hinting

e338e32

This was a true optional

40badd5

More fixups

db2a6dc

Oops, they reused a variable

c8687bc

simoncozens added 6 commits October 19, 2023 09:59

Fix assertion

916aa85

Make mypy happy without check-untyped-defs

7d3d512

As close to done as I'm getting

84fe6b0

Fix failing test

6ac132e

Use typing_extensions to support <3.11

ef05a40

Restore formatting

5e5a0e3

simoncozens force-pushed the fixups-and-types branch from f82dd2b to 5e5a0e3 Compare October 19, 2023 08:59

skef reviewed Oct 19, 2023

View reviewed changes

simoncozens added 13 commits October 20, 2023 22:11

Ignore mypy error

60f2ea3

Use lo/hi, not tuple

f4128b9

pathElement is optional in hintSegment

f11236d

Good catch

68a6f63

Check all methods

f4c5c6b

Add mypy to requirements-dev

e8375c9

Run mypy on build

4c286d0

Restore code style

859f98d

Slacken off the weakref type to make 3.8 happier

2e7f3a2

Remove dead import

9046243

pytype does not support Self yet

40522b1

Explain Protocol class

d034b45

List can be subscripted, list can't

0945357

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing to otfautohint #1719

Add typing to otfautohint #1719

simoncozens commented Oct 18, 2023

skef commented Oct 18, 2023

josh-hadley commented Oct 18, 2023

simoncozens commented Oct 18, 2023

skef commented Oct 18, 2023 •

edited

Loading

skef commented Oct 18, 2023

simoncozens commented Oct 18, 2023

simoncozens commented Oct 19, 2023

skef left a comment

skef Oct 19, 2023

skef Oct 19, 2023

skef Oct 19, 2023

simoncozens Oct 19, 2023

skef Oct 20, 2023

skef Oct 19, 2023

skef Oct 19, 2023

skef Oct 19, 2023

simoncozens Oct 19, 2023

skef Oct 19, 2023

skef commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

simoncozens commented Oct 19, 2023

Add typing to otfautohint #1719

Are you sure you want to change the base?

Add typing to otfautohint #1719

Conversation

simoncozens commented Oct 18, 2023

Description

Checklist:

skef commented Oct 18, 2023

josh-hadley commented Oct 18, 2023

simoncozens commented Oct 18, 2023

skef commented Oct 18, 2023 • edited Loading

skef commented Oct 18, 2023

simoncozens commented Oct 18, 2023

simoncozens commented Oct 19, 2023

skef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skef commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

simoncozens commented Oct 19, 2023

skef commented Oct 18, 2023 •

edited

Loading