Skip to content
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

Use of cast within ranges module #193

Open
Peter554 opened this issue Jan 28, 2025 · 0 comments
Open

Use of cast within ranges module #193

Peter554 opened this issue Jan 28, 2025 · 0 comments
Assignees

Comments

@Peter554
Copy link
Contributor

Peter554 commented Jan 28, 2025

Problem

Within the ranges module we use typing.cast in some places to cast the return type. For example, here when taking the intersection of two FiniteRanges we cast the return type as FiniteRange. This will make the returned value appear as a FiniteRange to type checkers (e.g. mypy) and to anyone inspecting the type annotations. However, if you actually make a runtime isinstance check then this will fail - the returned value is actually not a FiniteRange but simply a Range.

r1 = FiniteRange(D(0), D(10))
r2 = FiniteRange(D(5), D(15))

# mypy is fine with this (because that is what the type annotations say!)
intersection: Optional[FiniteRange[D]] = r1 & r2

# Sanity check
assert intersection is not None

# However the below line of code will fail!
assert isinstance(intersection, FiniteRange)

Why does this matter?

  • It's confusing and can potentially lead to bugs.
  • Example: In Optimise FiniteDatetimeRange __lt__, intersection and union #187 we introduced an optimisation to the intersection and union methods of FiniteDatetimeRange. However, these optimisations rely on an isinstance check. If a range is lying about its type then this can't work.

Proposed solution

  • I believe that we are misusing cast here (and in other places in the ranges module). I believe the intention has been to use cast here to avoid any performance penalty. However, the cast is simply not true! cast should only be used when we can be certain that the cast type is actually correct.
  • Remove this use of cast (and likely the others in the ranges module). Instead, actually instantiate a FiniteRange.
base_intersection = super().intersection(other)
if base_intersection is None:
    return None
return FiniteRange(
    # This use of cast is okay - we can be sure that `base_intersection.start` is actually of type `T`.
    cast(T, base_intersection.start)
    cast(T, base_intersection.end)
)
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

No branches or pull requests

1 participant