Instrumentation appears to be breaking constants

[davesag]

I have a library as follows

pragma solidity ^0.4.18;

import 'zeppelin-solidity/contracts/token/ERC20.sol';


library KeyUtils {
    address constant private KEY_ADDRESS = 0x0;

    function getKEY() internal pure returns (ERC20) {
        return ERC20(KEY_ADDRESS);
    }
}

and normal tests of this run fine.

But when I run with solidity-coverage I get this error

/Users/davesag/src/myawesomeproject/coverageEnv/contracts/utils/KeyUtils.sol:13:5: TypeError: Library cannot have non-constant state variables
    address private KEY_ADDRESS = 0x0;
    ^-------------------------------^
Compilation failed. See above.

Somehow the instrumentation process appears to have removed the constant keyword.

[area]

Ah, interesting. Yes, that is deliberate, as functions declared as constant / pure / view can't have events in them, which we use for instrumentation. So we strip those out so that the contract will compile with our instrumentation, but we hadn't appreciated that there was this use case for constant.

[cgewecke]

Should we stop supporting constant as a function modifier in 4.0? Looks deprecated in the docs, I.e doesn't appear as an option.

[area]

It is described as an alias to view. It would also mean that we forced a requirement of Solidity ^0.4.17 on the users.

[davesag]

Couldn't you simply not instrument constant functions? Not ideal granted but better than actually breaking code. Otherwise could you just strip constant from functions and not from actual constants. I've not looked into how you do it but a regex of $(\s+[a-z]+\s)(constant) would be enough to distinguish between

address constant MY_THING = 0x0`

and

function something() constant returns (address)

[area]

Couldn't you simply not instrument constant functions? Not ideal granted but better than actually breaking code.

This wasn't a deliberate design decision, just something that we overlooked! I desperately want to keep instrumenting constant functions though, as they are an important part of any contract and can be used by non-constant functions, and so are able to influence the blockchain.

We do currently use a very simple regex, but extending it is non-trivial ("now we have two problems"). For example, the following is valid (if oddly formatted!) solidity:

pragma solidity ^0.4.0;


contract Test {
    address constant public x = 1;
    
    function hash (string _target) 
    public constant returns (bytes32)  {
        return sha3(_target);
    }
    
}

[davesag]

That certainly is an edge-case kind of function

[area]

@cgewecke Is there a reason we didn't do this with something like the following in the preprocessor?

      } else if (node.type === 'FunctionDeclaration' && node.modifiers) {
          // We want to remove constant / pure / view from functions
          for (let i = 0; i < node.modifiers.length; i++) {
            if (['pure', 'constant', 'view'].indexOf(node.modifiers[i].name) > -1) {
              contract = contract.slice(0, node.modifiers[i].start) + contract.slice(node.modifiers[i].end);
              keepRunning = true;
              this.stopTraversal();
            }
          }
        }

[cgewecke]

@area That's looks great - does it work for the edge case elena raised here?

[area]

If we run the preprocessor over everything instead of running the regex over everything, I see no reason why it wouldn't.

[cgewecke]

@davesag Thanks so much for reporting this and @area thank you for fixing. Please re-open there continue to be issues:

Published in 0.4.1

[davesag]

Cool, I'll merge these changes into #151

[davesag]

done