eof: Contract creation fix and tests.

[rodiazet]

• Fix wrong container ID calculation in case of unreferenced container before the referenced one.
• Add test
• Small refactor of ContainerID usage

[github-actions]

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

[cameel]

i will overflow and the loop will never stop when there are 256 sections. You need a bigger type to iterate over sections:

Suggested change

	for (uint8_t i = 0; i < static_cast<ContainerID>(m_subs.size()); ++i)
	for (size_t i = 0; i < m_subs.size(); ++i)

Would be best if we could have this corner case covered with a test. Maybe one with empty subobjects to keep it compact?

[rodiazet]

Sure. Shit I assumed for some reason that 0x100 == 100 👯

[cameel]

This really makes me question whether not investing more time into tests at this stage is a wise decision. Ultimately mistakes like this should be caught by tests. We're rushing towards semantic tests, but some cases like this one definitely would not be caught by them. And these smaller PRs are the only place to consider corner cases in detail. Once it's all in, no one will have time to go back through the whole backend and figure out where extra tests are needed.

[rodiazet]

Adding tests too.

[rodiazet]

This reminds me also to add proper error to AsmAnalysis covering the case when we have more then 256 subcontainers.

[rodiazet]

Done

[cameel]

Maybe we should be removing unreferenced containers somewhere before assembling after all. It's actually a bit weird that we show them in assembly but then they're missing from the bytecode. It makes the assembly less reliable as way for users to inspect what is being generated (and for me to verify that the expectations are actually correct because the change is only in the binary :)).

Of course it's not something to address in this PR, but maybe something to think about. The problem is actually similar to removing unreferenced functions from CFG in a way.

[rodiazet]

I would also remove the subcontainers earlier. On CFG level ideally.

[cameel]

Looks good, I'd just tweak one test (see below).

Also, the PR now does quite a bit more than just fix the IDs - it also adds a new validation and fixes the infinite loop bug. If this wasn't experimental code, each of those would get a separate changelog entry so I think at least putting them in separate commits is warranted. The commit description no longer accurately reflects what's inside.