Versioning#263
Conversation
stringhandler
left a comment
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 |
Sdoba16
force-pushed
the
feature/versions
branch
3 times, most recently
from
ef379ee to
975817b
Compare
|
Do we have an issue for discussion this? |
|
|
Needs rebase |
|
c56e75d needs rebase |
|
|
||
| Every `.simf` file must begin with a compiler version directive: | ||
| ```rust | ||
| #![compiler_version(">=0.6.0")] |
There was a problem hiding this comment.
This syntax suggests we have a preprocessing phase, which is not true. Since SimplicityHL is a Rust-like language, users will see #![...] and assume it works like Rust's preprocessor.
Let's avoid the confusion and switch to simc X.Y.Z. Since pragma version is not approved yet, we are looking at variants like compiler, compiler-version, or simc. In my opinion, simc is the most optimal variant because it is concise and saves keystrokes for the user.
| ## For Compiler Contributors | ||
|
|
||
| If you are contributing to the SimplicityHL compiler and bumping the version in `Cargo.toml`, you must update the version directives in all examples and tests. | ||
|
|
||
| You can do this automatically from the project root: | ||
| ```bash | ||
| python3 update_examples.py <new_version> | ||
| ``` | ||
| See `CONTRIBUTING.md` for more details. No newline at end of file |
There was a problem hiding this comment.
Regarding the update_examples.py script and our test contracts, I do not see a strict need for pinned compiler versions here.
We can simplify our workflow by using a range like pragma version >=0.5.0 across all test files. If a specific contract introduces breaking changes or becomes incompatible in the future, we can simply bump the minimum requirement for that file alone. Otherwise, we can leave them as they are and avoid running an update script every time the compiler version changes.
| @@ -0,0 +1,394 @@ | |||
| use crate::driver::tests::setup_graph_raw; | |||
There was a problem hiding this comment.
Let's add a quick note to the docstring explaining the motivation behind this macro
| if $expect_success { | ||
| assert!(graph_opt.is_some() && !handler.has_errors(), | ||
| "Scenario failed unexpectedly. Errors:\n{}", handler); | ||
| } else { | ||
| assert!(graph_opt.is_none() || handler.has_errors(), | ||
| "Scenario succeeded when it should have failed."); | ||
| let expected_err: Option<&str> = $expected_err; | ||
| if let Some(err) = expected_err { | ||
| assert!(handler.to_string().contains(err), | ||
| "Expected error containing '{}' but got:\n{}", err, handler); | ||
| } |
There was a problem hiding this comment.
The nested blocks can be rewritten as follows:
if $expect_success {
assert!(graph_opt.is_some() && !handler.has_errors(),
"Scenario failed unexpectedly. Errors:\n{}", handler);
return;
}
assert!(graph_opt.is_none() || handler.has_errors(),
"Scenario succeeded when it should have failed.");
let expected_err: Option<&str> = $expected_err;
if let Some(err) = expected_err {
assert!(handler.to_string().contains(err),
"Expected error containing '{}' but got:\n{}", err, handler);
}
|
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. |
5 participants
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: