Behavior Change: Identical Function Objects will be rawequal

Hello developers,

This is an announcement for an upcoming change to script semantics around function object identity. We expect that it generally doesn’t really affect experiences, but there are some odd corner cases in existing experiences that may make them incompatible with this.

In short: If you rely on function object identity and haven’t tested your project in Studio in the last month, you should make sure your experience works correctly in Studio to prepare for this change.

What is the change?

In Luau before this change, and in Lua 5.1, all function() expressions return a unique, newly created, object. For example, calling foo multiple times returns multiple different objects:

function foo()
    return function() end
end

In Lua 5.2 and 5.3, this behavior changes so that function objects are sometimes shared. Luau is getting a similar change, so that calling foo() multiple times will return the same function object here.

While the specific semantics are subject to change, an easy way to think about it is that the behavior of calling the function is always going to stay the same. For example, here foo will never return the same object for different values of arg, because doing so will break code that executes it:

function foo(arg)
    return function() print(arg) end
end

Currently Luau compiler makes a decision to share the functions when the functions don’t have upvalues (that is, don’t refer to locals outside of their scope), or all upvalues are declared at the script level (aka top-level) and aren’t modified after initialization.

This is a very important optimization:

It can often elide function allocations in calls to pcall, table.sort and other functional code, as well as object constructors for some OOP code, making code that relies on creating function objects faster to execute.

When will the change go live?

The change has been live in Studio for over a month now. We plan to enable this change for all live experiences on November 1st - since we’ve had this change live in Studio for many weeks and this was mentioned in release notes multiple times, we hope existing experiences already work well with it.

We briefly enabled this change last week, but it caused a subtle bug, so we decided to disable it, post this notice that the change is coming, and enable later.

Please let us know if you have concerns with this change. We understand that this change isn’t completely backwards compatible, but based on our testing almost all the code and almost all the experiences should work fine with this change enabled. We’d like to enable this across the platform to make sure we can realize the performance gains.

What does this change affect?

We know of two cases where the change affects behavior of existing code.

One comes with setfenv use:

setfenv can change the environment of an individual function, and if the function is the same, calling setfenv will modify the same function object. We automatically disable this optimization when the function that creates new functions has a custom environment, so code that uses sandboxing techniques should typically not be affected because it already calls setfenv before creating new functions.

Another one comes with using functions as table keys:

For example, if you used dummy function objects (function() end) to act as unique tokens, this code may break as you’ll see the same key being used. We suggest that the code shouldn’t rely on function identity in general, and in this specific case migrates to newproxy() as a way to generate unique tokens.

Again, please check your projects in Studio and let us know if you have any concerns with this change. We plan to enable it for all experiences on November 1st.

90 Likes

This topic was automatically opened after 10 minutes.

I use the observer pattern like the following all over my code:

--!nocheck

-- Here is some kind of observer pattern, common throughout my codebase in a decentralized way.
local observerSet = {}

local function subscribe(cb)
    observerSet[cb] = true
    return function()
        observerSet[cb] = nil
    end
end

local function yell(message)
    for cb in pairs(observerSet) do
        cb(message)
    end
end

local unsubscribe1 = subscribe(function(msg)
    print('1!')
end)
local unsubscribe2 = subscribe(function(msg)
    print('2!')
end)

-- This mimics Janitor/Maid/Finalizer types of classes in which callbacks are usually collected in a set.
local taskSet = {}
taskSet[unsubscribe1] = true
taskSet[unsubscribe2] = true -- If this callback object is identical to the one above, this could cause problems!

local function cleanup()
    for task in pairs(taskSet) do
        task()
    end
    taskSet = {}
end

yell() -- Expected outputs: 1!; 2!
cleanup()
yell() -- No output is expected here

In this case, there is an upvalue/closures, but I’m wondering if there are cases where there’s an observer + finalizer pattern where this can lead to undefined behavior? I have patterns like this all over my code and I’m not even sure how I would do a comprehensive search over my code base to find these edge cases, and my code heavily uses finalizer classes that store callbacks in a “set” like this.

I’m kind of worried, because this is the kind of thing that might go unnoticed and be super hard to debug due to the undefined behavior.

4 Likes

There needs to be a way to cancel tasks created by task. A lot of times you will delay a function like this:

