Layer dependencies mask user-configured layers (specific issue: can't disable auto-completion for org)

[dankessler]

trafficstars

Description :octocat:

When I have the lsp layer enabled, auto-completion does not respect the :disabled-for org property and so company remains enabled for Org buffers. This seems to be a symptom of a much more general issue where layer dependencies mask user-configured layers

Reproduction guide :beetle:

• Start Emacs
• Open a buffer in Org-Mode (e.g., SPC b b tmp.org)
• Start typing a word, e.g., Thi

Observed behaviour: :eyes: :broken_heart: Company jumps in to attempt to auto-complete, even though when I load auto-completion, I use

(auto-completion :disabled-for org)

Expected behaviour: :heart: :smile: Company is disabled in Org buffers by dint of the :disabled-for org option given when auto-completion is loaded

System Info :computer:

• OS: darwin
• Emacs: 27.2
• Spacemacs: 0.300.0
• Spacemacs branch: develop (rev. 120ce6959)
• Graphic display: t
• Distribution: spacemacs
• Editing style: hybrid
• Completion: helm
• Layers:

(lsp
 (auto-completion :disabled-for org :variables some-var nil)
 emacs-lisp git
 (helm :variables helm-use-fuzzy 'source)
 osx org pdf
 (shell :variables shell-default-height 30 shell-default-position 'bottom)
 spacemacs-navigation spell-checking syntax-checking treemacs)

• System configuration features: NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS MODULES THREADS JSON PDUMPER GMP

Explanation

Above I have described how to reproduce this issue specifically for org mode (somewhat relevant to #8175 as I am using the "idiomatic" way of disabling auto-completion for org as documented in the manual), but I think this issue is more general. Because the lsp layer declares auto-completion as a dependency, spacemacs ignores any configuration I had set explicitly in my (auto-completion :disabled-for org) invocation. This happens regardless of ordering (I've tried moving lsp earlier/later in the layer list). A specific consequence is that whenever lsp layer is enabled, auto-completion layer will be enabled and will ignore any user-config specified for auto-completion layer. For example, in #13464 the OP is trying to do virtually the same thing as me but for python mode, and I notice that they have the lsp layer enabled. I suspect that when lsp's dependency auto-completion is loaded, it tramples the configuration OP had specified.

From reading through core-configuration-layer.el, I think that dependencies are added to the layer hash table last and so mask the original layer object. In my layer definition above, you can see that I've added :variables some-var nil to the auto-completion setup. If I run (oref (configuration-layer/get-layer 'auto-completion) :variables), the response is nil. If I disable the lsp layer and relaunch spacemacs, then not only is company appropriately disabled in org buffers, but running (oref (configuration-layer/get-layer 'auto-completion) :variables) returns the some-var variable I had defined.

The fix is probably to first check whether "dependency" layers were also explicitly activated by the user, and if so, to just stop there and only add the layer with a "vanilla" configuration if the user did not also explicitly add the layer.

[dankessler]

I think the problem is happening here where the "configured" version of the layer is already in the hash table, but then the dependency (which has no config) gets added next and overwrites it.

It wouldn't be too hard to add a bit of logic here so that layers that are added earlier get priority (and the hash table is only updated if the key doesn't already exist) which I think would fix this since dependency layers get added after those that are explicitly added by the user, although of course if that logic changes (e.g., gets reversed) we could be right back where we started.

Happy to do a quick PR if this is the desired approach, but I'm new to the deeper internals of spacemacs and don't want to inadvertantly undermine this behavior if it's a deliberately engineered principle.

[lebensterben]

you can go ahead and try to fix it

[dankessler]

Turns out this is trickier than I thought. As far as I can tell, the hash table, which holds layers and their configs, gets populated three times.

1. All of the layers get added as part of the call to configuration-layer/discover-layers
2. The layers specified by the user get added (with their configs)
3. Layers specified as dependencies get added, overwriting the configs specified in (2)

While my attempt at just making the hash table refuse to take additional entries does prevent step 3 from overwriting step 2, however it also prevents step 2 from overwriting step 1, which is necessary in order for the user config (like disabled-for org) to have any effect.

Essentially, I think I need to know the rules governing the "addition" operation for layers, i.e., what do we do when a layer (with config) is explicitly declared by a user and it is declared as a dependency (possibly with config) by one or more layers (either explicitly declared or themselves a dependency of an explicitly declared layer or another dependency).

As an example, suppose the user specifies the following for dotspacemacs-configuration-layers

`((auto-completion :variables some-var t :disabled-for org)
other-layer)

and other-layer has a file layers.el which specifies

(configuration-layer/declare-layer-dependencies 
`(auto-completion :variables some-var nil :disabled-for emacs-lisp)

what should the "winning" configuration of the auto-completion layer be? It seems reasonably to use the object-add-to-list function for things like :disabled-for and perhaps we could do something similar for :variables (although the extreme case where varx has different values in the layers being "added" is thorny).

The docstring for configuration-layer/make-layer suggests that if an obj (layer) is passed, then the layer-specs will be copied to it, but that means literally copied overwriting whatever is there. Perhaps this could be changed so that it returns the "union" (in some sensibly defined sense) of the respective slots of both obj and the specification in layer-specs.

[lebensterben]

ideally we should discover layer and identify any dependencies, then topologically sort them by out-degree (number of times being a dependency of other layer), then load the layer config altogether.

layer config declared in dotspacemacs should always take priority over dependency layers. Should they conflict, just throw an error.

[lebensterben]

we can refrain ourselves that a dependency layer must have empty (or default) config, and when it's also directly declared in dotspacemacs, we just take the config from dotspacemacs.

[lebensterben]

1. parse the dotspacemacs and find layers declared by users, store them in a alist in the form of ((layer-name . layer-config) ...)
2. Parse layers.el of the layers found in step 1, and store them in a hashtable where keys are names of dependency layers and values are lists of other layers that require them.
3. Topologically sort the hashtable in step 2. The result is a list whose elements are keys of hashtable, and those with more dependent layers are ranked first.
4. Combine alist in step 1 and list in step 2. If a layer is in both lists, the one from alist takes precedence.

[lebensterben]

Note that the total number of layers is rather small, at least currently, so any naive implementation of sorting algorithm won't be too slow.

After all we are only sorting those layers required by other layers, which is of an even smaller number. (Should this number be large, it would already been an issue for our layer system as we explicitly want to avoid dependencies.)

[dankessler]

I went searching across GitHub for topological-sort implementations and found links back to this implementation in Common Lisp (but which at a quick glance looks already elisp-compatible).

No other updates for now; I'll need to wrap my head back around the layer discovery/configuration flow, but once I do that I can take a crack at this.

[dankessler]

After some more digging and thinking, I believe this is less complex than it appeared.

The only way to declare a dependency is with the function configuration-layer/declare-layer-dependencies, which takes only layer names. Thus, at present it's not possible to specify further customization when declaring a dependency. In this case, I think the only place the customizations can come into play is with layers that are manually specified by the user.

So, the priority scheme should be that user-specified customizations are always honored (as previously suggested), but I think there's no need to further determine priority among (sub)-dependencies since they are all specified in vanilla form.

A crude hack (which I've confirmed seems to work) is to modify configuration-layer--declare-used-layers so that it loops over layer-specs (which adds the customized layers to the hash table), then handles dependencies (which potentially overwrites the customizations), then loops over layer-specs again (overwriting with the customizations again), but that's ugly.

I'm working on a (hopefully more elegant) solution where I add a slot to the cfgl-layer class which tracks whether a layer is a "user-layer" (i.e., specified in the dotfile) or not (e.g., a dependency). I then adjust some relevant functions to both set this correctly and to respect it to avoid clobbering user customizations. I'll submit a PR once I have it working locally so that I can get input on the design pattern I'm using.

Along the way I noticed an issue with cfgl-layer which I've tried to fix in #15376

[lebensterben]

you can define a subclass user-layer that inherits from our official layer class. And instantiated user-layers can be determined by user-layer-p.

[dankessler]

That's an interesting idea. I can then tweak the logic that retrieves/overwrites the hash table of layers to behave (slightly) differently if the object retrieved (or object to be inserted) is of the (say) cfgl-user-layer subclasss

[YujiShen]

Still experience this. Any solution besides disabling auto-completion completely?

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this issue is still valid!