Support redefinable def and defmacro bindings using :redef

[pyrmont]

This PR adds support for using the metadata keyword :redef with def and defmacro bindings. A binding that has this metadata can be be redefined and existing references to that binding will use the redefined value. This is useful for interactive environments in which a dynamic environment is important (e.g. a REPL). All top-level def and defmacro bindings can be made redefinable by settings the :redefs dynamic binding to true (as done by the new command line option -D).

Background

Janet supports three types of bindings: var bindings, def bindings and defmacro bindings. var bindings store the evaluated result in an array associated with the :ref keyword in the binding table. Both def and defmacro bindings store the evaluated result as a value associated with the :value keyword.

When var bindings are referenced from other contexts, the Janet compiler stores a reference to the :ref array and retrieves the value from the array at runtime. In contrast, when def and defmacro bindings are referenced from other contexts, the Janet compiler inserts a direct reference to the value. In most situations, this is appropriate. def and defmacro bindings are not intended to be mutable; indeed, that is the reason Janet has var bindings.

The exception is in interactive environments (e.g. a REPL). In these environments, if a user redefines a def or defmacro binding because they are iteratively developing a program, they most likely want all references to that binding to use the updated value.

Implementation

This PR creates a new member in the JanetBinding struct, .dynamic, the value for which can either be JANET_BINDING_STATIC or JANET_BINDING_DYNAMIC. When .dynamic is set to JANET_BINDING_DYNAMIC, def and defmacro bindings function like var bindings (although they cannot be changed using set).

A user can specify that a def or defmacro binding is redefinable using the :redef metadata value. Alternatively, the user can make almost all¹ def and defmacro bindings redefinable by setting the :redefs dynamic binding to true. A user can force all def and defmacro bindings in a Janet REPL to be redefinable by using the command line option -D.

Footnotes

1. A user can ensure that a def or defmacro binding is never made redefinable by setting the :redef metadata value to false. ↩

[sogaiu]

The following is what is meant by "expand function", right?

janet/src/boot/boot.janet

Line 2272 in cddc2a8

* `:expander` - an optional function that is called on each top level form before being compiled.

[pyrmont]

@sogaiu Yes, thanks for the clarification. A function passed with that key to run-context could inspect each top-level form and add the :redef metadata to each def, defn and defmacro invocation.

[andrewchambers]

Not sure I understand why this is different to a var?

[sogaiu]

IIUC, it's related to this idea:

Dynamic development with the REPL - Janet uses early binding. So if you recompile a function in the REPL, any function that uses the recompiled function will not use the newest definition. This is usually not what you want. To get the late binding that Clojure uses, redefine "defn" to "varfn" during development. For other dynamic behaviors the environment in Janet is first-class and inspectable and changeable through "(curenv)".

via: http://thegeez.net/2021/10/28/janet_web_app_aws_lambda.html

[pyrmont]

@andrewchambers Assuming that #897 is merged and the source map data associated with vars is updated when they are redefined, then in most cases they'll work similarly. That said, they're not completely the same. set won't work with a def with :redef metadata because the binding is not flagged as being mutable. You'll also be able to use this with defmacro in a way that has no analogy to var.

However, the key point is that you don't need to change all your bindings to make REPL-driven development more practical (and then change them all back again when you're finished; being careful not to change the vars that are intentionally vars).

[andrewchambers]

I wonder if it could just be an option that gets passed to the compiler on a global level.

[elimisteve]

I wonder if it could just be an option that gets passed to the compiler on a global level.

That sounds much simpler and less subtle to me 👍 .

[pyrmont]

@andrewchambers How about a dynamic binding (e.g. :dynamic-defs) that can be enabled with a command line switch and that's checked by the compiler when compiling forms? You'd get the benefits of a dynamic approach without the complexity of needing some way to manually add :redef to every def form. I haven't thought this all the way through but perhaps you don't need a new def 'type' in that case and just need a conditional in specials.c's defleaf function?

@bakpakin Do you have any thoughts?

