Small refactoring as a follow-up of the initial ADT introduction commit:
as constr_unify is now implemented for both enum rows and record rows,
it's cleaner to put it in a trait. It's also renamed for a bit more
clarity.
This commit fixes an issue that has been introduced by the removal of
generated variables, together with the `should_update` optimization,
which tries to avoid thunk update that are unnecessary.
However, as detailed in the added documentation for should_update, this
didn't work well with the removal of generated variables, which would
materialize by not sharing inner expressions for variables whose value
was already a data structure in weak head normal form (arrays, records
or now enum variants). Inner expressions would be recomputed.
This commit fixes the issue by adding a condition that datastructure
that aren't yet closurized shouldn't eschew thunk update.
Doing so, this commit also adds the missing closurization of ADTs, which
caused enum variant's arguments to also be potentially re-evaluated many
time.
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
Implement the missing case of the `==` operator for newly introduced
ADTs (enum variants), together with basic tests.
Co-authored-by: jneem <joeneeman@gmail.com>
* Add term variant for ADTs
This commit is a very first step toward adding structural Algebraic Data
Types to Nickel. This commit adds a new node in the AST for an enum tag
with an argument (a variant), and fill the blanks/compiler errors from
there.
A temporary syntax, choosen mostly to be obscure, ugly, easy to add and
to not conflict with existing syntax has been added for further testing
and experimentation: 'SomeTag..(argument).
* Add typechecking capabilities for ADTs
This commit extend enum row types to accept a variant, that is to handle
types such as `[| 'Foo Number, 'Bar {foo : String} |]`, which are
basically structural ADTs.
The bulk of the work consist to extend most function operating on enum
rows to now take into account the fact that enum rows can have types
inside, much as record rows. That is, the code handling enum rows and
record rows is much more similar than before; the main difference being
that for now, enum rows have an optional type argument (the variant,
which can be there or not), while a record row always have a type
assignment.
At this point, no elimination forms for ADTs have been introduced yet
(that is, you can't match on/unwrap ADTs). This part is intentionally on
the side for the time being.
* Fix tests
* Add syntax for ADTs enum types
Add missing surface syntax to write enum rows with a variant (an
argument). Fix some pretty printing shortcomings as well.
* Extend mk_xxx macros for ADTs, fix rustc errors
* Add error reporting for enum row mismatch
Extend various typechecker internal error types with new cases for
augmented (with variants) enum rows
* Fix typo in code comment
* Fix incorrect code documentation
* Small fix to tests and compilation
* Add passing tests for ADTs MVP
* Adapt test infra to new type errors
* Pretty printing of enum variants: match current syntax
* Add test of failure cases of typechecking ADTs
* Remove dead comments, minor comment rewording
* Update core/src/typecheck/eq.rs
* Update core/src/typecheck/mod.rs
* Update core/src/typecheck/unif.rs
* Update core/src/parser/grammar.lalrpop
Co-authored-by: jneem <joeneeman@gmail.com>
* Improve description of EnumVariant
* Update obsolete description of EnumRowF
* Update core/src/typecheck/unif.rs
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
---------
Co-authored-by: jneem <joeneeman@gmail.com>
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
* Make base branch configurable in release script
Instead of hardcoding master as the base branch for a new release, put
it inside a shell variable than can be overriden (at least by modifying
the script) for patch releases, where it's more common to start off a
specific branch which isn't master (but the previous release + selected
bug fixes).
Additionally, add a missing phase before releasing to crates.io: get rid
of `lsp-harness` dependencies in the LSP.
* [release.sh] update to 1.4.1
* Update scripts/release.sh
* correctly drop Array::IntoIter
If you called Array::into_iter() you could leak elements in two circumstances:
1. If you did not iterate through all elements before dropping, since they had been converted to `ManuallyDrop`
2. If the Array had multiple references when the iterator was created, but the other references were dropped before the iterator was dropped, then the existence of the iterator would prevent those Rcs from dropping their contents, but then because the iterator is of type ManuallyDrop, it wouldn't drop those elements either
* eta reduction
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@tweag.io>
* update comments and fix eta-reduction error
---------
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@tweag.io>
The `--field` argument has been added in 1.3 to act only on a specific
field of the configuration. However, pending contracts (contracts
attached to the field by metadata annotations or propagated through
merging) weren't properly applied, leading to illegal values being
accepted.
This commit fixes the issue by adding the missing contract application
step. A snapshot test is added as a regression test. While it's not
ideal (we don't encode in the test what is the expected result), it's
the best we can do currently for CLI-only tests.
Argument to match expressions that wouldn't match any of the branches
used to have a pretty bad error message ("expecting Enum, found Enum").
This commit creates a proper variant for non-matching expressions,
pointing to the match expression and to the value the argument evaluated
to.
Currently, one position that might be lost in translation is where the
match expression was applied. It's a bit harder to track; for now, this
commit is a trivial change and is a strict improvement over the previous
situation. Let's reconsider later if this is insufficient.
Metadata keywords (optional, default, etc.) are numerous and they can't
clash with field names, because they don't appear in the same positions.
Thus, to avoid having user to quote their fields, this commit allows
unambiguously the usage of metadata keyword in field position.
Craming "error: contract broken by ...", a potential addendum for
parametric contracts, and the contract's custom error message into the
main error label of a contract error proved to be hard to read.
This PR simply introduces some spacing to make it better (mostly new
lines and indentation), while keeping the same error structure.
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
Prveiusly we checked whether `stdout` is a terminal to make our coloring
decision, and then we proceeded to happily assume the same state applies
to `stderr` for ad mctual error messages. However, if someone redirects
`stdout` to capture evaluation results, `stderr` may still be a terminal
and should receive colored output in this case. This PR omplements this
behavior and makes the choice of which output stream to check explicit.
We now had a failure to build release binaries due to a spot instance
getting canceled early for the second time. For now, I propose not using
a spot instance. While this will increase the cost, with it only getting
run once per release, I don't think there's cause for concern.
This change is change is already deployed to the AWS account to build
the 1.4 release artifacts.
* Only update relevant deps in Cargo.lock
The release script needs to update Cargo.lock after having updated
Cargo.toml with the new version of various Nickel crates.
Indeed, we are running some checks via Nix, and Nix builds are sandboxed
without network access. This requires the Nix build to use `cargo
--frozen --offline`, which in turns require the Cargo.lock file to be
up-to-date or it will fail.
The previous version of the release script would do a simple `cargo
update`, which has the default of updating all dependencies. This commit
changes that to only udpate dependencies which we might have bumped.
* Add pause to commit release notes in release script
* Fix release script for LSP
The release script was missing a part: the removal of `lsp-harness` (a
local, unpublished util crates) from the nickel-lang-lsp package priori
to publication to crates.io.
Passing by, other small improvements and fixes are included as well.
* [release.sh] update to 1.4.0
* Specify 'do not commit' when updating release notes
* release guide: add missing step for lsp-harness removal
* Rename FieldHaver -> Container and add an Array variant
* Support arrays in the field resolver
* Add a test
* Do better on the renaming
* Don't look at pending_contracts
The curried version of the field access operator `(.)` wasn't handled
properly by the Nickel tree-sitter grammar, and thus Topiary, and thus
`nickel format`.
This commit simply updates to the latest version of Topiary. We also
switch from using tree-sitter-nickel from crates.io to using the same
version as Topiary (from the repository).
* Fix order of arguments for curried dot operator
* Add std.record.get to stdlib
* Update release note with breaking changes for (.)
Co-authored-by: jneem <joeneeman@gmail.com>
* Remove snapshot test for std.record.get
Unfortunately, the output of the error message shows a closure address
(in practice, a memory address), which makes it non-determinstic and
thus unfit for a snpashot test.
Instead, it's been replaced with an integration test, which
Unfortunately won't allow to detect a deterioriation in error reporting,
but at least ensure that the contract of `std.record.get` works as
expected.
---------
Co-authored-by: jneem <joeneeman@gmail.com>
* Add a release script
First draft of a release script to automate most of the painful details
of releasing a new version of Nickel and associated crates.
* Update the releasing guide with new script
* Minor fixes to release script
* Change casing, use namerefs and other small improvements
* Make sure to checkout master before release process
* Make release.sh executable
* Shellcheck pass
* Various fixes and improvement
* More polishing of the release script
* Try sed-based Cargo.toml modifications
* WIP
* Fix cleanup actions after using it add
* Improve confirmation printing
* Fix undefined variable and cleanup actions
* Fix cleanup of ./Cargo.toml
* Fix jq invocation and circular reference warning
* Run cargo update during release script to fix Nix flake check
* Add cargo as requirement for release.sh
* Comment out effectful commands, fix Cargo.lock cleanup
* Add topiary cleaning to the release script
* [TODROP] disable nix flake check for rapid iteration
* [TODROP] Disable cleanup for debugging purpose
* Tentative fix of the remove Topiary dep step
* Avoid trailing commas when removing Topiary
* Improve output of removing topiary dependencies
* Fix issues with format removal step
* More fixing of the format removal step
* Put back actual git commands and cleanup
* Move release script to its own directory
* Make release script re-entrant once release branch exists
* Cd to the root git dir in the release script
* Disable some shellcheck warnings, remove trailing spaces, add missing git switch
* Run checks even if the release branch exists
* Describe Topiary removal in the releasing guide
* Update RELEASING.md
Co-authored-by: jneem <joeneeman@gmail.com>
* Cosmetic improvements to release script
---------
Co-authored-by: jneem <joeneeman@gmail.com>
The doc subcommand has been reported to be a bit confusing, because it
generates file in a default location but just returns without further
details upon success. This commits add a print statement that says where
the document has been generated instead.
* Bump version numbers for 1.3 release
* Add release notes (part 1)
* Bump latest version mentioned in READMEs to 1.3
* Bump latest version mentioned in manual to 1.3
* Update RELEASING guide
* Factor out extract_field() from query()
* Add selectable field to Program
* Add --field to supported commands
This commit finishes the series on adding `--field` argument to the CLI.
This argument is factorized as a new Customize implementer which
regroups commands that don't support general customization (overriding)
but still support selecting a specific field from the configuration.
It's added to the full customization as well.
In consequence, commands that didn't support selecting particular field
before now generically support the `--field` argument. The `--path`
argument for `--query` is removed and is just a `--field` argument for
`nickel query`.
* Fix not taking the --field into account when not customizing
* Formatting
* Fix tests for new --field argument
* Fix clippy warning
* Fix compilation errors, tests, and improve error messages
* Avoid duplicating 'field' declaration in clap
* Customize mode new assignment syntax, part 1
* Add StaticPathField parsing rule
* Add cli assignment parsing rule
* Use new assignment parsing rule, refactor common code
* Fix compilation warnings
* Improve the output of `-- list` including type and contracts
* Remove unused datatype
* Formatting
* Fix old tests and add new for customize mode
* Reword documentation of FieldInterface::is_input
* Pass on code documentation
* Remove obsolete comment
* Update cli/tests/snapshot/inputs/customize-mode/unknown_override.ncl
Co-authored-by: jneem <joeneeman@gmail.com>
* Update cli/tests/snapshot/inputs/customize-mode/unkonwn_field_path.ncl
Co-authored-by: jneem <joeneeman@gmail.com>
* Update cli/src/customize/interface.rs
Co-authored-by: jneem <joeneeman@gmail.com>
* Fix typos in snapshot tests
* Add suggestion for customize mode unknown field errors
* Update snapshot tests with new output
* Get rid of '-- show', hint at using query instead
---------
Co-authored-by: jneem <joeneeman@gmail.com>
* Add strsim dependency
* Move error module into its own directory
* Add error::suggest module and fn to find best suggestion
* Add field_names() method to RecordData
* Make find_match more generic
* Add conversion identifier/nickel string
* Add suggestion to missing field errors
* Fix tests and rename item
* Add snapshot test for field suggestion
* Remove debug println and fix code comment
* Review suggestion: improve missing field error message
* Formatting
* Fix unsound ptr eq check in contract equality
For performance reason, the contract equality checker performs a quick
pointer check to see if the two terms to compare are physically equal.
In this case, it used to return `true` directly, eschewing more costly
recursive checks.
However, this is unsound, because the same term (physically) might be in two
different environments, and have two different values. This is typically
the case with function application: when evaluating `f 1` and `f 2`,
both terms will physically point to the body of `f`, but in a different
environment. Thus, we also need to ensure that environment are equals as
well in this quick check.
This commit adds a fast `ptr_eq` check to the environment as well, and
now checks that both terms and their respective environments are
pairwise physically equals.
Passing by, this commits also fix unrelated clippy warning, after the
udpate to clippy 1.73.
* Add regression test for #1700
* Implement env equality during typechecking
* Cosmetic renaming and slight rework of comments
* Fix contract test by deeply evaluating it
* Remove confusing comments
