-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
Check that parameters used in parsers are by-name (or of wildcard type) #204
base: master
Are you sure you want to change the base?
Conversation
Hey @lihaoyi. Thoughts about moving forward with this? |
@LPTK haven't had time to properly review it yet; do you mind if I just leave this here for a while? |
Not at all, no worries 😉 |
@lihaoyi Any progress on this? |
@LPTK Any update? |
@LPTK I think that's because there well need a Scala 3 part too now ? thanks for the update. |
Would be nice if there was a check there as well I guess (haven't tried the Scala 3 version), but that shouldn't be a requirement for adding the Scala 2 check in this PR. |
The right way of doing parameterized parsers is to use by-name parameters, as in:
However, currently nothing prevents users from not using a by-name parameter. And nothing in the API seems to imply that parser parameters should be by-name.
I made the mistake myself, and was bitten by a class-cast exception coming out of nowhere. If you're lucky, you'll get a strange warning saying "a pure expression does nothing in statement position" which hints at a problem – but you won't necessarily get that warning, and it's not sufficient for users to know how to solve the problem anyway.
This PR augments the fastparse macros with a checking pass that makes sure the parsers that come from parameters are from by-name parameters.
It's not a complete check by any means and can be subverted, but I think it's much better than nothing.