task.delay(5, function()
     thing()
end)

But imagine that you want to cancel this, and then whatever code triggers the delay runs again. Now you have two delayed functions, and you need to check like this:

local function delayed()
     if globalDelayed ~= delayed then
        return
    end

    thing()
end

task.delay(delayed)

This is annoying, easy to forget to do, and maybe in the future with this optimization using an upvalue might not be enough so you need to keep a separate table or userdata to compare to.

4 Likes

Closures that are created in different source code locations will never be shared, so I suspect that this will not be a problem in a pattern like this.

The only case where it would be would be one where you’re subscribing a function that doesn’t depend on the environment and expecting a call to that function to occur multiple times:

for i=1,2 do
    subscribe(function() print("hi") end)
end

cleanup()

Does that make it better? :slight_smile:

edit I forgot to note that in spirit this change should be compatible with the callback pattern like this in general. Note that only functions that do the exact same thing using the exact same arguments would be shared. That is, f == g is only true if calling f and calling g have equivalent behavior. I’d expect the callback managers to manage callbacks with unique behavior.

3 Likes
--!nocheck

local observerSet = {}

local function subscribe(cb)
    observerSet[cb] = true
    return function()
        observerSet[cb] = nil
    end
end

local function yell(message)
    for cb in pairs(observerSet) do
        cb(message)
    end
end

for i=1,2 do
	subscribe(function() print("hi") end)
end

yell() -- Old behavior: prints 'hi' twice; new behavior: prints 'hi' once

Not going to lie, this gives me a lot anxiety. I don’t know if there’s cases where this undefined behavior matters in my code, but I don’t know how I would debug it to find out.

6 Likes

That’s correct, but note that this code before that change would print it once:

function hi() print("hi") end

for i=1,2 do
	subscribe(hi)
end
3 Likes

What about self-references? As an example, here’s a simplified version of my signal implementation with the irrelevant bits cut out:

local function newSignal()
	local listeners = {}
	local function connect()
		local function disconnect()
			listeners[disconnect] = nil
		end
		listeners[disconnect] = true
		return disconnect
	end
	return connect
end

This implementation saves a bit of memory by reusing the disconnect function as a unique token. The upvalues in this case are the listeners table and the disconnect function itself. listeners is not unique per call to connect, so it will not force disconnect to be unique. On the other hand, disconnect refers to itself, so how is that resolved?

1 Like

This code would currently not be affected because the compiler will notice that listeners value will change on different invocations of newSignal. I do believe this code would not work in Lua 5.2/5.3 though as they have a more general (and as such less safe) version of this optimization.

5 Likes

I don’t know if I will welcome this update.

I’m of the mind-set that in this case, it’s probably better left to the developer to decide whether or not they wish to return the same function, or a new one. It seems strange to force this optimization, when it’s potentially destructive to the functionality of existing codebases, and when developers are already fully capable of integrating a cache for identical functions in their systems. I won’t know how much of a headache it will be until I begin developing with this change pushed out, but it seems unnecessarily invasive.

3 Likes

Yeah this optimization is interesting in that:

  • It’s valuable for certain types of code
  • If we started from scratch we would likely incorporate it into the language spec
  • It’s part of Lua 5.2/5.3, but not 5.1 [or 5.4…]
  • There’s no reasonable way for us to flag code where it might change behavior automatically

So far we haven’t seen very apparent cases for why it’s a bad idea, but in private conversations we heard equal share of “this is awesome, we should do it even though it may change behavior” and “this makes me uneasy”.

Because we can’t automatically figure out the scope of impact, we need to gather feedback from the wider community, hence a dedicated thread (this one!) to know if it’s safe to proceed with a platform wide test.

5 Likes

Example based on something real world I have: you have a tool that when you click, it waits 3 seconds and then spawns a flying orb. But if a player unequips the tool before those 3 seconds are up, you want to stop the orb from being created. They can equip the tool again and trigger another delayed orb creation, so you need to make sure that the delayed orb creation function is the current one. This optimization could break this.

It looks like I’m only saved right now from having to check in dozens or possibly a few hundred callsites because everything is object-oriented and there’s very little top-level, and this could change in the future. There are many cases where the behavior of the function may be the same and you just don’t want a race condition.

This could all be fixed by returning a second value from task creation functions:

