[RFC007] Improve/simplify record representation in the new AST #2102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yannham
commented
Nov 21, 2024
Comment on lines
+269
to
+290
| /// Allocate an AST element in the arena. | ||
| /// | ||
| /// [Self] never guarantees that all destructors are going to be run when using such a generic | ||
| /// allocation function. We don't want to allocate values that need to be dropped through this | ||
| /// method, typically because they own heap-allocated data, such as numbers or parse errors. | ||
| /// That's why we use a marker trait to specify which types can be allocated freely. Types that | ||
| /// need to be dropped have a dedicated method for allocation. | ||
| pub fn alloc<T>(&self, value: T) -> &T { | ||
| self.generic_arena.alloc(value) | ||
| } | ||
|
|
||
| /// Allocate a sequence of AST elements in the arena. | ||
| /// | ||
| /// See [Self::alloc]. | ||
| pub fn alloc_iter<T, I>(&self, iter: I) -> &[T] | ||
| where | ||
| I: IntoIterator<Item = T>, | ||
| I::IntoIter: ExactSizeIterator, | ||
| { | ||
| self.generic_arena.alloc_slice_fill_iter(iter) | ||
| } | ||
|
|
There was a problem hiding this comment.
That's why I talk about a marker trait that is yet to be written, but will exist in the future.
yannham
commented
Nov 21, 2024
| } | ||
|
|
||
| impl<'ast> FromAst<record::FieldDef<'ast>> for (FieldName, term::record::Field) { | ||
| fn from_ast(field: &record::FieldDef<'ast>) -> Self { |
There was a problem hiding this comment.
This is mostly parser::utils::build_record adapted to take a new AST as an input.
There was a problem hiding this comment.
Sorry, this is rather elaborate_field_def.
yannham
commented
Nov 21, 2024
| Vec<(term::RichTerm, term::record::Field)>, | ||
| ) | ||
| { | ||
| fn from_ast(record: &Record<'ast>) -> Self { |
There was a problem hiding this comment.
This is mostly parser::utils::build_record adapted to take a new AST as an input.
yannham
commented
Nov 21, 2024
| /// https://github.com/tweag/nickel/issues/1427. | ||
| /// | ||
| /// This is a helper for the conversion of a record definition to mainline. | ||
| fn merge_fields( |
There was a problem hiding this comment.
This was taken and type-adapted from parser::utils as well.
Bencher Report
Instead of elaborating piecewise definitions (such as `{foo.bar = 1,
foo.baz = 2}`) directly at the parsing stage, this commit makes the new
AST closer to the source language by making record a list of field
definition, where the field "name" can be a sequence of identifiers and
strings. This representation is used internally by the parser; we now
make it the default in the AST, such that the migration of the parser
won't have to do this elaboration at all. The elaboration is offloaded
to the conversion to `RichTerm`, which happens in the `ast::compat`
module.
This makes the AST closer to the source language.
The first motivation is that it'll be better for the LSP, where some
open issues on the tracker are caused by the inability to trace what the
LSP get back to the original piecewise definitions.
The second reason is that we can't actually elaborate a piecewise
definition while staying in the new AST correctly as of today: the new
AST only has one record variant, which is recursive by default, but this
doesn't match the way recursion and scoping work for piecewise
definition. For example, `{foo.bar = 1, baz.foo = foo + 1}` works fine
in today's Nickel (evaluate to `{foo = {bar = 1}, baz {foo = 2}}`), but
if we elaborate it in the new AST naively, we'll get an infinite
recursion: `{foo = {bar = 1}, baz = {foo = foo + 1}}`.
Mailine Nickel currently uses a non recursive `Record` for that, but we
don't want to introduce such "runtime dictionary" in the new AST as they
can't be expressed in the source language. Instead, we rather keep
record as defined piecewise and will do further elaboration when needed,
during typechecking, future compilation, or in the meantime when
converting the new AST representation to mainline Nickel.
yannham
force-pushed
the
rfc007/records-as-field-defs
branch
from
6e607db
to
01a2688
Compare
jneem
approved these changes
Nov 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of elaborating piecewise definitions (such as
{foo.bar = 1, foo.baz = 2}) directly at the parsing stage, this commit makes the new AST closer to the source language by making record a list of field definitions, where the field "name" (left hand side of=) can be a sequence of identifiers and dynamic strings. This representation is used internally by the parser; we now make it the default in the new AST, such that the migration of the parser in #2083 won't have to do this elaboration at all. The elaboration is offloaded to the conversion toRichTerm, which happens in theast::compatmodule.This makes the AST closer to the source language.
The first motivation is that it'll be better for the LSP, where some open issues on the tracker are caused by the inability to trace what the LSP get back to the original piecewise definitions.
The second reason is that we can't actually elaborate a piecewise definition while staying in the new AST correctly as of today: the new AST only has one record variant, which is recursive by default, but this doesn't match the way recursion and scoping work for piecewise definition. For example,
{foo.bar = 1, baz.foo = foo + 1}works fine in today's Nickel (evaluate to{foo = {bar = 1}, baz {foo = 2}}), but if we elaborate it in the new AST naively, we'll get an infinite recursion:{foo = {bar = 1}, baz = {foo = foo + 1}}.Mailine Nickel currently uses a non recursive
Recordfor that, but we don't want to introduce such "runtime dictionary" in the new AST as they can't be expressed in the source language. Instead, we rather keep records as piecewise defined without transformation and will do further elaboration when needed later in the pipeline, during typechecking, future compilation, or in the meantime when converting the new AST representation to mainline Nickel.