Type checking for contract explicit storage base location

[matheusaaguiar]

• depends on Parser support for explicit storage locations #15463.

[cameel]

Two main issues: layout on interfaces is not disallowed and also the issue unnecessarily restricts that a contract can only be inherited from once when there's a layout.

I also think that it would be good to move checks to parts of analysis other than TypeChecker. It seems perfectly feasible for this particular feature and TypeChecker is already a beast.

Other than that the usual stuff. Especially tests.

[cameel]

This is a pretty bad name :)

Also we're inside the solidity namespace here.

Suggested change

	solidity::u256 value;
	u256 baseSlot;

[cameel]

Also, you should make it SetOnce.

[cameel]

What about interfaces?

[cameel]

This is a perfectly valid case.

The derivedContractSpecifyingStorageLayout annotation seems to have been added just to check for it. You don't need it or the check. A contract that's not at the top-level of the hierarchy can be a part of multiple layouts.

[cameel]

Suggested change

	"The storage layout must be specified by an expression that can be evaluated at compilation time."
	"The base slot of the storage layout must be a compile-time constant expression."

[cameel]

Suggested change

	auto const* expressionType = type(*baseLocation);
	BoolResult result = expressionType->isImplicitlyConvertibleTo(*TypeProvider::uint256());
	auto const* baseSlotExpressionType = type(*baseLocation);
	BoolResult convertibleToUint = expressionType->isImplicitlyConvertibleTo(*TypeProvider::uint256());

[cameel]

IMO you should check this first, then fractionals, then range. Finally just assert that the value is convertible to uint and evaluate the value. This will make the message in most weird cases very clear (it's not a number literal). Currently you get a message about exceeding uint range in many weird cases, which suboptimal.

Later, when we open the functionality up to non-literals, we'll replace the assert with a message saying that type X is not implicitly convertible, which should also be clear.

[cameel]

Note that u256() conversion just truncates and does not throw when the value is out of range. You only indirectly check that it's in range, so you should also assert that it really is.

Also assert that denominator is in fact 1.

[cameel]

These checks would probably be better off in PostTypeChecker/PostTypeContractLevelChecker. TypeChecker is a beast already (4k+ lines) and we seem to be dumping everything in it :)

The checks and the annotation do not seem to be things that other checks will need to depend on so it should be fairly straightforward to move them. To the contrary - they use isPure annotation, which is fully filled out only after TypeChecker. We're in endVisit() so it should already be set on the expression, but still, it would be cleaner if all of it was not crammed into the same analysis pass.

[cameel]

Similarly, verifying that base contracts do not have layout specifiers seems like something that can be checked already by ContractLevelChecker and does not have to wait until we know the types.

[cameel]

This should be changed back.

[cameel]

Needs more tests. There's quite a few, but note that many of these will be simple one-liners and you can usually drop everything from one bullet point into one file because errors are not fatal.

The most necessary minimum:

• Numbers with denominations (42 weeks, 666 gwei)
• Rational numbers with zero fractional part (42.0, 2.5e10)
• Interface with a layout
• layout at 1 / 0
• layout at -1

Scoping

• Constants defined in inherited contracts
• State variables defined in inherited contracts
• Things defined in foreign contracts

Inheritance

• Contract with a layout inheriting from an abstract contract/interface
• Contract with a layout inheriting from itself
• Inheriting from multiple contracts where first/last already has layout

Things that are now rejected but will have to work eventually so we may as well cover them already:

• type(uint).max
• uint(42)
• uint40(bytes5(hex"0011223344"))
• ~uint(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
• [1, 2, 3][0]
• string("ABCD").length
• abi.decode(abi.encode(42), (uint))
• uint(bytes32(type(I).interfaceId))
• uint32(f.selector)
• uint64(bytes8(bytes.concat("ABCD", "EFGH")))
• uint(keccak256(bytes.concat("ABCD"))
• uint8(b[1]), where b is bytes32 constant b
• N / ~N where N is a constant.
• Ternary operator
• T.unwrap()

Wild things that should not work:

• N / 0 where N is a constant
• Expressions that would overflow in checked arithmetic
• Expressions using custom operators defined on uint
• Various non-uint literals: address, hex string, string, boolean, enum, [1, 2, 3], (1, 2, 3)
• ++2, 1 = 2, delete 2
• T.wrap()
• f.selector, f.address
• Builtins that depend on state: blockhash(), block.chainid, block.number, msg.value, tx.gasprice, <address>.balance, etc. Especially those that return uint.
• Precompiles returning uint: addmod() etc.
• Bound pure library function called on a literal (1.f())
• Weird stuff: error, event, module, contract, type, magic, etc.
• address(new C())
• uint160(address(this))