Versioning#263
Conversation
stringhandler
left a comment
There was a problem hiding this comment.
Concept ACK.
I've come around to the idea that users should optionally be able to specify the compiler version. However, I don't like the pragma keyword as it deviates from rust, which we haven't done previously. Below are some suggestions.
I also don't think it should be required in the .simc, which will allow us to specify it in a project level manifest for when multiple files are included and for libraries in future. Also, the user should not be forced to provided it even in a single file scenario. This is just extra friction for new developers.
| @@ -1,3 +1,5 @@ | |||
| pragma version 0.5.0-rc.0; | |||
There was a problem hiding this comment.
should we maybe rather use a global attribute syntax
#![version("0.5.0-rc.0")]
I would also suggest naming it compiler-version or simc_version or even just simc, so that users don't think it is annotating this file with a version.
| /// Records _what_ happened but not where. | ||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
| pub enum Error { | ||
| VersionMismatch { |
There was a problem hiding this comment.
| VersionMismatch { | |
| SimcVersionMismatch { |
I prefer stating that it's the compiler version here as well
|
|
||
| let insertion_point = Span::new(first_item_span.start, first_item_span.start); | ||
|
|
||
| return Err(Error::MissingPragma { |
There was a problem hiding this comment.
I think pragmas should be optional
| expected: String, | ||
| compiler: String, | ||
| }, | ||
| MissingPragma { |
There was a problem hiding this comment.
I don't think stating the compiler version should be required
|
ab41fec needs rebase |
ef379ee to
975817b
Compare
|
Do we have an issue for discussion this? |
|
|
Needs rebase |
|
c56e75d needs rebase |
|
Is it required to add the compiler version? I would say the examples should only specify the minimum version needed to compile, which for all of these would not be necessary as they don't have any breaking changes. The only ones that might need to be specified are the ones using the It is generally fine to include a compiler version, but these look like they need to keep being updated. |
| @@ -1,3 +1,5 @@ | |||
| simc ">=0.6.0"; | |||
There was a problem hiding this comment.
Should this not just be simc "0.6.0"?
Can we guarantee it will work on 0.7.0?
|
|
||
| The SimplicityHL compiler enforces strict version compatibility to prevent contracts from silently breaking due to compiler updates, semantic changes, or new language features. | ||
|
|
||
| Every `.simf` file must begin with a compiler version directive: |
There was a problem hiding this comment.
| Every `.simf` file must begin with a compiler version directive: | |
| Every `.simf` file may begin with a compiler version directive: |
I think it is incredibly limiting to new developers to enforce this. Surely this should be opt-in.
There was a problem hiding this comment.
I do not fully understand how it will work. If we load a file without a version, then how are we supposed to determine it? Do we assume that it is always correct? What if the functionality in that file becomes deprecated? What if we have some vulnerabilities in the file? Thus, I think we need to make it mandatory for users to use versions
There was a problem hiding this comment.
Let me walk through the two concrete scenarios:
Case 1: File uses newer syntax (e.g., pub or use, introduced in simc 0.6.0)
If no version is specified, the file simply fails to compile on older compilers with a syntax error. The simc field doesn't change this outcome - it just gives a much better error message ("this file requires simc >= 0.6.0") instead of a confusing parser error.
Case 2: File uses deprecated or removed functionality
Same story: without a version constraint, the file fails to compile once that feature is removed. The simc field lets authors signal simc >=0.5.0, <0.6.0 to make that explicit, but it's still an error either way.
On vulnerabilities: the simc version field controls compilation compatibility, not runtime security. Vulnerabilities in Simplicity programs are addressed at the consensus/jet layer, independently of the compiler version.
So omitting the simc field is never silently wrong - the compiler still catches incompatibilities. Making it mandatory adds friction for new developers who just want to write a simple program, and requires them to understand version semantics before writing their first line of code. Keeping it as may means it's there when you need better error messages, but doesn't block the happy path.
| for (path, content) in files { | ||
| let full_path = format!("workspace/{}", path); | ||
| let created_file = canon(&ws.create_file(&full_path, content)); | ||
| let injected_content = if crate::version::has_version_header(content) |
There was a problem hiding this comment.
This could use a comment.
|
|
||
| version_test!( | ||
| exact_match_all, true, None, | ||
| "main.simf" => "simc \"={v}\";\nuse lib::A::foo;\nfn main() {}", |
There was a problem hiding this comment.
| "main.simf" => "simc \"={v}\";\nuse lib::A::foo;\nfn main() {}", | |
| "main.simf" => r#"simc ""={v}""; | |
| use lib::A::foo; | |
| fn main() {}"#, |
These would be much easier to review if you use raw strings
|
cd5b1e9 needs rebase |
521e33f to
1ac6efd
Compare
a413ba1 to
fed34ae
Compare
| /// Replace the directive with equal-length spaces so the parser never sees it | ||
| /// while later byte offsets (and thus error spans) stay correct. | ||
| pub(crate) fn blank_version_directive(content: &str) -> Cow<'_, str> { |
There was a problem hiding this comment.
This function answers the question "What does this function do?", but not "Why was this function needed?". I looked at the following commits to see its usage inside the parse.rs file, but I still do not understand the reason for using it
| /// A directive-looking line that is not a well-formed `simc "...";`. | ||
| Malformed(Span), | ||
| /// More than one directive. | ||
| Duplicate(Span), | ||
| /// A well-formed directive that appears after program code. | ||
| Misplaced(Span), |
There was a problem hiding this comment.
Let's stick to the style that was mentioned here: #328 (comment)
|
|
||
| #[test] | ||
| fn test_debug_and_jet_tracing() { | ||
| let program_text = TEST_PROGRAM; |
There was a problem hiding this comment.
I do not think we need to change this file at all
Closes #248
Add compiler versioning for SimplicityHL contracts
This PR introduces strict compiler version requirements
.simffile must now begin with asimc "..."directive. The directive is safely extracted (ignoring comments) and validated before full AST construction.>=0.6.0,^0.6.0,~0.6.0)main.simfand all imported--deplibraries)update_examples.pyand a CI workflow to easily manage version across all examples and tests.Syntax:
Needed simplex addition for this PR: