From 8cc4bb21867900ea5bf48f0fedd65bdd0d7c41df Mon Sep 17 00:00:00 2001 From: zuqini Date: Sat, 11 Apr 2026 01:01:19 +0000 Subject: [PATCH] refactor: collapse opts merging to single authoritative path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the DEEP_MERGE strategy and drop `opts` from `merge_specs` entirely. Function-form opts could never be resolved at merge time (the `plugin` arg doesn't exist yet), so `resolve_opts` already had to run at load time — the merge-time DEEP_MERGE pass just produced an orphaned value that no runtime code read for its value, only for its nil-ness. This left two parallel opts pipelines and made `merged_spec.opts` subtly wrong whenever function-form and table-form were mixed. Now: - `merge_specs` skips `opts`. `merged_spec.opts` is always nil. - `resolve_all` computes `entry.has_opts` once, a boolean summary used by the three existence checks in `plugin_loader` and `startup`. - `run_config` always calls `resolve_opts(sorted_specs, plugin)` — the single-spec fast path is gone, which has the side benefit of always handing `config()` a fresh table instead of a live reference to the spec's underlying opts. - `merge_spec_array` now always folds from an empty base, so the returned table is always freshly allocated and never aliases an input spec. Tests updated to reflect the new contract: - `opts_test.lua`: assert `merged_spec.opts == nil` and check `entry.has_opts` / `sorted_specs[1].opts` instead. - `merge_test.lua`: the old "merge_specs deep merges opts" test now asserts merge_specs skips opts and that `resolve_opts` is the deep-merge path. - `is_single_spec_test.lua`: same treatment. --- lua/zpack/merge.lua | 45 +++++++++++++++++++---------------- lua/zpack/plugin_loader.lua | 18 ++++---------- lua/zpack/startup.lua | 2 +- lua/zpack/types.lua | 1 + tests/is_single_spec_test.lua | 8 ++++--- tests/merge_test.lua | 19 +++++++++++---- tests/opts_test.lua | 21 +++++++++------- 7 files changed, 64 insertions(+), 50 deletions(-) diff --git a/lua/zpack/merge.lua b/lua/zpack/merge.lua index 6ca3806..0b3e6d6 100644 --- a/lua/zpack/merge.lua +++ b/lua/zpack/merge.lua @@ -2,10 +2,14 @@ local M = {} M.OVERRIDE = "override" M.LIST_EXTEND = "list_extend" -M.DEEP_MERGE = "deep_merge" M.AND_LOGIC_COND = "and_logic_cond" M.AND_LOGIC_ENABLED = "and_logic_enabled" +-- `opts` is intentionally absent from this table. It is resolved lazily at +-- load time by `resolve_opts` so that function-form opts (which receive the +-- accumulated opts and may return a replacement) are handled with a single +-- authoritative code path. `merge_specs` skips `opts` entirely; callers that +-- need to know "does any spec contribute opts?" read `entry.has_opts`. M.field_strategies = { name = M.OVERRIDE, main = M.OVERRIDE, @@ -26,8 +30,6 @@ M.field_strategies = { ft = M.LIST_EXTEND, keys = M.LIST_EXTEND, - opts = M.DEEP_MERGE, - cond = M.AND_LOGIC_COND, enabled = M.AND_LOGIC_ENABLED, } @@ -176,7 +178,10 @@ function M.merge_specs(base, incoming) for k in pairs(incoming) do all_keys[k] = true end for key in pairs(all_keys) do - if internal_fields[key] then + if key == "opts" then + -- Skip: opts is resolved lazily via resolve_opts at load time. + -- See the comment above M.field_strategies. + elseif internal_fields[key] then result[key] = incoming[key] ~= nil and incoming[key] or base[key] else local strategy = M.field_strategies[key] or M.OVERRIDE @@ -191,12 +196,6 @@ function M.merge_specs(base, incoming) result[key] = incoming_val elseif strategy == M.LIST_EXTEND then result[key] = extend_unique(to_array(base_val), to_array(incoming_val)) - elseif strategy == M.DEEP_MERGE then - if type(base_val) == "table" and type(incoming_val) == "table" then - result[key] = vim.tbl_deep_extend("force", base_val, incoming_val) - else - result[key] = incoming_val - end elseif strategy == M.AND_LOGIC_COND then result[key] = merge_and_cond(base_val, incoming_val) elseif strategy == M.AND_LOGIC_ENABLED then @@ -227,19 +226,15 @@ function M.sort_specs(specs) return sorted end ----Merge an array of specs in order (lowest priority first) +---Merge an array of specs in order (lowest priority first). +---Always returns a fresh table (never a reference to any input spec), and +---`opts` is always absent from the result — it is resolved at load time via +---`resolve_opts` so function-form opts are handled consistently. ---@param specs zpack.Spec[] ---@return zpack.Spec function M.merge_spec_array(specs) - if #specs == 0 then - return {} - end - if #specs == 1 then - return specs[1] - end - - local result = specs[1] - for i = 2, #specs do + local result = {} + for i = 1, #specs do result = M.merge_specs(result, specs[i]) end return result @@ -368,6 +363,16 @@ function M.resolve_all() entry.sorted_specs = M.sort_specs(entry.specs) entry.merged_spec = M.merge_spec_array(entry.sorted_specs) entry.enabled_result = utils.check_enabled(entry.merged_spec) + -- `opts` is deliberately not stored on merged_spec; compute a boolean + -- summary once here so existence checks in plugin_loader / startup can + -- answer "does any spec contribute opts?" without re-scanning. + entry.has_opts = false + for _, s in ipairs(entry.sorted_specs) do + if s.opts ~= nil then + entry.has_opts = true + break + end + end -- cond_result is intentionally left nil here. It is written later by -- registration.register_all's vim.pack.add load callback (which needs -- the live plugin arg for function-form conds). Readers must treat diff --git a/lua/zpack/plugin_loader.lua b/lua/zpack/plugin_loader.lua index 90a2627..0939193 100644 --- a/lua/zpack/plugin_loader.lua +++ b/lua/zpack/plugin_loader.lua @@ -37,17 +37,9 @@ end ---@param spec zpack.Spec function M.run_config(src, plugin, spec) local registry_entry = state.spec_registry[src] - local resolved_opts - if registry_entry.sorted_specs and #registry_entry.sorted_specs > 1 then - resolved_opts = merge.resolve_opts(registry_entry.sorted_specs, plugin) - else - local opts = spec.opts - if type(opts) == "function" then - resolved_opts = opts(plugin, {}) or {} - else - resolved_opts = opts or {} - end - end + -- resolve_opts is the single authoritative path for opts; it walks + -- sorted_specs fresh so function-form opts compose correctly. + local resolved_opts = merge.resolve_opts(registry_entry.sorted_specs or {}, plugin) local main = utils.resolve_main(plugin, spec) if type(spec.config) == "function" then @@ -56,7 +48,7 @@ function M.run_config(src, plugin, spec) if not ok then utils.schedule_notify(("Failed to run config for %s: %s"):format(src, err), vim.log.levels.ERROR) end - elseif spec.config == true or spec.opts ~= nil then + elseif spec.config == true or registry_entry.has_opts then if not main then utils.schedule_notify( ("Could not determine main module for %s. Please set `main` explicitly or use `config = function() ... end`.") @@ -135,7 +127,7 @@ M.process_spec = function(pack_spec, opts) end end - if spec.config or spec.opts ~= nil then + if spec.config or registry_entry.has_opts then M.run_config(pack_spec.src, plugin, spec) end diff --git a/lua/zpack/startup.lua b/lua/zpack/startup.lua index 1d7ac0d..9e1d766 100644 --- a/lua/zpack/startup.lua +++ b/lua/zpack/startup.lua @@ -102,7 +102,7 @@ M.process_all = function(ctx) local entry = state.spec_registry[pack_spec.src] local spec = entry.merged_spec - if spec.config or spec.opts ~= nil then + if spec.config or entry.has_opts then loader.run_config(pack_spec.src, entry.plugin, spec) end end diff --git a/lua/zpack/types.lua b/lua/zpack/types.lua index 31c1d27..163e3ab 100644 --- a/lua/zpack/types.lua +++ b/lua/zpack/types.lua @@ -74,6 +74,7 @@ ---@field specs zpack.Spec[] ---@field sorted_specs? zpack.Spec[] ---@field merged_spec? zpack.Spec +---@field has_opts? boolean Whether any spec in this entry contributes opts; authoritative existence check ---@field plugin zpack.Plugin? ---@field load_status zpack.LoadStatus ---@field enabled_result? boolean diff --git a/tests/is_single_spec_test.lua b/tests/is_single_spec_test.lua index 65b71dc..7ed4321 100644 --- a/tests/is_single_spec_test.lua +++ b/tests/is_single_spec_test.lua @@ -17,7 +17,8 @@ return function() local entry = state.spec_registry['https://github.com/test/plugin'] helpers.assert_not_nil(entry, "plugin should be registered") - helpers.assert_equal(entry.merged_spec.opts.foo, true, "opts should be preserved") + helpers.assert_true(entry.has_opts, "entry should record that opts were contributed") + helpers.assert_equal(entry.sorted_specs[1].opts.foo, true, "opts should be preserved") helpers.cleanup_test_env() end) @@ -64,8 +65,9 @@ return function() local dep_entry = state.spec_registry['https://github.com/test/dep'] helpers.assert_not_nil(dep_entry, "dependency should be registered") - helpers.assert_not_nil(dep_entry.merged_spec.opts, "opts should be preserved on dependency") - helpers.assert_equal(dep_entry.merged_spec.opts.select.lookahead, true, + helpers.assert_true(dep_entry.has_opts, "dep entry should record that opts were contributed") + helpers.assert_not_nil(dep_entry.sorted_specs[1].opts, "opts should be preserved on dependency") + helpers.assert_equal(dep_entry.sorted_specs[1].opts.select.lookahead, true, "opts fields should be preserved") helpers.assert_not_nil(dep_entry.merged_spec.init, "init should be preserved on dependency") diff --git a/tests/merge_test.lua b/tests/merge_test.lua index 444b0bb..ea8e6de 100644 --- a/tests/merge_test.lua +++ b/tests/merge_test.lua @@ -251,18 +251,27 @@ return function() end) helpers.describe("Merge Module Unit Tests", function() - helpers.test("merge_specs deep merges opts", function() + helpers.test("merge_specs skips opts (resolved lazily by resolve_opts)", function() helpers.setup_test_env() local merge = require('zpack.merge') + -- opts is intentionally not collapsed by merge_specs. It is resolved + -- at load time by resolve_opts so function-form opts compose correctly + -- with a single authoritative code path. See merge.lua field_strategies + -- comment. Callers that need the deep-merged value should invoke + -- merge.resolve_opts(sorted_specs, plugin). local base = { opts = { a = 1, nested = { x = 1 } } } local incoming = { opts = { b = 2, nested = { y = 2 } } } local result = merge.merge_specs(base, incoming) - helpers.assert_equal(result.opts.a, 1) - helpers.assert_equal(result.opts.b, 2) - helpers.assert_equal(result.opts.nested.x, 1) - helpers.assert_equal(result.opts.nested.y, 2) + helpers.assert_nil(result.opts, "merge_specs must never populate opts") + + -- resolve_opts is the authoritative path and deep-merges table-form opts. + local resolved = merge.resolve_opts({ base, incoming }, {}) + helpers.assert_equal(resolved.a, 1) + helpers.assert_equal(resolved.b, 2) + helpers.assert_equal(resolved.nested.x, 1) + helpers.assert_equal(resolved.nested.y, 2) helpers.cleanup_test_env() end) diff --git a/tests/opts_test.lua b/tests/opts_test.lua index aee6552..43b63c5 100644 --- a/tests/opts_test.lua +++ b/tests/opts_test.lua @@ -2,7 +2,7 @@ local helpers = require('helpers') return function() helpers.describe("Plugin opts and auto-setup", function() - helpers.test("opts table is stored in spec", function() + helpers.test("opts table is recorded on the registry entry", function() helpers.setup_test_env() require('zpack').setup({ @@ -18,15 +18,18 @@ return function() helpers.flush_pending() local state = require('zpack.state') local src = 'https://github.com/test/plugin' - local spec = state.spec_registry[src].merged_spec - helpers.assert_not_nil(spec.opts, "opts should be stored") - helpers.assert_equal(spec.opts.enabled, true) - helpers.assert_equal(spec.opts.theme, 'dark') + local entry = state.spec_registry[src] + helpers.assert_true(entry.has_opts, "has_opts should be true") + -- opts is intentionally not stored on merged_spec; the raw value lives + -- on sorted_specs and is resolved at load time via resolve_opts. + helpers.assert_nil(entry.merged_spec.opts, "merged_spec.opts must always be nil") + helpers.assert_equal(entry.sorted_specs[1].opts.enabled, true) + helpers.assert_equal(entry.sorted_specs[1].opts.theme, 'dark') helpers.cleanup_test_env() end) - helpers.test("opts function is stored in spec", function() + helpers.test("opts function is recorded on the sorted spec", function() helpers.setup_test_env() local opts_fn = function() return { test = true } end @@ -43,8 +46,10 @@ return function() helpers.flush_pending() local state = require('zpack.state') local src = 'https://github.com/test/plugin' - local spec = state.spec_registry[src].merged_spec - helpers.assert_equal(type(spec.opts), 'function') + local entry = state.spec_registry[src] + helpers.assert_true(entry.has_opts, "has_opts should be true for function-form opts") + helpers.assert_nil(entry.merged_spec.opts, "merged_spec.opts must always be nil") + helpers.assert_equal(type(entry.sorted_specs[1].opts), 'function') helpers.cleanup_test_env() end)