-
Notifications
You must be signed in to change notification settings - Fork 663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Illegal use of reserved keywords should be caught during contract analysis #3176
Comments
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 (define-public (call-this (fail bool))
(let ((attempt (try-me fail)))
(ok attempt))) |
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. |
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 (define-public (call-this (fail bool))
(let ((try (try-me fail)))
(ok true))) |
For 2.1, we can make it so that binding to reserved keywords will result in a |
Linking #2696 as well! |
Are there actually any keywords in Clarity that strictly have to be reserved and not allowed in local bindings? |
Some (I think all top level) function names aren't considered as illegal at all ie. In short I would say that every single keyword that is part of language syntax should be considered as reserved. |
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, 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. |
I'm inclined to agree with @njordhov on this topic on a high level (without involving my thinking in the implementation details)... A couple of spontaneous thoughts:
I think it's important to remember that even if one of the key goals of Clarity is to be "clear", the average user isn't going to read the contracts. I think that maybe focusing on auditing tooling (e.g. / my 2 cents |
These desired benefits of reserving keywords are moot with Stacks 2.1. The new Clarity version scheme defeats readability/security gains of reserving names: A black hat can simply upload a confusing contract as Clarity 1 while pretending to call native Clarity 2 functions, redefining them in the contract to do something else than expected. |
Good point. I agree, that's a good argument for the in-contract version indicator. |
This 👍 🚀 |
SIP-015 allows the publish transaction variant to include a version byte to explicitly set the VM version to use. If not provided, the default version for the epoch (e.g. Clarity 2 for Stacks 2.1) will be used. |
I think that a version metadata tag at the top of a contract (like docker compose) would be more of a help to tooling (IDE's, clarinet, etc.) than anything, but also make it clear in public contracts on e.g. github which Clarity version the contract was written for (removing the need to explicitly specify this in the publish trx). It also fixes the Clarity version directly to the contract, which would be beneficial in the case where new Clarity versions change/remove functions, where automatically using the current epoch version might fail (if forgotten to explicitly set on the pub trx). Of course, it's not strictly needed, would just make things more explicit 🤷♂️ |
So can this: ;; This is a Clarity 2 contract.
Unless you're packaging up your code directly into a transaction (in which case, you should know better), there's no reason the contract-publish tooling can't just do this for you. In fact, the tooling could require a pragma of some kind (e.g. specified in a code comment) in order to determine which transaction variant to create. The reasons I'm against adding a Clarity function to encode the version are:
Anyway, this is definitely not happening in Stacks 2.1, and until we run out of values to put in that byte, I think we can close this issue. @obycode Regarding the original issue at hand -- illegal use of reserved keywords -- is this fixed by the new parser? |
Adding a note here following discussion at the engineering weekly meeting -- this is intentionally not changed in 2.1's parser or type-checker changes, since there was still open discussion about it and it is not explicitly clarified in the SIPs. That being said, Clarinet is intentionally opinionated, so we could catch these situations in Clarinet and report errors there. I have reopened the related issue in the Clarinet repo. |
Assigning this to @obycode. Please feel free to close or re-assign. This won't ship in 2.1, but it could ship with sBTC. |
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:This passes a clarinet check, but when executed on clarinet console returns a RuntimeError:
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 errorNameAlreadyUsed
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.
The text was updated successfully, but these errors were encountered: