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

Add support for cfg_attr-like attributes #2113

Merged
merged 5 commits into from
May 28, 2024

Conversation

saks
Copy link
Contributor

@saks saks commented May 16, 2024

Hello uniffi team!

I came across issue #2000 in my code and came up with what may solve this problem for a number scenarios. I do realize that this is not 100% bulletproof, but may work good enough before we figure out a better approach. Happy to add support for more scenarios. Any feedback is welcome.

@saks saks requested a review from a team as a code owner May 16, 2024 02:47
@saks saks requested review from mhammond and removed request for a team May 16, 2024 02:47
@saks saks changed the title add support for cfg_attr-like attributes Add support for cfg_attr-like attributes May 16, 2024
@bendk
Copy link
Contributor

bendk commented May 16, 2024

IIUC, this change makes it so that if a UniFFI attribute appears inside a nested meta list then we apply it to the current item. I think this is what #2086 was doing as well. I'm not sure if it will be the final solution, but it could be a nice hack that solves 80% of the cases. I'm interested to hear what @rafaelbeckel and @jplatte think though.

Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

The code looks correct to me, @rafaelbeckel likes it. Can we merge this one?

@jplatte
Copy link
Collaborator

jplatte commented May 23, 2024

I'd still much prefer the main attribute be constrained to be exactly cfg_attr, and the first argument be constrained to exactly uniffi.

IMHO it is more important that accidentally doing the wrong thing is prevented (i.e. a user writes #[cfg_attr(actually_meaningful_cfg_that_only_applies_sometimes, uniffi::method(name = "conditionally_different_name"))], it appears to work, later they discover that the rename is actually always applied), than supporting the obvious thing (#[cfg_attr(feature = "uniffi", uniffi::attribute(args))]). And even if the obvious thing is changed to work later using an implementation approach like what @bendk suggested in the issue where cfg's are actually properly evaluated, there could still be value in the uniffi pseudo-cfg being supported (since it is shorter to write / easier to read).

@rafaelbeckel
Copy link
Contributor

I have no strong opinion about this, and restricting it to cfg_attr does the job for me just fine.

@saks
Copy link
Contributor Author

saks commented May 24, 2024

Thank you @bendk, @jplatte and @rafaelbeckel for the feedback. I've limited the new functionality to only support cfg_attr attributes. Please have a look at the updated code and let me know if this matches your expectations.

@mhammond mhammond requested review from jplatte and bendk and removed request for mhammond May 24, 2024 18:19
Copy link
Contributor

@rafaelbeckel rafaelbeckel left a comment

Choose a reason for hiding this comment

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

Looks good to me. Limiting it to cfg_attr solves the original issue just fine. I can't think of other use cases. If there is one, someone else will open another issue in the future.

@mhammond
Copy link
Member

Can we please add a section at the end of here (I guess?) and maybe even a changelog? :)

@saks
Copy link
Contributor Author

saks commented May 25, 2024

thank you @mhammond, should have done it earlier. Please see if update to docs is making sense. Copywriting is not necessarily my strength :)

@mhammond
Copy link
Member

Thanks!

Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for adding those docs.

@bendk bendk merged commit e83fc4e into mozilla:main May 28, 2024
5 checks passed
@saks saks deleted the possible_fix_for_2000 branch May 28, 2024 15:45
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.

None yet

5 participants