Skip to content

flag ambiguous protocol members without type annotations#3012

Closed
knQzx wants to merge 2 commits intofacebook:mainfrom
knQzx:detect-ambiguous-protocol-member
Closed

flag ambiguous protocol members without type annotations#3012
knQzx wants to merge 2 commits intofacebook:mainfrom
knQzx:detect-ambiguous-protocol-member

Conversation

@knQzx
Copy link
Copy Markdown
Contributor

@knQzx knQzx commented Apr 4, 2026

summary

  • add new UnannotatedProtocolMember error kind for protocol members assigned a value without an explicit type annotation
  • matches mypy behavior - protocol members must have declared types
  • updates existing bug test case to verify the new diagnostic fires correctly

test plan

  • updated test_protocol_ambiguous_member to expect the new error on unannotated assignments like x = None
  • added positive case (class Ok(Protocol)) with annotated members to confirm no false positives
  • all 34 existing protocol tests still pass

fixes #2925

add UnannotatedProtocolMember error kind that fires when a protocol
class body assigns a value to a member without an explicit type
annotation - matching mypy's behavior of requiring all protocol
members to have declared types.
@meta-cla meta-cla bot added the cla signed label Apr 4, 2026
@github-actions github-actions bot added the size/s label Apr 4, 2026
@Arths17
Copy link
Copy Markdown
Contributor

Arths17 commented Apr 5, 2026

Hey @knQzx, nice work on this. Parity with mypy regarding unannotated protocol members is definitely important for consistency across the ecosystem.

I've looked through the summary, and the test plan seems solid especially verifying that existing protocol tests still pass. One quick question: for members assigned a value like x = None without an annotation, does the new UnannotatedProtocolMember error provide a clear suggestion to the user to add a type hint?

I'm happy to pull this branch and verify it against a few edge cases (like nested Protocols) to make sure there aren't any weird regressions!

@knQzx
Copy link
Copy Markdown
Contributor Author

knQzx commented Apr 5, 2026

thanks, yeah the error points to the unannotated member and suggests adding a type annotation

@Arths17
Copy link
Copy Markdown
Contributor

Arths17 commented Apr 5, 2026

Appreciate the quick response! Suggesting the type hint directly in the error message is a huge plus for developer experience.

Parity with mypy on unannotated protocol members is a major step for making Pyrefly a solid drop-in replacement. Since the existing 34 tests are passing and the diagnostic is clear, this looks great to me.

Copy link
Copy Markdown
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks! I'll try to get a second review and get it landed this week

@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync bot commented Apr 8, 2026

@stroxler has imported this pull request. If you are a Meta employee, you can view this in D100014044.

Copy link
Copy Markdown
Contributor

@rchen152 rchen152 left a comment

Choose a reason for hiding this comment

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

Review automatically exported from Phabricator review in Meta.

@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync bot commented Apr 8, 2026

@stroxler merged this pull request in 7df2cd6.

@lolpack
Copy link
Copy Markdown
Contributor

lolpack commented Apr 8, 2026

I thought we were going to have this error code off by default per the discussion in the original ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ambiguous protocol member with value assignment

5 participants