Zilliqa / scilla

Scilla - A Smart Contract Intermediate Level Language
https://scilla-lang.org
GNU General Public License v3.0
240 stars 78 forks source link

Cashflow analyser doesn't analyse field initialisers #713

Open jjcnn opened 4 years ago

jjcnn commented 4 years ago

The cashflow analyser only analyses procedure and transition bodies, and not field initialisers.

This can be seen by running the cashflow analyser with the hint -cf-token-field balances on the contract tests/contracts/fungible_token.scilla. This hint should cause the total_tokens parameter should to be tagged as Money, but it is still tagged as NoInfo.

jjcnn commented 4 years ago

@simonelnahas : I think this issue would be a good introduction to the cashflow analyser, if you are still up for playing around with it.

simonelnahas commented 4 years ago

what i'm thinking:

I've found cf_tag_expr and cf_tag_stmt inside Cashflow.ml i expect that i could look into the possible types for the pattern matching and find the one missing for field initialisers.

I'm now trying to add that, but unfortunately i still have problems with compiling. So i have to get that working first. Do you recommend just proceeding in this direction and learn from figuring it out myself or asking for help? @jjcnn

jjcnn commented 4 years ago

@simonelnahas : I think you should try to look at the code before you propose a solution.

Also, you do need to be able to compile Scilla before you can get anywhere. Try reinstalling everything from scratch, and follow the installation instructions precisely. If that doesn't work, try asking for help on Discord.

simonelnahas commented 4 years ago

Do you agree that it would make sense to extrapolate the tags from field initialisers before the statement lists are traversed and not as a part of the statement list traversal?

simonelnahas commented 4 years ago

changing the cashflow.ml doesn't seem to have an effect:

  1. making main return two empty lists ([],[])
  2. make clean; make
  3. ➜  scilla git:(master) ✗ dune exec tests/testsuite.exe                                                                                         
    ocamlopt tests/testsuite.exe
    ld: warning: directory not found for option '-L/opt/local/lib'
    .....
    Ran: 1263 tests in: 32.79 seconds.
    OK
    ➜  scilla git:(master) ✗ ./bin/scilla-checker -gaslimit 10000 -libdir src/stdlib tests/contracts/fungible-token.scilla -cf-token-field balances
    {
    "cashflow_tags": {
    "State variables": [
      { "field": "owner", "tag": "NoInfo" },
      { "field": "total_tokens", "tag": "NoInfo" },
      { "field": "decimals", "tag": "NoInfo" },
      { "field": "name", "tag": "NoInfo" },
      { "field": "symbol", "tag": "NoInfo" },
      { "field": "balances", "tag": "(Map Money)" },
      { "field": "allowed", "tag": "(Map (Map Money))" }
    ],
    "ADT constructors": []
    },
    "warnings": [
    {
      "warning_message":
        "No transition in contract FungibleToken contains an accept statement\n",
      "start_location": { "file": "", "line": 0, "column": 0 },
      "end_location": { "file": "", "line": 0, "column": 0 },
      "warning_id": 1
    }
    ],
    "gas_remaining": "10000"
    }

    still the cashflow tags are found. So there must be someting wrong with my workflow?

@anton-trunov @jjcnn

anton-trunov commented 4 years ago

I get a bunch of errors in the case you described:

ulimit -s 128 -n 1024; dune exec tests/testsuite.exe -- -print-diff true
....................................................................................................................................................................
.....................................................................................................................................................................
.....................................................................................................................................................................
.....................................................................................................................................................................
.....................................................................................................................................................................
.....................................................................................................................................................................
........................................................................................................................................FFF...FFFFF.F.FFF
FF...FFFFFFFFFFF.F.......FF..FFF.FF.F.....FF..FFF......................................................................FF
simonelnahas commented 4 years ago

i'm sorry you where totally right about that the need for a new clone, due to the files being moved, this also solves #737

it was caused by having an unused copy of cashflow.ml in src/lang/checkers since checkers/ has now been moved to its parent directory.

Thanks for your quick response.

PS: Does you workflow look the same when working on the compiler or do you have an easier/faster way?

anton-trunov commented 4 years ago

PS: Does you workflow look the same when working on the compiler or do you have an easier/faster way?

Could you expand your question? I don't really understand it. We rarely move things around, so this shouldn't be an issue in general. But this time we needed it for the upcoming split of the Scilla codebase into several packages.

simonelnahas commented 4 years ago

Let my clarify: My question was not on the moving of the code, but more whether the workflow of:

  1. change something in the compiler code
  2. make clean; make
  3. run a test to see it's effect

was the same you were using?

anton-trunov commented 4 years ago

@simonelnahas I simply make changes and run make test (with occasional make gold to fix tests or introduce new tests)

jjcnn commented 4 years ago
3. run a test to see it's effect

You need to run the test described in the first comment to see whether the change has had an effect. The test has not been added to the test suite, because that would require a change to the automated test setup. (I would consider that change to be part of this issue).

simonelnahas commented 4 years ago

Yes, Thanks @jjcnn i am aware of that. i created a concrete test case to simplify the issue: field tokens1 : Uint128 = total_tokens. using make; ./bin/scilla-checker -gaslimit 10000 -libdir src/stdlib tests/contracts/fungible-token.scilla -cf-token-field total_tokens for testing.

Currently i can see that inside the let rec tagger only the components are tagged with cf_tag_component I want to add now an equivalent version cf_tag_field just for fields that updates the environments accordingly inside the tagger.

I really enjoy using the types. It helps a lot!

So this is how i'm proceeding so far. Do you agree with the approach?

jjcnn commented 4 years ago

That sounds about right.

simonelnahas commented 4 years ago

It finally works! 👍 How do i make a new test case for ./bin/scilla-checker -gaslimit 10000 -libdir src/stdlib tests/contracts/fungible-token.scilla -cf-token-field balances ?

jjcnn commented 3 years ago

See #740 for a partial fix to this issue.