hyperledger-solang / solang

Solidity Compiler for Solana and Polkadot
https://solang.readthedocs.io/
Apache License 2.0
1.27k stars 216 forks source link

Add missing Identifier locations to AST #1580

Closed chioni16 closed 1 year ago

chioni16 commented 1 year ago

Adds missing Identifier locations to AST. This solves some of the issues currently faced when using Language Server features like rename.

codecov[bot] commented 1 year ago

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (45f01b4) 87.65% compared to head (002992a) 87.74%. Report is 2 commits behind head on main.

Files Patch % Lines
src/sema/dotgraphviz.rs 0.00% 15 Missing :warning:
src/bin/doc/mod.rs 0.00% 3 Missing :warning:
src/codegen/cfg.rs 0.00% 1 Missing :warning:
src/sema/expression/function_call.rs 98.33% 1 Missing :warning:
src/sema/expression/member_access.rs 90.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1580 +/- ## ========================================== + Coverage 87.65% 87.74% +0.08% ========================================== Files 133 133 Lines 64300 64419 +119 ========================================== + Hits 56363 56524 +161 + Misses 7937 7895 -42 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

seanyoung commented 1 year ago

An internal function call can have too parts: base contract and function name.

abstract contract a {
    function foo() internal virtual returns (int) {
        return 1;
    }
}

contract b is a {
    function foo() internal override returns (int) {
        return 2;
    }

    function bar() public returns (int x) {
        x = a.foo();
    }
}

Here a.foo is ast::Expression::InternalFunction. In fact, you can also import a function from another file via an import object. So, the id field should be an IdentifierPath.

chioni16 commented 1 year ago

An internal function call can have too parts: base contract and function name.

abstract contract a {
  function foo() internal virtual returns (int) {
      return 1;
  }
}

contract b is a {
  function foo() internal override returns (int) {
      return 2;
  }

  function bar() public returns (int x) {
      x = a.foo();
  }
}

Here a.foo is ast::Expression::InternalFunction. In fact, you can also import a function from another file via an import object. So, the id field should be an IdentifierPath.

Do we have something that can also hold the types of individual parts of the IdentifierPath? This is because when the rename functionality is invoked on contract a in your example, we would also like to rename the a in a.foo(), which is currently not possible. One more example that I can think of is enum literals of the form enum.variant. We would like to treat this as two separate parts enum and variant. But currently the whole thing evaluates to NumLiteral, which is not ideal when rename / references / any other functionality is invoked.

I'm thinking that part of this type resolution will happen when the language server functionality is invoked/ we are preparing things for the language server. And some type information may not be possible to be derived in the language server and hence we will have to make some changes in sema. These are just my initial thoughts though.

@seanyoung

seanyoung commented 1 year ago

Here a.foo is ast::Expression::InternalFunction. In fact, you can also import a function from another file via an import object. So, the id field should be an IdentifierPath.

Do we have something that can also hold the types of individual parts of the IdentifierPath? This is because when the rename functionality is invoked on contract a in your example, we would also like to rename the a in a.foo(), which is currently not possible.

We don't have that right now, but in this case you can assume that the last part if the function name.

One more example that I can think of is enum literals of the form enum.variant. We would like to treat this as two separate parts enum and variant.

enum variants are not stored as IdentifierPath.

But currently the whole thing evaluates to NumLiteral, which is not ideal when rename / references / any other functionality is invoked.

This is a different problem than function names,

I'm thinking that part of this type resolution will happen when the language server functionality is invoked/ we are preparing things for the language server. And some type information may not be possible to be derived in the language server and hence we will have to make some changes in sema. These are just my initial thoughts though.

We'll have to deal with each individual case here.