fix: prohibit NEAR function generics #980
Conversation
itegulov
requested review from
austinabell and
ChaoticTempest
as code owners
|
UPD: Let's move this conversation to #974 |
| @@ -38,6 +38,12 @@ impl AttrSigInfo { | |||
| original_attrs: &mut Vec<Attribute>, | |||
| original_sig: &mut Signature, | |||
| ) -> syn::Result<Self> { | |||
| if !original_sig.generics.params.is_empty() { | |||
There was a problem hiding this comment.
Let's maybe only do this for pub-lic functions?
There was a problem hiding this comment.
Dude, why is this method even being called for private methods 🤔
I am guessing there are two separate logic flows happening in this method: checking that there are no weird variadic async etc modifiers (that is supposed to work on both private and public functions) and processing near_bindgen stuff (only for public functions). I will refactor this.
|
Decided to leave lifetimes out of this PR for now, we can discuss this separately under #974. This became more of a refactoring PR, but I feel like the current logic is backward from what it should be. I left async/variadic/extern checks for all impl methods (even private), but honestly not sure that is necessary. For example, an async function is compilable for wasm on its own, you just wouldn't be able to use it in any meaningful way since public functions can't be async. @austinabell you would probably have the most context on what the original intention of these checks was. |
There was a problem hiding this comment.
I left async/variadic/extern checks for all impl methods (even private), but honestly not sure that is necessary. For example, an async function is compilable for wasm on its own, you just wouldn't be able to use it in any meaningful way since public functions can't be async.
Eh, should be fine to retain those checks.
A small side effect of the current logic (or perhaps intentional?) is we're no longer checking them for #[ext_contract] traits. And while technically, neither async traits nor C-variadic methods are stable, explicit definition of support on our end would be nice.
near-sdk-macros/src/core_impl/info_extractor/impl_item_method_info.rs
Outdated
Show resolved
Hide resolved
| @@ -241,7 +241,7 @@ mod tests { | |||
| #[warn(unused)] | |||
| pub fn method(&self) { } | |||
| }; | |||
| let method_info = ImplItemMethodInfo::new(&mut method, impl_type).unwrap(); | |||
| let method_info = ImplItemMethodInfo::new(&mut method, false,impl_type).unwrap().unwrap(); | |||
There was a problem hiding this comment.
curious, doesn't look like cargo-fmt acted on these..
| let method_info = ImplItemMethodInfo::new(&mut method, false,impl_type).unwrap().unwrap(); | |
| let method_info = ImplItemMethodInfo::new(&mut method, false, impl_type).unwrap().unwrap(); |
There was a problem hiding this comment.
Hmm, that's interesting. Looks like a bug, wondering if I can create a minimal reproduction.
| pub fn is_ident<T>(&self, val: T) -> T { | ||
| val | ||
| } |
There was a problem hiding this comment.
Once the logic is extended to disallow const generics, lets add a case here.
There was a problem hiding this comment.
Created a separate test case so that we can check the error message (it only reports one at a time, probably because the macro errors work differently from regular compilation errors).
There was a problem hiding this comment.
.. should resolve this issue
There was a problem hiding this comment.
Unfortunately, resolving this issue would require doing the same refactoring in ItemImplInfo, ItemTraitInfo and potentially some other places. I think this is better done in a separate PR.
There was a problem hiding this comment.
Ahh, right! That's unfortunate.. combining errors doesn't apply then, since AttrSigInfo is called per-method signature. But hey, at least we get good spans 😅.
But refactoring the macro system is in order tbh.
| pub fn is_ident<T>(&self, val: T) -> T { | ||
| val | ||
| } |
There was a problem hiding this comment.
.. should resolve this issue
Co-authored-by: Miraculous Owonubi <[email protected]>
Closes #979