Reject zero type when declare event fields #774
Conversation
vuvoth
changed the title
| @@ -40,7 +40,17 @@ pub fn event_type(db: &dyn AnalyzerDb, event: EventId) -> Analysis<Rc<types::Eve | |||
| } = &field.kind; | |||
|
|
|||
| let typ = type_desc(&mut scope, typ_node).and_then(|typ| match typ { | |||
| typ if typ.has_fixed_size(scope.db()) => Ok(typ), | |||
| typ if typ.has_fixed_size(scope.db()) => { | |||
There was a problem hiding this comment.
There is no need to reject all zero-sized fields. It'd be better to reject a field iff its type is zero-sized and it is indexed.
| Type::Base(Base::Unit) => true, | ||
| Type::Contract(inner) => inner.fields(db).is_empty(), | ||
| Type::Struct(inner) => inner.fields(db).is_empty(), | ||
| _ => false, |
There was a problem hiding this comment.
contract/struct can be zero-sized even if their fields are not empty.
Example.
struct Zero {
x: ()
}
Also, other types can be zero-sized.
Tuplebecomes a zero-sized type if it has no elements or all elements are zero-sized type.Arraybecomes a zero-sized type if its length is 0 or its element type is a zero-sized type.
There was a problem hiding this comment.
I create salsa query is_zero_sized to handle it. I think about this should be type_size will more general.
| true | ||
| } | ||
| Type::Contract(contract_id) => { | ||
| for field_type_id in db.contract_all_fields(contract_id).iter() { |
There was a problem hiding this comment.
Logic to handle struct and contract very similar. Have any better way to refactor it....
There was a problem hiding this comment.
A contract type is really just an address (its size is that of the address type), so you don't need to look at the fields.
There was a problem hiding this comment.
A contract type is really just an address (its size is that of the
addresstype), so you don't need to look at the fields.
Thanks! I fixed it
| } | ||
| true | ||
| } | ||
| Type::Generic(_) | Type::Map(_) | Type::SelfContract(_) => false, |
There was a problem hiding this comment.
Is Generic type always have size > 0?
There was a problem hiding this comment.
It could be zero-sized or not, depending on how its used. For example:
event Foo<T> {
idx val: T
}
pub fn emit_foo<T>(ctx: Context, val: T) {
emit Foo(val)
}
emit_foo(ctx, 10) // ok
emit_foo(ctx, ()) // not ok, T is zero-sized
For now, it's safe to use unreachable!() for this case, because we don't support generic fields in structs or events yet. When we do, we'll have to determine the size of the concrete type.
(Map and SelfContract can also be unreachable!(), as they can't appear in a field.)
There was a problem hiding this comment.
I think better we should throw fe error in this case. Use unreachable!() maybe non sense I think.
There was a problem hiding this comment.
I don't think returning Error here is a good option since using errors to report/hide internal bugs is not a good practice. Entering the Type::Generic arm means ICE in the current state, so unreachable!() would be suitable here.
vuvoth
force-pushed
the
zero-typed-in-event
branch
from
3687f31
to
3e5f7a5
Compare
vuvoth
force-pushed
the
zero-typed-in-event
branch
from
becc1a0
to
382d3a0
Compare
| } | ||
| true | ||
| } | ||
| Type::String(FeString { max_size }) => max_size == 0, |
There was a problem hiding this comment.
As long as we continue using solidity ABI, String never becomes a zero-sized type even if its max_length is 0.
Also, we allocate 256 bits to store a string length, so the minimum String size is 32 bytes even in the Fe's string representation.
| pub fn is_zero_sized_cycle(_db: &dyn AnalyzerDb, _cycle: &[String], _ty: &types::TypeId) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
This feels like a little bit dirty hack. I think it'd be better to detect a struct cycle in type_desc, but I'm not sure.
Co-authored-by: Yoshitomo Nakanishi <yurayura.rounin.3@gmail.com>
What was wrong?
#712
How was it fixed?
Add salsa db query
is_zero_typeand check declare field data in event should not zero-sizedSpecial thank to @Y-Nak!
To-Do
OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history