fix: check new variable bindings do not shadow global constants #680
Conversation
|
Please let me know if there are any issues in this PR and I'll happily fix those in a follow-up, but now I'm going to merge this one to fix the other issues for v1.4.2 |
| ): StatementContext { | ||
| checkVariableExists(src, name); // Should happen earlier | ||
| checkVariableExists(ctx, sctx, name); // Should happen earlier | ||
| if (isWildcard(name)) { |
There was a problem hiding this comment.
Ohhh, interesting. I did not check in the interpreter the use of wildcard identifiers (to be honest, I was not aware of them). Where is the wildcard _ allowed in Tact? Anywhere where an identifier is allowed? I looked in the grammar and it seems that _ is allowed by the id rule. _ allows you to ignore that identifier, right? I am asking this for the interpreter :)
There was a problem hiding this comment.
Right, underscores (_) means an identifier that cannot be used (so the shadowing rules do not apply to them). You can use it as a function parameter, in catch-blocks, foreach-loops, unused let-bindings.
| @@ -197,7 +209,7 @@ function processStatements( | |||
| ctx = resolveExpression(s.expression, sctx, ctx); | |||
|
|
|||
| // Check variable name | |||
| checkVariableExists(sctx, s.name); | |||
| checkVariableExists(ctx, sctx, s.name); | |||
There was a problem hiding this comment.
Is there any reason as to why call checkVariableExists at this point if later addVariable will call it in both branches of the conditional? I guess to give the user the variable duplication and shadowing errors as soon as possible.
There was a problem hiding this comment.
I actually didn't think about this, I just added ctx mechanically here.
| @@ -486,11 +498,12 @@ function processStatements( | |||
| let catchCtx = sctx; | |||
|
|
|||
| // Process catchName variable for exit code | |||
| checkVariableExists(initialCtx, s.catchName); | |||
| checkVariableExists(ctx, initialSctx, s.catchName); | |||
There was a problem hiding this comment.
Again. Why call checkVariableExists if addVariable is the first thing that it does in the next line?
There was a problem hiding this comment.
We could try refactor this, although I'm not sure how useful this is going to be since we are going to replace the existing typechecker with a new one
There was a problem hiding this comment.
Agreed. I think there is no need to refactor.
Closes #676
[ ] I have documented my contribution in Tact Docs: https://github.com/tact-lang/tact-docs/pull/PR-NUMBER