[djolertrk]

Populate the type and pointer tables of ethdebug/format/info/resources. The schema's type documents and pointers are modelled as data structures in the ethdebug schema unit, so that what the compiler emits is checked by the type system rather than assembled as JSON.

For every contract, the types of its state variables and of the parameters and return variables of the functions and modifiers compiled into it get an ethdebug/format/type document, keyed by the compiler's type identifier; composed types reference their components by that identifier. Compile-time-only types, such as literals, have no document.

Every state variable in storage and transient storage gets a pointer template derived from the storage layout: packed value types, dynamic and static arrays, the compact bytes/string encoding, structs and mappings, whose keys become the template's expected parameters.

[djolertrk]

cc @nikola-matic @cameel

[cameel]

Here's some initial feedback.

[djolertrk]

@cameel @nikola-matic I also added walnuthq/soldb#181, it is a draft PR in the consumer of these things, that reads the data solc is about to emit.

It covers also covers some extra things (https://github.com/walnuthq/solidity/tree/feature/ethdebug-program-context) such as program context, which will come after this PR here in solidity repo.

[cameel]

Why? Doesn't it make more sense to reuse them too?

[djolertrk]

Well, I tried it, but the function schema doesn't allow it. The parameters has to wrap the tuple itself, not a reference ($defs/Parameters in type/complex/function.schema.yaml). The tuple's components are references though. Anyway, I added that to the doc.

[cameel]

Maybe something to change in the spec upstream? It would be a smaller change than reworking the whole schema so I bet we could get that in pretty quickly.

[djolertrk]

Hmm, let me double check this in the latest spec. It seems that it was addressed recently, so we will need to bump the schema dependency.

[djolertrk]

Yep, bumped the pinned schemas to upstream main and addressed this.

[cameel]

You also don't seem to have any tests that actually show the type description for a function type taking/returning multiple arguments.

[djolertrk]

Yeah, the definitions now register the same tuples too. Also clarified that the outer tuple groups the parameters/returns, even for one item, while tuple valued items stay nested. And the components are referenced by ID.

Added tests in solc and soldb for empty/single/multiple lists, reuse and nesting.

[cameel]

Thanks.

Though actually, I think those top-level tuples are quite redundant and just bloat the output. I mean, we have to have them to comply with the spec, but maybe it would make sense to ask @gnidan to simply allow specifying a list of parameters? We could remove those tuples later if that is addressed.

Especially the fact that we need to explicitly represent an empty parameter list with a dedicated tuple type bothers me a bit. Not a deal-breaker for this PR of course.

[cameel]

Could you report an upstream issue for it?

[djolertrk]

Sure: ethdebug/format#403

[cameel]

I think this will walk all the definitions from an imported file even if the module does not use wildcard imports and intentionally only imports some of them. Is this intentional? I.e. do we actually want to include stuff that may not be used by anything?

[djolertrk]

Not intended. It now takes the functions and modifiers from the contract’s call graphs, so only the ones actually used are included. I updated the test to cover that.

[cameel]

Right now the docs say this:

The table contains a document for the types of the state variables of every contract and of the parameters and return variables of every function and modifier compiled into it, including the ones inherited from base contracts, free functions and the internal functions of libraries.

Can you confirm this is accurate? I.e. that we do not want types from:

• Locals
• Constants and immutables
• Events and errors
• Interfaces
• External/public functions
• Functions that are referenced but not actually called (e.g. only to get a selector or address)?
• Types that are referenced by the contract but not used to declare variables? Contract-scope type definitions, type conversions, types used with new, struct constructors, types used in statements with no effect, using for, arguments of abi.decode(), bytes in bytes.concat(), etc.

What is the exact criterion? I assumed it's about having types for all variable declarations and parameters, but then I'm not entirely sure why parameters of external functions or constants/immutables are not being included.

BTW, I think we should have a test case showing which of these are pulled into the output and which are not.

[djolertrk]

OK, so, the locals, constants and immutables were missing, so I added their types now. Public/external and interface parameters were already there. Also clarified the collection rules and added tests for the cases you listed.

[cameel]

Nice. So now have some clarity on what should be collected.

[cameel]

The PR still seems to be handling/allowing some impossible cases. These should likely be converted to asserts:

• TypeRegistry::definition(): can _declaration.name() really be empty?
  • Same for variable->name() in ethdebug::resources().
• TypeRegistry::canonical(): array slices are an unnamed type. Can they really appear here? You don't seem to have them in TYPE_ID_PATTERNS either so you're either missing a test case or this condition is unnecessary.
• TypeRegistry::sourceRange():
  • Can it really receive a SourceLocation for a file that's not in m_sourceIndices?
  • Is the location ever empty (!hasText())? We do that for error messages sometimes, but I think things in the AST should always have a valid one. Otherwise we may want to think about how to map our corner cases to ethdebug.
• TypeRegistry::document()
  • StateMutability can only be payable or non-payable for an address
  • hasDeclaration() should always be false for function types
• PointerTemplateRegistry::component(): for mappings needsTemplate() must be true
• TemplateReference/Templates: are duplicate names in yields/expect allowed?
• Pointer::Arithmetic: can number of operands be zero?
• Range of _layoutOffset parameter in PointerTemplateRegistry functions.

[cameel]

Somewhat related: is a sanity check possible that would catch us forgetting to add a type/pointer field newly added in the spec? Will conformance tests catch that if the field is optional?

[cameel]

Missing sanity checks in test_ethdebug_schema_conformity.py:

• Some functions are not robust against the spec adding new fields. For example assertPointerIsClosed() hard-codes slot, offset and length as the only fields that need checking, which is true now but might change in the future. Similarly pointer_expression_names() has a list of expressions that may contain references. In such cases the function should have an assert against the full list of known fields, which will fail when a new one is added and force us to make an explicit decision.
• referenced_type_ids(): the check against fields other than id looks like it should be an assert. Otherwise we will silently start skipping references it the spec ever allows extra fields there.
• Things that are enforced by JSON schema do not need to be tested. They should be sanity checks instead. For example the ones in final else of assertPointerIsClosed() look like this. There are more like this.
• region_names(): will not tell us if region names are not unique. We probably can't assert this, but then this should be one of the tests.

[djolertrk]

The PR still seems to be handling/allowing some impossible cases. These should likely be converted to asserts:

• TypeRegistry::definition(): can _declaration.name() really be empty?

  • Same for variable->name() in ethdebug::resources().

• TypeRegistry::canonical(): array slices are an unnamed type. Can they really appear here? You don't seem to have them in TYPE_ID_PATTERNS either so you're either missing a test case or this condition is unnecessary.

• TypeRegistry::sourceRange():

  • Can it really receive a SourceLocation for a file that's not in m_sourceIndices?
  • Is the location ever empty (!hasText())? We do that for error messages sometimes, but I think things in the AST should always have a valid one. Otherwise we may want to think about how to map our corner cases to ethdebug.

• TypeRegistry::document()

  • StateMutability can only be payable or non-payable for an address
  • hasDeclaration() should always be false for function types

• PointerTemplateRegistry::component(): for mappings needsTemplate() must be true

• TemplateReference/Templates: are duplicate names in yields/expect allowed?

• Pointer::Arithmetic: can number of operands be zero?

• Range of _layoutOffset parameter in PointerTemplateRegistry functions.

True and it is addressed. Added the asserts and removed the unreachable branches. The schema allows empty sums/products. We never generate them, so I added an assert for that.
And also, added asserts for repeated keys in yields and names in expect. Two regions can still map to the same name.

[djolertrk]

Somewhat related: is a sanity check possible that would catch us forgetting to add a type/pointer field newly added in the spec? Will conformance tests catch that if the field is optional?

Hmm, no, the conformance tests won’t catch an omitted optional field. I added a list of the known schema fields and a check against the schemas, so adding a field now fails the tests until we review it and update the list.

[djolertrk]

Missing sanity checks in test_ethdebug_schema_conformity.py:

• Some functions are not robust against the spec adding new fields. For example assertPointerIsClosed() hard-codes slot, offset and length as the only fields that need checking, which is true now but might change in the future. Similarly pointer_expression_names() has a list of expressions that may contain references. In such cases the function should have an assert against the full list of known fields, which will fail when a new one is added and force us to make an explicit decision.
• referenced_type_ids(): the check against fields other than id looks like it should be an assert. Otherwise we will silently start skipping references it the spec ever allows extra fields there.
• Things that are enforced by JSON schema do not need to be tested. They should be sanity checks instead. For example the ones in final else of assertPointerIsClosed() look like this. There are more like this.
• region_names(): will not tell us if region names are not unique. We probably can't assert this, but then this should be one of the tests.

Addressed!

[cameel]

A few more asserts:

• PointerTemplateRegistry::takeTemplates():
  • that we're not in the middle of building something (building/pending are empty)
• PointerTemplateRegistry::byteComponent():
  • assert locations it supports (I don't think it can handle all of them)
• PointerTemplateRegistry::collectTypes():
  • that value types are only passed in with storage location (or better, make the location unspecified in that case?)
  • that there are no other types to handle (in else - we want it to fail if a new reference type ever gets introduced)
  • that only possible locations are storage, transient, memory, calldata (not sure about this one; only if we have a way to enumerate them).
• PointerTemplateRegistry::build()
  • that only transient and storage are possible for value types

[cameel]

Hmm, no, the conformance tests won’t catch an omitted optional field. I added a list of the known schema fields and a check against the schemas, so adding a field now fails the tests until we review it and update the list.

I see you did both, a test and a check. TBH I did not expect you to compare the whole JSON structure as it's pretty heavy, though I guess it's also the safest. Let's keep it for now, but it's probably something we'll need to take a closer look at when benchmarking. We should probably be using the plain assert() for such heavy checks, but first we'd have to actually enable debug asserts in our builds. Which is not a task to focus on now.

[djolertrk]

Added the asserts. Value types also need memory/calldata and transient layouts, so those stay allowed.

And, I also left the JSON checks as-is with a benchmarking TODO.

[cameel]

A few remarks on tests:

• pointers_dollar_names.sol: uses $ only in variable names. Why not in type names as well?
• types_enums_aliases_contracts_functions.sol: what it's testing are UDVTs, not aliases. To test aliases you need to create one via an import.
• pointers_packed_values.sol: it does not really let us see how the code handles the packing, because all the types are at the top level. Perhaps it would be better to put them in a struct?
• types_value.sol: what this is testing are not really value types. You're missing contracts, enums, functions, etc. while you include bytes and string (reference types). Unless you meant what the spec calls "elementary" types?

[djolertrk]

Addressed all 4. Thanks!

[cameel]

A few more things:

• pointers_mappings.sol and types_structs_and_mappings.sol: we don't have a case with named key/value in a mapping, do we?
• pointers_dollar_names.sol: now that I think of it, you could also try $ or _$_ to make sure it does not lead to name collisions.
• types_elementary.sol: just noticed that it's not testing contract/interface types. Also, it doesn't look like we'll ever emit a library type, will we?

It would also be good to have more grouping there via common name prefixes like type_discovery, type_structure, pointer_structure, etc. This would make it clearer what we're looking for in each test. And where it makes sense to print whole types and where just showing the list of names is enough (like in discovery tests).

[cameel]

Also a general remark: when checking tests I focused mostly on whether the coverage is adequate. I spot-checked expectations in some cases but TBH it's pretty hard to do when each test includes so many things. Breaking those up would have probably made that easier, because lists and counts at least could be verified at a glance. On the other hand I don't really want to request that at this point, because this would add a lot of churn and would make this already tedious PR even more annoying :) So in the end I hope you verified them in more detail than I did. And a large part of value in these is also seeing later how future PRs affect the output (or making sure they don't).

One thing that would still be helpful though is to make it clear which things are not supposed to appear in expectations. For example in types_variable_declarations.sol its the variables in the unused() function. And in types_expression_references.sol it's everything except the constructor parameter. A short comment would do and make future expectation updates easier even for someone without much context.

[djolertrk]

Added the missing cases, grouped the tests, and commented what should stay out of the output.
And you’re right about libraries, we can’t use them as variable types, so we don’t emit those. Checked the new expectations too.

[cameel]

This gets generated for

abstract contract C {
    function f() public virtual returns (bytes32);
}
interface C {
    function f() external;
}

which means that templates for all locations are generated unconditionally, even when not needed. Is that intentional?

[djolertrk]

Yeah, that was too eager. It now only emits layouts actually used, while keeping the type info for interfaces and abstract declarations. Updated the tests and docs too.

[cameel]

This should be available as a standalone script so that we can easily regenerate schema_fields.json.

In fact, what test_known_schema_fields() does is not really a test against compiler's input but a prerequisite for the code and tests to be even valid. It could be an assert somewhere during setup, though as standalone script it could also be executed directly in CI before the test suite runs.

[djolertrk]

Right, added a script to regenerate schema_fields.json. The check now runs in setup, before compiling anything.

[cameel]

Here's the final batch of stuff. Mostly nitpicks at this point, though I'd really prefer to have the naming fixed, because that made understanding the code much harder than it should have been and it's been bothering me a lot.

[cameel]

These are hard to distinguish without context:

• produce() vs build() - produce/build what? They sound like the same thing.
• m_building vs m_pending - building/pending what? Also sound kinda the same.

And you could really use some nouns. Omitting details like this makes code hard read quickly and out of order (which is what I do all the time when reviewing):

• Built -> PointerDraft? TemplateDraft? Misleading. It's not build, it's being built, right?
• Built::expect -> expectedRegions?
• Built::produced -> producedRegions?
  • Similarly for _produced and produce()

And these really need a verb: byteComponent(), component(), elementList(), bytesOrString(), dynamicArray(). Are they building things? How do they differ from buildX functions? Names like this are IMO only appropriate for variables and very light getters. Anything heavier should make it clear that it's a significant part of the implementation and not just a very thin helper/wrapper.

Similar points apply to some locals and parameters. For example these from PointerTemplateRegistry::byteComponent() omit important details that are needed for comfortable reading of code:

• nested: nested what?
• _head -> _headAddress?
• _base -> _abiOffsetBase?

[cameel]

using Expression is also very confusing in context where we also have AST nodes in scope. Actually, in any scope, I associate it pretty strongly with AST. If the schema struct has to use the same name, it should always be qualified.

Another case are the pointer helpers which use very short names such as read(), sum(), keccak256(). Perhaps they should be namespaced. Though they themselves seem to be a workaround for excessive use of namespacing elsewhere. If we could directly use e.g. the Keccak256 struct in the current scope, that would be much less ambiguous.

[cameel]

BTW, shouldn't produced be a set? Is the order relevant?

[djolertrk]

OK, I renamed the builders, state and locals, qualified Pointer::Expression, and namespaced the helpers.
Also, the producedRegions is now a set, and the order didn’t matter. And, used expectedParameters for expect, since it lists parameters, not regions.

[cameel]

This class shares a lot of naming problems with EthdebugPointerTemplateRegistry. For example these read like simple getters. More annoying since e.g. wrapper() sounds like something very ignorable while it's really how the main registerType() is called recursively. registerAndWrapType() is the information I really need to see there.

Also, the fact that functions like document() are just nouns makes naming locals harder. I see that you have to resort to using this-> to disambiguate. If you have to do this, I'd say the name is clearly bad.

registerType() itself would be better off as registerTypeDecomposition() to emphasize that it's a deep walk rather than a simple one-off operation.

Locals with too little context:

• TypeRegistry::sourceRange(): it -> sourceIndexIt
• TypeRegistry::registerType(): id -> typeIdentifier
• _type vs type in the same scope: one is canonical the other is not. Some distinguishing info should be in the name.

[djolertrk]

Hmm ok, renamed these.

[cameel]

Some things here:

• pointer_expression_names() -> extract_names_referenced_by_pointer_expression()
• region_names_and_duplicates() -> extract_defined_region_names() (it's more important how these names differ from the ones from the other function returning names (defined vs referenced); the fact that it returns duplicates is better conveyed through return variable name)
• composed -> composite

[cameel]

Also:

• schema_fields.json -> known_schema_fields.json or maybe schema_field.lock.json?

[djolertrk]

OK. Renamed the helpers and return variables, changed composed to composite, and went with known_schema_fields.json. And updated the generator and README as well.

[cameel]

Finally, some minor style rules we usually follow for readability:

• Explicit conditions (only == nullptr is an exception for historical reasons):
  • thing.has_value() over thing for optionals.
  • container.contains() or container.count() != 0 over container.count().
  • Also (especially) in Python: assert len(PROGRAM_OUTPUTS) != 0 over assert PROGRAM_OUTPUTS. It just does not read like a condition at all.
• Wrap ifs with a variable declaration on ; to make the condition visible.
• POTS fields that do not need an initializer should still be initialized with {} just in case, so that they have a deterministic value if left uninitialized by mistake.
• Use our helpers where available:
  • Instead of a fake return after solAssert() when the C++ compiler can't figure out it's unreachable, use util::unreachable().
  • We have type() as a shorthand for Type::annotation().type. For this reason it's also best to avoid using type to name variables.
• When wrapping parameters/enums/dicts, put each entry on its own line.
• Omit default in switch if possible (see your functions handling DataLocation)
• Drop namespace prefixes for our own modules (util::, langutil::, etc.) by adding using namespace ... in the file.
  • Also: solidity::test::CommonOptions::get().evmVersion() -> CommonOptions::get().evmVersion()
• Use dangling commas wherever C++ allows them to minimize noise in diffs.
• Prefer /// over /** */ for docstrings.
• Put starting and ending """ in Python docstrings on a new line.
• Avoid declaring a lambda and immediately calling it in place.
• Use one sentence per line style in RST/Markdown (test/ethdebugSchemaTests/README.md)
• Indent JSON with 4 spaces to make nesting more readable. We never use 2 spaces for anything unless we are forced to (arrrgh, YAML).
• Always parenthesize tuples (e.g. no names, duplicates = set(), set()). for loops are fine as an exception.

Sorry for a big batch. I did not want to bother you with trivial stuff like this earlier but this means it all accumulated. I really need to prepare a doc listing those so that I don't have to constantly point them out in PRs. It's unfortunate that they're not clear from context, because old code is messy and often does not adhere to it.

[djolertrk]

OK. I addressed this. TBH, it was not clear to me, neither from the rest of the code, nor from docs. But will remember this locally at least, so I can follow this from now on.

[cameel]

Going through it again I found some new things in the code that was added/changed since my last pass, but nothing major. Normally I'd prefer to keep iterating on it in the PR since this keeps a clear boundary between code that is done and one that still needs more work, but this PR is so big that this workflow no longer works well. A quick review is no longer possible and it seems like it might actually be easier to have it all go in and try to improve parts of if afterwards. The big important issues are all resolved at this point at least.

So let's do that. I'm going to approve and merge it as is and give you my feedback elsewhere. Some directly, some in follow-up PRs if when they touch the relevant bits.