Illegal use of reserved keywords should be caught during contract analysis

[obycode]

Copied from hirosystems/clarinet#418 reported by @FriendsFerdinand. I opened a PR to fix this in clarinet, but a similar change should likely be made here, for 2.1, so that this type of problem will cause an error when deploying the contract instead of only reporting a runtime error when it is called.

Original issue:

Consider the following contract where I use try! as a variable name:

(define-public (call-this (fail bool))
  (let (
    (try! (try-me fail))
  )
    (ok true)
  )
)

(define-public (try-me (fail bool))
  (begin
    (asserts! fail (err u999))
    (ok true)
  )
)

This passes a clarinet check, but when executed on clarinet console returns a RuntimeError:

Runtime Error: Runtime error while interpreting ST1PQHQKV0RJXZFY1DGX8MNSNYVE3VGZJSRTPGZGM.contract-7: Unchecked(NameAlreadyUsed("try!"))

If you use an asserts! instead, it will not pass clarinet check, naming a syntax error.
If I write: (define-data-var try! uint u0), then it will return a Runtime error NameAlreadyUsed after running clarinet check.

Conclusion

I think running clarinet check should be able to detect this kind of error before trying to run the contract code on clarinet console/test.

[njordhov]

For consideration, there are forward compatibility benefits of going the other direction, allowing local bindings to shadow globals. For example, say the binding instead is to a symbol attempt as below. Now, if attempt becomes a reserved keyword in a future version of Clarity, the contract will fail when redeployed.

(define-public (call-this (fail bool))
  (let ((attempt (try-me fail)))
    (ok attempt)))

[obycode]

That's a good point @njordhov, and would make a lot of sense for other languages, but for Clarity, I think we're looking to maximize readability and minimize the potential for misleading code, so I'd argue that the forward incompatibility is a reasonable price to pay to disallow potentially confusing uses of reserved keywords.

[njordhov]

Forward compatibility is particularly relevant for Clarity, as contracts deployed on-chain cannot be updated.

There is a considerable trade-off in sacrificing forward compatibility for the sake of disallowing potentially confusing uses of reserved keywords. Without such forward compatibility, every new function added to the language is a breaking change, requiring special handling of existing contracts. The VM will increase in complexity to accommodate incompatible contracts (see issue #3131).

Yet little is gained as disallowing some symbols doesn't hinder developers from using similarly confusing symbols that resemble built-in functions. Advising developers on problematic name choices is better covered by a linter like clarinet check, leaving the VM to focus on correctly executing contracts.

For your particular example, the confusion can be resolved by using a more conventional formatting of the code, making it apparent that try is not called as a function:

(define-public (call-this (fail bool))
  (let ((try (try-me fail)))
    (ok true)))

[jcnelson]

For 2.1, we can make it so that binding to reserved keywords will result in a CheckError, thereby preventing such transactions from getting mined.

[whoabuddy]

Linking #2696 as well!

[njordhov]

Are there actually any keywords in Clarity that strictly have to be reserved and not allowed in local bindings?

[LNow]

block-height, tx-sender, contract-caller, true, false, none, some, burn-block-height, stx-liquid-supply - these are considered as illegal during execution, but not during analysis (contract deployment).

Some (I think all top level) function names aren't considered as illegal at all ie. define-data-var, define-public.
Most if not all function names are considered as illegal only during execution.

In short I would say that every single keyword that is part of language syntax should be considered as reserved.

[njordhov]

In Clarity, keywords don't have to be in the language syntax, thus strictly don't have to be reserved.

Clarity has a simple syntax in the form of S-expressions where the parsing is independent of the keywords. In other languages, keywords are reserved because they are part of the grammar, with the keywords determining the parsing.

[obycode]

I agree, @njordhov, that they are not required to be reserved, but again, to avoid confusing (innocent or malicious) contracts, I agree with @LNow above and prefer to reserve all of the keywords.

[njordhov]

@obycode, the argument posted by @LNow does not support your position. @LNow argued that every single keyword that is part of language syntax should be considered as reserved but we already agree that no keywords have to be part of the language syntax. There are better ways to solve the concern of potentially confusing local bindings without the sacrifices and complications associated with reserving keywords.