Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 16, 2025

Summary

There are still many tests missing from this test suite (see the long TODO list at the bottom of this file). But this PR already adds around 1,000 lines of tests, which feels like enough for now: I'd like to actually start implementing some of this stuff 😄

TODO: figure out how to make pre-commit happy...

Test Plan

this pr is all tests

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Apr 16, 2025
@AlexWaygood
Copy link
Member Author

@github-actions
Copy link
Contributor

github-actions bot commented Apr 16, 2025

mypy_primer results

No ecosystem changes detected ✅

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 16, 2025

we really need to figure out why we aren't getting any red-knot tests run on PRs that only touch MarkDown files right now 😄

but I ran the new tests locally and I promise that they all pass!

@AlexWaygood AlexWaygood added the testing Related to testing Ruff itself label Apr 16, 2025
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Fantastically thorough!

Comment on lines 587 to 588
satisfy the interface declared by `X`. But if `Y` is not `@final`, then this does not hold true, since a subclass of `Y`
could always provide additional methods or attributes that would lead to it satisfying `X`'s interface:
Copy link
Contributor

Choose a reason for hiding this comment

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

If Y does provide methods or attributes that are part of Xs interface, and the types of those are such that no subtype of Y could (respecting Liskov) possibly satisfy Xs interface, we can still consider them disjoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed! This is something I wanted to discuss; thanks for bringing it up. I seem to remember us having this conversation about nominal types on one of @sharkdp's PRs (I can't easily find the reference), and IIRC you were opposed to applying the same principle there. E.g. we could consider A and B here disjoint, because they both have a covariant (read-only) x attribute, but the type of A.x is disjoint from B.x. There can be no possible Liskov-compliant subclass of both A and B here; they are definitely disjoint:

class A:
    @property
    def x(self) -> bool:
        return True

class B:
    @property
    def x(self) -> str:
        return "x"

Is it justifiable to apply this principle to structural types but not nominal types? It feels inconsistent to me

Copy link
Member Author

Choose a reason for hiding this comment

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

(Going to land the PR for now as I don't think my current language precludes the idea that we might infer other pairs of protocols as disjoint in the future. Happy to revisit it later!)

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think there might be reasons to consider it for structural types and not nominal types, but I also think it's not high priority and fine to defer it either way, so let's just not worry about it for now.

@AlexWaygood AlexWaygood force-pushed the alex/protocol-tests branch from 683c2d6 to 5e059ee Compare April 17, 2025 11:33
@AlexWaygood AlexWaygood enabled auto-merge (squash) April 17, 2025 11:36
@AlexWaygood AlexWaygood merged commit bd89838 into main Apr 17, 2025
22 checks passed
@AlexWaygood AlexWaygood deleted the alex/protocol-tests branch April 17, 2025 11:36
dcreager added a commit that referenced this pull request Apr 17, 2025
* main:
  [red-knot] Detect version-related syntax errors (#16379)
  [`pyflakes`] Add fix safety section (`F841`) (#17410)
  [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450)
  Bump 0.11.6 (#17449)
  Auto generate `visit_source_order` (#17180)
  [red-knot] Initial tests for protocols (#17436)
  [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428)
  [red-knot] Dataclasses: support `order=True` (#17406)
dcreager added a commit that referenced this pull request Apr 18, 2025
* main: (123 commits)
  [red-knot] Handle explicit class specialization in type expressions (#17434)
  [red-knot] allow assignment expression in call compare narrowing (#17461)
  [red-knot] fix building unions with literals and AlwaysTruthy/AlwaysFalsy (#17451)
  [red-knot] Type narrowing for assertions (take 2) (#17345)
  [red-knot] class bases are not affected by __future__.annotations (#17456)
  [red-knot] Add support for overloaded functions (#17366)
  [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444)
  [red-knot] more type-narrowing in match statements (#17302)
  [red-knot] Add some narrowing for assignment expressions (#17448)
  [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446)
  Server: Use `min` instead of `max` to limit the number of threads (#17421)
  [red-knot] Detect version-related syntax errors (#16379)
  [`pyflakes`] Add fix safety section (`F841`) (#17410)
  [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450)
  Bump 0.11.6 (#17449)
  Auto generate `visit_source_order` (#17180)
  [red-knot] Initial tests for protocols (#17436)
  [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428)
  [red-knot] Dataclasses: support `order=True` (#17406)
  [red-knot] Super-basic generic inference at call sites (#17301)
  ...
@carljm carljm mentioned this pull request Apr 24, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to testing Ruff itself ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants