Fix `selectrum-read-library-name`.

Use find-library-source-path in Emacs versions starting with 28.1.

The variable find-function-source-path was renamed to find-library-source-path starting with Emacs 28.1.
Always use a text property in selectrum-read-library-name.

This fixes #577. If the user chooses a matching candidate with selectrum-submit-exact-input, then the returned candidate won't have the needed property. In that case, we select the first item in the propertized candidate list that is equal to the selected candidate.
Update changelog.

@raxod502 @clemera What was the need for adding the disambiguating prefix to candidates in selectrum-read-library-name?

I have a copy of map.el with Straight in my load-path. The current behavior of Selectrum is to show map twice, with the built-in version always before the downloaded version. There's no prefix property for these candidates, I guess since the paths are fully different.

Without Selectrum, it seems to me that Emacs 26 & 28.0.90 only show candidates for a single version of map.el, though the default completion method does include the extra map.elc candidate which refers to the downloaded version.

Since the built-in version of the library always comes before the downloaded version in the current Selectrum (ignoring the load-path order), Selectrum is causing the wrong library to be opened when finding the library at point with Embark (related to #577) whose symbol matches a candidate in the list.

I would like to change Selectrum to be closer to Emacs in representing the library once, like in the code below, but I don't want to accidentally re-introduce old bugs.

find-library also works on map.el and map.elc, so I feel like those should work in with Embark. This might mean not removing the almost duplicate candidates like map.elc from the list

Please advise.

(defun selectrum-read-library-name ()
  (let ((suffix-regexp (concat (regexp-opt (find-library-suffixes)) "\\'"))
        (table (make-hash-table :test #'equal))
        (lst nil))
    ;; For each directory in the load path, files matching the
    ;; suffixes returned by `find-library-suffixes'.
    (dolist (dir (or (if (<= 28 emacs-major-version)
                         find-library-source-path
                       find-function-source-path)
                     load-path))
      (condition-case _
          (mapc
           (lambda (entry)
             (unless (string-match-p "^\\.\\.?$" entry)
               (let* ((base (string-trim-right
                             (file-name-nondirectory entry)
                             suffix-regexp)))
                 (unless (gethash base table)
                   (puthash base entry table)))))
           (directory-files dir 'full suffix-regexp 'nosort))
        (file-error)))
    (maphash (lambda (key value)
               (push (propertize
                      key
                      ;; 'selectrum-candidate-display-suffix
                      ;; (concat " " value)
                      'fixedcase 'literal
                      'selectrum--lib-path value)
                     lst))
             table)
    ;; If the user chooses a matching candidate with
    ;; `selectrum-submit-exact-input', then the returned candidate
    ;; won't have the needed property. In that case, we select the
    ;; first item in the propertized candidate list that is `equal' to
    ;; the selected candidate. See issue #577.
    (let ((cands-copy (copy-sequence lst))
          (chosen-cand (selectrum--read "Library name: " lst
                                        :require-match t)))
      (or (get-text-property 0 'selectrum--lib-path chosen-cand)
          ;; Find the first candidate in `lst' that equals the chosen
          ;; candidate output.
          (cl-loop for lib-cand in cands-copy
                   when (equal chosen-cand lib-cand)
                   return (get-text-property 0 'selectrum--lib-path
                                             lib-cand))))))

Hmm, so, the original reason for this behavior was to make sure that if you have load-path shadows, then this fact is communicated to the user. It sounds like a bug that the built-in version of map.el is shown before the version from straight.el; the shadows should be sorted such that the topmost candidate is the one earliest on the load-path.

I think only showing libraries once would be suboptimal, since (if I understand correctly) it would make it no longer possible to realize that there is shadowing, and if so to navigate to the shadowed library. But we should definitely be showing the shadowed libraries later in the list.

I think these most recent changes should do it.

Only the disambiguated candidates are used interactively, but the file-name candidates are used as fall-backs via completion-table-in-turn. @zabe40, please test whether this works for you. I know that Selectrum had trouble with completion-table-in-turn over a year ago, but it seems to work for me now on Emacs 28.
As I read it, there was an assumption that multiple targets for the same library can't occur within the same directory. Now that candidates like "map.elc" are supported, that doesn't hold, and the bit of processing to avoid duplicate candidates for the same directory makes the command a tad slower. @raxod502, please let me know if you would like to change how this is implemented.

(defun selectrum-read-library-name ()
  "Read and return a library name.

Similar to `read-library-name' except it handles `load-path'
shadows correctly. Interactively, this function assumes that a
directory does not contain multiple versions of the same library.

While only library names are displayed interactively, file names
will be used as fallback candidates to accept the same input as
the built-in `read-library-name'."
  (eval-and-compile
    (require 'find-func))
  (let ((suffix-regexp
         ;; "elc" is excluded by `find-library-suffixes', but we want
         ;; to include it for candidates like `map.elc'.  Normal
         ;; completion doesn't seem to have that issue.
         (concat (regexp-opt (cons ".elc" (find-library-suffixes)))
                 "\\'"))
        (table (make-hash-table :test #'equal))
        (all-cands nil)           ; All library files
        (presented-cands nil)     ; Cleaned-up and disambiguated cands
        (alternate-cands nil))    ; Other candidates.
    (dolist (dir (or (if (<= 28 emacs-major-version)
                         find-library-source-path
                       find-function-source-path)
                     load-path))
      (condition-case _
          (let ((found-libs))
            (mapc
             (lambda (entry)
               (unless (string-match-p "^\\.\\.?$" entry)
                 (let ((base (string-trim-right
                              (file-name-nondirectory entry)
                              suffix-regexp)))
                   ;; When we disambiguate the same-named libraries,
                   ;; we assume that there is only one version of the
                   ;; library in each directory. We produce a unique,
                   ;; cleaned-up candidate for each unique directory.
                   (unless (member base found-libs)
                     (push entry (gethash base table))
                     (push base found-libs)))
                 (push (propertize (file-name-nondirectory entry)
                                   'fixedcase 'literal
                                   'selectrum--lib-path entry)
                       alternate-cands)))
             (directory-files dir 'full suffix-regexp 'nosort)))
        (file-error)))
    (maphash
     (lambda (lib-name paths)
       (setq paths (nreverse paths))
       (cl-block nil
         (let ((num-components 1)
               (max-components
                (apply #'max (mapcar (lambda (path)
                                       (1+ (cl-count ?/ path)))
                                     paths))))
           (while t
             (let ((abbrev-paths
                    (seq-uniq
                     (mapcar (lambda (path)
                               (string-trim-right
                                (selectrum--trailing-components
                                 num-components path)
                                suffix-regexp))
                             paths))))
               (when (or (= num-components max-components)
                         (= (length paths) (length abbrev-paths)))
                 (let ((candidate-paths
                        (mapcar (lambda (path)
                                  (propertize
                                   lib-name
                                   'selectrum-candidate-display-prefix
                                   (file-name-directory
                                    (file-name-sans-extension
                                     (selectrum--trailing-components
                                      num-components path)))
                                   'fixedcase 'literal
                                   'selectrum--lib-path path))
                                paths)))
                   (setq presented-cands
                         (nconc candidate-paths presented-cands)))
                 (cl-return)))
             (cl-incf num-components)))))
     table)
    (setq alternate-cands (nreverse alternate-cands)
          all-cands (append presented-cands alternate-cands))
    ;; If the user chooses a matching candidate with
    ;; `selectrum-submit-exact-input', then the returned candidate
    ;; won't have the needed property. In that case, we select the
    ;; first item in the propertized candidate list that is `equal' to
    ;; the selected candidate. See issue #577.
    (let ((chosen-cand
           (selectrum--read
            "Library name: " nil
            :require-match t
            :mc-table
            ;; Emacs accepts candidates like `map.el', `map.el.gz',
            ;; and `map.elc' as candidates. We want to fall back to
            ;; such candidates if none of the "cleaned up" candidates
            ;; match.
            (completion-table-in-turn
             (lambda (input predicate action)
               (complete-with-action action presented-cands
                                     input predicate))
             (lambda (input predicate action)
               (complete-with-action action alternate-cands
                                     input predicate))))))
      (or (get-text-property 0 'selectrum--lib-path chosen-cand)
          ;; Find the first candidate in `all-cands' that equals
          ;; the chosen candidate output.
          (cl-loop for lib-cand in all-cands
                   when (equal chosen-cand lib-cand)
                   return (get-text-property 0 'selectrum--lib-path
                                             lib-cand))))))

