Report all `#[error]` and `#[event]` field problems in record field analysis

Open
  • Is something wrong?
#1,624 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
52/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
rust
Domain
blockchain, compilers

Research direction

Start with crates/hir/src/analysis/ty/abi_record_fields.rs and the policy handling in crates/hir/src/analysis/ty/mod.rs; compare those checks with signature construction in crates/hir/src/core/lower/event.rs. Run the three programs described in the issue and inspect the existing event fixtures. Done means each field problem is reported once, generated TOPIC0, SELECTOR, and emit add no duplicate or missing diagnostics, and failing constants behave consistently for msg, error, and event records.

Written by the indexing model from the issue text.

Description

After #1589, record field analysis (crates/hir/src/analysis/ty/abi_record_fields.rs) owns the field problems of generated ABI records: it reports one diagnostic per field, and the generated LAYOUT, HEAD_SIZE and IS_DYNAMIC are checked only when every field passes (AssocConstBodyCheckPolicy::AbiRecordFields(kind)). Three field problems of #[error] and #[event] structs are still reported somewhere else, late, or not at all.

1. ExpansionSourceCompatibility hides a missing SolCompat

AssocConstBodyCheckPolicy::ExpansionSourceCompatibility has two users left. Both come from the shared signature builder create_sol_signature_const (crates/hir/src/core/lower/event.rs): the TOPIC0 of an #[event] with fields, and the SELECTOR of an #[error]. Body analysis checks these bodies and then drops their type errors. Its doc comment calls it a compatibility exception.

A field type without SolCompat passes fe check, on master and after #1589:

use core::abi::{Abi, AbiSize, Encode, store_word}
struct Bad { word: u256 }
impl AbiSize for Bad {
    const HEAD_SIZE: u256 = 32
    const IS_DYNAMIC: bool = false
}
impl<A: Abi> Encode<A> for Bad {
    fn encode(own self, _ ptr: *u8) { store_word(ptr: ptr, value: self.word) }
}
#[event]
struct Ev { value: Bad }
#[error]
struct Er { value: Bad }

fe check prints nothing. Emitting Ev in a test fails only in MIR lowering: semantic constant TOPIC0 referenced from Expr(ExprId(25)) failed CTFE: Ctfe(InvalidBody { .. }). The missing <Bad as SolCompat>::SOL_TYPE is a type error in the body of TOPIC0, and the policy drops it. No other analysis reports it.

2. A failing constant in a field type gives an extra error

type Buf<const N: usize> = [u8; { 10 / N }]

#[error]
struct E { a: Buf<0> }

#[event]
struct Ev { a: Buf<0> }

msg M {
    #[selector = 1]
    V { a: Buf<0> },
}

The division by zero is reported once, at the constant (3-0025). The msg variant adds nothing. But E and Ev each also give 8-0031 type annotation is needed at the whole struct, with a hint about dynamic_payload_size<_>. Record field analysis turns an invalid constant into a field issue only for msg (abi_record_fields.rs, the AbiTypeError::InvalidConst check under kind == MsgVariant), so for errors and events the generated bodies are still checked and fail again.

3. A failing data field hides indexed field errors

struct Foo { x: u256 }
#[event]
struct Ev {
    #[indexed]
    a: Foo,
    b: Foo,
}

After #1589 this reports only b (10-0007 unsupported ABI field type). Master also reported that Foo does not implement TopicValue. When a data field fails, every generated function of the event is skipped (crates/hir/src/analysis/ty/mod.rs), including emit, and emit is the only place that reports a missing TopicValue on an indexed field. The user sees the second error only after fixing the first. The comment in abi_record_field_positions.fe ("the generated emit reports it") holds only when the data fields pass.

Proposed fix

  1. Record field analysis requires SolCompat on every #[error] field and every #[event] field, indexed or not, since the signature names them all.
  2. It also takes over the indexed field rules: TopicValue on indexed fields, and the check that indexed fields are not dynamic (today done by type name through is_dynamic_event_ty).
  3. It reports an invalid constant in a field type as a field issue for every kind, not only msg.
  4. TOPIC0 and SELECTOR use AbiRecordFields(kind), and ExpansionSourceCompatibility is deleted.
  5. Fixtures for the three programs above: one located error per field problem, nothing from TOPIC0, SELECTOR or the generated emit, and a msg, #[error] and #[event] case for the failing constant.

Why it was not part of #1589

Steps 1 and 2 make record field analysis own the rules for indexed event fields, which it does not cover today. Those rules have two other owners now: the generated emit reports a missing TopicValue, and event lowering reports dynamic indexed fields. Moving them changes event_shadowed_address_type.snap and event_indexed_static_array_alias.snap, and deserves its own review. Problem 1 is also a bug on master, not something #1589 introduced. Problems 2 and 3 are smaller than on master (master gave four errors for the second program), but they are still noise or delay.

Dominant language
Rust
Stars
1.7k
Forks
219
Avg merge
1d 3m
Merged PRs (30d)
162

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from argotorg/fe

All issues in argotorg/fe

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.