From 7acb3f1c060872b1802e7548d991ca8a9f0fef01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= Date: Mon, 1 May 2023 15:55:32 +0200 Subject: [PATCH 1/4] Add the function declaration and property `important-return-value` Now the declaration (declare (important-return-value t)) can be used to have the byte-compiler warn when the return value from a call is discarded (bug#61730). * lisp/emacs-lisp/bytecomp.el (byte-compile-form) (important-return-value-fns): Use the function property `important-return-value` instead of looking through a static list. * lisp/emacs-lisp/byte-run.el (byte-run--set-important-return-value) (defun-declarations-alist): New function declaration, setting the property of the same name. * lisp/emacs-lisp/cl-macs.el: * lisp/subr.el (assoc-default): Set the property. * doc/lispref/functions.texi (Declare Form): * doc/lispref/symbols.texi (Standard Properties): Document. * etc/NEWS: Announce. --- doc/lispref/functions.texi | 6 +++ doc/lispref/symbols.texi | 6 +++ etc/NEWS | 21 +++++++--- lisp/emacs-lisp/byte-run.el | 7 ++++ lisp/emacs-lisp/bytecomp.el | 82 ++++++++++--------------------------- lisp/emacs-lisp/cl-macs.el | 40 ++++++++++++++++++ lisp/subr.el | 1 + 7 files changed, 96 insertions(+), 67 deletions(-) diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi index 42441361fea..dc0d182d50d 100644 --- a/doc/lispref/functions.texi +++ b/doc/lispref/functions.texi @@ -2631,6 +2631,12 @@ so the byte compiler can ignore calls whose value is ignored. This is the same as the @code{side-effect-free} property of the function's symbol, @pxref{Standard Properties}. +@item (important-return-value @var{val}) +If @var{val} is non-@code{nil}, the byte compiler will warn about +calls to this function that do not use the returned value. This is the +same as the @code{important-return-value} property of the function's +symbol, @pxref{Standard Properties}. + @item (speed @var{n}) Specify the value of @code{native-comp-speed} in effect for native compilation of this function (@pxref{Native-Compilation Variables}). diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi index c6a0408abd1..34db0caf3a8 100644 --- a/doc/lispref/symbols.texi +++ b/doc/lispref/symbols.texi @@ -643,6 +643,12 @@ ignore a call whose value is unused. If the property's value is calls. In addition to byte compiler optimizations, this property is also used for determining function safety (@pxref{Function Safety}). +@item important-return-value +@cindex @code{important-return-value} property +A non-@code{nil} value makes the byte compiler warn about code that +calls the named function without using its returned value. This is +useful for functions where doing so is likely to be a mistake. + @item undo-inhibit-region If non-@code{nil}, the named function prevents the @code{undo} operation from being restricted to the active region, if @code{undo} is invoked diff --git a/etc/NEWS b/etc/NEWS index 87d312596cd..b989f80f3c3 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -507,17 +507,26 @@ the warning name 'suspicious'. --- *** Warn about more ignored function return values. The compiler now warns when the return value from certain functions is -ignored. Example: +implicitly ignored. Example: (progn (nreverse my-list) my-list) will elicit a warning because it is usually pointless to call -'nreverse' on a list without using the returned value. To silence the -warning, make use of the value in some way, such as assigning it to a -variable. You can also wrap the function call in '(ignore ...)'. +'nreverse' on a list without using the returned value. -This warning can be suppressed using 'with-suppressed-warnings' with -the warning name 'ignored-return-value'. +To silence the warning, make use of the value in some way, such as +assigning it to a variable. You can also wrap the function call in +'(ignore ...)', or use 'with-suppressed-warnings' with the warning +name 'ignored-return-value'. + +The warning will only be issued for calls to functions declared +'important-return-value' or 'side-effect-free' (but not 'error-free'). + ++++ +** New function declaration and property 'important-return-value'. +The declaration '(important-return-value t)' sets the +'important-return-value' property which indicates that the function +return value should probably not be thrown away implicitly. +++ ** New function 'file-user-uid'. diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el index fd9913d1be8..5b415c5e1f4 100644 --- a/lisp/emacs-lisp/byte-run.el +++ b/lisp/emacs-lisp/byte-run.el @@ -145,6 +145,11 @@ So far, FUNCTION can only be a symbol, not a lambda expression." (list 'function-put (list 'quote f) ''side-effect-free (list 'quote val)))) +(defalias 'byte-run--set-important-return-value + #'(lambda (f _args val) + (list 'function-put (list 'quote f) + ''important-return-value (list 'quote val)))) + (put 'compiler-macro 'edebug-declaration-spec '(&or symbolp ("lambda" &define lambda-list lambda-doc def-body))) @@ -226,6 +231,8 @@ This may shift errors from run-time to compile-time.") (list 'side-effect-free #'byte-run--set-side-effect-free "If non-nil, calls can be ignored if their value is unused. If `error-free', drop calls even if `byte-compile-delete-errors' is nil.") + (list 'important-return-value #'byte-run--set-important-return-value + "If non-nil, warn about calls not using the returned value.") (list 'compiler-macro #'byte-run--set-compiler-macro) (list 'doc-string #'byte-run--set-doc-string) (list 'indent #'byte-run--set-indent) diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el index c84c70971b3..6c804056ee7 100644 --- a/lisp/emacs-lisp/bytecomp.el +++ b/lisp/emacs-lisp/bytecomp.el @@ -3502,67 +3502,7 @@ lambda-expression." ;; so maybe we don't need to bother about it here? (setq form (cons 'progn (cdr form))) (setq handler #'byte-compile-progn)) - ((and (or sef - (memq (car form) - ;; FIXME: Use a function property (declaration) - ;; instead of this list. - '( - ;; Functions that are side-effect-free - ;; except for the behaviour of - ;; functions passed as argument. - mapcar mapcan mapconcat - cl-mapcar cl-mapcan cl-maplist cl-map cl-mapcon - cl-reduce - assoc assoc-default plist-get plist-member - cl-assoc cl-assoc-if cl-assoc-if-not - cl-rassoc cl-rassoc-if cl-rassoc-if-not - cl-member cl-member-if cl-member-if-not - cl-adjoin - cl-mismatch cl-search - cl-find cl-find-if cl-find-if-not - cl-position cl-position-if cl-position-if-not - cl-count cl-count-if cl-count-if-not - cl-remove cl-remove-if cl-remove-if-not - cl-member cl-member-if cl-member-if-not - cl-remove-duplicates - cl-subst cl-subst-if cl-subst-if-not - cl-substitute cl-substitute-if - cl-substitute-if-not - cl-sublis - cl-union cl-intersection - cl-set-difference cl-set-exclusive-or - cl-subsetp - cl-every cl-some cl-notevery cl-notany - cl-tree-equal - - ;; Functions that mutate and return a list. - cl-delete-if cl-delete-if-not - ;; `delete-dups' and `delete-consecutive-dups' - ;; never delete the first element so it's - ;; safe to ignore their return value, but - ;; this isn't the case with - ;; `cl-delete-duplicates'. - cl-delete-duplicates - cl-nsubst cl-nsubst-if cl-nsubst-if-not - cl-nsubstitute cl-nsubstitute-if - cl-nsubstitute-if-not - cl-nunion cl-nintersection - cl-nset-difference cl-nset-exclusive-or - cl-nreconc cl-nsublis - cl-merge - ;; It's safe to ignore the value of `sort' - ;; and `nreverse' when used on arrays, - ;; but most calls pass lists. - nreverse - sort cl-sort cl-stable-sort - - ;; Adding the following functions yields many - ;; positives; evaluate how many of them are - ;; false first. - - ;;delq delete cl-delete - ;;nconc plist-put - ))) + ((and (or sef (function-get (car form) 'important-return-value)) ;; Don't warn for arguments to `ignore'. (not (eq byte-compile--for-effect 'for-effect-no-warn)) (byte-compile-warning-enabled-p @@ -3598,6 +3538,26 @@ lambda-expression." (byte-compile-discard)) (pop byte-compile-form-stack))) +(let ((important-return-value-fns + '( + ;; These functions are side-effect-free except for the + ;; behaviour of functions passed as argument. + mapcar mapcan mapconcat + assoc plist-get plist-member + + ;; It's safe to ignore the value of `sort' and `nreverse' + ;; when used on arrays, but most calls pass lists. + nreverse sort + + ;; Adding these functions causes many warnings; + ;; evaluate how many of them are false first. + ;;delq delete + ;;nconc plist-put + ))) + (dolist (fn important-return-value-fns) + (put fn 'important-return-value t))) + + (defun byte-compile-normal-call (form) (when (and (symbolp (car form)) (byte-compile-warning-enabled-p 'callargs (car form))) diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index 5382e0a0a52..04567917189 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -3657,6 +3657,46 @@ macro that returns its `&whole' argument." '(cl-list* cl-acons cl-equalp cl-random-state-p copy-tree)) +;;; Things whose return value should probably be used. +(mapc (lambda (x) (function-put x 'important-return-value t)) + '( + ;; Functions that are side-effect-free except for the + ;; behaviour of functions passed as argument. + cl-mapcar cl-mapcan cl-maplist cl-map cl-mapcon + cl-reduce + cl-assoc cl-assoc-if cl-assoc-if-not + cl-rassoc cl-rassoc-if cl-rassoc-if-not + cl-member cl-member-if cl-member-if-not + cl-adjoin + cl-mismatch cl-search + cl-find cl-find-if cl-find-if-not + cl-position cl-position-if cl-position-if-not + cl-count cl-count-if cl-count-if-not + cl-remove cl-remove-if cl-remove-if-not + cl-remove-duplicates + cl-subst cl-subst-if cl-subst-if-not + cl-substitute cl-substitute-if cl-substitute-if-not + cl-sublis + cl-union cl-intersection cl-set-difference cl-set-exclusive-or + cl-subsetp + cl-every cl-some cl-notevery cl-notany + cl-tree-equal + + ;; Functions that mutate and return a list. + ;;cl-delete + cl-delete-if cl-delete-if-not + cl-delete-duplicates + cl-nsubst cl-nsubst-if cl-nsubst-if-not + cl-nsubstitute cl-nsubstitute-if cl-nsubstitute-if-not + cl-nunion cl-nintersection cl-nset-difference cl-nset-exclusive-or + cl-nreconc cl-nsublis + cl-merge + ;; It's safe to ignore the value of `cl-sort' and `cl-stable-sort' + ;; when used on arrays, but most calls pass lists. + cl-sort cl-stable-sort + )) + + ;;; Types and assertions. ;;;###autoload diff --git a/lisp/subr.el b/lisp/subr.el index 427014cedc3..1452a1117d3 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -862,6 +862,7 @@ If that is non-nil, the element matches; then `assoc-default' If no element matches, the value is nil. If TEST is omitted or nil, `equal' is used." + (declare (important-return-value t)) (let (found (tail alist) value) (while (and tail (not found)) (let ((elt (car tail))) From aa135e09b6735cd50742b1023fc901feb32e6a52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= Date: Mon, 1 May 2023 17:22:02 +0200 Subject: [PATCH 2/4] Declare `cl-delete` to have important-return-value (bug#61730) * lisp/emacs-lisp/cl-macs.el: Set property on `cl-delete`. * lisp/progmodes/python.el (python-shell--add-to-path-with-priority): Prevent warning by cleaner code. --- lisp/emacs-lisp/cl-macs.el | 3 +-- lisp/progmodes/python.el | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index 04567917189..8fdafe18c50 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -3683,8 +3683,7 @@ macro that returns its `&whole' argument." cl-tree-equal ;; Functions that mutate and return a list. - ;;cl-delete - cl-delete-if cl-delete-if-not + cl-delete cl-delete-if cl-delete-if-not cl-delete-duplicates cl-nsubst cl-nsubst-if cl-nsubst-if-not cl-nsubstitute cl-nsubstitute-if cl-nsubstitute-if-not diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el index 5bb15c60956..f9b645cc3df 100644 --- a/lisp/progmodes/python.el +++ b/lisp/progmodes/python.el @@ -2720,8 +2720,7 @@ dedicated to the current buffer or its project (if one is found)." (defmacro python-shell--add-to-path-with-priority (pathvar paths) "Modify PATHVAR and ensure PATHS are added only once at beginning." `(dolist (path (reverse ,paths)) - (cl-delete path ,pathvar :test #'string=) - (cl-pushnew path ,pathvar :test #'string=))) + (setq ,pathvar (cons path (cl-delete path ,pathvar :test #'string=))))) (defun python-shell-calculate-pythonpath () "Calculate the PYTHONPATH using `python-shell-extra-pythonpaths'." From d01543f1147b3514fd06ffcdf4be64a6cabe0018 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Mon, 1 May 2023 09:49:39 -0700 Subject: [PATCH 3/4] ; Use $GIT_DIR to find the .git directory inside our hooks This resolves an issue with running the hooks from a worktree. See . * build-aux/git-hooks/post-commit: * build-aux/git-hooks/pre-push: Use $GIT_DIR. --- build-aux/git-hooks/post-commit | 2 +- build-aux/git-hooks/pre-push | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/build-aux/git-hooks/post-commit b/build-aux/git-hooks/post-commit index 68d9995d915..05f2d778b5c 100755 --- a/build-aux/git-hooks/post-commit +++ b/build-aux/git-hooks/post-commit @@ -42,4 +42,4 @@ else fi git rev-parse HEAD | $awk -v reason=post-commit \ - -f .git/hooks/commit-msg-files.awk + -f ${GIT_DIR:-.git}/hooks/commit-msg-files.awk diff --git a/build-aux/git-hooks/pre-push b/build-aux/git-hooks/pre-push index 8e8277cba4f..6ff59102fd7 100755 --- a/build-aux/git-hooks/pre-push +++ b/build-aux/git-hooks/pre-push @@ -83,4 +83,4 @@ $awk -v origin_name="$1" ' # Print every SHA after oldref, up to (and including) newref. system("git rev-list --first-parent --reverse " oldref ".." newref) } -' | $awk -v reason=pre-push -f .git/hooks/commit-msg-files.awk +' | $awk -v reason=pre-push -f ${GIT_DIR:-.git}/hooks/commit-msg-files.awk From 7d246c359cf3d25cab5134076e393c4d25015827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= Date: Mon, 1 May 2023 19:08:12 +0200 Subject: [PATCH 4/4] ; * lisp/progmodes/c-ts-mode.el: allow loading file without treesit --- lisp/progmodes/c-ts-mode.el | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lisp/progmodes/c-ts-mode.el b/lisp/progmodes/c-ts-mode.el index 4971ed0b7c2..1f420689432 100644 --- a/lisp/progmodes/c-ts-mode.el +++ b/lisp/progmodes/c-ts-mode.el @@ -1001,13 +1001,14 @@ For BOL see `treesit-simple-indent-rules'." (looking-at c-ts-mode--for-each-tail-regexp)))) (defvar c-ts-mode--emacs-c-range-query - (treesit-query-compile - 'emacs-c `(((declaration - type: (macro_type_specifier - name: (identifier) @_name) - @for-each-tail) - (:match ,c-ts-mode--for-each-tail-regexp - @_name)))) + (and (treesit-available-p) + (treesit-query-compile + 'emacs-c `(((declaration + type: (macro_type_specifier + name: (identifier) @_name) + @for-each-tail) + (:match ,c-ts-mode--for-each-tail-regexp + @_name))))) "Query that finds a FOR_EACH_* macro with an unbracketed body.") (defvar-local c-ts-mode--for-each-tail-ranges nil