Combine cljs and clj namespace alias candidates for magic requires

[dgtized]

This is the companion feature issue for #525 as requested.

Currently if a cljr-slash magic require is triggered in a project with mixed cljs, cljc and clj files, cljr-slash first prompts for the language context and then only allows selection of namespace aliases from cases that were previously seen in that context.

As example, in scratch.cljc if I type gl/

After selecting clj I get:

And if I select cljs I am prompted to select either thi.ng.geom.line or thi.ng.geom.gl.core, which then adds the selected namespace:

In this case, both thi.ng.geom.line & thi.ng.geom.gl.core are cljc libraries and are applicable in both a clj and cljs context. However for my example project, thi.ng.geom.gl.core has only been used in a cljs context previously, so the middleware incorrectly believes that alias is only applicable in a cljs context.

There are several problems above, but I think think the two most critical is that cljr-slash incorrectly assumes that namespaces previously encountered in one context should only be presented in a matching context. The second and overlapping interface problem is that it prompts twice, and doesn't suggest all the available choices for the user to decide in a single prompt.

In #525, with the defcustom cljr-magic-require-prompts-includes-context enabled, the behavior is:

This is a single prompt, and allows the user to make their own determination if the other available aliases are applicable in the current context. This functionality is complete in #525 without requiring any changes to the middleware.

Potential Future Improvements

Force a prompt where language context does matter

In #525 if only one namespace usage matches an alias, regardless of the language context, it will immediately insert that without prompting. If we had more reliable detection of language context, or even if the current file did not match any of the language contexts the alias had previously been seen in, then it could prompt the user to verify the new require makes sense before inserting a new require.

Simplifying middleware response to reduce elisp code and allow more fine grained responses in the future

#525 converts the middleware responses from an EDN format like:

