Handle more SCALE attributes: skip, index #44
Conversation
By default, parity-scale-codec does not provide Encode/Decode impls for an owned String. This is only provided under the "full" feature which is not used by the substrate runtime, because it should not be used for consensus critical code. So in order for the CompactForm to be integrated into the substrate runtime, or wherever the "full" feature cannot be used, then we must parameterize the `String` type so that it can be both an `&'static str` on the runtime side where it is encoded, and a `String` in client/consuming code where it is decoded.
Add a `compact` member to `Field` to indicate it is `scale_codec::Compact`
…ncoded/decoded as a SCALE Compact type
Handle `codec(skip)` and `codec(index = $int)` attributes
Ensure we're working with an outer attribute
Co-authored-by: Andrew Jones <[email protected]>
Better error message Remove redundant tests Test more enum cases to document index/discriminant interaction
| @@ -16,4 +16,4 @@ error: proc-macro derive panicked | |||
| 11 | #[derive(TypeInfo, Encode)] | |||
| | ^^^^^^^^ | |||
| | | |||
| = help: message: Internal error, parse_meta must have been checked: Error("expected literal") | |||
| = help: message: Internal error. `#[codec(…)]` attributes must be checked in `parity-scale-codec`. This is a bug.: Error("expected literal") | |||
There was a problem hiding this comment.
I still don't 100% get this - the errors above were generated by parity-scale-codec so the check appears to have happened correctly - so this error is not necessary?
There was a problem hiding this comment.
Right, it’s there to chastise sloppy developers and should never reach users. It means that the attribute syntax checks never happened or are buggy; the derive author has to explicitly make a method call so there’s nothing stopping human error.
We could unwrap here but that’s not better.
The trybuild is here as a guard against buggy parity-scale-codec releases.
There was a problem hiding this comment.
should never reach users
Forgive me 🙈 , but doesn't this trybuild demonstrate that this error does in fact reach the user even though the syntax checks did happen?
There was a problem hiding this comment.
You are right and I am wrong. First of all I'm heaping on a bunch of different syntax errors in the same enum which makes it hard to understand what is going on. If I use a single-syntax error at a time it's easier to follow.
#[derive(Encode, TypeInfo)]
enum EnumAttrsValidation {
#[codec(index = a)]
Thing(u32),
}Gives this error:
error: proc-macro derive panicked
--> $DIR/fail_with_invalid_codec_attrs.rs:17:18
|
17 | #[derive(Encode, TypeInfo)]
| ^^^^^^^^
|
= help: message: Internal error. `#[codec(…)]` attributes must be checked in `parity-scale-codec`. This is a bug.: Error("expected literal")
error: expected literal
--> $DIR/fail_with_invalid_codec_attrs.rs:19:21
|
19 | #[codec(index = a)]
| ^
So parity-scale-codec is giving the proper error message back, whereas scale-info just panics. What I was trying to say with this test is that scale-info will not try to check attributes from parity-scale-codec.
We have no way of knowing if the Encode macro ran or how it went, so we can either panic or re-implement their attribute checking and print the proper error twice.
I think we should panic but my error message must change. How about: scale-info: Bad index in `#[codec(index = …)]`: Error("expected literal").
Wdyt?
There was a problem hiding this comment.
Cool, maybe could mention somehow to see the other error generated by parity-scale-codec.
|
@Robbepop This one should be pretty non-controversial. |
Handle
#[codec(skip)]and#[codec(index = $int)](on variants) and use the utility code copied fromparity-scale-codec-derive.In
parity-scale-codec-derivethere is a fair amount of validation made on attributes being in the right place. This PR does not include those checks – I assume that a type containing any of these attributes will deriveEncodeand so run the validation so duplicating them seems dumb.Builds on #42Closes #43
Closes #45