Skip to content

Commit

Permalink
Revert behavior of deep= on mutable keys.
Browse files Browse the repository at this point in the history
Mutable keys are a minefield for comparisons, as resolving
equality require re-implementing a lot of the internal structures, as
well as dealing with multiple mutable keys that are in the same
equivalency class by deep=.

Simplifying the implementation to not resole mutable keys is much
simpler, faster, and has the benefit that deep= and deep-not= do not
need to allocate.
  • Loading branch information
bakpakin committed Dec 21, 2024
1 parent 1b49934 commit 746ced5
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 22 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
All notable changes to this project will be documented in this file.

## ??? - Unreleased
- Add `struct/rawget`
- Fix `deep=` and `deep-not=` to better handle degenerate cases with mutable table keys
- Long strings will now dedent on `\r\n` instead of just `\n`.
- Add `ev/to-file` for synchronous resource operations
Expand Down
16 changes: 2 additions & 14 deletions src/boot/boot.janet
Original file line number Diff line number Diff line change
Expand Up @@ -2259,8 +2259,6 @@
:string (buffer ds)
ds))

(def- mutable-types {:table true :array true :buffer true})

(defn deep-not=
``Like `not=`, but mutable types (arrays, tables, buffers) are considered
equal if they have identical structure. Much slower than `not=`.``
Expand All @@ -2282,20 +2280,10 @@
(or (= tx :struct) (= tx :table))
(or (not= (length x) (length y))
(do
(def rawget (if (= tx :struct) struct/rawget table/rawget))
(var ret false)
(def mut-keys-x @{})
(eachp [k v] x
(if (get mutable-types (type k))
(let [kk (freeze k)]
(put mut-keys-x kk (put (get mut-keys-x kk @{}) (freeze v) true)))
(if (deep-not= (get y k) v) (break (set ret true)))))
(when (next mut-keys-x) # handle case when we have mutable keys separately
(def mut-keys-y @{})
(eachp [k v] y
(if (get mutable-types (type k))
(let [kk (freeze k)]
(put mut-keys-y kk (put (get mut-keys-y kk @{}) (freeze v) true)))))
(set ret (deep-not= mut-keys-x mut-keys-y)))
(if (deep-not= (rawget y k) v) (break (set ret true))))
ret))
(= tx :buffer) (not= 0 (- (length x) (length y)) (memcmp x y))
(not= x y))))
Expand Down
11 changes: 11 additions & 0 deletions src/core/struct.c
Original file line number Diff line number Diff line change
Expand Up @@ -294,13 +294,24 @@ JANET_CORE_FN(cfun_struct_to_table,
return janet_wrap_table(tab);
}

JANET_CORE_FN(cfun_struct_rawget,
"(struct/rawget st key)",
"Gets a value from a struct `st` without looking at the prototype struct. "
"If `st` does not contain the key directly, the function will return "
"nil without checking the prototype. Returns the value in the struct.") {
janet_fixarity(argc, 2);
JanetStruct st = janet_getstruct(argv, 0);
return janet_struct_rawget(st, argv[1]);
}

/* Load the struct module */
void janet_lib_struct(JanetTable *env) {
JanetRegExt struct_cfuns[] = {
JANET_CORE_REG("struct/with-proto", cfun_struct_with_proto),
JANET_CORE_REG("struct/getproto", cfun_struct_getproto),
JANET_CORE_REG("struct/proto-flatten", cfun_struct_flatten),
JANET_CORE_REG("struct/to-table", cfun_struct_to_table),
JANET_CORE_REG("struct/rawget", cfun_struct_rawget),
JANET_REG_END
};
janet_core_cfuns_ext(env, NULL, struct_cfuns);
Expand Down
16 changes: 10 additions & 6 deletions test/suite-boot.janet
Original file line number Diff line number Diff line change
Expand Up @@ -1004,12 +1004,16 @@

# issue #1535
(loop [i :range [1 1000]]
(assert (deep= @{:key1 "value1" @"key" "value2"}
@{:key1 "value1" @"key" "value2"}) "deep= mutable keys"))
(assert (deep-not= @{:key1 "value1" @"key" "value2"}
@{:key1 "value1" @"key" "value2"}) "deep= mutable keys"))
(assert (deep-not= {"abc" 123} {@"abc" 123}) "deep= mutable keys vs immutable key")
(assert (deep= {@"" 1 @"" 2 @"" 3} {@"" 1 @"" 2 @"" 3}) "deep= duplicate mutable keys")
(assert (deep= {@"" @"" @"" @"" @"" 3} {@"" @"" @"" @"" @"" 3}) "deep= duplicate mutable keys 2")
(assert (deep= {@[] @"" @[] @"" @[] 3} {@[] @"" @[] @"" @[] 3}) "deep= duplicate mutable keys 3")
(assert (deep= {@{} @"" @{} @"" @{} 3} {@{} @"" @{} @"" @{} 3}) "deep= duplicate mutable keys 4")
(assert (deep-not= {@"" 1 @"" 2 @"" 3} {@"" 1 @"" 2 @"" 3}) "deep= duplicate mutable keys")
(assert (deep-not= {@"" @"" @"" @"" @"" 3} {@"" @"" @"" @"" @"" 3}) "deep= duplicate mutable keys 2")
(assert (deep-not= {@[] @"" @[] @"" @[] 3} {@[] @"" @[] @"" @[] 3}) "deep= duplicate mutable keys 3")
(assert (deep-not= {@{} @"" @{} @"" @{} 3} {@{} @"" @{} @"" @{} 3}) "deep= duplicate mutable keys 4")
(assert (deep-not= @{:key1 "value1" @"key2" @"value2"}
@{:key1 "value1" @"key2" "value2"}) "deep= mutable keys")
(assert (deep-not= @{:key1 "value1" [@"key2"] @"value2"}
@{:key1 "value1" [@"key2"] @"value2"}) "deep= mutable keys")

(end-suite)
4 changes: 2 additions & 2 deletions test/suite-marsh.janet
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ neldb\0\0\0\xD8\x05printG\x01\0\xDE\xDE\xDE'\x03\0marshal_tes/\x02
(assert (= 2 (length tclone)) "table/weak-values marsh 2")
(gccollect)
(assert (= 1 (length t)) "table/weak-value marsh 3")
(assert (deep= t tclone) "table/weak-values marsh 4")
(assert (deep= (freeze t) (freeze tclone)) "table/weak-values marsh 4")

# tables with prototypes
(def t (table/weak-values 1))
Expand All @@ -219,7 +219,7 @@ neldb\0\0\0\xD8\x05printG\x01\0\xDE\xDE\xDE'\x03\0marshal_tes/\x02
(assert (= 2 (length tclone)) "marsh weak tables with prototypes 2")
(gccollect)
(assert (= 1 (length t)) "marsh weak tables with prototypes 3")
(assert (deep= t tclone) "marsh weak tables with prototypes 4")
(assert (deep= (freeze t) (freeze tclone)) "marsh weak tables with prototypes 4")
(assert (deep= (getproto t) (getproto tclone)) "marsh weak tables with prototypes 5")

(end-suite)

0 comments on commit 746ced5

Please sign in to comment.