[andrewchambers]

That's the sort of thing I was talking about, but I don't really know what is the best way as I haven't needed such a feature so far.

[pyrmont]

Update: Following the discussion below, I've gone back to using :redef rather than :dynamic.

---

I've changed the implementation slightly and now track whether a def or defmacro binding is dynamic (i.e. redefinable) using a new member .dynamic in the struct JanetBinding. This reduces the number of changes that are needed and I think more accurately describes the binding. To use consistent nomenclature, I've switched to using the metadata value :dynamic rather than :redef.

Together with this change, I've added global support for dynamic def and defmacro bindings using the :dynamic-defs dynamic binding. If that binding is set to true, all top-level def and defmacro bindings are dynamic. I also added a new command line option, -D, to the janet binary that sets the :dynamic-defs dynamic binding so that this can be invoked easily from the command line.

You can see examples of how to use the functionality in the updated test suite and I've also updated the OP to reflect the changes to make it easier for those coming to this PR in the future.

[sogaiu]

Tried it out briefly:

$ ./build/janet -D
Janet 1.19.2-ec65f038 linux/x64 - '(doc)' for help
repl:1:> (def a 1)
1
repl:2:> (dyn 'a)
@{:dynamic true :source-map ("repl" 1 1) :value @[1]}
repl:3:> (defn b [] a)
<function b>
repl:4:> (b)
1
repl:5:> (def a 2)
2
repl:6:> (b)
2

Looks good so far :)

@saikyun May be this is of interest to you too?

[saikyun]

Isn't it a bit confusing to call it :dynamic when there is already a concept called dynamic?

How about something like :redefinable or :late-bound?

[elimisteve]

@saikyun Good point!

[pyrmont]

@saikyun If we went with something else, I'd suggest going back to :redef. It's shorter than any of the other alternatives, doesn't have the problem that you might forget if it's :redefinable or :redefineable and is consistent in effect (if not implementation) with the same metadata keyword in Clojure.

[saikyun]

For me it's important to be able to redefine things across files / modules. I've managed to do this earlier by never replacing :ref-arrays, instead using existing ones if they already exist in module/cache. Here's a module that does this (though currently has some other issues, so don't use it): https://github.com/saikyun/freja/blob/multi-windows/freja/ns.janet#L54

I couldn't get this scenario to work using :dynamic, am I doing something wrong, or is it not meant to work?

Example 1

Code

jona@wings-of-fire:~/programmering/test-redef$ cat uselul.janet
(spit "lul.janet" `(def a :dynamic 10) (print "1st a is: " a)`)

(import ./lul :as l :fresh true)

(defn b [] (+ l/a l/a))

(print "call 1: " (b))


(spit "lul.janet" `(def a :dynamic 1337) (print "2nd a is: " a)`)
(import ./lul :as l :fresh true)

(print "call 2: " (b))

Execution

jona@wings-of-fire:~/programmering/test-redef$ janet uselul.janet 
1st a is: 10
call 1: 20
2nd a is: 1337
call 2: 20

Example 2

Code

jona@wings-of-fire:~/programmering/test-redef$ cat uselul2.janet 
(spit "lul.janet" `(def a :dynamic 10) (print "1st a is: " a)`)

(import ./lul :as l)

(defn b [] (+ l/a l/a))

(print "call 1: " (b))


(spit "lul.janet" `(def a :dynamic 1337) (print "2nd a is: " a)`)

(dofile "lul.janet")

(print "call 2: " (b))

Execution

jona@wings-of-fire:~/programmering/test-redef$ janet uselul2.janet 
1st a is: 10
call 1: 20
2nd a is: 1337
call 2: 20

[pyrmont]

@saikyun You're right that, by itself, this PR won't allow you to update the bindings created via import. The problem in both of your examples is that you never send the existing bindings 'back' to be updated. Without doing something funky with the module loaders, I'm not sure if this is possible via import but you can make something similar work via use:

(spit "lul.janet" `(def a 10) (print "1st a is: " a)`)

