Skip to content

starlark: check for duplicate kwarg keys in dict #63

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

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

josharian
Copy link
Collaborator

Fixes #55

@josharian
Copy link
Collaborator Author

Oops. Do not review. This fix is in the wrong place.

@josharian
Copy link
Collaborator Author

PTAL

@josharian
Copy link
Collaborator Author

Note that doing bazelbuild/starlark#21 will render this unnecessary.

@alandonovan
Copy link
Contributor

alandonovan commented Dec 12, 2018

I don't think it's unnecessary in all cases. A call to dict or dict.update should not have repeated keys either statically (as in bazelbuild/starlark#21) or dynamically, as in this PR. Two separate checks are required. All other built-ins either don't accept keyword args, or call UnpackArgs, which performs the dynamic check. All Starlark defined functions have the check in setArgs.

dict and dict.update are unusual in they are built-ins that accept keyword args but don't use UnpackArgs.

@josharian
Copy link
Collaborator Author

Oh right, I forgot about the mixed static and dynamic kwargs case.

Copy link
Contributor

@alandonovan alandonovan left a comment

Choose a reason for hiding this comment

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

Thanks for your diligence here. I think this change will be necessary exactly as it is even if we implement a static check for repeated keyword args in a call.

Copy link
Collaborator Author

@josharian josharian left a comment

Choose a reason for hiding this comment

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

Done

@josharian
Copy link
Collaborator Author

All done; thanks for the review.

@alandonovan alandonovan merged commit 66ac3a2 into google:master Dec 13, 2018
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

Successfully merging this pull request may close these issues.

2 participants