* bug#24697: 25.1; find-lisp-object-file-name may return wrong locations @ 2016-10-14 22:19 Alex 2017-06-17 22:26 ` Alex 0 siblings, 1 reply; 6+ messages in thread From: Alex @ 2016-10-14 22:19 UTC (permalink / raw) To: 24697 #+BEGIN_SRC elisp (defface tab-width '((t :foreground "red")) "A face with the same name as a variable.") (find-lisp-object-file-name 'tab-width 'defface) #+END_SRC The above results in the symbol 'C-source'. The result should be nil as the 'tab-width' face was evaluated in *scratch*. Putting the above definition into an actual file and using `load-file' does return the proper source location. #+BEGIN_SRC elisp (find-lisp-object-file-name 'mapatoms 1) #+END_SRC This should return 'C-source' but instead it returns nil. #+BEGIN_SRC elisp (find-lisp-object-file-name 'tab-width 1) #+END_SRC This should return nil but instead it returns 'C-source'. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24697: 25.1; find-lisp-object-file-name may return wrong locations 2016-10-14 22:19 bug#24697: 25.1; find-lisp-object-file-name may return wrong locations Alex @ 2017-06-17 22:26 ` Alex 2017-06-18 19:51 ` Dmitry Gutov 0 siblings, 1 reply; 6+ messages in thread From: Alex @ 2017-06-17 22:26 UTC (permalink / raw) To: 24697 [-- Attachment #1: Type: text/plain, Size: 810 bytes --] Alex <agrambot@gmail.com> writes: > #+BEGIN_SRC elisp > (defface tab-width > '((t :foreground "red")) > "A face with the same name as a variable.") > > > (find-lisp-object-file-name 'tab-width 'defface) > #+END_SRC > > > The above results in the symbol 'C-source'. The result should be nil as > the 'tab-width' face was evaluated in *scratch*. > > Putting the above definition into an actual file and using `load-file' > does return the proper source location. > > #+BEGIN_SRC elisp > (find-lisp-object-file-name 'mapatoms 1) > #+END_SRC > > > This should return 'C-source' but instead it returns nil. > > #+BEGIN_SRC elisp > (find-lisp-object-file-name 'tab-width 1) > #+END_SRC > > This should return nil but instead it returns 'C-source'. The first and last of these are fixed by the following diff. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: find-lisp-object.diff --] [-- Type: text/x-diff, Size: 462 bytes --] diff --git a/lisp/help-fns.el b/lisp/help-fns.el index 2c635ffa50..bcf33131fa 100644 --- a/lisp/help-fns.el +++ b/lisp/help-fns.el @@ -319,6 +319,7 @@ find-lisp-object-file-name (help-C-file-name type 'subr) 'C-source)) ((and (not file-name) (symbolp object) + (eq type 'defvar) (integerp (get object 'variable-documentation))) ;; A variable defined in C. The form is from `describe-variable'. (if (get-buffer " *DOC*") [-- Attachment #3: Type: text/plain, Size: 476 bytes --] The second is a bit odder. For some reason, find-lisp-object-file-name searches for an internal function definition using TYPE instead of OBJECT. I would have expected it to use OBJECT like the rest of the tests do. I see no reason for the current behaviour. Either the documentation should be changed to clearly indicate the current behaviour, or the function should be changed so that OBJECTs for which (subrp (symbol-function OBJECT)) returns t should return 'C-source'. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#24697: 25.1; find-lisp-object-file-name may return wrong locations 2017-06-17 22:26 ` Alex @ 2017-06-18 19:51 ` Dmitry Gutov 2017-06-19 2:59 ` Alex 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Gutov @ 2017-06-18 19:51 UTC (permalink / raw) To: Alex, 24697 On 6/18/17 1:26 AM, Alex wrote: > The first and last of these are fixed by the following diff. > > ... > > > The second is a bit odder. For some reason, find-lisp-object-file-name > searches for an internal function definition using TYPE instead of > OBJECT. I would have expected it to use OBJECT like the rest of the > tests do. I see no reason for the current behaviour. Thanks. Do you think you can write test cases for these problems? There are some existing ones in test/lisp/help-fns-tests.el. > Either the documentation should be changed to clearly indicate the > current behaviour, or the function should be changed so that OBJECTs for > which (subrp (symbol-function OBJECT)) returns t should return > 'C-source'. With a test case, you might also find it easier to make a choice regarding this problem. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24697: 25.1; find-lisp-object-file-name may return wrong locations 2017-06-18 19:51 ` Dmitry Gutov @ 2017-06-19 2:59 ` Alex 2017-09-28 21:26 ` Dmitry Gutov 2020-08-11 13:26 ` Lars Ingebrigtsen 0 siblings, 2 replies; 6+ messages in thread From: Alex @ 2017-06-19 2:59 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 24697 [-- Attachment #1: Type: text/plain, Size: 1905 bytes --] Dmitry Gutov <dgutov@yandex.ru> writes: > On 6/18/17 1:26 AM, Alex wrote: > >> The first and last of these are fixed by the following diff. >> >> ... >> >> >> The second is a bit odder. For some reason, find-lisp-object-file-name >> searches for an internal function definition using TYPE instead of >> OBJECT. I would have expected it to use OBJECT like the rest of the >> tests do. I see no reason for the current behaviour. > > Thanks. Do you think you can write test cases for these problems? There are some > existing ones in test/lisp/help-fns-tests.el. Sure, I've attached a patch below for the simple cases. As I couldn't find a satisfactory way to make a temporary face, I just made an uninterned symbol that find-lisp-object-file-name would treat as an internal variable. >> Either the documentation should be changed to clearly indicate the >> current behaviour, or the function should be changed so that OBJECTs for >> which (subrp (symbol-function OBJECT)) returns t should return >> 'C-source'. > > With a test case, you might also find it easier to make a choice regarding this > problem. I'm not sure. I still don't understand why the design decision was made. I suppose one benefit is that one can search explicitly for internal functions rather than lisp functions, but the function could have just accepted 'subr instead of 'defun to do that. Perhaps the current use of searching with TYPE should be left in for backwards compatibility (a Github search shows at least 2 instances of 3rd-party code that makes use of that behaviour). For instance, here's how you find mapatoms' file: (find-lisp-object-file-name 'mapatoms (symbol-function 'mapatoms)) You should just be able to do the following: (find-lisp-object-file-name 'mapatoms 'defun) Or without searching for lisp functions named mapatoms first: (find-lisp-object-file-name 'mapatoms 'subr) What do you think? [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Only-search-for-a-variable-when-instructed.patch --] [-- Type: text/x-diff, Size: 1872 bytes --] From 2ff6a3417278a5fae3328b1bdae5afcc7094d646 Mon Sep 17 00:00:00 2001 From: Alexander Gramiak <agrambot@gmail.com> Date: Sun, 18 Jun 2017 18:59:11 -0600 Subject: [PATCH] Only search for a variable when instructed * lisp/help-fns.el (find-lisp-object-file-name): Check for 'defvar argument before searching for an internal variable (Bug#24697). * test/lisp/help-fns-tests.el: New tests. --- lisp/help-fns.el | 1 + test/lisp/help-fns-tests.el | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/lisp/help-fns.el b/lisp/help-fns.el index 2c635ffa50..bcf33131fa 100644 --- a/lisp/help-fns.el +++ b/lisp/help-fns.el @@ -319,6 +319,7 @@ find-lisp-object-file-name (help-C-file-name type 'subr) 'C-source)) ((and (not file-name) (symbolp object) + (eq type 'defvar) (integerp (get object 'variable-documentation))) ;; A variable defined in C. The form is from `describe-variable'. (if (get-buffer " *DOC*") diff --git a/test/lisp/help-fns-tests.el b/test/lisp/help-fns-tests.el index 0ab6c3cae7..98898624db 100644 --- a/test/lisp/help-fns-tests.el +++ b/test/lisp/help-fns-tests.el @@ -118,4 +118,16 @@ defgh\\\[universal-argument\]b\`c\'d\\e\"f (goto-char (point-min)) (should (looking-at "^font-lock-comment-face is ")))) +\f +;;; Tests for find-lisp-object-file-name +(ert-deftest help-fns-test-bug24697-function-search () + (should-not (find-lisp-object-file-name 'tab-width 1))) + +(ert-deftest help-fns-test-bug24697-non-internal-variable () + (let ((help-fns--test-var (make-symbol "help-fns--test-var"))) + ;; simulate an internal variable + (put help-fns--test-var 'variable-documentation 1) + (should-not (find-lisp-object-file-name help-fns--test-var 'defface)) + (should-not (find-lisp-object-file-name help-fns--test-var 1)))) + ;;; help-fns.el ends here -- 2.11.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#24697: 25.1; find-lisp-object-file-name may return wrong locations 2017-06-19 2:59 ` Alex @ 2017-09-28 21:26 ` Dmitry Gutov 2020-08-11 13:26 ` Lars Ingebrigtsen 1 sibling, 0 replies; 6+ messages in thread From: Dmitry Gutov @ 2017-09-28 21:26 UTC (permalink / raw) To: Alex; +Cc: 24697 On 6/19/17 5:59 AM, Alex wrote: >> Thanks. Do you think you can write test cases for these problems? There are some >> existing ones in test/lisp/help-fns-tests.el. > > Sure, I've attached a patch below for the simple cases. As I couldn't > find a satisfactory way to make a temporary face, I just made an > uninterned symbol that find-lisp-object-file-name would treat as an > internal variable. Thanks. Now, the patch looks correct to me, but did you encounter a practical problem that prompted you to look into this discrepancy? I'd like to know what it was. >> With a test case, you might also find it easier to make a choice regarding this >> problem. > > I'm not sure. I still don't understand why the design decision was made. > I suppose one benefit is that one can search explicitly for internal > functions rather than lisp functions, but the function could have just > accepted 'subr instead of 'defun to do that. There is a FIXME comment with the same question there. So you are not alone in wondering. > Perhaps the current use of searching with TYPE should be left in for > backwards compatibility (a Github search shows at least 2 instances of > 3rd-party code that makes use of that behaviour). > > For instance, here's how you find mapatoms' file: > > (find-lisp-object-file-name 'mapatoms (symbol-function 'mapatoms)) > > You should just be able to do the following: > > (find-lisp-object-file-name 'mapatoms 'defun) > > Or without searching for lisp functions named mapatoms first: > > (find-lisp-object-file-name 'mapatoms 'subr) > > What do you think? Maybe you're right, but backward compatibility seems important here as well. You're talking about a separate change, right? We could consider it for Emacs 27. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24697: 25.1; find-lisp-object-file-name may return wrong locations 2017-06-19 2:59 ` Alex 2017-09-28 21:26 ` Dmitry Gutov @ 2020-08-11 13:26 ` Lars Ingebrigtsen 1 sibling, 0 replies; 6+ messages in thread From: Lars Ingebrigtsen @ 2020-08-11 13:26 UTC (permalink / raw) To: Alex; +Cc: 24697, Dmitry Gutov Alex <agrambot@gmail.com> writes: >> Thanks. Do you think you can write test cases for these problems? >> There are some >> existing ones in test/lisp/help-fns-tests.el. > > Sure, I've attached a patch below for the simple cases. As I couldn't > find a satisfactory way to make a temporary face, I just made an > uninterned symbol that find-lisp-object-file-name would treat as an > internal variable. Thanks; I've applied your patch to Emacs 28. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-11 13:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-14 22:19 bug#24697: 25.1; find-lisp-object-file-name may return wrong locations Alex 2017-06-17 22:26 ` Alex 2017-06-18 19:51 ` Dmitry Gutov 2017-06-19 2:59 ` Alex 2017-09-28 21:26 ` Dmitry Gutov 2020-08-11 13:26 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).