Hmm, now the call to selectrum--read is returning a string without text properties even when selecting the current candidate via selectrum-select-current-candidate, making it impossible to select a shadowed library. FYI, I'm on emacs 27.2, so I'm don't have any emacs 28 completion-table improvements (if there are any).

Hmm, I'm getting Symbol’s value as variable is void: find-library-source-path on GNU Emacs 28.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.25, cairo version 1.16.0) of 2021-09-06. Perhaps we should instead simply condition on whether the variable exists on not, rather than on the Emacs version?

After changing that, I tested your version and it seems to have the behavior that I would expect, although I did notice a peculiarity (this is also present in the version on master). If you type in an exact match, then shadowed libraries are not shown:

That may be intentional, it was just surprising to me so I thought I would mention it.

Hmm, now the call to selectrum--read is returning a string without text properties even when selecting the current candidate via selectrum-select-current-candidate, making it impossible to select a shadowed library. FYI, I'm on emacs 27.2, so I'm don't have any emacs 28 completion-table improvements (if there are any).

My apologies. Emacs 27 and below strip string properties when returning candidate from completion table functions. That's more optional in Emacs 28. I think that, to support all of the candidates that default completion does, it will be a normal list of candidates.

Hmm, I'm getting Symbol’s value as variable is void: find-library-source-path on GNU Emacs 28.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.25, cairo version 1.16.0) of 2021-09-06. Perhaps we should instead simply condition on whether the variable exists on not, rather than on the Emacs version?

