Skip to content

fix iterable check for Python 3.13.4 and newer #3855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 13, 2025

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Jun 7, 2025

Fix the feaLib/ast.py snippet used to check whether a type is iterable to work correctly with Python 3.13.4. The snippet wrongly assumed that a generator expression will raise immediately when the RHS of in is not iterable. This is no longer the case with Python 3.13.4, and such a generator only raises when you actually start iterating. Use a plain for expression to start iterating and catch the problem more reliably.

Fixes #3854

Fix the `feaLib/ast.py` snippet used to check whether a type is iterable
to work correctly with Python 3.13.4.  The snippet wrongly assumed
that a generator expression will raise immediately when the RHS of `in`
is not iterable.  This is no longer the case with Python 3.13.4,
and such a generator only raises when you actually start iterating.
Use a plain `for` expression to start iterating and catch the problem
more reliably.

Fixes fonttools#3854
@justvanrossum
Copy link
Collaborator

While this is a bug with Python 3.13.4, I'm inclined to accept this fix.

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would consume the first item if lookup already is an iterable. That doesn't happen in the actual code, though.

I wonder if we should do something like this instead:

        if not isinstance(lookup, typing.Sequence):
            self.lookups[i] = [lookup]

@mgorny
Copy link
Contributor Author

mgorny commented Jun 7, 2025

Hmm, good point, I've assumed the type will involve only containers and not iterators.

I don't know how reliable isinstance() is, I would probably go for hasattr(..., "__iter__").

@dscorbett
Copy link
Member

The exact equivalent to the original generator expression is iter(lookup). The replacements with isinstance and hasattr are subtly different.

Switch to using `iter(lookup)` as suggested by @dscorbett as the exact
equivalent of the original expression.  Most importantly, this avoids
incidentally taking the first item off `lookup` if it is an iterator.
@mgorny
Copy link
Contributor Author

mgorny commented Jun 9, 2025

Updated to use iter(lookup).

@behdad
Copy link
Member

behdad commented Jun 11, 2025

Can this be merged? I'm seeing bot fails due to this I believe:
https://github.com/fonttools/fonttools/actions/runs/15596131723/job/43926843712?pr=3485

@anthrotype anthrotype merged commit 84b2fce into fonttools:main Jun 13, 2025
11 checks passed
@mgorny mgorny deleted the iter-check branch June 13, 2025 10:44
@mgorny
Copy link
Contributor Author

mgorny commented Jun 13, 2025

Thank you!

@anthrotype
Copy link
Member

Thank you. Somehow I missed this. I also just tagged a new release, should get published soon once the CI completes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regressions with Python 3.13.4: TypeError: 'LookupBlock' object is not iterable
5 participants