(use ./lul)

(defn b [] (+ a a))

(print "call 1: " (b))

(spit "lul.janet" `(def a 1337) (print "2nd a is: " a)`)

(dofile "lul.janet" :env (curenv))

(print "call 2: " (b))

Using use puts the bindings into the current environment. We then pass this environment as an argument to dofile so that when lul.janet is evaluated by run-context it has the existing bindings to update.

The result:

$ ./build/janet -D saikyun.janet
1st a is: 10
call 1: 20
2nd a is: 1337
call 2: 2674

I use the new command line argument and so don't need to manually add :dynamic to the bindings in the Janet code.

[pyrmont]

@saikyun Got it working with your first example:

$ ./build/janet -D -e "(put module/loaders :source (fn source-loader [path args] (put module/loading path true) (defer (put module/loading path nil) (def env (get module/cache path)) (dofile path :env env ;args))))" saikyun.janet
1st a is: 10
call 1: 20
2nd a is: 1337
call 2: 2674

The new module loader is almost the same as the one in boot.janet. The difference is that we pull the environment created during the original import out of the cache:

(def env (get module/cache path))
(dofile path :env env ;args)

[saikyun]

That's nice! However, now we have a new problem, that you can run code using old symbols after having removed them:

Code

# uselul.janet
(spit "lul.janet"
      ``
      (def a :dynamic 10)
      (def b :dynamic 20)
      (print "1st a is: " a)
      ``)

(import ./lul :as l :fresh true)

(defn b [] (+ l/a l/a))

(print "call 1: " (b))


(spit "lul.janet"
      ``
      (def a :dynamic 1337)
      (print "2nd a is: " a)
      (print "accessing non-existing b: " b)
      ``)
(import ./lul :as l :fresh true)

(print "call 2: " (b))

Executing

janet -D -e "(put module/loaders :source (fn source-loader [path args] (put module/loading path true) (defer (put module/loading path nil) (def env (get module/cache path)) (dofile path :env env ;args))))" uselul.janet 
1st a is: 10
call 1: 20
2nd a is: 1337
accessing non-existing b: 20
call 2: 2674

[saikyun]

Some notes:

main

1. With -D or (setdyn :dynamic-defs true) you can't have a main function, I get:

error: expected integer key in range [0, 1), got "freja/main2.janet"
  in _thunk [freja/main2.janet] (tailcall) on line -1, column -1
  in cli-main [boot.janet] on line 3672, column 17

2. Writing {:dynamic false} doesn't seem to counteract (setdyn :dynamic-defs true)
3. Doing a (setdyn :dynamic-defs false) before defining main fixes the above error.

Gist with example code: https://gist.github.com/saikyun/90a9bda09f1ff4dd199b02d8929d3c74

Importing spork/path

path/ext breaks when using -D, can't seem to fix with setdyn around the import.

janet -D -e '(import spork/path) (print (path/ext "a.b"))'
error: expected integer key in range [0, 3), got nil
  in _thunk [eval-string] (tailcall) on line 1, column 28
  in eval1 [boot.janet] on line 2341, column 16
  in run-context [boot.janet] on line 2398, column 13
  in eval-string [boot.janet] on line 2437, column 3
  in e-switch [boot.janet] (tailcall) on line 3623, column 12
  in cli-main [boot.janet] on line 3650, column 13

This works though: janet -D -e '(import spork/path) (print (path/abspath "a.b"))'

Maybe the error has to do with redef? https://github.com/janet-lang/spork/blob/master/spork/path.janet#L131

[saikyun]

Is it meant to work with var? Currently getting this behaviour:

# basic-var.janet
(var a 10)
(defn b [] a)
(pp (b))
(var a 20)
(pp (b))

# execution
janet -D basic-var.janet
10
10

[sogaiu]

I might be off here, but for the var thing to work, it may be that #897 needs to be merged first.

Here's a bit from pyrmont (see #897) about the situation:

