Skip to content
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

Improve contractimpl attribute field parsing #259

Merged
merged 1 commit into from Jul 20, 2022
Merged

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Jul 20, 2022

What

Error if contractimpl contains unexpected fields, and make suggestions in compile errors for what the field should be named.

Why

Typos are easy and we should utilize the fact that proc-macros can generate compile errors to alert the developer they are using the macro incorrectly.

Close #258

Known limitations

This partially closes #258, because unexpected fields will now error. This change uses darling because that crate will help us scale the attribute use cases into additional cases with less code. However the exact case that @jonjove experienced in #258 is not yet fixed because of a bug in darling where string literals without a field name in the attribute list are ignored. I've opened TedDriggs/darling#192. I'm going to mark #258 as resolved because I think this partial solution and using darling is better than us continuing to extend the existing get_args code to handle the error cases. We've done it elsewhere in macros, but in this specific case darling is a better long term solution.

@leighmcculloch leighmcculloch enabled auto-merge (squash) July 20, 2022 15:30
@leighmcculloch leighmcculloch merged commit 237f582 into main Jul 20, 2022
@leighmcculloch leighmcculloch deleted the betterattrs branch July 20, 2022 15:32
@jonjove
Copy link
Contributor

jonjove commented Jul 20, 2022

Should we leave #258 open until the actual issue described there is resolved? I agree with the approach, but I want to make sure we do eventually get this resolved fully.

@leighmcculloch
Copy link
Member Author

I think we should consider #258 fixed because the issue you experienced in the case that most others will experience it is addressed. Most developers who experience the issue will do so because they make a typo in a field name, not because they omit the field name. The case you experienced with the field name omitted was unique because the attribute used to behave that way. I doubt most people will omit the field name because it is a less subtle mistake to make. We could say that technically the issue is "won't fix" but I think the issue did highlight a meaningful problem to address. In your case the mistake was made because we had the attribute work in a way that is not common which we've also addressed.

@leighmcculloch
Copy link
Member Author

There's a fix for the specific issue of a literal in the meta list in TedDriggs/darling#193.

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.

contractimpl macro doesn't fail when given positional argument
2 participants