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

[feature] #2488: Add support for trait impls in ffi_export #2601

Merged
merged 9 commits into from
Aug 15, 2022

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Aug 9, 2022

Description of the Change

  • Add support for trait impls (methods are generated with following name Type__Trait__method)
  • Remove requirements for items to have doc string

Issue

Closes #2488.

Benefits

No need to manually implement trait methods.

Possible Drawbacks

None.

Usage Examples or Tests [optional]

cargo +nightly-2022-04-20 miri test --package iroha_ffi

Alternate Designs [optional]

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Aug 9, 2022
data_model/src/name.rs Outdated Show resolved Hide resolved
ffi/tests/ffi_export.rs Outdated Show resolved Hide resolved
ffi/derive/src/impl_visitor.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #2601 (e5e9c99) into iroha2-dev (a16d9c3) will decrease coverage by 1.72%.
The diff coverage is 63.31%.

❗ Current head e5e9c99 differs from pull request most recent head 250134c. Consider uploading reports for the commit 250134c to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev    #2601      +/-   ##
==============================================
- Coverage       67.61%   65.88%   -1.73%     
==============================================
  Files             140      156      +16     
  Lines           26173    28155    +1982     
==============================================
+ Hits            17696    18550     +854     
- Misses           8477     9605    +1128     
Impacted Files Coverage Δ
cli/derive/src/lib.rs 74.72% <ø> (ø)
cli/src/torii/mod.rs 28.88% <ø> (ø)
cli/src/torii/routing.rs 69.92% <0.00%> (-2.87%) ⬇️
client/src/client.rs 43.36% <0.00%> (-2.96%) ⬇️
client_cli/src/main.rs 0.26% <ø> (ø)
config/base/src/runtime_upgrades.rs 35.63% <ø> (ø)
core/src/block.rs 70.00% <ø> (ø)
core/src/queue.rs 95.67% <ø> (-0.08%) ⬇️
core/src/smartcontracts/isi/asset.rs 54.82% <ø> (ø)
core/src/smartcontracts/wasm.rs 95.39% <ø> (+0.12%) ⬆️
... and 154 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ffi/derive/src/ffi_fn.rs Outdated Show resolved Hide resolved
ffi/derive/src/ffi_fn.rs Outdated Show resolved Hide resolved
data_model/src/name.rs Outdated Show resolved Hide resolved
@Erigara Erigara force-pushed the ffi_export_trait_impls branch 2 times, most recently from 7d62ecf to 883307d Compare August 12, 2022 06:43
ffi/derive/src/ffi_fn.rs Show resolved Hide resolved
ffi/derive/src/ffi_fn.rs Outdated Show resolved Hide resolved
ffi/derive/src/ffi_fn.rs Outdated Show resolved Hide resolved
ffi/tests/unambiguous.rs Outdated Show resolved Hide resolved
mversic
mversic previously approved these changes Aug 12, 2022
ffi/derive/src/impl_visitor.rs Outdated Show resolved Hide resolved
ffi/derive/src/impl_visitor.rs Outdated Show resolved Hide resolved
ffi/derive/src/impl_visitor.rs Show resolved Hide resolved
ffi/derive/src/impl_visitor.rs Show resolved Hide resolved
if self.trait_name.is_none() && !matches!(node.vis, syn::Visibility::Public(_)) {
abort!(
node.vis,
"Methods defined in the inherit impl block must be public"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Methods defined in the inherit impl block must be public"
"Methods defined in the inherent `impl` block must be public"

On another note, I'd actually prefer if you just skipped them, instead of failing to compile. Notify via https://docs.rs/proc-macro-error/latest/proc_macro_error/macro.emit_warning.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are aborting execution all over the place (i mean ffi_export/import macros) where we don't support such item.
What benefits will we get if we skip private functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, you wouldn't force the user to add the function elsewhere. A private function is perfectly valid code, while it shouldn't be reflected in the ffi export, forcing people to have multiple inherent impls is not good design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, in this case we should make impl blocks more permissive in general, skip consts, types, ... instead of aborting execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now private functions are skipped, however i left this abortion, to indicate that something going wrong if we try to export private function.

ffi/derive/src/lib.rs Show resolved Hide resolved
ffi/derive/tests/ui_fail/private_method_inside_impl.rs Outdated Show resolved Hide resolved
ffi/tests/ffi_export.rs Show resolved Hide resolved
@mversic mversic requested review from mversic and removed request for appetrosyan August 15, 2022 05:31
mversic
mversic previously approved these changes Aug 15, 2022
@appetrosyan appetrosyan marked this pull request as draft August 15, 2022 08:51
@Erigara Erigara marked this pull request as ready for review August 15, 2022 11:21
@mversic mversic self-requested a review August 15, 2022 11:30
@Erigara Erigara merged commit 52adcbf into hyperledger-iroha:iroha2-dev Aug 15, 2022
@Erigara Erigara deleted the ffi_export_trait_impls branch August 15, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants