Skip to content
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

Allow metadata keywords in field position #1768

Merged
merged 1 commit into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions core/src/parser/grammar.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ TypeArray: Type = "Array" <t: AsType<Atom>> =>

// A record operation chain, such as `{foo = data}.bar.baz`.
RecordOperationChain: RichTerm = {
<t: AsTerm<Atom>> "." <id: Ident> => mk_term::op1(UnaryOp::StaticAccess(id), t).with_pos(id.pos),
<t: AsTerm<Atom>> "." <id: ExtendedIdent> => mk_term::op1(UnaryOp::StaticAccess(id), t).with_pos(id.pos),
<t: AsTerm<Atom>> "." <t_id: WithPos<StrChunks>> => mk_access(t_id, t),
};

Expand Down Expand Up @@ -500,7 +500,7 @@ pub CliFieldAssignment: (Vec<LocIdent>, RichTerm, RawSpan) =
=> (path, value, mk_span(src_id, start, end));

FieldPathElem: FieldPathElem = {
<Ident> => FieldPathElem::Ident(<>),
<ExtendedIdent> => FieldPathElem::Ident(<>),
<WithPos<StrChunks>> => FieldPathElem::Expr(<>),
};

Expand Down Expand Up @@ -557,6 +557,30 @@ Match: Match = {
// A default annotation in a pattern.
DefaultAnnot: RichTerm = "?" <t: Term> => t;

// A metadata keyword returned as an indent. In some positions, those are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this for all (or most) of our keywords? I think even a let or rec should be unambiguous in field name positions.

Copy link
Member Author

@yannham yannham Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for doing this for metadata keyword only at first. Although I don't have right now any example of an existing keyword or a future extension that could cause an issue, let's just give it a bit more thinking (metadata keywords are much more restricted in where they can appear). At worst we can do that in a follow-up PR.

// considered valid identifiers. See ExtendedIdent below.
MetadataKeyword: LocIdent = {
"doc" => LocIdent::new("doc"),
"default" => LocIdent::new("default"),
"force" => LocIdent::new("force"),
"priority" => LocIdent::new("priority"),
"optional" => LocIdent::new("optional"),
"not_exported" => LocIdent::new("not_exported"),
};

// We allow metadata keywords (optional, default, doc, etc.) as field names
// because:
//
// 1. There are many metadata keywords, and it's annoying to quote them all
// (and they might be growing, which is causing backward compatibility issues)
// 2. Metadata keyword can't appear anywhere in field position (and vice-versa), so there's no clash.
//
// Thus, for fields, ExtendedIdent is use in place of Ident.
ExtendedIdent: LocIdent = {
<WithPos<MetadataKeyword>>,
<Ident>,
};

Ident: LocIdent = <l:@L> <i: "identifier"> <r:@R> =>
LocIdent::new_with_pos(i, mk_pos(src_id, l, r));

Expand Down
190 changes: 107 additions & 83 deletions core/tests/integration/pass/core/records.ncl
Original file line number Diff line number Diff line change
@@ -1,141 +1,165 @@
# test.type = 'pass'
Copy link
Member Author

@yannham yannham Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has a big diff because I formatted it with Topiary. You can ignore most of the changes but the very end, where I added a test for metadata keyword in field position.

let {check, ..} = import "../lib/assert.ncl" in
let { check, .. } = import "../lib/assert.ncl" in

[
# accesses
({foo = 3, bar = true}).bar == true,
{"%{if true then "foo" else "bar"}" = false, bar = true}.foo
== false,

({foo = 3, bar = true})."bar" == true,
{"%{if true then "foo" else "bar"}" = false, bar = true}."%{"foo"}"
== false,

(std.record.insert "foo" true {bar = 3}).foo == true,
({ foo = 3, bar = true }).bar == true,
{ "%{if true then "foo" else "bar"}" = false, bar = true }.foo == false,
({ foo = 3, bar = true })."bar" == true,
{ "%{if true then "foo" else "bar"}" = false, bar = true }."%{"foo"}" == false,
(std.record.insert "foo" true { bar = 3 }).foo == true,

# primitive_ops
std.record.has_field "foo" {foo = 1, bar = 2},
std.record.has_field "fop" {foo = 1, bar = 2} == false,
({foo = 2, bar = 3}
|> std.record.remove "foo"
|> std.record.has_field "foo")
== false,

{bar = 3}
std.record.has_field "foo" { foo = 1, bar = 2 },
std.record.has_field "fop" { foo = 1, bar = 2 } == false,
(
{ foo = 2, bar = 3 }
|> std.record.remove "foo"
|> std.record.has_field "foo"
) == false,
{ bar = 3 }
|> std.record.insert "foo" 1
|> std.record.has_field "foo",

# laziness of map
(std.record.map (fun x y => y + 1) {foo = 1, bar = "it's lazy"}).foo
== 2,

let r = std.record.map
(std.record.map (fun x y => y + 1) { foo = 1, bar = "it's lazy" }).foo == 2,
let r =
std.record.map
(fun y x => if %typeof% x == 'Number then x + 1 else 0)
{foo = 1, bar = "it's lazy"} in
(r.foo) + (r.bar) == 2,
{ foo = 1, bar = "it's lazy" }
in
(r.foo) + (r.bar) == 2,

# merging
{a = 1} & {b=true} == {a = 1, b = true},
{a = 1, b = 2} & {b = 2, c = 3}
== {a = 1, b = 2, c = 3},

{a = {b = 1}} & {a = {c = true}}
== {a = {b = 1, c = true}},

{a = 'A, b = 'B} & {a = 'A, c = 'C}
== {a = 'A, b = 'B, c = 'C},
{ a = 1 } & { b = true } == { a = 1, b = true },
{ a = 1, b = 2 }
& { b = 2, c = 3 } == { a = 1, b = 2, c = 3 },
{ a = { b = 1 } }
& { a = { c = true } } == { a = { b = 1, c = true } },
{ a = 'A, b = 'B }
& { a = 'A, c = 'C } == { a = 'A, b = 'B, c = 'C },

# merge_complex
let rec1 = {
a = false,
b = if true then (1 + 1) else (2 + 0),
c= ((fun x => x) (fun y => y)) 2,
} in
let rec2 = {
b = ((fun x => x) (fun y => y)) 2,
c = if true then (1 + 1) else (2 + 0),
d = true,
} in
let result = {
a = false,
b = 2,
c = 2,
d = true,
} in
rec1 & rec2 == result,
a = false,
b = if true then (1 + 1) else (2 + 0),
c = ((fun x => x) (fun y => y)) 2,
}
in
let rec2 = {
b = ((fun x => x) (fun y => y)) 2,
c = if true then (1 + 1) else (2 + 0),
d = true,
}
in
let result = {
a = false,
b = 2,
c = 2,
d = true,
}
in
rec1 & rec2 == result,

# merge_with_env
(fun y => ((fun x => {a=y}) 1) & ({b=false})) 2
== {a = 2, b = false},
(fun y => ((fun x => { a = y }) 1) & ({ b = false })) 2 == { a = 2, b = false },

# merge_with_env_nested
{b={c=10}} & ((fun x => {a=x, b={c=x}}) 10)
== {a=10, b = {c = 10}},
{ b = { c = 10 } }
& ((fun x => { a = x, b = { c = x } }) 10) == { a = 10, b = { c = 10 } },

# recursive_records
{a = 1, b = a + 1, c = b + a} == {a = 1, b = 2, c = 3},
{f = fun x y =>
{ a = 1, b = a + 1, c = b + a } == { a = 1, b = 2, c = 3 },
{
f = fun x y =>
if x == 0 then y else f (x + (-1)) (y + 1)
}.f 5 5
== 10,

}.f
5
5 == 10,
let with_res = fun res =>
{
f = fun x =>
if x == 0 then
res
else g x,
else
g x,
g = fun y => f (y + (-1))
}.f 10 in
}.f
10
in
with_res "done" == "done",

# piecewise signatures
{
foo : Number,
bar = 3,
foo = 5
foo : Number,
bar = 3,
foo = 5
}.foo == 5,
{
foo : Number,
foo = 1,
bar : Number = foo,
foo : Number,
foo = 1,
bar : Number = foo,
}.bar == 1,
let {foo : Number} = {foo = 1} in foo == 1,
let { foo : Number } = { foo = 1 } in foo == 1,

# recursive overriding with common fields
# regression tests for [#579](https://github.com/tweag/nickel/issues/579)
# and [#583](https://github.com/tweag/nickel/issues/583)
({foo.bar = foo.baz, foo.baz | default = 2} & {foo.baz = 1}).foo.bar == 1,
({ foo.bar = foo.baz, foo.baz | default = 2 } & { foo.baz = 1 }).foo.bar == 1,
let Name = fun b label name => if b then "hijack" else std.contract.blame label in
let Config = {
b | Bool,
name | Name b,
} in
b | Bool,
name | Name b,
}
in
let data | Config = {
b = true,
name = "name",
} in
}
in
data.name == "hijack",

# recursive overriding with dictionaries
# regression tests for [#892](https://github.com/tweag/nickel/issues/892)
(({a = 1, b = a} | {_ | Number}) & { a | force = 2}).b == 2,

({
b = { foo = c.foo },
c = {}
} | {_ | {
foo | default = 0
} }).b.foo == 0,
(({ a = 1, b = a } | { _ | Number }) & { a | force = 2 }).b == 2,
(
{
b = { foo = c.foo },
c = {}
}
| {
_ | {
foo | default = 0
}
}
).b.foo == 0,

# regression test for [#1161](https://github.com/tweag/nickel/issues/1161)
(std.record.map (std.function.const std.function.id) { a = 1, b = a }).b == 1,

# regression test for [#1224](https://github.com/tweag/nickel/issues/1224)
std.record.fields ({} | { field | optional = "value" }) == [ "field" ],
std.record.fields ({} | { field | optional = "value" }) == ["field"],

# check that record type don't propagate through merging
({foo = "a"} | {_ : String}) & {foo | force = 1} & {bar = false}
== {foo = 1, bar = false},
({ foo = "a" } | { _ : String })
& { foo | force = 1 }
& { bar = false } == { foo = 1, bar = false },

# check that metadata keyword can be used as record fields without quoting
let r = {
optional = true,
default = false,
doc = "hello",
priority = 0,
force = "no"
}
in
(
r.optional
&& !r.default
&& r.doc == "hello"
&& r.priority == 0
&& r.force == "no"
)
]
|> check
Loading