Skip to content

Commit

Permalink
Forward reference deprioritization.
Browse files Browse the repository at this point in the history
This commit deprioritizes @beartype-specific **forward reference
proxies** (i.e., internal objects proxying external user-defined types
that have yet to be defined) in type tuples passed as the second
arguments to the `isinstance()` builtin, reducing the likelihood that
type-checks involving forward references will raise unexpected
exceptions. For example, consider this simple example:

```python
from beartype import beartype

@beartype
def muh_func(muh_arg: 'UndefinedType' | None = None): ...

class UndefinedType(object): ...
```

In the above example, @beartype should ideally be able to type-check
**at decoration time** that the default value of the optional `muh_arg`
parameter of the `muh_func()` function satisfies the type hint
`'UndefinedType' | None` – despite the fact that the `UndefinedType`
class is undefined! To do so, @beartype now internally reorders the
types comprising this union when type-checking this default value:

```python
# ...from this default type-check, which would raise a decoration-time
# exception due to "UndefinedType" being undefined...
isinstance(muh_arg, (UndefinedTypeProxy, NoneType))

# ...to this default type-check, which should raise *NO* decoration-time
# exception. Why? Because the default value "None" for the "muh_arg"
# parameter satisfies the first "NoneType" type, which then
# short-circuits the isinstance() call and thus ignores the problematic
# "UndefinedTypeProxy" type altogether.
isinstance(muh_arg, (NoneType, UndefinedTypeProxy))
```

In short, a nothingburger – but a promising nothingburger that may yield
future delights in the incautious event that we actually elect to try
type-checking defaults at decoration time again. *Gulp.* (*Helpless kelp!*)
  • Loading branch information
