Skip to content

Comments

Stop forwarding all non-bindgen attributes to _Ext methods#959

Merged
austinabell merged 2 commits intonear:masterfrom
mooori:no-fwd
Nov 8, 2022
Merged

Stop forwarding all non-bindgen attributes to _Ext methods#959
austinabell merged 2 commits intonear:masterfrom
mooori:no-fwd

Conversation

@mooori
Copy link
Contributor

@mooori mooori commented Nov 8, 2022

As mentioned in this comment, non-bindgen attributes on Contract methods shouldn't be forwarded to ContractExt methods.

In some cases it might lead to compilation errors or unintended behavior, see #952 for example.

Note: I assume technically that's a breaking change - but then, it's hard to think of a case where contract developers want these attributes to be forwarded (besides cfg).

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

I think it's more correct to not forward on the attributes. I'm inclined to include this in our release going out today or tomorrow, anything else you wanted to change about this?

Edit: and we can consider #952 in a future change, but I'd like to avoid adding extra semantics if possible

@mooori
Copy link
Contributor Author

mooori commented Nov 8, 2022

Thanks for the quick review, would be great if that could make it into the upcoming release. There's nothing left to do from my side (just kept it in draft mode while CI was running).

Not forwarding any attributes also seems preferable to me. However that breaks this compilation test:

#[near_bindgen]
impl Incrementer {
#[cfg(feature = "myfeature")]
pub fn new() -> Self {
Self {value: 0}
}
#[cfg(not(feature = "myfeature"))]
pub fn new() -> Self {
Self {value: 1}
}

Without forwarding #[cfg(...)] the methods new and inc will be defined twice for IncrementerExt.

How about merging as is so compilation_tests/cond_compilation needs no special treatment? Then we could:

#[near_bindgen(ext_fn_attr_whitelist(cfg))]
impl Incrementer { /* */ }

@mooori mooori marked this pull request as ready for review November 8, 2022 15:24
@austinabell
Copy link
Contributor

Not forwarding any attributes also seems preferable to me

Possibly, I don't see an issue with forwarding cfg options. This seems more intuitive to me. Why does this seem preferable not to forward anything?

@austinabell austinabell merged commit bf783dd into near:master Nov 8, 2022
@mooori
Copy link
Contributor Author

mooori commented Nov 9, 2022

Why does this seem preferable not to forward anything?

Now there's a whitelist that's kind of hidden from contract devs and I'm not sure if they always want to forward cfg attributes. Not forwarding cfg by default would make forwarding more explicit and allow to not forward cfg if it's not desired.

Though if the only expected use case for cfg attributes on contract methods is something like in compilation_tests/cond_compilation.rs, then forwarding cfg by default is probably the most convenient option for contract devs.

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