* bug#31641: 26.1; iter-do variable not left unused warning @ 2018-05-29 13:12 Christopher Wellons 2018-05-29 22:13 ` Noam Postavsky 0 siblings, 1 reply; 8+ messages in thread From: Christopher Wellons @ 2018-05-29 13:12 UTC (permalink / raw) To: 31641 When byte-compiling an iter-do form with a variable intended to be left unused, the compiler emits a false warning: ;;; -*- lexical-binding: t; -*- (require 'generator) (iter-do (_ i)) ;; -> "Warning: variable ‘_’ not left unused" Giving the variable a name has the opposite effect, though it's not a false warning in this case: (iter-do (v i)) ;; -> "Unused lexical variable ‘v’" This issue does not occur with similar, long-established forms: ;;; -*- lexical-binding: t; -*- (dolist (_ '(a b c))) (dotimes (_ 10)) ;; -> no warnings ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#31641: 26.1; iter-do variable not left unused warning 2018-05-29 13:12 bug#31641: 26.1; iter-do variable not left unused warning Christopher Wellons @ 2018-05-29 22:13 ` Noam Postavsky 2021-02-04 10:05 ` Lars Ingebrigtsen 0 siblings, 1 reply; 8+ messages in thread From: Noam Postavsky @ 2018-05-29 22:13 UTC (permalink / raw) To: Christopher Wellons; +Cc: 31641 tags 31641 + confirmed severity 31641 minor quit Christopher Wellons <wellons@nullprogram.com> writes: > When byte-compiling an iter-do form with a variable intended to be > left unused, the compiler emits a false warning: > > ;;; -*- lexical-binding: t; -*- > (require 'generator) > (iter-do (_ i)) > ;; -> "Warning: variable ‘_’ not left unused" Looking at the expansion, I guess the setf should be dropped if the variable name starts with _. (let (_ #3=#:iter-do-result11 (#1=#:iter-do-iterator-done8 nil) (#2=#:iter-do-iterator10 i)) (while (not #1#) (condition-case #4=#:iter-do-condition9 (setf _ (iter-next #2#)) (iter-end-of-sequence (setf #3# (cdr #4#)) (setf #1# t))) (unless #1#)) #3#) ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#31641: 26.1; iter-do variable not left unused warning 2018-05-29 22:13 ` Noam Postavsky @ 2021-02-04 10:05 ` Lars Ingebrigtsen 2021-02-04 16:36 ` Stefan Monnier 2021-02-05 17:12 ` Basil L. Contovounesios 0 siblings, 2 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2021-02-04 10:05 UTC (permalink / raw) To: Noam Postavsky; +Cc: Christopher Wellons, 31641, Stefan Monnier Noam Postavsky <npostavs@gmail.com> writes: >> When byte-compiling an iter-do form with a variable intended to be >> left unused, the compiler emits a false warning: >> >> ;;; -*- lexical-binding: t; -*- >> (require 'generator) >> (iter-do (_ i)) >> ;; -> "Warning: variable ‘_’ not left unused" > > Looking at the expansion, I guess the setf should be dropped if the > variable name starts with _. > > (let (_ > #3=#:iter-do-result11 > (#1=#:iter-do-iterator-done8 nil) > (#2=#:iter-do-iterator10 i)) > (while (not #1#) > (condition-case #4=#:iter-do-condition9 > (setf _ (iter-next #2#)) > (iter-end-of-sequence > (setf #3# (cdr #4#)) > (setf #1# t))) > (unless #1#)) > #3#) The following patch does this, but I'm not sure whether this is correct or not -- in other cases, the _ convention just removes the warning, but doesn't change the semantics. I wondered whether we could just suppress this warning like this: ,(if (string-match-p "\\`_" (symbol-name var)) `(with-suppressed-warnings ((not-unused ,var)) (setf ,var (iter-next ,it-symbol))) `(setf ,var (iter-next ,it-symbol))) But no, cconv--analyze-use is called too early, and would have to be taught about `with-suppressed-warnings'... which, looking at the code, isn't immediately obvious how to do. So does anybody have any ideas here? diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el index 9eb6d95964..0b644cc72c 100644 --- a/lisp/emacs-lisp/generator.el +++ b/lisp/emacs-lisp/generator.el @@ -731,7 +731,10 @@ iter-do (,it-symbol ,iterator)) (while (not ,done-symbol) (condition-case ,condition-symbol - (setf ,var (iter-next ,it-symbol)) + ;; Variables that start with an underscore shouldn't be set. + ,(if (string-match-p "\\`_" (symbol-name var)) + nil + `(setf ,var (iter-next ,it-symbol))) (iter-end-of-sequence (setf ,result-symbol (cdr ,condition-symbol)) (setf ,done-symbol t))) -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#31641: 26.1; iter-do variable not left unused warning 2021-02-04 10:05 ` Lars Ingebrigtsen @ 2021-02-04 16:36 ` Stefan Monnier 2021-02-05 8:53 ` Lars Ingebrigtsen 2021-02-06 10:31 ` Lars Ingebrigtsen 2021-02-05 17:12 ` Basil L. Contovounesios 1 sibling, 2 replies; 8+ messages in thread From: Stefan Monnier @ 2021-02-04 16:36 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Christopher Wellons, 31641, Noam Postavsky >>> When byte-compiling an iter-do form with a variable intended to be >>> left unused, the compiler emits a false warning: >>> >>> ;;; -*- lexical-binding: t; -*- >>> (require 'generator) >>> (iter-do (_ i)) >>> ;; -> "Warning: variable ‘_’ not left unused" >> >> Looking at the expansion, I guess the setf should be dropped if the >> variable name starts with _. >> >> (let (_ >> #3=#:iter-do-result11 >> (#1=#:iter-do-iterator-done8 nil) >> (#2=#:iter-do-iterator10 i)) >> (while (not #1#) >> (condition-case #4=#:iter-do-condition9 >> (setf _ (iter-next #2#)) >> (iter-end-of-sequence >> (setf #3# (cdr #4#)) >> (setf #1# t))) >> (unless #1#)) >> #3#) FWIW, I find the above expansion to provide somewhat "dirty" semantics in the sense that (let ((funs '())) (iter-do (n i) (push (lambda () n) funs)) funs) will return a list of functions which all return the same value (the last `n`). You can clean up this semantics and the warning at the same time by using an expansion like: (let (#3=#:iter-do-result11 (#1=#:iter-do-iterator-done8 nil) (#2=#:iter-do-iterator10 i)) (while (not #1#) (let ((_ (condition-case #4=#:iter-do-condition9 (iter-next #2#) (iter-end-of-sequence (setf #3# (cdr #4#)) (setf #1# t))) (unless #1# [BODY]) #3#) BTW, I think we can remove the duplicate #1 test by moving the body of the `while` into its test, e.g.: (let (#3=#:iter-do-result11 (#1=#:iter-do-iterator-done8 nil) (#2=#:iter-do-iterator10 i)) (while (let ((_ (condition-case #4=#:iter-do-condition9 (iter-next #2#) (iter-end-of-sequence (setf #3# (cdr #4#)) (setf #1# t))))) (unless #1# [BODY] t))) #3#) It's too bad that [BODY] can throw `iter-end-of-sequence`, since otherwise we could move the `condition-case` outside of the loop and get something more efficient. Stefan > The following patch does this, but I'm not sure whether this is correct > or not -- in other cases, the _ convention just removes the warning, but > doesn't change the semantics. > > I wondered whether we could just suppress this warning like this: > > ,(if (string-match-p "\\`_" (symbol-name var)) > `(with-suppressed-warnings ((not-unused ,var)) > (setf ,var (iter-next ,it-symbol))) > `(setf ,var (iter-next ,it-symbol))) > > But no, cconv--analyze-use is called too early, and would have to be > taught about `with-suppressed-warnings'... which, looking at the code, > isn't immediately obvious how to do. > > So does anybody have any ideas here? > > diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el > index 9eb6d95964..0b644cc72c 100644 > --- a/lisp/emacs-lisp/generator.el > +++ b/lisp/emacs-lisp/generator.el > @@ -731,7 +731,10 @@ iter-do > (,it-symbol ,iterator)) > (while (not ,done-symbol) > (condition-case ,condition-symbol > - (setf ,var (iter-next ,it-symbol)) > + ;; Variables that start with an underscore shouldn't be set. > + ,(if (string-match-p "\\`_" (symbol-name var)) > + nil > + `(setf ,var (iter-next ,it-symbol))) > (iter-end-of-sequence > (setf ,result-symbol (cdr ,condition-symbol)) > (setf ,done-symbol t))) ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#31641: 26.1; iter-do variable not left unused warning 2021-02-04 16:36 ` Stefan Monnier @ 2021-02-05 8:53 ` Lars Ingebrigtsen 2021-02-05 15:03 ` Stefan Monnier 2021-02-06 10:31 ` Lars Ingebrigtsen 1 sibling, 1 reply; 8+ messages in thread From: Lars Ingebrigtsen @ 2021-02-05 8:53 UTC (permalink / raw) To: Stefan Monnier; +Cc: Christopher Wellons, 31641, Noam Postavsky Stefan Monnier <monnier@iro.umontreal.ca> writes: > FWIW, I find the above expansion to provide somewhat "dirty" semantics > in the sense that > > (let ((funs '())) > (iter-do (n i) (push (lambda () n) funs)) > funs) > > will return a list of functions which all return the same value (the > last `n`). > > You can clean up this semantics and the warning at the same time by > using an expansion like: If I'm reading that correctly, that does seem like more obvious semantics, but is it too late to change this now? I'm not sure how much generator.el is used in the wild yet... -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#31641: 26.1; iter-do variable not left unused warning 2021-02-05 8:53 ` Lars Ingebrigtsen @ 2021-02-05 15:03 ` Stefan Monnier 0 siblings, 0 replies; 8+ messages in thread From: Stefan Monnier @ 2021-02-05 15:03 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Christopher Wellons, 31641, Noam Postavsky > If I'm reading that correctly, that does seem like more obvious > semantics, but is it too late to change this now? I'm not sure how much > generator.el is used in the wild yet... The change in semantics is fairly subtle (e.g. Common-Lisp allows both choices for the similar choice of behavior in `dotimes` and `dolist`), so it's rather unlikely to break existing code, IMO (and I think the likelihood would be higher if the change were in the other direction). Stefan ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#31641: 26.1; iter-do variable not left unused warning 2021-02-04 16:36 ` Stefan Monnier 2021-02-05 8:53 ` Lars Ingebrigtsen @ 2021-02-06 10:31 ` Lars Ingebrigtsen 1 sibling, 0 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2021-02-06 10:31 UTC (permalink / raw) To: Stefan Monnier; +Cc: Christopher Wellons, 31641, Noam Postavsky Stefan Monnier <monnier@iro.umontreal.ca> writes: > You can clean up this semantics and the warning at the same time by > using an expansion like: > > (let (#3=#:iter-do-result11 > (#1=#:iter-do-iterator-done8 nil) > (#2=#:iter-do-iterator10 i)) > (while (not #1#) > (let ((_ (condition-case #4=#:iter-do-condition9 > (iter-next #2#) > (iter-end-of-sequence > (setf #3# (cdr #4#)) > (setf #1# t))) > (unless #1# [BODY]) > #3#) So I tried this... diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el index 9eb6d95964..dfd2513350 100644 --- a/lisp/emacs-lisp/generator.el +++ b/lisp/emacs-lisp/generator.el @@ -725,17 +725,18 @@ iter-do (condition-symbol (cps--gensym "iter-do-condition")) (it-symbol (cps--gensym "iter-do-iterator")) (result-symbol (cps--gensym "iter-do-result"))) - `(let (,var - ,result-symbol + `(let (,result-symbol (,done-symbol nil) (,it-symbol ,iterator)) (while (not ,done-symbol) - (condition-case ,condition-symbol - (setf ,var (iter-next ,it-symbol)) - (iter-end-of-sequence - (setf ,result-symbol (cdr ,condition-symbol)) - (setf ,done-symbol t))) - (unless ,done-symbol ,@body)) + (let ((,var + (condition-case ,condition-symbol + (iter-next ,it-symbol) + (iter-end-of-sequence + (setf ,result-symbol (cdr ,condition-symbol)) + (setf ,done-symbol t))))) + (unless ,done-symbol + ,@body))) ,result-symbol))) (defvar cl--loop-args) But then this fails: 1 unexpected results: FAILED iter-lambda-variable-shadowing Uhm... oh, it fails even without that change? ... If I just say "make check", then it doesn't fail, but if I say "make generator-tests", then it fails? Very odd. > BTW, I think we can remove the duplicate #1 test by moving the body of > the `while` into its test, e.g.: > > (let (#3=#:iter-do-result11 > (#1=#:iter-do-iterator-done8 nil) > (#2=#:iter-do-iterator10 i)) > (while > (let ((_ (condition-case #4=#:iter-do-condition9 > (iter-next #2#) > (iter-end-of-sequence > (setf #3# (cdr #4#)) > (setf #1# t))))) > (unless #1# > [BODY] > t))) > #3#) Indeed. I've now pushed something like this to Emacs 28. > It's too bad that [BODY] can throw `iter-end-of-sequence`, since > otherwise we could move the `condition-case` outside of the loop and get > something more efficient. Yup. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#31641: 26.1; iter-do variable not left unused warning 2021-02-04 10:05 ` Lars Ingebrigtsen 2021-02-04 16:36 ` Stefan Monnier @ 2021-02-05 17:12 ` Basil L. Contovounesios 1 sibling, 0 replies; 8+ messages in thread From: Basil L. Contovounesios @ 2021-02-05 17:12 UTC (permalink / raw) To: Lars Ingebrigtsen Cc: Christopher Wellons, 31641, Noam Postavsky, Stefan Monnier Lars Ingebrigtsen <larsi@gnus.org> writes: > diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el > index 9eb6d95964..0b644cc72c 100644 > --- a/lisp/emacs-lisp/generator.el > +++ b/lisp/emacs-lisp/generator.el > @@ -731,7 +731,10 @@ iter-do > (,it-symbol ,iterator)) > (while (not ,done-symbol) > (condition-case ,condition-symbol > - (setf ,var (iter-next ,it-symbol)) > + ;; Variables that start with an underscore shouldn't be set. > + ,(if (string-match-p "\\`_" (symbol-name var)) > + nil > + `(setf ,var (iter-next ,it-symbol))) Nit - AKA the following? ,(unless (string-prefix-p "_" (symbol-name var)) `(setf ,var (iter-next ,it-symbol))) -- Basil ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-06 10:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-29 13:12 bug#31641: 26.1; iter-do variable not left unused warning Christopher Wellons 2018-05-29 22:13 ` Noam Postavsky 2021-02-04 10:05 ` Lars Ingebrigtsen 2021-02-04 16:36 ` Stefan Monnier 2021-02-05 8:53 ` Lars Ingebrigtsen 2021-02-05 15:03 ` Stefan Monnier 2021-02-06 10:31 ` Lars Ingebrigtsen 2021-02-05 17:12 ` Basil L. Contovounesios
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).