Skip to content

Prevent Required and NotRequired qualifiers together in TypedDict fields #447

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

Conversation

ajaymiranda
Copy link
Contributor

Summary:

This PR resolves #414 by adding a check and testcase to prevent Required and NotRequired from being used together as qualifiers in TypedDict fields.

Changes:

  • Implemented a validation check in solve.rs to reject conflicting qualifiers.
  • Added a testcase in typed_dict.rs to cover this case.

Copy link
Contributor

@ndmitchell ndmitchell 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 great! I suggested a minor performance tweak, but definitely a good change.


let has_required = ann.qualifiers.contains(&Qualifier::Required);
let has_not_required = ann.qualifiers.contains(&Qualifier::NotRequired);
if (qualifier == Qualifier::Required && has_not_required)
Copy link
Contributor

Choose a reason for hiding this comment

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

This code first checks if the required is present (I think using a linear scan), but only uses that result if qualifier == Qualifier::Required. It would be more efficient to inline has_required and has_not_required, then you only do the scan if you are on a relevant qualifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly! Thanks, I’ll update the code.

@yangdanny97
Copy link
Contributor

I'd expect test.py to generate some changes to the conformance outputs - was this not the case?

@ajaymiranda
Copy link
Contributor Author

I'd expect test.py to generate some changes to the conformance outputs - was this not the case?

Apologies for the oversight. The outputs were generated, but I missed committing them after running test.py. I'll add them, thanks.

@ajaymiranda ajaymiranda force-pushed the feature/typed_dict-required-notrequired branch from 335a998 to 85f51b8 Compare June 9, 2025 16:56
@facebook-github-bot
Copy link
Contributor

@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
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.

@facebook-github-bot
Copy link
Contributor

@yangdanny97 merged this pull request in 0dc7768.

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.

[feature] don't allow Required & NotRequired to appear at the same time as a qualifier for TypedDict fields
5 participants