local time, taskCancel = task.delay(3, function(0

end)

taskCancel()

This is way too much boilerplate for something so common:

local globalTbl = {}

local function delayTask()
    local tbl = {} 
    task.delay(3, function()
        if globalTbl ~= tbl then
            return
        end
        
        thing()
    edn)
    globalTbl = tbl
end
2 Likes

I’m not sure I get it, can you explain how cancellation of task.delay is related to this change? (irrespective of whether we should support that as a built-in feature or not)

3 Likes

Would it elide the allocation in this case?

local DataStoreService = game:GetService"DataStoreService"
local GameData = DataStoreService:GetDataStore"GameData"
-- DataStoreService and GameData are never modified after initialization
-- ...
local Success,Result = pcall(function()
	return GameData:GetAsync"1234"
end)
-- or this case?
local Result2
local Success2 = pcall(function()
	Result2 = GameData:GetAsync"1234"
end)

What does the unless all upvalues are declared at the script level (aka top-level) mean? A function with no upvalues is also a function with all upvalues declared at script level (since all of 0 upvalues satisfy the condition).

local a = ...
local boom = function(i)print(i,a)end
if a == nil then
	debug.info(1,"f")(42)
	boom"A"
else
	boom"B"
end

If it elided the second function object then call boom"B" would see a as nil because that is what the value of a is in the first instance.

2 Likes

Yes for the first one. No for the second one, we also don’t elide allocations when upvalues are mutated.

Sorry this is worded poorly, this meant to say “or” – you’re correct that technically first is a subset of the second.

Here in the first invocation a is nil and in the second it’s 42 so the two function objects will be distinct, I believe. You should be able to check as this is enabled in Studio.

2 Likes

Does this mean we can write OOP classes like this:

function Class.new()
     local self = {}

     self.Member = 1
     self.Method = function()
         -- don't need to pass self using : because self is in this scope
     end
     return self
end

Instead of

local Class = {}
Class.__index = Class

function Class.new()
     local self = setmetatable({},Class)

     self.Member = 1

     return self
end

function Class:Method()
      
end

I don’t really understand all this technical stuff (what’s an upvalue?)

3 Likes

I have some questions (they are on the last line of each piece of code):

local Object = {}
local F1 = function()
    print(Object)
end
local F2 = function()
    print(Object)
end
print(F1 == F2) --> Will this be 'true' once this change comes up?
local function Create()
     local Object = {}
     return function()
        print(Object)
     end
end
print(Create() == Create()) --> Will this be 'false' once this change comes up?
local Object = {}
local function Create()
     return function()
        print(Object)
     end
end
print(Create() == Create()) --> Will this be 'true' once this change comes up?
2 Likes

Only the last one will be true; the first one is technically and theoretically in scope of this optimization (meaning, it follows the spirit of “functions have the exact same behavior when being called”), but we don’t perform this merging for different source locations right now and probably won’t do that in the future either.

5 Likes

From testing I’ve found that some upvalues that are not modified after initialization do not get optimized:

local up1 = 1 -- simple
for _=1,2 do
	print(function()return up1 end)
end
print"---"
local up2 = 0/0 -- NaN
for _=1,2 do
	print(function()return up2 end)
end
print"---"
local up3 = game
           :FindFirstChildWhichIsA"<<<ROOT>>>"
           :FindFirstAncestorWhichIsA"<<<ROOT>>>"
           :GetService"Workspace"
           .Terrain -- complex
for _=1,2 do
	print(function()return up3 end)
end
print"---"
local up4 = ... -- from arguments
for _=1,2 do
	print(function()return up4 end)
end

The second function function()return up2 end isn’t getting optimized, even though its only upvalue is at script level and is never changed after initialization.

It seems like calling the main function again will stop the optimization when it references only upvalues at the script level:

local a = ...
for _=1,2 do
	print(function(i)print(i,a)end)
end
if a == nil then
	debug.info(1,"f")(42)
end

This will print out the same function the first two times, and then the next functions will be unique

2 Likes

Correct; us using top-level values is really a heuristic that says “this code almost never runs more than once so upvalues will almost always be consistent”. This isn’t always true, one example being the one you posted (there’s others, like loadstring/debug.loadmodule). We validate the values by comparing them at runtime, which is also why using NaN breaks sharing.

2 Likes