Currently, if you call var repeatedly, Janet will create a new binding that does not use the :ref value of the existing binding. This might be intentional but it is inconsistent with the way that varfn works

May be pyrmont can confirm / clarify / deny :)

[pyrmont]

@saikyun Thanks for really stress testing this. In response:

1. I've reverted to using :redef rather than :dynamic as the metadata keyword. Coincident with this, the dynamic def to make def bindings redefinable is now :redefs rather than :dynamic-defs. I've also made it so that you can ensure a def binding is not redefinable by setting the metadata value of :redef to false. An example demonstrating this is now part of the test suite.

2. The run-main function in boot.janet assumed that the value of the function was stored directly under the binding table's :value key. This is not true if -D has been set. I've updated boot.janet and this now works as expected. I've also updated a number of other places in boot.janet that used :value.

3. The way that path/ext is defined relies on the path/ext binding being static. When the path/ext is reset to nil (here), then when redefinable bindings are on, it sets the value of posix/ext and win32/ext to nil as well. There are two ways to fix this: either a user of the library turns :redefs off before importing spork/path or the ext binding in spork/path is changed to use {:redef false} (assuming this PR is accepted).

4. Repeated invocations of var do not currently redefine a binding. As @sogaiu noted, I think that they should but since that's a separate issue to the subject matter of this PR, it was submitted as PR Use existing :ref array for redefined var bindings #897.

[bakpakin]

Just getting back from vacation, this looks pretty good to me. Glad we have gone to a global dynamic setting rather than a per binding indication, since that is the main point - a global setting that will allow interactive development with no change to code.

However, wondering if the existing :debug dyn could be used instead rather than a new :redef dyn. Or have the existing -d flag set both :debug and :redef. That way, there is less need to call janet -d -D when trying to do "repl driven development".

[pyrmont]

@bakpakin Hope you had a good break :D

My responses:

1. The current implementation uses the global setting to cause the compiler to add the :redef metadata keyword to def and defmacro bindings. From src/core/specials.c:

           } else if (janet_checktype(meta_redef, JANET_NIL) && janet_truthy(janet_dyn("redefs"))) {
               janet_table_put(entry, janet_ckeywordv("redef"), janet_wrap_true());
               is_redef = 1;
           }

   Are you OK with that approach?

2. I don't have a strong opinion on whether the global redefinable setting should be separate from the global debug setting. My inclination is yes since debugging and redefining seem to me to be separate things (albeit that you may often want to do both together). For users who do want to do both, typing an extra flag at the command line doesn't seem onerous.

[sogaiu]

Some tooling I use talks to janet via its repl (not netrepl). I'd like the option of only having the redefining behavior without debugging being enabled (though I imagine occasionally I'd want both). IIUC keeping things separate seems to be a straight-forward way to achieve this. Please indicate if this understanding seems off.

[saikyun]

If debug-mode affects performance, I'd like them to be separate (though maybe `-d` should always enable `-D` as well?). I plan on having `-D` always on in Freja, so that one can change internal functions. But I don't necessarily want debug-mode to always be on. It would also be OK if I can use neither `-d` nor `-D` but instead enable redef by using the `(setdyn :redefs true)`.

[sogaiu]

IIUC, currently -d leads to the dynamic variable :debug being true. This in turn leads to enter-debugger being invoked in boot.janet's repl under certain circumstances:

janet/src/boot/boot.janet

Line 3373 in 199ec36

(if (e :debug) (enter-debugger f x))))))

I don't think that's something I want to happen most of the time, though I envision using the redef behavior while developing.

Not sure what else is affected.

To get the effect of both at the same time, I think it makes more sense to:

• add another command line option to get both -d and -D's behavior or
• specify both

rather than changing -d's behavior. I think it's more flexible that way as well as less likely to break existing things.

[sogaiu]

@bakpakin It looks like -D has been removed.

Was this intentional?

[elimisteve]

@sogaiu Was it? I see https://github.com/janet-lang/janet/pull/898/files#diff-e120880268b4f0f04177470180f50ee0d2c7ac13cb83bb778b6d81efda1cbbccR3586