leycec committed Apr 4, 2024
1 parent b4265d5 commit 3f6354c
Show file tree
Hide file tree
Showing 9 changed files with 322 additions and 197 deletions.
158 changes: 103 additions & 55 deletions beartype/_check/code/codescope.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
from beartype._cave._cavemap import NoneTypeOr
from beartype._check.forward.reference.fwdrefmake import (
make_forwardref_indexable_subtype)
from beartype._check.forward.reference.fwdrefmeta import BeartypeForwardRefMeta
from beartype._check.forward.reference.fwdreftest import is_forwardref
from beartype._check.code.snip.codesnipstr import (
CODE_HINT_REF_TYPE_BASENAME_PLACEHOLDER_PREFIX,
CODE_HINT_REF_TYPE_BASENAME_PLACEHOLDER_SUFFIX,
Expand Down Expand Up @@ -288,7 +288,7 @@ def add_func_scope_types(
types: SetOrTupleTypes,

# Optional parameters.
is_unique: bool = False,
is_unique: Optional[bool] = None,
exception_prefix: str = (
'Globally or locally scoped set or tuple of classes '),
) -> str:
Expand All @@ -308,27 +308,38 @@ def add_func_scope_types(
----------
func_scope : LexicalScope
Local or global scope to add this object to.
types : _SetOrTupleOfTypes
types : SetOrTupleOfTypes
Set or tuple of arbitrary types to be added to this scope.
is_unique : bool, optional
``True`` only if the caller guarantees this tuple to contain *no*
duplicate types. This boolean is ignored if ``types`` is a set rather
than tuple. Defaults to :data:`False`. :data:`False`, this function
assumes this tuple to contain duplicate types by internally:
#. Coercing this tuple into a set, thus implicitly ignoring both
duplicates and ordering of types in this tuple.
#. Coercing that set back into another tuple.
#. If these two tuples differ, the passed tuple contains one or more
duplicates; in this case, the duplicate-free tuple is cached and
passed.
#. Else, the passed tuple contains no duplicates; in this case, the
passed tuple is cached and passed.
This boolean does *not* simply enable an edge-case optimization, though
it certainly does that; this boolean enables callers to guarantee that
this function caches and passes the passed tuple rather than a new
tuple internally created by this function.
is_unique : Optional[bool]
Tri-state boolean governing whether this function attempts to
deduplicate types in the ``types`` iterable. Specifically, either:
* :data:`True`, in which case the caller guarantees ``types`` to contain
*no* duplicate types.
* :data:`False`, in which case this function assumes ``types`` to
contain duplicate types by internally (in order):
#. Coercing this tuple into a set, thus implicitly ignoring both
duplicates and ordering of types in this tuple.
#. Coercing that set back into another tuple.
#. If these two tuples differ, the passed tuple contains one or more
duplicates; in this case, the duplicate-free tuple is cached and
passed.
#. Else, the passed tuple contains no duplicates; in this case, the
passed tuple is cached and passed.
* :data:`None`, in which case this function reduces this parameter to
either:
* :data:`True` if ``types`` is a :class:`tuple`.
* :data:`False` if ``types`` is a :class:`set`.
This tri-state boolean does *not* simply enable an edge-case
optimization, though it certainly does that; this boolean enables
callers to guarantee that this function caches and passes the passed
tuple rather than a new tuple internally created by this function.
Defaults to :data:`None`.
exception_prefix : str, optional
Human-readable label prefixing the representation of this object in the
exception message. Defaults to a sensible string.
Expand Down Expand Up @@ -356,7 +367,8 @@ def add_func_scope_types(
generate name collisions. This exception is thus intentionally raised
as a private rather than public exception.
'''
assert isinstance(is_unique, bool), f'{repr(is_unique)} not bool.'
assert isinstance(is_unique, NoneTypeOr[bool]), (
f'{repr(is_unique)} neither bool nor "None".')

# ....................{ VALIDATE }....................
# If this container is neither a set nor tuple, raise an exception.
Expand All @@ -382,8 +394,16 @@ def add_func_scope_types(
)
# Else, this container either contains two or more types.

# If the caller did *NOT* explicitly pass the "is_unique" parameter, default
# this parameter to true *ONLY* if this container is a set.
if is_unique is None:
is_unique = isinstance(types, set)
# Else, the caller explicitly passed the "is_unique" parameter.
#
# In either case, "is_unique" is now a proper bool.
assert isinstance(is_unique, bool)

# ....................{ FORWARDREF }....................
#FIXME: Unit test this up, please.
# True only if this container contains one or more beartype-specific forward
# reference proxies. Although these proxies are technically isinstanceable
# classes, attempting to pass these proxies as the second parameter to the
Expand All @@ -397,13 +417,14 @@ def add_func_scope_types(
# For each type in this container...
for cls in types:
# If this type is a beartype-specific forward reference proxy...
if cls.__class__ is BeartypeForwardRefMeta:
if is_forwardref(cls):
# print(f'Found forward reference proxy {repr(cls)}...')
# Note that this container contains at least one such proxy.
is_types_ref = False
is_types_ref = True

# Halt iteration.
break

# If this container contains at least one such proxy...
if is_types_ref:
# List of all such proxies in this container.
Expand All @@ -413,47 +434,74 @@ def add_func_scope_types(
types_ref: List[type] = []

# List of all other types in this container (i.e., normal types that are
# *NOT* beartype-specific forward references).
# *NOT* beartype-specific forward reference proxies).
types_nonref: List[type] = []

# For each type in this container...
for cls in types:
# If this type is such a proxy, append this proxy to the list of all
# such proxies.
if cls.__class__ is BeartypeForwardRefMeta:
if is_forwardref(cls):
types_ref.append(cls)
# Else, this type is *NOT* such a proxy. In this case, append this
# proxy to the list of all non-proxy types.
# Else, this type is *NOT* such a proxy. In this case...
else:
# print(f'Appending non-forward reference proxy {repr(cls)}...')

# If this non-proxy is *NOT* an isinstanceable class, raise an
# exception.
#
# Note that the companion "types_ref" tuple is intentionally
# *NOT* validated above. Why? Because doing so would prematurely
# invoke the __instancecheck__() dunder method on the metaclass
# of the proxies in that tuple, which would then erroneously
# attempt to resolve the possibly undefined types to which those
# proxies refer. Instead, simply accept that tuple of proxies as
# is for now and defer validating those proxies for later.
die_unless_type_isinstanceable(
cls=cls, exception_prefix=exception_prefix)

# Append this proxy to the list of all non-proxy types
types_nonref.append(cls)

# Reconstitute this container such that all non-proxy types appear
# *BEFORE* all proxy types.
types = tuple(types_nonref + types_ref)
# If the caller guaranteed these tuples to be duplicate-free,
# efficiently concatenate these lists into a tuple such that all
# non-proxy types appear *BEFORE* all proxy types.
if is_unique:
types = tuple(types_nonref + types_ref)
# Else, the caller failed to guarantee these tuples to be
# duplicate-free. In this case, coerce these tuples into (in order):
# * Sets, thus ignoring duplicates and ordering.
# * Back into duplicate-free tuples.
else:
types = tuple(set(types_nonref)) + tuple(set(types_ref))
# Else, the caller guaranteed these tuples to be duplicate-free.
# Else, this container contains *NO* such proxies. In this case, preserve
# the ordering of items in this container as is.
else:
# If this container is a set, coerce this frozenset into a tuple.
if isinstance(types, Set):
types = tuple(types)
# Else, this container is *NOT* a set. By elimination, this container
# should now be a tuple.
#
# In either case, this container should now be a tuple.

# If this container is *NOT* a tuple or is a tuple containing one or
# more items that are *NOT* isinstanceable classes, raise an exception.
die_unless_object_isinstanceable(
obj=types, exception_prefix=exception_prefix)
# Else, this container is a tuple of only isinstanceable classes.

# If the caller failed to guarantee this tuple to be duplicate-free,
# coerce this tuple into (in order):
# * A set, thus ignoring duplicates and ordering.
# * Back into a duplicate-free tuple.
if not is_unique:
# print(f'Uniquifying type tuple {repr(types)} to...')
types = tuple(set(types))
# print(f'...uniquified type tuple {repr(types)}.')
# Else, the caller guaranteed this tuple to be duplicate-free.

# ....................{ DUPLICATES }....................
# If this container is a set, coerce this frozenset into a tuple.
if isinstance(types, Set):
types = tuple(types)
# Else, this container is *NOT* a set. By elimination, this container
# should now be a tuple.
#
# In either case, this container should now be a tuple.

# If either this container is *NOT* a tuple or is a tuple containing one or
# more items that are *NOT* isinstanceable classes, raise an exception.
die_unless_object_isinstanceable(
obj=types, exception_prefix=exception_prefix)
# Else, this container is a tuple containing only isinstanceable classes.

# If this container is a tuple *AND* the caller failed to guarantee this
# tuple to be duplicate-free, coerce this tuple into (in order):
# * A set, thus ignoring duplicates and ordering.
# * Back into a duplicate-free tuple.
if isinstance(types, tuple) and not is_unique:
types = tuple(set(types))
# In either case, this container is now guaranteed to be a tuple containing
# only duplicate-free classes.
assert isinstance(types, tuple), (
Expand Down
49 changes: 24 additions & 25 deletions beartype/_check/forward/reference/fwdreftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,27 @@
This private submodule is *not* intended for importation by downstream callers.
'''

#FIXME: Currently unused, but preserved for posterity. *shrug*
# # ....................{ IMPORTS }....................
# from beartype._check.forward.reference.fwdrefmeta import BeartypeForwardRefMeta
#
# # ....................{ TESTERS }....................
# #FIXME: Unit test us up, please.
# def is_forwardref(obj: object) -> bool:
# '''
# :data:`True` only if the passed object is a **forward reference subclass**
# (i.e., class whose metaclass is class:`.BeartypeForwardRefMeta`).
#
# Parameters
# ----------
# obj : object
# Object to be tested.
#
# Returns
# -------
# bool
# :data:`True` only if this object is a forward reference subclass.
# '''
#
# # Return true only if the class of this object is the metaclass of all
# # forward reference subclasses, implying this object to be such a subclass.
# return obj.__class__ is BeartypeForwardRefMeta
# ....................{ IMPORTS }....................
from beartype._check.forward.reference.fwdrefmeta import BeartypeForwardRefMeta

# ....................{ TESTERS }....................
#FIXME: Unit test us up, please.
def is_forwardref(obj: object) -> bool:
'''
:data:`True` only if the passed object is a **forward reference subclass**
(i.e., class whose metaclass is class:`.BeartypeForwardRefMeta`).
Parameters
----------
obj : object
Object to be tested.
Returns
-------
bool
:data:`True` only if this object is a forward reference subclass.
'''

# Return true only if the class of this object is the metaclass of all
# forward reference subclasses, implying this object to be such a subclass.
return obj.__class__ is BeartypeForwardRefMeta
8 changes: 4 additions & 4 deletions beartype_test/a00_unit/a20_util/func/test_utilfuncscope.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ def when_in_the_springtime_of_the_year():
@attach_func_locals(func_stack_frames_ignore=1)
def when_the_trees_are_crowned_with_leaves() -> type_hint:
'''
Arbitrary nested callable annotated by a PEP-compliant type hint
localized to the parent callable and decorated by a decorator attaching
the local scope of that parent callable to a new ``func_locals``
attribute of this nested callable.
Arbitrary nested callable annotated by a type hint localized to the
parent callable and decorated by a decorator attaching the local scope
of that parent callable to a new ``func_locals`` attribute of this
nested callable.
'''

return 'When the ash and oak, and the birch and yew'
Expand Down
43 changes: 40 additions & 3 deletions beartype_test/a00_unit/a60_check/a00_code/test_codescope.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ def test_add_func_scope_types() -> None:
from beartype._cave._cavemap import NoneTypeOr
from beartype._check.code.codescope import add_func_scope_types
from beartype._util.utilobject import get_object_type_basename
from beartype_test.a00_unit.data.check.forward.data_fwdref import (
FORWARDREF_ABSOLUTE,
FORWARDREF_RELATIVE,
)
from beartype_test.a00_unit.data.data_type import (
Class,
NonIsinstanceableClass,
Expand Down Expand Up @@ -172,6 +176,41 @@ def test_add_func_scope_types() -> None:
assert func_scope[types_scope_name_b] == types_b
assert func_scope[types_scope_name_a] != func_scope[types_scope_name_b]

# ....................{ PASS ~ forwardref }....................
# Assert that this function implicitly reorders tuples containing:
# * One or more forward reference proxies.
# * One or more standard classes that are *NOT* forward reference proxies.
#
# Notably, assert that this function reorders all standard classes *BEFORE*
# all proxies to reduce the likelihood of raising exceptions during
# isinstance()-based type checks whose second arguments are such tuples.
types = (
FORWARDREF_ABSOLUTE,
FORWARDREF_RELATIVE,
str,
int,
)
types_scope_name = add_func_scope_types(
types=types, func_scope=func_scope, is_unique=False)
added_types = func_scope[types_scope_name]
assert set(added_types[:2]) == {str, int}
assert set(added_types[2:]) == {
FORWARDREF_ABSOLUTE,
FORWARDREF_RELATIVE,
}

# Repeat the same operation but with the "is_unique=False" parameter.
types_scope_name = add_func_scope_types(
types=types, func_scope=func_scope, is_unique=True)
added_types = func_scope[types_scope_name]
expected_types = (
str,
int,
FORWARDREF_ABSOLUTE,
FORWARDREF_RELATIVE,
)
assert added_types == expected_types

# ....................{ FAIL }....................
# Arbitrary scope to be added to below.
func_scope = {}
Expand Down Expand Up @@ -216,9 +255,7 @@ def test_add_func_scope_types() -> None:
# __instancecheck__() dunder method to unconditionally raise exceptions.
with raises(BeartypeDecorHintPep3119Exception):
add_func_scope_types(
types=(bool, NonIsinstanceableClass, float,),
func_scope=func_scope,
)
types=(bool, NonIsinstanceableClass, float), func_scope=func_scope)

# ....................{ TESTS ~ expresser : type }....................
def test_express_func_scope_type_ref() -> None:
Expand Down

2 comments on commit 3f6354c

@JWCS
Copy link

@JWCS JWCS commented on 3f6354c Apr 4, 2024

Choose a reason for hiding this comment

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

Note: since using an undefined type before its defined (ie class/obj) results in a NameError anyway, it should be impossible (with this change) for python to not pre-abort before the problem ever gets to beartype.
def muh_func(muh_arg: 'Undef' | None = Undef()): # raises NameError 'Undef'
But is there any "special" types that would invalidate this assumption?

@leycec
Copy link
Member Author

@leycec leycec commented on 3f6354c Apr 5, 2024

Choose a reason for hiding this comment

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

Hah, hah! You're exactly right. It's possible that the upcoming PEP 649 of which much has been said and shouted about would invalidate this assumption – but probably not. That's just a distraction.

No, this commit is intended solely to handle the opposite case to the one you helpfully presented, @JWCS:

def muh_func(muh_arg: 'Undef' | None = None):

One of the reasons that @beartype 0.18.0 blew up so catastrophically in my pasty face is that I failed to account for the above edge case. It turned out that whether or not @beartype 0.18.0 correctly type-checked the above default value of None was entirely non-deterministic. Depending on whether @beartype internally ordered the undefined Undef type before the defined type(None) type, that type-check would spuriously fail with a beartype.roar.BeartypeCallHintForwardRefException exception. Why? Because @beartype type-checks that default value with an internal call to the isinstance() builtin passed a 2-tuple resembling either:

  • isinstance(muh_arg, (UndefForwardRef, type(None)), which is bad.
  • isinstance(muh_arg, (type(None), UndefForwardRef), which is good.

Now, @beartype ensures that internal call always resembles the good ordering. And one less catastrophic edge case was born.

Look. Runtime type-checking is Hell™. I can only stroke my neckbeard and pretend I know what I'm doing. 🧙‍♂️

Please sign in to comment.