ink
ink copied to clipboard
Divert parameters leak into surrounding context
Minimal reproduction
~temp scene = 0
->Start(scene)
===Start(ref scene)
~scene++
SCENE {scene}
-> END
The crash occurs in every runtime I've tried so far, but there are several ways around it: rename one of the variables, or use a tunnel instead of a divert, and the variable resolves successfully.
The tracebacks I've looked at all repeat the same few calls until it overflows that stack. I believe the specific calls indicate that "scene" is getting treated as a pointer reference to itself, rather than as a reference to the calling context.
EDIT: I found a repro of the underlying issue that shows a bug without barfing out tons of traceback.
~temp scene = 0
->Start("blah")
===Start(scene)
-> next
===next
SCENE1 {scene} // Prints "SCENE1 blah", unless you change the name of the parameter to Start.
->END
After poking at this some more, I think the resolution is "tell people not to use pass-by-reference when defining normal knots". (Because it doesn't actually accomplish anything. The contents of the knot see the temporary variable, because it's global.)
I'm not sure there's any way to enforce this, since looking at a knot definition, you can't tell how it's entered.
Ooooor, introduce scoping rules for naming temporaries, so the compiler rewrites them in a hygienic way.
I thought a little more, and I have a proposal that might work without sacrificing functionality, but it could break save compatibility. Because knots with parameters (ref and non-ref) act like they've pushed a new stack frame (allocating the parameters as temp variables), there should be a way to represent them as distinct Elements, doing some kind of tail-call elimination when they hit another normal divert.
EDIT: But if the divert takes a local temporary value by reference... Oh dear. I guess it would have to deref any pointers referring to the current context before it replaces the context?
EDIT again: Sorry for the tweet-sized conversation with myself, but I just realized this is good, actually. Because the proper implementation here is something like "become", then that means that this can be handled entirely in runtime without affecting save compatibility, just by changing the handling of the -> instruction.
EDIT once more: The compiler generates -> instructions to handle other constructs. I think this needs a change to the compiled code, but I can see a way to do it without breaking compatibility.