feat: prevent variable declaration from shadowing functions #323
Conversation
src/types/resolveStatements.ts
Outdated
| const staticFunction = getAllStaticFunctions(ctx); | ||
| if (Object.values(staticFunction).some((f) => f.name === s.name)) { | ||
| throwError(`Variable shadows existing function: ${s.name}`, s.ref); | ||
|
|
||
| } |
There was a problem hiding this comment.
This looks a bit like an ad hoc approach to me. For instance, would it work for contract functions? This should be solved with a unified variable context if you ask me. So, a bit of refactoring is needed here
vitorpy
force-pushed
the
avoid-shadowing-functions
branch
from
e33a35c
to
2d32676
Compare
|
@anton-trunov Sorry, can you provide some guidance here on which cases this is expected to cover? ie. which test cases are missing? |
| @@ -510,6 +510,16 @@ Line 8, col 12: | |||
| " | |||
| `; | |||
|
|
|||
| exports[`resolveStatements should fail statements for case-51 1`] = ` | |||
| "<unknown>:4:5: Variable already defined : rec | |||
There was a problem hiding this comment.
The error message is a bit confusing here: it should say "function 'rec' blah blah"
|
@anton-trunov @novusnota Hey guys, should this be allowed?
|
@vitorpy Tact doesn't have first-class functions, so we can allow such declarations, I think. Let's wait for input from @anton-trunov though. |
This is an instance of type-directed disambiguation and it's an opportunity to mess up. I say we go for simplicity and ban this, no shadowing should be allowed at this moment. I mean it's easier to ban some behavior and relax the restrictions in later language versions. And it's backwards compatible! |
|
@vitorpy Let's disallow |
Closes #307
Just checking if this is a sensible approach to handle avoiding variable declarations from shadowing functions.
I'm guessing I should also avoid shadowing other identifiers, such as contract names? Also non-static functions I guess.