I will do that.

After changing that, I tested your version and it seems to have the behavior that I would expect, although I did notice a peculiarity (this is also present in the version on master). If you type in an exact match, then shadowed libraries are not shown:

That may be intentional, it was just surprising to me so I thought I would mention it.

It is not, and I am surprised. I'm also seeing this with a normal list of candidates in Emacs 27.1, though the shadowed library is again made visible using map$ instead of just map. I will look into it more.

@zabe40, please try the most recent changes. It's still the function completion table, but it should work this time. I've tested it on Emacs 26.3 and Emacs 28. If it works, I'll merge these changes after cleaning up the names and comments.

Thanks.

These are the new definitions:

(cl-defun selectrum--read
    (prompt candidates &rest args &key
            default-candidate initial-input require-match
            history mc-table mc-predicate mc-allow-text-properties)
  "Prompt user with PROMPT to select one of CANDIDATES.
Return the selected string.

CANDIDATES is a list of strings or a function to dynamically
generate them. If CANDIDATES is a function, then it receives one
argument, the current user input, and returns the list of
strings. If CANDIDATES are nil the candidates will be computed
from MINIBUFFER-COMPLETION-TABLE.

Instead of a list of strings, the function may alternatively
return an alist with the following keys:
- `candidates': list of strings, as above.
- `input' (optional): transformed user input, used for
  highlighting (see `selectrum-highlight-candidates-function').

PROMPT should generally end in a colon and space. Additional
keyword ARGS are accepted.

DEFAULT-CANDIDATE, if provided, is sorted first in the list if
it's present. If can be a symbol, a string, or a list of those.
In case of a list the first candidate of the list gets sorted
first.

INITIAL-INPUT, if provided, is inserted into the user input area
initially (with point at the end).

REQUIRE-MATCH, if `confirm' or `confirm-after-completion' the
user needs to confirm the input if it isn't a member of the
candidates. Any other non-nil value won't let the user exit in
this case.

HISTORY is the `minibuffer-history-variable' to use (by default
`minibuffer-history').

For MC-TABLE and MC-PREDICATE see `minibuffer-completion-table'
and `minibuffer-completion-predicate'. They are used for internal
purposes and compatibility to Emacs completion API. They will be
locally in the minibuffer.

If MC-ALLOW-TEXT-PROPERTIES is non-nil, candidates selected from
function completion tables will always keep their text
properties. This argument is for internal purposes. Standard
Emacs behavior, with which Selectrum complies, is to strip text
properties except when selecting the default candidate."
  (let* ((minibuffer-allow-text-properties t)
         (resize-mini-windows 'grow-only)
         (prompt (selectrum--remove-default-from-prompt prompt))
         ;; <https://github.com/raxod502/selectrum/issues/99>
         (icomplete-mode nil)
         (buf (current-buffer))
         (res
          (selectrum--minibuffer-with-setup-hook
              (lambda ()
                ;; Already set the active flag as early as possible
                ;; so client setup hooks can use it to detect if
                ;; they are running in a Selectrum session.
                (setq-local selectrum-is-active t)
                (setq-local minibuffer-completion-table mc-table)
                (setq-local minibuffer-completion-predicate mc-predicate))
            (selectrum--minibuffer-with-setup-hook
                (:append (lambda ()
                           (setq-local selectrum--match-is-required
                                       require-match)
                           (selectrum--setup
                            candidates
                            (or minibuffer-default default-candidate)
                            buf)))
              (read-from-minibuffer
               prompt initial-input selectrum-minibuffer-map nil
               (or history 'minibuffer-history) default-candidate)))))
    (cond (mc-table
           ;; Behave like completing-read-default which strips the
           ;; text properties but leaves the default unchanged
           ;; when submitting the empty prompt to get it (see
           ;; #180, #107).
           (let ((exit-string (default-value 'selectrum--last-input))
                 (default (default-value 'selectrum--default-candidate)))
             (if (and exit-string
                      (string-empty-p exit-string)
                      (equal res default))
                 (or (car-safe default-candidate)
                     default-candidate)
               (if mc-allow-text-properties
                   res
                 (substring-no-properties res)))))
          (t res))))

;; In Emacs 28.1, `find-function-source-path' was renamed to
;; `find-library-source-path'.
(defvar find-library-source-path)
(defvar find-function-source-path)

;;;###autoload
(defun selectrum-read-library-name ()
  "Read and return a library name.

Similar to `read-library-name' except it handles `load-path'
shadows correctly. Interactively, this function assumes that a
directory does not contain multiple versions of the same library.

While only library names are displayed interactively, file names
will be used as fallback candidates to accept the same input as
the built-in `read-library-name'."
  (eval-and-compile
    (require 'find-func))
  (let ((suffix-regexp
         ;; "elc" is excluded by `find-library-suffixes', but we want
         ;; to include it for candidates like `map.elc'.  Normal
         ;; completion doesn't seem to have that issue.
         (concat (regexp-opt (cons ".elc" (find-library-suffixes)))
                 "\\'"))
        (table (make-hash-table :test #'equal))
        (lib-name-cands nil)     ; Cleaned-up and disambiguated cands
        (file-name-cands nil))    ; Other candidates.
    (dolist (dir (or (if (boundp 'find-library-source-path)
                         find-library-source-path
                       find-function-source-path)
                     load-path))
      (condition-case _
          (let ((found-libs))
            (mapc
             (lambda (entry)
               (unless (string-match-p "^\\.\\.?$" entry)
                 (let* ((file-name (file-name-nondirectory entry))
                        (base (string-trim-right
                               file-name suffix-regexp)))
                   ;; We want to visually disambiguate libraries
                   ;; for which there are multiple versions in the
                   ;; load path.  In normal Selectrum completion,
                   ;; we only want to display a single candidate
                   ;; for each directory in the load path in which the
                   ;; library is located.  For whatever reason,
                   ;; returning a path to an ELC file always opens
                   ;; the loaded version of the library, regardless of
                   ;; where the ELC file is located.
                   (unless (or (member base found-libs)
                               (string-match-p "\\.elc\\'" file-name))
                     (push entry (gethash base table))
                     (push base found-libs))
                   ;; Emacs only shows the non-shadowed library.
                   ;; For each library, in addition to the library
                   ;; name, it presents multiple file-name candidates
                   ;; for each library.
                   (unless (member file-name file-name-cands)
                     (push (propertize file-name
                                       'fixedcase 'literal
                                       'selectrum--lib-path entry)
                           file-name-cands)))))
             (directory-files dir 'full suffix-regexp 'nosort)))
        (file-error)))
    (maphash
     (lambda (lib-name paths)
       (setq paths (nreverse paths))
       (cl-block nil
         (let ((num-components 1)
               (max-components
                (apply #'max (mapcar (lambda (path)
                                       (1+ (cl-count ?/ path)))
                                     paths))))
           (while t
             (let ((abbrev-paths
                    (seq-uniq
                     (mapcar (lambda (path)
                               (string-trim-right
                                (selectrum--trailing-components
                                 num-components path)
                                suffix-regexp))
                             paths))))
               (when (or (= num-components max-components)
                         (= (length paths) (length abbrev-paths)))
                 (let ((candidate-paths
                        (mapcar (lambda (path)
                                  (propertize
                                   lib-name
                                   'selectrum-candidate-display-prefix
                                   (file-name-directory
                                    (file-name-sans-extension
                                     (selectrum--trailing-components
                                      num-components path)))
                                   'fixedcase 'literal
                                   'selectrum--lib-path path))
                                paths)))
                   (setq lib-name-cands
                         (nconc candidate-paths lib-name-cands)))
                 (cl-return)))
             (cl-incf num-components)))))
     table)
    ;; If the user chooses a matching candidate with
    ;; `selectrum-submit-exact-input', then the returned candidate
    ;; won't have the needed property. In that case, we select the
    ;; first item in the propertized candidate list that is `equal' to
    ;; the selected candidate. See issue #577.
    (let* ((all-cands (append lib-name-cands file-name-cands))
           (chosen-cand
            (selectrum--read "Library name: "  nil
                             :require-match t
                             :mc-allow-text-properties t
                             :mc-table (completion-table-in-turn
                                        lib-name-cands
                                        file-name-cands))))
      (or (get-text-property 0 'selectrum--lib-path chosen-cand)
          ;; Find the first candidate in `all-cands' that equals
          ;; the chosen candidate output.
          (cl-loop for lib-cand in all-cands
                   when (equal chosen-cand lib-cand)
                   return (get-text-property 0 'selectrum--lib-path
                                             lib-cand))))))

Yup, it works for me! Thanks so much for the fix!

radian-software / selectrum

Fix `selectrum-read-library-name`. #580