{:clj  {t [clojure.test] set [clojure.set] alias [namespace]}
 :cljs {t [cljs.test] set [clojure.set] alias2 [namespace2}}

Into a list of lists like:

'(((alias namespace (:clj))
   (alias2 namespace2 (:cljs))
   (set clojure.set (:clj :cljs))
   (t clojure.test (:clj))
   (t cljs.test (:cljs))

What I would propose is that the middleware should instead return something like:

[{alias: "alias" libspec: "[namespace :as alias]" context: #{:clj}}
  {alias: "alias2" libspec: "[namespace2 :as alias]" context: #{:cljs}}
  {alias: "set", libspec: "[clojure.set :as set]" context: #{:clj :cljs}}
  {alias: "t", libspec: "[clojure.test :as t]" context: #{:clj}}
  {alias: "t", libspec: "[cljs.test :as t]" context: #{:cljs}}]

This would reduce the amount of elisp code to convert formats, and allow for future extensions. By listing exact libspecs, it allows the completing-read to return a libspec typed in by the user manually. It also allows suggesting libspecs for an alias with an :as-alias, :refer or :include-macros. A good example would be to promp with the following libspec in a CLJS file: [clojure.test :as t :refer [deftest is] :include-macros true]. This could also be extended to include a suggested ranking order, or another field for inserting matching :import fields, if we can reliably associate requires aliases to imports.

Automatically Suggest Reader Conditionals for Alias in CLJC file

Occasionally in a CLJC file, duplicate aliases are used in a reader conditional so that multiple libraries with identical functions can be used under a single namespace alias. For those cases, the middleware could respond something like:

[{alias: "priority" 
  libspec: "#?(:clj [clojure.data.priority-map :as priority] :cljs [tailrecursion.priority-map :as priority])" 
  context: #{:cljc}}
 {alias: "priority" libspec: "[tailrecursion.priority-map :as priority]" context: #{:cljs}}
 {alias: "priority" libspec: "[clojure.data.priority-map :as priority]" context: #{:clj}}]

Critically, it should only suggest a reader conditional if an existing libspec in the project uses a reader conditional to dispatch between different namespaces under a single alias. The latter two examples would only show up if there were clj or cljs files using that libspec in the project. This might be one of the few cases where a matching alias should be excluded from the prompt if the context does not match, as I don't think you can use a reader conditional in a cljs or clj file, and in those cases we should prefer the context specific namespace.

[dgtized]

A few specific cases to consider:
set/ should add [clojure.set :as set] regardless of language context if it's the only mapping of that alias to a namespace. It should only prompt if the project has another namespace aliased with set.

io/ in a CLJS file where the project previously used clojure.java.io, should probably force a prompt with a language context annotation or decline to add a require, as that namespace is only valid in a CLJ context.

alias/ where alias already maps to a.alias and b.core in some mixture of clj and cljs files, should prompt to select between [a.alias :as alias] and [b.core :as alias] and provide some context about language environments it was previously encountered in to help the user decide.

t/ should prompt for [cljs.test :as t] in a CLJS file, [clojure.test :as t] in a CLJ file if those libspecs are previously encountered. However, technically clojurescript supports internally aliasing cljs.test as clojure.test, so in the best possible world, in a cljs or cljc file it would prompt for [clojure.test :as t :include-macros true] along with any common :refer fields used elsewhere. Another example is with priority above when it's used to map to a reader conditional in CJLC context, or the context specific library in a CLJS or CLJ context.

---

One other future improvement to consider is:

Consider merging defcustom mappings with the list returned by the middleware

In addition to prompting for alias -> namespace mappings from the middleware, a user can add custom mappings to cljr-magic-require-namespaces. Those mappings are only visible if the middleware reports no matches to an alias. In those cases the prompt falls back to suggest examples from the defcustom mapping. It might be better to merge those defcustom examples with the middleware response, deleting any duplicates. The advantage there would be we could potentially look for closest matches from an alias to the set of namespace strings, if no existing alias matched.

Unfortunately, currently, the defcustom does not mark if an alias is for a particular language context. For the default settings, it will add [clojure.java.io :as io] in a CLJS file.

[vemv]

Thanks much for the issue! Will read carefully and ponder on it.

As a quick one, did you know about

clj-refactor.el/clj-refactor.el

Lines 1945 to 1949 in 32883b3

	(defcustom cljr-suggest-namespace-aliases t
	"If `t', `namespace-aliases' and `cljr-slash' will take into account suggested namespace aliases,
	following this convention: https://stuartsierra.com/2015/05/10/clojure-namespace-aliases"
	:group 'cljr
	:type 'boolean)

?

(This feature is best explained at clojure-emacs/refactor-nrepl#369 )

I'm giving it great use. Do you think it would compose well with what you envision? Any possible conflict, necessary refinement, etc?

[dgtized]

I was aware of cljr-suggest-namespace-aliases, but I don't think it particularly impacts any of the changes above though. As far as I understood, it's only present in the client to tell the middleware to add additional namespace suggestions. I suppose in my example above for the new middleware response edn, that it might be helpful to report if a namespace alias mapping appeared in the project or was injected because cljr-suggest-namespace-aliases was true. Regardless, the client is agnostic to where the alias suggestions are coming from.

[dgtized]

I'm not sure about cljr-add-missing-libspec though. At the moment that is a completely orthogonal feature, but I strongly suspect that there is overlap between cljr-slash and cljr-add-missing-libspec that we could consider unifying at some point.

[vemv]

Yeah, nowadays I only use cljr-add-missing-libspec for inserting imports. For require it's easy enough to type / and delete it, for letting cljr-slash do its thing.

[vemv]

Would you be so generous to fill a table approximately like this...?

Existing aliases in the codebase	Extension of the current buffer's filename	Offered completions (or direct action)

With all possible iterations.

That way I'll be able to see the intent/impact without any misunderstanding.

Thanks - V

[dgtized]

I believe I've already accounted for this both in the comment above & in test cases included in the PR. I will attempt to summarize again though;

If a codebase contains a previous usage of [clojure.set :as set] in any of cljs, cljc, or clj, and the user types set/ in any language context, and it's the only usage of the alias set, then [clojure.set :as set] will be inserted into the require.

If a codebase contains multiple usages of an alias, mapping to more than one namespace, when the user types alias/ in any file, then it will prompt the user to select one of the namespaces referenced by that alias, showing annotations of the language contexts each of those namespaces were previously used in.

If the codebase contains no matches for the alias, then it fallback to the listing in cljr-magic-require-namespaces as it always did, and then will prompt if there are multiple matches there or insert it immediately if it's the only candidate.

The key difference is the current file extension is not material. If I'm in a cljs, file and I'm referencing a cljc library, when I go to a clj or cljc file I still want to see that alias suggested because it's likely it's still applicable. If I'm in a cljc file, then I want suggestions from both cljs and cljc aliases as I may be using a library from either. The annotations simply represent file types they have previously appeared in as relayed by the middle-ware, leaving the decision to the user to decide if that's the appropriate namespace to use for the given alias.

In table form I guess that's:

input	existing aliases	completions	result
set/	clojure.set :as set	[clojure.set :as set ] (annotations)	inserts libspec
alias/	a.core :as alias b.core :as alias	[a.core :as alias] (annotations) [b.core :as alias] (annotations)	prompts user
set/	no previous use, but listed in cljr-magic-require-namespaces	[clojure.set :as set]	inserts libspec

Also, as before, if cljr-magic-requires is set to :prompt then it will always prompt with a completing read before inserting, even if just one option is available.

Again, I made sure to add the change under a defcustom, so this is all opt-in behavior. Every new method is tested and I'm happy to field questions if there are gaps there.

[vemv]

Thanks for the table!

Again, I made sure to add the change under a defcustom, so this is all opt-in behavior.

Regardless, features are meant to be agreed-on, non-transient, and coordinated with refactor-nrepl.

Otherwise you can always experiment indefinitely in a local fork (which, if you ask me, is exactly what I do for my own productivity).

I'll check #528 (comment) carefully, but please note that every time that I'm reading something that wasn't quite the requested thing (e.g. paragraphs, PR code), I'm taking time away from other things.

[vemv]

n.b. I'm essentally asking for this sort of collaboration model https://github.com/clj-kondo/clj-kondo/blob/af9db3c2bbf0ed592254b0f179ffef345de3a626/doc/dev.md#start-with-an-issue-before-writing-code which is not exactly unusual.

I'd understand if it can be frustrating at first, but ultimately everyone benefits from it!

[dgtized]

I have no trouble with that collaboration model. I also understand that it can be frustrating for you to have a change dropped in your lap without sufficient context. I do appreciate your time, and I realize you are maintaining a number of different projects, so I do understand that it takes time to get in and out of context. That is why I have tried to carefully explain the scope of the problem to help clarify context, both in the original PR and then again in this issue at your request. I apologize if that has turned out to be too verbose or somehow unclear and made it worse, that is not my intent. I'm just trying to improve the Clojurescript development experience in Emacs, and to give back to open source tooling that I have benefited form.

Unfortunately, I am still uncertain what the next steps are. I would greatly appreciate feedback on the feature as I believe it's a substantial improvement on the existing behavior that others will benefit from. I do appreciate that can take a while and you are busy with many projects, so I can wait if that's the current issue. I am worried that there is some kind of misunderstanding and you are waiting on something from me. I know we discussed adding an issue requesting changes to the refactor-nrepl middleware, both to simplify this feature and to support additional feature. I'm still prepared to work on that, but I believe those are dependent on our agreement on the overarching feature described here. Is there anything else I can contribute in terms of further development or explanation to help move this along as it currently stands?

[vemv]

(getting back to this asap)

[vemv]

I've drafted my thoughts and will split this issue into 3-4 issues soon enough

[dgtized]

Any updates on splitting this into smaller issues?

[vemv]

It's coming! Sorry for the delay

…

On Mon, Aug 15, 2022 at 11:20 PM Charles Comstock ***@***.***> wrote: Any updates on splitting this into smaller issues? — Reply to this email directly, view it on GitHub <#528 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI354RB3LZYDQREZR5WOH3VZKYDJANCNFSM535LYLWQ> . You are receiving this because you commented.Message ID: ***@***.***>

[vemv]

Alright, so the split is as follows:

#530
clojure-emacs/refactor-nrepl#384
#531

Each issue depends on the previous one. If interested you'd work on 530 and 531. I'd work on 384.

Sorry for the slowness, this was some dense design work which I spread across several days.

Do feel free to check out 384 carefully and possibly seek any clarification.

I have the feeling that the middleware would be implemented quite quicker than the tickets. Also, because of its design, it can be incrementally implemented (e.g. leave fancy :refers for slightly later) while changing no APIs at all.

Cheers - V