[red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` #17551

AlexWaygood · 2025-04-22T13:55:10Z

Summary

Passing a non-protocol class to get_protocol_members fails at runtime. This PR adds logic to red-knot so that we detect that error and warn the user about it.

Stacked on top of #17550

Test Plan

Existing mdtests updated and extended. Snapshots added!

Screenshot of what the diagnostics look like in the terminal:

github-actions · 2025-04-22T14:02:27Z

`mypy_primer` results

No ecosystem changes detected ✅

AlexWaygood · 2025-04-22T14:14:36Z

I don't know why the ruff (not red-knot!) ecosystem check is failing, but it doesn't seem related to this PR.

dhruvmanila · 2025-04-22T14:25:10Z

I don't know why the ruff (not red-knot!) ecosystem check is failing, but it doesn't seem related to this PR.

It's because you're using stacked PRs, once the PR that's below this one has the ecosystem check completed, you can re-run here so that it can find the base ecosystem report which would be part of that PR.

AlexWaygood · 2025-04-22T16:53:44Z

I don't know why the ruff (not red-knot!) ecosystem check is failing, but it doesn't seem related to this PR.

It's because you're using stacked PRs, once the PR that's below this one has the ecosystem check completed, you can re-run here so that it can find the base ecosystem report which would be part of that PR.

Hrm, I've tried rerunning the job several times now (and the PR below this one has definitely completed its CI!).

I think it's possibly because the PR below this one (#17550) had the Linux build skipped entirely in its CI run? I'm not sure why -- PRs that only change .md files don't seem to have any tests run on them right now, which is sorta problematic for red-knot PRs 😆

dhruvmanila · 2025-04-22T17:01:40Z

I think it's possibly because the PR below this one (#17550) had the Linux build skipped entirely in its CI run?

Ah, yeah that's probably the case. The Ruff ecosystem checks needs a base report to compare against. This is usually from main but as this is a stacked PR, it'll fetch it from the previous PR (which is correct) but that doesn't exists and so this workflow fails.

...s/mdtest/snapshots/protocols.md_-_Protocols_-_Invalid_calls_to_`get_protocol_members()`.snap

carljm

Nice!

carljm · 2025-04-22T23:57:52Z

crates/red_knot_python_semantic/src/types/diagnostic.rs

+    let Some(builder) = context.report_lint(&INVALID_ARGUMENT_TYPE, call) else {
+        return;
+    };
+    let db = context.db();
+    let mut diagnostic = builder.into_diagnostic("Invalid argument to `get_protocol_members`");
+    diagnostic.set_primary_message("This call will raise `TypeError` at runtime");
+    diagnostic.info("Only protocol classes can be passed to `get_protocol_members`");
+
+    let class_scope = class.body_scope(db);
+    let class_node = class_scope.node(db).expect_class();
+    let class_name = &class_node.name;
+    let class_def_diagnostic_range = TextRange::new(
+        class_name.start(),
+        class_node
+            .arguments
+            .as_deref()
+            .map(Ranged::end)
+            .unwrap_or_else(|| class_name.end()),
+    );
+    let mut class_def_diagnostic = SubDiagnostic::new(
+        Severity::Info,
+        format_args!("`{class_name}` is declared here, but it is not a protocol class:"),
+    );
+    class_def_diagnostic.annotate(Annotation::primary(
+        Span::from(class_scope.file(db)).with_range(class_def_diagnostic_range),
+    ));
+    diagnostic.sub(class_def_diagnostic);
+
+    diagnostic.info(
+        "A class is only a protocol class if it directly inherits \
+            from `typing.Protocol` or `typing_extensions.Protocol`",
+    );
+    diagnostic.info("See https://typing.python.org/en/latest/spec/protocol.html#");
+}


This is a lovely diagnostic, and a great example for what we can be doing with the new diagnostic framework in other cases -- but it's also a lot of code for an extremely narrow case (misuse of one specific function)! Not really suggesting we change anything here, just not sure it will be worth investing this much in every narrow edge-case diagnostic...

Yeah, I know what you mean.

I think there might be ways to make the diagnostic APIs a little less verbose? The code here isn't that complicated, just a little boilerplate-y. And I think the way you get a reputation for having "excellent error messages" like rustc is by paying attention to lots of little details like this.

…ocol_members`

* main: (28 commits) [red-knot] Make `BoundMethodType` a salsa interned (#17581) [red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` (#17551) [red-knot] Add more tests for protocol members (#17550) [red-knot] Assignability for subclasses of `Any` and `Unknown` (#17557) [red-knot] mypy_primer: add strawberry, print compilation errors to stderr (#17578) [red-knot] GenericAlias instances as a base class (#17575) Remove redundant `type_to_visitor_function` entries (#17564) Fixes how the checker visits `typing.cast`/`typing.NewType` arguments (#17538) [red-knot] Class literal `__new__` function callable subtyping (#17533) [red-knot] Surround intersections with `()` in potentially ambiguous contexts (#17568) [minor] Delete outdated TODO comment (#17565) [red-knot] add regression test for fixed cycle panic (#17535) [red-knot] fix unions of literals, again (#17534) red_knot_python_semantic: remove last vestige of old diagnostics! red_knot_python_semantic: migrate `types` to new diagnostics red_knot_python_semantic: migrate `types/diagnostic` to new diagnostics red_knot_python_semantic: migrate `types/call/bind` to new diagnostics red_knot_python_semantic: migrate `types/string_annotation` to new diagnostics red_knot_python_semantic: migrate `types/infer` to new diagnostic model red_knot_python_semantic: migrate INVALID_ASSIGNMENT for inference ...

…var-instance * dcreager/generic-constructor: (29 commits) We don't need this [red-knot] Make `BoundMethodType` a salsa interned (#17581) [red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` (#17551) [red-knot] Add more tests for protocol members (#17550) [red-knot] Assignability for subclasses of `Any` and `Unknown` (#17557) [red-knot] mypy_primer: add strawberry, print compilation errors to stderr (#17578) [red-knot] GenericAlias instances as a base class (#17575) Remove redundant `type_to_visitor_function` entries (#17564) Fixes how the checker visits `typing.cast`/`typing.NewType` arguments (#17538) [red-knot] Class literal `__new__` function callable subtyping (#17533) [red-knot] Surround intersections with `()` in potentially ambiguous contexts (#17568) [minor] Delete outdated TODO comment (#17565) [red-knot] add regression test for fixed cycle panic (#17535) [red-knot] fix unions of literals, again (#17534) red_knot_python_semantic: remove last vestige of old diagnostics! red_knot_python_semantic: migrate `types` to new diagnostics red_knot_python_semantic: migrate `types/diagnostic` to new diagnostics red_knot_python_semantic: migrate `types/call/bind` to new diagnostics red_knot_python_semantic: migrate `types/string_annotation` to new diagnostics red_knot_python_semantic: migrate `types/infer` to new diagnostic model ...

AlexWaygood added the ty Multi-file analysis & type inference label Apr 22, 2025

AlexWaygood requested review from carljm, dcreager and sharkdp as code owners April 22, 2025 13:55

AlexWaygood force-pushed the alex/get-protocol-members-validation branch from 8eac7ad to 9255038 Compare April 22, 2025 13:57

AlexWaygood force-pushed the alex/protocol-member-tests branch from 801147a to abc9ff6 Compare April 22, 2025 16:40

AlexWaygood force-pushed the alex/get-protocol-members-validation branch from 9255038 to 0d68ffd Compare April 22, 2025 16:44

AlexWaygood force-pushed the alex/get-protocol-members-validation branch 3 times, most recently from ece0c86 to b3a1538 Compare April 22, 2025 19:35

AlexWaygood commented Apr 22, 2025

View reviewed changes

...s/mdtest/snapshots/protocols.md_-_Protocols_-_Invalid_calls_to_`get_protocol_members()`.snap Show resolved Hide resolved

AlexWaygood force-pushed the alex/get-protocol-members-validation branch from b3a1538 to e34384c Compare April 22, 2025 19:44

carljm approved these changes Apr 22, 2025

View reviewed changes

Base automatically changed from alex/protocol-member-tests to main April 23, 2025 10:03

[red-knot] Emit a diagnostic if a non-protocol is passed to `get_prot…

0c975b4

…ocol_members`

AlexWaygood force-pushed the alex/get-protocol-members-validation branch from e34384c to 0c975b4 Compare April 23, 2025 10:09

AlexWaygood enabled auto-merge (squash) April 23, 2025 10:10

AlexWaygood merged commit 0a1f9d0 into main Apr 23, 2025
30 checks passed

AlexWaygood deleted the alex/get-protocol-members-validation branch April 23, 2025 10:13

carljm mentioned this pull request Apr 24, 2025

support Protocols astral-sh/ty#161

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` #17551

[red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` #17551

Uh oh!

AlexWaygood commented Apr 22, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 22, 2025 •

edited

Loading

Uh oh!

AlexWaygood commented Apr 22, 2025

Uh oh!

dhruvmanila commented Apr 22, 2025

Uh oh!

AlexWaygood commented Apr 22, 2025

Uh oh!

dhruvmanila commented Apr 22, 2025

Uh oh!

Uh oh!

carljm left a comment

Uh oh!

carljm Apr 22, 2025

Uh oh!

AlexWaygood Apr 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[red-knot] Emit a diagnostic if a non-protocol is passed to get_protocol_members #17551

[red-knot] Emit a diagnostic if a non-protocol is passed to get_protocol_members #17551

Uh oh!

Conversation

AlexWaygood commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

AlexWaygood commented Apr 22, 2025

Uh oh!

dhruvmanila commented Apr 22, 2025

Uh oh!

AlexWaygood commented Apr 22, 2025

Uh oh!

dhruvmanila commented Apr 22, 2025

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` #17551

[red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` #17551

AlexWaygood commented Apr 22, 2025 •

edited

Loading

github-actions bot commented Apr 22, 2025 •

edited

Loading

`mypy_primer` results