Address review on the role lookups

- player_has_any_permission and player_has_all_permission, matching the
  account checks the web ui has.
- Roles are no longer sorted on the way out of every lookup. get_ordered_roles
  and the public sort_roles cover the two guis and the command which present
  roles in order, and the highest role is found with a single scan.
- get_held_role_ids returns the list and the set it already built, rather than
  a second function rebuilding the set from the list.
- get_player_names collects into a set before listing, so a player holding the
  role in both the synced and the local list is counted once.
- The role metatable is registered directly under the plugin name, dropping
  the storage import.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

.gitignore

@@ -2,3 +2,4 @@ dist/
 node_modules/
 .vscode
 .luarc.json
+.tap/

exp_roles/module/control.lua

@@ -19,7 +19,6 @@ every player has.
 local clusterio_api = require("modules/clusterio/api")
 local compat = require("modules/clusterio/compat") --[[@as LibCompat]]
 local Async = require("modules/exp_util/async")
-local Storage = require("modules/exp_util/storage")
 
 --- @class ExpRoles
 local ExpRoles = {
@@ -45,7 +44,8 @@ local Role = {}
 ExpRoles._prototype = Role
 
 --- Registered so roles keep their methods across save and load
-local role_metatable = Storage.register_metatable("Role", { __index = Role })
+local role_metatable = { __index = Role }
+script.register_metatable("exp_roles.Role", role_metatable)
 
 --- @class ExpRoles.Role : ExpRoles.RolePrototype
 --- @field id number Clusterio role id
@@ -114,7 +114,7 @@ end
 --- Sort roles in place, the most privileged first
 --- @param roles ExpRoles.Role[]
 --- @return ExpRoles.Role[]
-local function sort_roles(roles)
+function ExpRoles.sort_roles(roles)
     table.sort(roles, function(a, b)
         if a.order == b.order then return a.id < b.id end
         return a.order < b.order
@@ -122,6 +122,19 @@ local function sort_roles(roles)
     return roles
 end
 
+--- Get the most privileged role of a list
+--- @param roles ExpRoles.Role[]
+--- @return ExpRoles.Role?
+local function highest_role_of(roles)
+    local highest
+    for _, role in pairs(roles) do
+        if highest == nil or role:is_higher_than(highest) then
+            highest = role
+        end
+    end
+    return highest
+end
+
 --- The server is represented by nil, or by a player object with index 0
 --- @param player LuaPlayer?
 --- @return LuaPlayer? # The player when it is not the server
@@ -132,7 +145,7 @@ end
 
 --- Role ids a player has been given, without the default role or priority applied
 --- @param player_name string
---- @return number[]
+--- @return number[], table<number, true>
 local function get_held_role_ids(player_name)
     local role_ids, seen = {}, {}
     for _, list in pairs{ script_data.synced_players[player_name], script_data.local_players[player_name] } do
@@ -143,18 +156,7 @@ local function get_held_role_ids(player_name)
             end
         end
     end
-    return role_ids
+    return role_ids, seen
-end
-
---- Role ids a player has been given, as a set
---- @param player_name string
---- @return table<number, true>
-local function get_held_role_set(player_name)
-    local rtn = {}
-    for _, role_id in pairs(get_held_role_ids(player_name)) do
-        rtn[role_id] = true
-    end
-    return rtn
 end
 
 --- Roles which apply to a player, with the default role and priority applied
@@ -183,7 +185,7 @@ local function get_effective_roles(player_name)
             rtn[#rtn + 1] = role
         end
     end
-    return sort_roles(rtn)
+    return rtn
 end
 
 --[[
@@ -207,14 +209,20 @@ function ExpRoles.get_role_by_name(name)
     return nil
 end
 
---- Get every role, the most privileged first
+--- Get every role, in no particular order
 --- @return ExpRoles.Role[]
 function ExpRoles.get_roles()
     local rtn = {}
     for _, role in pairs(script_data.roles) do
         rtn[#rtn + 1] = role
     end
-    return sort_roles(rtn)
+    return rtn
+end
+
+--- Get every role, the most privileged first
+--- @return ExpRoles.Role[]
+function ExpRoles.get_ordered_roles()
+    return ExpRoles.sort_roles(ExpRoles.get_roles())
 end
 
 --- Get the role every player holds
@@ -233,7 +241,7 @@ function ExpRoles.get_higher_roles(role)
             rtn[#rtn + 1] = other
         end
     end
-    return sort_roles(rtn)
+    return rtn
 end
 
 --- Get a role and every role less privileged than it, the default role excluded
@@ -246,7 +254,7 @@ function ExpRoles.get_lower_roles(role)
             rtn[#rtn + 1] = other
         end
     end
-    return sort_roles(rtn)
+    return rtn
 end
 
 --- Role used when there is no player, such as for commands run by the server
@@ -279,7 +287,7 @@ end
 --- @param player LuaPlayer? nil for the server
 --- @return ExpRoles.Role
 function ExpRoles.get_player_highest_role(player)
-    local role = ExpRoles.get_player_roles(player)[1]
+    local role = highest_role_of(ExpRoles.get_player_roles(player))
     return (assert(role, "Player has no roles, is the default role set and exp_roles syncing?"))
 end
 
@@ -287,21 +295,56 @@ end
     Permission checks
 ]]
 
---- Check if a player has a permission through any of their roles
+--- Check if any of the roles grants a permission
---- @param player LuaPlayer? nil for the server
+--- @param roles ExpRoles.Role[]
---- @param permission string A clusterio permission such as `exp_scenario.command.kill`
+--- @param permission string
 --- @return boolean
-function ExpRoles.player_has_permission(player, permission)
+local function roles_grant(roles, permission)
-    for _, role in pairs(ExpRoles.get_player_roles(player)) do
+    for _, role in pairs(roles) do
         local permissions = role.permissions
         if permissions["core.admin"] or permissions[permission] then
             return true
         end
     end
-
     return false
 end
 
+--- Check if a player has a permission through any of their roles
+--- @param player LuaPlayer? nil for the server
+--- @param permission string A clusterio permission such as `exp_scenario.command.kill`
+--- @return boolean
+function ExpRoles.player_has_permission(player, permission)
+    return roles_grant(ExpRoles.get_player_roles(player), permission)
+end
+
+--- Check if a player has at least one of the permissions
+--- @param player LuaPlayer? nil for the server
+--- @param ... string Clusterio permission names
+--- @return boolean
+function ExpRoles.player_has_any_permission(player, ...)
+    local roles = ExpRoles.get_player_roles(player)
+    for index = 1, select("#", ...) do
+        if roles_grant(roles, (select(index, ...))) then
+            return true
+        end
+    end
+    return false
+end
+
+--- Check if a player has every one of the permissions
+--- @param player LuaPlayer? nil for the server
+--- @param ... string Clusterio permission names
+--- @return boolean
+function ExpRoles.player_has_all_permission(player, ...)
+    local roles = ExpRoles.get_player_roles(player)
+    for index = 1, select("#", ...) do
+        if not roles_grant(roles, (select(index, ...))) then
+            return false
+        end
+    end
+    return true
+end
+
 --- Check if a player is more privileged than another player
 --- A player with core.admin, which includes the server, outranks every player
 --- @param player LuaPlayer? nil for the server
@@ -309,8 +352,8 @@ end
 --- @return boolean
 function ExpRoles.player_outranks(player, other)
     if ExpRoles.player_has_permission(player, "core.admin") then return true end
-    local highest = ExpRoles.get_player_roles(player)[1]
+    local highest = highest_role_of(ExpRoles.get_player_roles(player))
-    local other_highest = ExpRoles.get_player_roles(other)[1]
+    local other_highest = highest_role_of(ExpRoles.get_player_roles(other))
     if highest == nil then return false end
     return other_highest == nil or highest:is_higher_than(other_highest)
 end
@@ -354,7 +397,8 @@ function Role.has_player(self, player)
     local valid = not_server(player)
     if not valid then return self == server_role end
     if self.id == script_data.default_role_id then return true end
-    return get_held_role_set(valid.name)[self.id] == true
+    local _, held = get_held_role_ids(valid.name)
+    return held[self.id] == true
 end
 
 --- Get the names of every player who has been given this role
@@ -363,21 +407,23 @@ end
 --- @param self ExpRoles.Role
 --- @return string[]
 function Role.get_player_names(self)
-    local names, seen = {}, {}
+    -- A player can be in both lists, so collect into a set first
+    local seen = {}
     for _, players in pairs{ script_data.synced_players, script_data.local_players } do
         for player_name, role_ids in pairs(players) do
-            if not seen[player_name] then
+            for _, role_id in pairs(role_ids) do
-                for _, role_id in pairs(role_ids) do
+                if role_id == self.id then
-                    if role_id == self.id then
+                    seen[player_name] = true
-                        seen[player_name] = true
+                    break
-                        names[#names + 1] = player_name
-                        break
-                    end
                 end
             end
         end
     end
 
+    local names = {}
+    for player_name in pairs(seen) do
+        names[#names + 1] = player_name
+    end
     return names
 end
 
@@ -490,7 +536,7 @@ local function emit_held_diff(player_name, before, by_player_name, silent)
     local player = game.get_player(player_name)
     if not player then return end
 
-    local after = get_held_role_set(player_name)
+    local _, after = get_held_role_ids(player_name)
     local assigned, unassigned = {}, {}
     for role_id in pairs(after) do
         if not before[role_id] then assigned[#assigned + 1] = script_data.roles[role_id] end
@@ -500,7 +546,7 @@ local function emit_held_diff(player_name, before, by_player_name, silent)
     end
 
     if #assigned > 0 or #unassigned > 0 then
-        emit_player_roles_changed(player, sort_roles(assigned), sort_roles(unassigned), by_player_name, silent)
+        emit_player_roles_changed(player, assigned, unassigned, by_player_name, silent)
     end
 end
 
@@ -591,7 +637,7 @@ local function change_player_role(role, player, assign, options)
     options = options or {}
 
     local player_name = valid.name
-    local before = get_held_role_set(player_name)
+    local _, before = get_held_role_ids(player_name)
     if not apply_local_change(player_name, role, assign, not options.local_only) then return end
 
     if not options.local_only then
@@ -732,7 +778,7 @@ end
 function ExpRoles.receive_assignment_updates(records)
     for _, record in pairs(records) do
         local player_name = record.name
-        local before = get_held_role_set(player_name)
+        local _, before = get_held_role_ids(player_name)
 
         if record.is_deleted then
             script_data.synced_players[player_name] = nil

exp_scenario/module/commands/_types.lua

@@ -27,7 +27,7 @@ local types = {} --- @class (partial) Commands.types
 types.role =
     add("role", function(input)
         local names = {}
-        for index, role in ipairs(Roles.get_roles()) do
+        for index, role in ipairs(Roles.get_ordered_roles()) do
             names[index] = role.name
         end
 

exp_scenario/module/commands/roles.lua

@@ -7,7 +7,7 @@ local format_player_name = Commands.format_player_name_locale
 local format_text = Commands.format_rich_text_color_locale
 
 local Roles = require("modules/exp_roles")
-local get_roles = Roles.get_roles
+local get_ordered_roles = Roles.get_ordered_roles
 local get_player_roles = Roles.get_player_roles
 
 --- Assigns a role to a player
@@ -40,11 +40,11 @@ Commands.new("get-roles", { "exp-commands_roles.description-get" })
     :add_aliases{ "roles" }
     :register(function(player, other_player)
         --- @cast other_player LuaPlayer?
-        local roles = get_roles()
+        local roles = get_ordered_roles()
         local roles_formatted = { "" } --- @type LocalisedString
         local response = { "exp-commands_roles.list-roles", roles_formatted } --[[@as LocalisedString]]
         if other_player then
-            roles = get_player_roles(other_player)
+            roles = Roles.sort_roles(get_player_roles(other_player))
             response[1] = "exp-commands_roles.list-player"
             response[3] = format_player_name(other_player)
         end

exp_scenario/module/gui/player_list.lua

@@ -203,7 +203,7 @@ function Elements.player_table.calculate_row_data()
     -- Flatten the roles into a single ordered list
     local count = 0
     local row_data = {}
-    for _, role in ipairs(Roles.get_roles()) do
+    for _, role in ipairs(Roles.get_ordered_roles()) do
         local role_name = role.name
         if players[role_name] then
             for _, player in pairs(players[role_name]) do

exp_scenario/module/gui/readme.lua

@@ -214,7 +214,7 @@ define_tab(
         container.add{ type = "flow" }.style.height = 4
 
         local role_names = {}
-        for i, role in ipairs(Roles.get_player_roles(player)) do
+        for i, role in ipairs(Roles.sort_roles(Roles.get_player_roles(player))) do
             role_names[i] = role.name
         end