Skip to content

Commit

Permalink
Remove recursive priorities (#1600)
Browse files Browse the repository at this point in the history
Recursive priorites were added pre-RFC005, where their semantics was
straightforward. Because metadata could appear anywhere, recursive
priorities (or also called push priorities) just amounted to have a
primop that pushed the priorities down a term, in one lazy step.

Since RFC005, this isn't possible anymore, because only record field can
hold metadata. It's now much more intricate to implement, because when
encountering a `rec force` at the field level, we first need to know if
there's any value able to receive it underneath this field to know if we
should attach it to the field or no. To know that, we might need to
evaluate the field first, and then decide.

Before sorting out both the new semantics and implementation, this
commit disable the syntax (but keep the rest of the machinery) from
1.2.0 so that we can do some design before shipping new recursive
priorities or scraping them entirely. Recursive priorities were
undocumented, and should have been disabled from 1.0.0, so it's not
considered a breaking change.
  • Loading branch information
yannham authored Sep 13, 2023
1 parent 7c9e771 commit 0eb45e8
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 34 deletions.
20 changes: 12 additions & 8 deletions core/src/parser/grammar.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,18 @@ SimpleFieldAnnotAtom<TypeRule>: FieldMetadata = {
// rule).
FieldAnnotAtom<TypeRule>: FieldExtAnnot = {
<SimpleFieldAnnotAtom<TypeRule>> => <>.into(),
"|" "rec" "force" => FieldExtAnnot {
rec_force: true,
..Default::default()
},
"|" "rec" "default" => FieldExtAnnot {
rec_default: true,
..Default::default()
},
// Recursive priorities are disabled as of 1.2.0. Their semantics is non trivial
// to adapt to RFC005 that landed in 1.0.0, so they are currently on hold. If we
// drop them altogether, we'll have to clean the corresponding code floating
// around (not only in the parser, but in the internals module, etc.)
// "|" "rec" "force" => FieldExtAnnot {
// rec_force: true,
// ..Default::default()
// },
// "|" "rec" "default" => FieldExtAnnot {
// rec_default: true,
// ..Default::default()
// },
}

// An annotation, with possibly many annotations chained.
Expand Down
19 changes: 15 additions & 4 deletions core/tests/integration/pass/merging/multiple_overrides.ncl
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,22 @@ let {check, ..} = import "../lib/assert.ncl" in
| default = "",
snd_data = "snd",
},
final_override | rec force = {
fst_data = "override",
snd_data = "override",
common.final = fst_data ++ "_" ++ snd_data,
# Recursive priorities are currently on hold since the implementation of
# RFC005, which changes their semantics. In the meantime, this test has been
# rewritten temporarily with the expected result of the original "rec force"
# instead
#
# final_override | rec force = {
# fst_data = "override",
# snd_data = "override",
# common.final = fst_data ++ "_" ++ snd_data,
# },
final_override = {
fst_data | force = "override",
snd_data | force = "override",
common.final | force = fst_data ++ "_" ++ snd_data,
},

} in
[
parent.fst_data & parent.snd_data == {
Expand Down
44 changes: 22 additions & 22 deletions core/tests/integration/pass/merging/priorities.ncl
Original file line number Diff line number Diff line change
Expand Up @@ -42,29 +42,31 @@ let {Assert, check, ..} = import "../lib/assert.ncl" in
d = 1,
} | Assert,

# TODO: restore (or not?). The previous behavior is harder to simulate after
# RFC005.
# {foo | rec default = 1} & {foo = 2} == {foo = 2} | Assert,
# {foo | rec force = 1} & {foo = 2} == {foo = 1} | Assert,
{val | rec default = {foo = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
{val | rec force = {foo = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
# All the following commented out tests are related to recursive priorities,
# which are on hold since RFC005 (and were disabled in 1.2.0). We need to
# rethink their semantics before proceeding further.

#{foo | rec default = 1} & {foo = 2} == {foo = 2} | Assert,
#{foo | rec force = 1} & {foo = 2} == {foo = 1} | Assert,
#{val | rec default = {foo = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
#{val | rec force = {foo = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,

# Pushed priorities should only modifies fields without explicitly set priorities
{val | rec force = {foo | priority -1 = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
{val | rec force = {foo | default = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
{val | rec default = {foo | priority 1 = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
{val | rec default = {foo | force = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
#{val | rec force = {foo | priority -1 = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
#{val | rec force = {foo | default = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
#{val | rec default = {foo | priority 1 = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
#{val | rec default = {foo | force = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,

let x = {
foo | force = "old",
bar = {
baz = "old",
baz' = "old"
} |> std.record.map (fun _name value => value)
} in
{val | rec default = x} & {val = {foo = "new", bar.baz = "new"}}
== { val = {foo = "old", bar = {baz = "new", baz' = "old"}}}
| Assert,
#let x = {
# foo | force = "old",
# bar = {
# baz = "old",
# baz' = "old"
# } |> std.record.map (fun _name value => value)
#} in
#{val | rec default = x} & {val = {foo = "new", bar.baz = "new"}}
# == { val = {foo = "old", bar = {baz = "new", baz' = "old"}}}
# | Assert,

# Interaction of recursive overriding and recursive priorities
#
Expand All @@ -74,8 +76,6 @@ let {Assert, check, ..} = import "../lib/assert.ncl" in
# (without `rec force` but with `force` directly) works.
# Maybe some field update issue?
#
# TODO: restore and understand what's going on
#
# let context = 1 in
# let x = {
# foo = context + 2 + bar,
Expand Down

0 comments on commit 0eb45e8

Please sign in to comment.