[sogaiu]

@elimisteve There is no -D in the latest janet:

janet/src/boot/boot.janet

Lines 3576 to 3639 in 07ec892

	{"h" (fn [&]
	(print "usage: " (dyn :executable "janet") " [options] script args...")
	(print
	```
	Options are:
	-h : Show this help
	-v : Print the version string
	-s : Use raw stdin instead of getline like functionality
	-e code : Execute a string of janet
	-E code arguments... : Evaluate an expression as a short-fn with arguments
	-d : Set the debug flag in the REPL
	-r : Enter the REPL after running all scripts
	-R : Disables loading profile.janet when JANET_PROFILE is present
	-p : Keep on executing if there is a top-level error (persistent)
	-q : Hide logo (quiet)
	-k : Compile scripts but do not execute (flycheck)
	-m syspath : Set system path for loading global modules
	-c source output : Compile janet source code into an image
	-i : Load the script argument as an image file instead of source code
	-n : Disable ANSI color output in the REPL
	-l lib : Use a module before processing more arguments
	-w level : Set the lint warning level - default is "normal"
	-x level : Set the lint error level - default is "none"
	-- : Stop handling options
	```)
	(os/exit 0)
	1)
	"v" (fn [&] (print janet/version "-" janet/build) (os/exit 0) 1)
	"s" (fn [&] (set raw-stdin true) (set should-repl true) 1)
	"r" (fn [&] (set should-repl true) 1)
	"p" (fn [&] (set exit-on-error false) 1)
	"q" (fn [&] (set quiet true) 1)
	"i" (fn [&] (set expect-image true) 1)
	"k" (fn [&] (set compile-only true) (set exit-on-error false) 1)
	"n" (fn [&] (set colorize false) 1)
	"m" (fn [i &] (setdyn :syspath (in args (+ i 1))) 2)
	"c" (fn c-switch [i &]
	(def path (in args (+ i 1)))
	(def e (dofile path))
	(spit (in args (+ i 2)) (make-image e))
	(set no-file false)
	3)
	"-" (fn [&] (set handleopts false) 1)
	"l" (fn l-switch [i &]
	(import* (in args (+ i 1))
	:prefix "" :exit exit-on-error)
	2)
	"e" (fn e-switch [i &]
	(set no-file false)
	(eval-string (in args (+ i 1)))
	2)
	"E" (fn E-switch [i &]
	(set no-file false)
	(def subargs (array/slice args (+ i 2)))
	(def src ~|,(parse (in args (+ i 1))))
	(def thunk (compile src))
	(if (function? thunk)
	((thunk) ;subargs)
	(error (get thunk :error)))
	math/inf)
	"d" (fn [&] (set debug-flag true) 1)
	"w" (fn [i &] (set warn-level (get-lint-level i)) 2)
	"x" (fn [i &] (set error-level (get-lint-level i)) 2)
	"R" (fn [&] (setdyn :profilepath nil) 1)})

-d does give one the redef behavior.

[elimisteve]

@sogaiu Good point; was removed here 1 hour ago: 99cfbaa . (The other changes look great to me, but not removing -D!)

[elimisteve]

@bakpakin Re: 99cfbaa#diff-e120880268b4f0f04177470180f50ee0d2c7ac13cb83bb778b6d81efda1cbbccR3661 , what if people want to make Janet's behavior more dynamic/redefinable without enabling debug mode?

[sogaiu]

@elimisteve I do this ATM: janet -e '(setdyn :redef true)' ...

So in a way -D still exists in a kind of virtual form perhaps.

[saikyun]

I can't thumbs up the merge so 🥳🥳🥳🥳🥳🥳🥳

[sogaiu]

@elimisteve and others - I found that the -e approach doesn't work as well as -d. I found a case where it fails while using -d succeeds.

To reproduce:

1. Clone path (at the time of this writing, the tested commit is: fd552e880cb3510f10a230ea5f9dcc227fcb521f)
2. From within the cloned directory, do:

$ janet -e '(setdyn :redef true)' -r
Janet 1.19.2-3b412d51 linux/x64 - '(doc)' for help
repl:1:> (import ./path)
error: path.janet:160:29: compile error: expected 2 arguments
  in dofile [boot.janet] (tailcall) on line 2683, column 7
  in source-loader [boot.janet] on line 2693, column 15
  in require-1 [boot.janet] on line 2713, column 18
  in import* [boot.janet] on line 2744, column 15
  in _thunk [repl] (tailcall) on line 1, column 1

3. Compare with result obtained using -d:

$ janet -d
Janet 1.19.2-3b412d51 linux/x64 - '(doc)' for help
repl:1:> (import ./path)
@{_ @{:value <cycle 0>} path/abspath @{:private true} path/abspath? @{:private true} path/basename @{:private true} path/delim @{:private true} path/dirname @{:private true} path/ext @{:private true} path/join @{:private true} path/normalize @{:private true} path/parts @{:private true} path/posix/abspath @{:private true} path/posix/abspath? @{:private true} path/posix/basename @{:private true} path/posix/delim @{:private true} path/posix/dirname @{:private true} path/posix/ext @{:private true} path/posix/join @{:private true} path/posix/normalize @{:private true} path/posix/parts @{:private true} path/posix/sep @{:private true} path/sep @{:private true} path/win32/abspath @{:private true} path/win32/abspath? @{:private true} path/win32/basename @{:private true} path/win32/delim @{:private true} path/win32/dirname @{:private true} path/win32/ext @{:private true} path/win32/join @{:private true} path/win32/normalize @{:private true} path/win32/parts @{:private true} ...}

[elimisteve]

@sogaiu Does having debugging enabled seem to mess anything up or slow anything down when wanting to make Janet more dynamic/"redefinable" rather than actually debug? (Thanks!)

[sogaiu]

@elimisteve One difference I'm aware of is what happens when there is an error, e.g.:

$ janet -d
Janet 1.19.2-3b412d51 linux/x64 - '(doc)' for help
repl:1:> (error "hi")
error: hi
  in _thunk [repl] (tailcall) on line 1, column 1
entering debug[1] - (quit) to exit
debug[1]:1:>

That is, the interactive debugger is invoked.

[sogaiu]

Tried another thing.

The following doesn't give an error:

$ janet -d -e '(setdyn :debug false)' -r
Janet 1.19.2-3b412d51 linux/x64 - '(doc)' for help
repl:1:> (import ./path)
@{_ @{:value <cycle 0>} path/abspath @{:private true} path/abspath? @{:private true} path/basename @{:private true} path/delim @{:private true} path/dirname @{:private true} path/ext @{:private true} path/join @{:private true} path/normalize @{:private true} path/parts @{:private true} path/posix/abspath @{:private true} path/posix/abspath? @{:private true} path/posix/basename @{:private true} path/posix/delim @{:private true} path/posix/dirname @{:private true} path/posix/ext @{:private true} path/posix/join @{:private true} path/posix/normalize @{:private true} path/posix/parts @{:private true} path/posix/sep @{:private true} path/sep @{:private true} path/win32/abspath @{:private true} path/win32/abspath? @{:private true} path/win32/basename @{:private true} path/win32/delim @{:private true} path/win32/dirname @{:private true} path/win32/ext @{:private true} path/win32/join @{:private true} path/win32/normalize @{:private true} path/win32/parts @{:private true} ...}

[sogaiu]

With pyrmont's #910 applied, I no longer see the reported behavior mentioned in #898 (comment)

[elimisteve]

@sogaiu Great! So -e works well without caveats, or are there still some?

[elimisteve]

You left a couple comments about different behavior so I'm just making sure 😉

[sogaiu]

@elimisteve I'm not aware of any other issues ATM :)

[elimisteve]

@sogaiu That's great news, thank you! And thank you @pyrmont for the bug fix 👌