Properly support `UserDefinedType`s to Scribble functions

[zain335]

Currently Scribble doesn't support User Defined types for Scribble functions arguments.
It would be great if you could add this feature.

[blitz-1306]

Hello there. Could you come with some example to clearly understand what do you expect to be supported syntactically? Thanks.

[zain335]

here is my code snippet

/// #define isPolicyValid(Policy policy) bool = <expression>;
contract Model{
struct Policy{
        bytes32 dn;
        uint exptime;
        uint vfee;
        <...>
    }
<...>
}

while running scribble i'm getting this error.

/usr/local/lib/node_modules/eth-scribble/dist/bin/scribble.js:523
        throw e;
        ^
Error: Couldn't find Policy
    at makeUserDefinedType (/usr/local/lib/node_modules/eth-scribble/dist/spec-lang/expr_parser.js:33:15)
    . . .

On discord, I spoke with @norhh, and he confirmed that currently the scribble function doesn't accept user-defined argument types and asked me to create feature request here.

[blitz-1306]

Well, I've investigated this case. First, I found that the basic sample, that you mentioning is more look like a bug in Scribble behavior, than a feature. Consider following sample:

/// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

/**
 * #define isPolicyValid(Policy policy) bool = policy.exptime > 0 && policy.vfee > 0;
 */
contract Feature181 {
    struct Policy {
        bytes32 dn;
        uint exptime;
        uint vfee;
    }
}

scribble/dist/spec-lang/expr_parser.js:33
        throw new Error(`Couldn't find ${name}`);
              ^

Error: Couldn't find Policy
    at makeUserDefinedType (scribble/dist/spec-lang/expr_parser.js:33:15)
    at peg$c301 (scribble/dist/spec-lang/expr_parser.js:693:16)
    at peg$parseUserDefinedType (scribble/dist/spec-lang/expr_parser.js:8813:22)
    at peg$parseSimpleType (scribble/dist/spec-lang/expr_parser.js:8544:38)
    at peg$parseArrayType (scribble/dist/spec-lang/expr_parser.js:8829:14)
    at peg$parseMappingType (scribble/dist/spec-lang/expr_parser.js:9101:18)
    at peg$parsePointerType (scribble/dist/spec-lang/expr_parser.js:9133:14)
    at peg$parseFunctionType (scribble/dist/spec-lang/expr_parser.js:9578:18)
    at peg$parseTypedArgs (scribble/dist/spec-lang/expr_parser.js:2448:14)
    at peg$parseUserFunctionDefinition (scribble/dist/spec-lang/expr_parser.js:2768:46)

---

However, if we try to be a bit more specific, then we can get it at least recognized:

/// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

/**
 * #define isPolicyValid(Feature181.Policy policy) bool = policy.exptime > 0 && policy.vfee > 0;
 */
contract Feature181 {
    struct Policy {
        bytes32 dn;
        uint exptime;
        uint vfee;
    }
}

../bug.sol:5:58 TypeError: Expression policy of type struct Feature181.Policy doesn't have a field exptime
../bug.sol:5:58:
 * #define isPolicyValid(Feature181.Policy policy) bool = policy.exptime > 0 && policy.vfee > 0;
                                                          ^^^^^^

Well this ^ is actually weird, because struct is correctly referenced and it has the mentioned fields in AST. Digging deeper showed that the issue is caused by not specifying memory location in-place (this is non-obvious due to confusing error message).

---

So the workaround sample would look like this:

/// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

/**
 * #define isPolicyValid(Feature181.Policy memory policy) bool = policy.exptime > 0 && policy.vfee > 0;
 */
contract Feature181 {
    struct Policy {
        bytes32 dn;
        uint exptime;
        uint vfee;
    }
}

It would result following instrumentation:

/// This file is auto-generated by Scribble and shouldn't be edited directly.
/// Use --disarm prior to make any changes.
pragma solidity 0.8.13;

///  #define isPolicyValid(Feature181.Policy memory policy) bool = policy.exptime > 0 && policy.vfee > 0;
contract Feature181 {
    struct Policy {
        bytes32 dn;
        uint exptime;
        uint vfee;
    }

    /// Implementation of user function define isPolicyValid(struct Feature181.Policy memory policy) bool = ((policy.exptime > 0) && (policy.vfee > 0))
    function isPolicyValid(Feature181.Policy memory policy) internal view returns (bool) {
        unchecked {
            return (policy.exptime > 0) && (policy.vfee > 0);
        }
    }
}

/// Utility contract holding a stack counter
contract __scribble_ReentrancyUtils {
    event AssertionFailed(string message);

    event AssertionFailedData(int eventId, bytes encodingData);

    bool __scribble_out_of_contract = true;
}

[blitz-1306]

@zain335 Basically, the feature is already there. However we still have caveats, that are causing confusion:

1. By some reason, user is unable to use unqualified names. This is probably related to a flaw in name / overloading resolution logic.
2. We have confusive error message, stating that struct definition is there, but it does not have a necessary fields. We should be more specific here and state that user is missing memory location specifier.

I guess we can fix this when we will have capacity.

@cd1m0 FYI, consider samples above.

[blitz-1306]

It appears that turning inclusive flag for resolveAny() here fixes the inability to use unqualified names:

https://github.com/ConsenSys/scribble/blob/57cda93113f1d6f18d552890ffd707f436414e86/src/spec-lang/expr_parser_header.ts#L95

Adding true as the fourth argument and rebuilding parser via npm run build-parsers makes the original sample work properly. But I guess we should check implications of this.

[zain335]

@blitz-1306 Thank you so much for your effort. I really appreciate it.

[blitz-1306]

@zain335 Thank you for the contribution.

[blitz-1306]

I also figured out that error message

Expression policy of type struct Feature181.Policy doesn't have a field exptime

Comes from here:

https://github.com/ConsenSys/scribble/blob/57cda93113f1d6f18d552890ffd707f436414e86/src/spec-lang/tc/typecheck.ts#L1816-L1820

Maybe we should be more clear in case if user specified UserDefinedType while there should be PointerType.

[blitz-1306]

You can track PR with fixes here: #184

[cd1m0]

This got closed by the first PR. Re-opening to track fixing the error message to be more descriptive when a user forgets to specify a memory location for a reference solidity type.