Disarm fails when instrumented code fails to compile

[wolflo]

If you have a contract that compiles before being instrumented but fails to compile after being instrumented, scribble will successfully arm the contract but fail to disarm it. This can leave a bit of a mess of armed files to clean up before you can fix or diagnose the issue.

Example

Shadowed.sol

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

interface IShadower {
    function shadowed(uint256 a) external;
    function shadowed(uint256 a, uint256 b) external;
}

/// #invariant {:msg "I0"} 0 == 0;
contract Scribbled {
    function entry() external {
        IShadower(address(0)).shadowed(1);
    }
}

solc Shadowed.sol

Succeeds.

scribble Shadowed.sol --output-mode files --arm

Succeeds, but leaves an instrumented Shadowed.sol that fails to compile.

In this case, it is because the function pointer passed to the _callsite_ method cannot discern between the two versions of IShadower.shadowed(). As far as I know, this is a limitation of solidity rather than a scribble bug, but used here as an example.

scribble Shadowed.sol --output-mode files --disarm

Because the newly instrumented Shadowed.sol won't compile, this fails and leaves armed files that must be sorted out manually or reverted to a previous commit.

Feature Request

• If it's not necessary to compile the armed contracts before disarming, that seems like an easy fix. Maybe this could be behind a --disarm --force flag.
• Attempt to compile the armed contracts and revert instrumenting if they fail to compile.

[wolflo]

An issue related to the reason the instrumented version of this contract won't compile: ethereum/solidity#12296

[cd1m0]

Thanks for the report @wolflo . Yeah, this needs to be fixed. The tricky bit is making sure we don't miss any files. This can be a problem, as in certain cases (e.g. when there are contract-wide invariants) scribble may instrument contracts even under node_modules/. A semi-clean solution would be for scribble to leave enough "bread crubms" during instrumentation. One thing that can serve the role of "bread crumbs" is the instrumentation metadata json file. Currently its only optionally emited using the --instrumentation-metadata-file command line option. So one way to achieve the desired result might be:

1. Scribble always emits an instrumentation metadata file with --arm. By default its in some location (e.g. .instrumentation.md.json, and users can optionally change that location with --instrumentation-metadata-file).

2. When dis-arming scribble looks for the instrumentation metadata in its default location (or a user can override this with --instrumentation-metadata-file), and uses the data in that to find the modified files

The only risk with this solution is that the metadata file somehow gets out-of-sync with the instrumented files in the repo

[JoranHonig]

@cd1m0 maybe we can include a series of comments on top of instrumented files indicating that they're auto generated and that they shouldn't be edited directly (only after it has been disarmed).

[cd1m0]

@cd1m0 maybe we can include a series of comments on top of instrumented files indicating that they're auto generated and that they shouldn't be edited directly (only after it has been disarmed).

I think I expressed myself incorrectly - I was worried about .original files being modified somehow? Or left over from an earlier arming and overwriting new code with stale code? This shouldn't happen under normal user patterns, just trying to think of edge cases.

[wolflo]

What is the reason for compiling the armed contracts before disarming?

[cd1m0]

After discussion with @blitz-1306 we decided:

0. Add a special marker comment to instrumented files saying "this is a scribble instrumented file. Don't touch it .."

1. We will require --instrumentation-metadata-file to be specified when we do --arm and we can't detect a project root. Project root is detected by scanning the tree up from the least common ancestor of all the target files/directories, looking for signs of project root: 'package.json', 'hardhat.config.ts', 'truffle-config.json', and whatever brownie does

2. When instrumenting with --arm, we will FIRST check if there already exists an instrumentation metadata file (as specified by the above algorithm), and NEXT we will check if the files in that instrumentation metadata files are still instrumented. The way we check if files in the instrumentation metadata file are instrumented is by looking for the marker specified in 0. (TODO: Decide if we should also check if .instrumented and .original files exists with the same base name. Decide what to do in this case). If instrumented files are detected, error should be thrown saying that the person should disarm first.

[blitz-1306]

This would require a fix in solc-typed-ast to respect StructuredDocumentation nodes in SourceUnit children during AST-to-source writing. Currently they are ignored.

[blitz-1306]

@wolflo

What is the reason for compiling the armed contracts before disarming?

I would say major reason is (and always been a big pain) an import resolution. We can resolve imports by relying on Solc callbacks while compiling files (unless breadcrumbs idea, suggested by @cd1m0). There are a lot of effort done to make that work:

• https://github.com/ConsenSys/solc-typed-ast/blob/daa2922c9fac23b97987c862adc424316f75a3c1/src/compile/utils.ts#L218-L249
• https://github.com/ConsenSys/solc-typed-ast/blob/daa2922c9fac23b97987c862adc424316f75a3c1/src/compile/utils.ts#L356-L371
• https://github.com/ConsenSys/solc-typed-ast/blob/daa2922c9fac23b97987c862adc424316f75a3c1/src/compile/utils.ts#L309-L310