Lexical binding bug in org-list.el?

Lexical binding bug in org-list.el?

[Kaushal Modi]

Hi,

Here the required info:

Org-mode version 8.3.2 (release_8.3.2-287-gcce317 @
/home/kmodi/usr_local/apps/6/emacs/master/share/emacs/site-lisp/org/)

The bug is very easy to recreate.

(1) In any buffer, M-x org-mode

(2) Type * heading 1

(3) Do M-RET
Now you have
* heading 1
*

(4) Do C-c -
Now you have
* heading 1
-

(5) Do C-c *
Boom!

The depth var is not bound to anything in that lexical scope.

===== BACKTRACE

Debugger entered--Lisp error: (void-variable depth)
  (funcall get-stars depth)
  eval((funcall get-stars depth))
  (concat (eval istart) "%s")
  (cond ((eq type (quote descriptive)) (concat (let ((s (eval
istart))) (or (and (string-match "[ \n
]+\\'" s) (replace-match "" t t s)) istart)) "%s" (eval ddend))) ((and
counter (eq type (quote ordered))) (concat (eval icount) "%s")) (t
(concat (eval istart) "%s")))
  (concat (cond ((eq type (quote descriptive)) (concat (let ((s (eval
istart))) (or (and (string-match "[ \n
]+\\'" s) (replace-match "" t t s)) istart)) "%s" (eval ddend))) ((and
counter (eq type (quote ordered))) (concat (eval icount) "%s")) (t
(concat (eval istart) "%s"))) (eval iend))
  (let* ((counter (car-safe (prog1 item (setq item (cdr item))))) (fmt
(concat (cond ((eq type (quote descriptive)) (concat (let ... ...)
"%s" (eval ddend))) ((and counter (eq type ...)) (concat (eval icount)
"%s")) (t (concat (eval istart) "%s"))) (eval iend))) (first (car
item))) (cond ((string-match "\\[CBON\\]" first) (setq first
(replace-match cbon t t first))) ((string-match "\\[CBOFF\\]" first)
(setq first (replace-match cboff t t first))) ((string-match
"\\[CBTRANS\\]" first) (setq first (replace-match cbtrans t t
first)))) (if nobr (progn (setq first (org-list-item-trim-br first))))
(if (eq type (quote descriptive)) (progn (let* ((complete
(string-match "^\\(.*\\)[ ]+::[ ]*" first)) (term (if complete (let
... ...) "???")) (desc (if complete (substring first ...) first)))
(setq first (concat (eval dtstart) term (eval dtend) (eval ddstart)
desc))))) (setcar item first) (format fmt (mapconcat (function (lambda
(e) (if (stringp e) e (funcall export-sublist e (1+ depth))))) item
(or (eval csep) ""))))
  (closure ((export-sublist) (nobr) (cbtrans . "TODO") (cboff .
"TODO") (cbon . "DONE") (csep if org--blankp "\n\n" "\n") (lsep) (isep
if org--blankp "\n\n" "\n") (iend) (icount funcall get-stars depth)
(istart funcall get-stars depth) (ddend) (ddstart) (dtend . " ")
(dtstart . " ") (dend) (dstart) (uend) (ustart) (oend) (ostart)
(splicep . t) (p :splice t :dtstart " " :dtend " " :istart (funcall
get-stars depth) :icount (funcall get-stars depth) :isep (if
org--blankp "\n\n" "\n") :csep (if org--blankp "\n\n" "\n") :cbon
"DONE" :cboff "TODO" :cbtrans "TODO") (params :splice t :dtstart " "
:dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(list unordered (nil "")) org-tab-ind-state t) (item type depth) (let*
((counter (car-safe (prog1 item (setq item (cdr item))))) (fmt (concat
(cond ((eq type ...) (concat ... "%s" ...)) ((and counter ...) (concat
... "%s")) (t (concat ... "%s"))) (eval iend))) (first (car item)))
(cond ((string-match "\\[CBON\\]" first) (setq first (replace-match
cbon t t first))) ((string-match "\\[CBOFF\\]" first) (setq first
(replace-match cboff t t first))) ((string-match "\\[CBTRANS\\]"
first) (setq first (replace-match cbtrans t t first)))) (if nobr
(progn (setq first (org-list-item-trim-br first)))) (if (eq type
(quote descriptive)) (progn (let* ((complete (string-match "^\\(.*\\)[
]+::[ ]*" first)) (term (if complete ... "???")) (desc (if complete
... first))) (setq first (concat (eval dtstart) term (eval dtend)
(eval ddstart) desc))))) (setcar item first) (format fmt (mapconcat
(function (lambda (e) (if (stringp e) e (funcall export-sublist e
...)))) item (or (eval csep) "")))))((nil "") unordered 0)
  funcall((closure ((export-sublist) (nobr) (cbtrans . "TODO") (cboff
. "TODO") (cbon . "DONE") (csep if org--blankp "\n\n" "\n") (lsep)
(isep if org--blankp "\n\n" "\n") (iend) (icount funcall get-stars
depth) (istart funcall get-stars depth) (ddend) (ddstart) (dtend . "
") (dtstart . " ") (dend) (dstart) (uend) (ustart) (oend) (ostart)
(splicep . t) (p :splice t :dtstart " " :dtend " " :istart (funcall
get-stars depth) :icount (funcall get-stars depth) :isep (if
org--blankp "\n\n" "\n") :csep (if org--blankp "\n\n" "\n") :cbon
"DONE" :cboff "TODO" :cbtrans "TODO") (params :splice t :dtstart " "
:dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(list unordered (nil "")) org-tab-ind-state t) (item type depth) (let*
((counter (car-safe (prog1 item (setq item (cdr item))))) (fmt (concat
(cond ((eq type ...) (concat ... "%s" ...)) ((and counter ...) (concat
... "%s")) (t (concat ... "%s"))) (eval iend))) (first (car item)))
(cond ((string-match "\\[CBON\\]" first) (setq first (replace-match
cbon t t first))) ((string-match "\\[CBOFF\\]" first) (setq first
(replace-match cboff t t first))) ((string-match "\\[CBTRANS\\]"
first) (setq first (replace-match cbtrans t t first)))) (if nobr
(progn (setq first (org-list-item-trim-br first)))) (if (eq type
(quote descriptive)) (progn (let* ((complete (string-match "^\\(.*\\)[
]+::[ ]*" first)) (term (if complete ... "???")) (desc (if complete
... first))) (setq first (concat (eval dtstart) term (eval dtend)
(eval ddstart) desc))))) (setcar item first) (format fmt (mapconcat
(function (lambda (e) (if (stringp e) e (funcall export-sublist e
...)))) item (or (eval csep) ""))))) (nil "") unordered 0)
  (closure ((fmt . "%s") (items (nil "")) (type . unordered) (depth .
0) (sub unordered (nil "")) (export-item closure ((export-sublist)
(nobr) (cbtrans . "TODO") (cboff . "TODO") (cbon . "DONE") (csep if
org--blankp "\n\n" "\n") (lsep) (isep if org--blankp "\n\n" "\n")
(iend) (icount funcall get-stars depth) (istart funcall get-stars
depth) (ddend) (ddstart) (dtend . " ") (dtstart . " ") (dend) (dstart)
(uend) (ustart) (oend) (ostart) (splicep . t) (p :splice t :dtstart "
" :dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(params :splice t :dtstart " " :dtend " " :istart (funcall get-stars
depth) :icount (funcall get-stars depth) :isep (if org--blankp "\n\n"
"\n") :csep (if org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO"
:cbtrans "TODO") (list unordered (nil "")) org-tab-ind-state t) (item
type depth) (let* ((counter (car-safe (prog1 item ...))) (fmt (concat
(cond ... ... ...) (eval iend))) (first (car item))) (cond
((string-match "\\[CBON\\]" first) (setq first (replace-match cbon t t
first))) ((string-match "\\[CBOFF\\]" first) (setq first
(replace-match cboff t t first))) ((string-match "\\[CBTRANS\\]"
first) (setq first (replace-match cbtrans t t first)))) (if nobr
(progn (setq first (org-list-item-trim-br first)))) (if (eq type
(quote descriptive)) (progn (let* (... ... ...) (setq first ...))))
(setcar item first) (format fmt (mapconcat (function (lambda ... ...))
item (or (eval csep) ""))))) (export-sublist) (nobr) (cbtrans .
"TODO") (cboff . "TODO") (cbon . "DONE") (csep if org--blankp "\n\n"
"\n") (lsep) (isep if org--blankp "\n\n" "\n") (iend) (icount funcall
get-stars depth) (istart funcall get-stars depth) (ddend) (ddstart)
(dtend . " ") (dtstart . " ") (dend) (dstart) (uend) (ustart) (oend)
(ostart) (splicep . t) (p :splice t :dtstart " " :dtend " " :istart
(funcall get-stars depth) :icount (funcall get-stars depth) :isep (if
org--blankp "\n\n" "\n") :csep (if org--blankp "\n\n" "\n") :cbon
"DONE" :cboff "TODO" :cbtrans "TODO") (params :splice t :dtstart " "
:dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(list unordered (nil "")) org-tab-ind-state t) (e) (funcall
export-item e type depth))((nil ""))
  mapconcat((closure ((fmt . "%s") (items (nil "")) (type . unordered)
(depth . 0) (sub unordered (nil "")) (export-item closure
((export-sublist) (nobr) (cbtrans . "TODO") (cboff . "TODO") (cbon .
"DONE") (csep if org--blankp "\n\n" "\n") (lsep) (isep if org--blankp
"\n\n" "\n") (iend) (icount funcall get-stars depth) (istart funcall
get-stars depth) (ddend) (ddstart) (dtend . " ") (dtstart . " ")
(dend) (dstart) (uend) (ustart) (oend) (ostart) (splicep . t) (p
:splice t :dtstart " " :dtend " " :istart (funcall get-stars depth)
:icount (funcall get-stars depth) :isep (if org--blankp "\n\n" "\n")
:csep (if org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans
"TODO") (params :splice t :dtstart " " :dtend " " :istart (funcall
get-stars depth) :icount (funcall get-stars depth) :isep (if
org--blankp "\n\n" "\n") :csep (if org--blankp "\n\n" "\n") :cbon
"DONE" :cboff "TODO" :cbtrans "TODO") (list unordered (nil ""))
org-tab-ind-state t) (item type depth) (let* ((counter (car-safe
(prog1 item ...))) (fmt (concat (cond ... ... ...) (eval iend)))
(first (car item))) (cond ((string-match "\\[CBON\\]" first) (setq
first (replace-match cbon t t first))) ((string-match "\\[CBOFF\\]"
first) (setq first (replace-match cboff t t first))) ((string-match
"\\[CBTRANS\\]" first) (setq first (replace-match cbtrans t t
first)))) (if nobr (progn (setq first (org-list-item-trim-br first))))
(if (eq type (quote descriptive)) (progn (let* (... ... ...) (setq
first ...)))) (setcar item first) (format fmt (mapconcat (function
(lambda ... ...)) item (or (eval csep) ""))))) (export-sublist) (nobr)
(cbtrans . "TODO") (cboff . "TODO") (cbon . "DONE") (csep if
org--blankp "\n\n" "\n") (lsep) (isep if org--blankp "\n\n" "\n")
(iend) (icount funcall get-stars depth) (istart funcall get-stars
depth) (ddend) (ddstart) (dtend . " ") (dtstart . " ") (dend) (dstart)
(uend) (ustart) (oend) (ostart) (splicep . t) (p :splice t :dtstart "
" :dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(params :splice t :dtstart " " :dtend " " :istart (funcall get-stars
depth) :icount (funcall get-stars depth) :isep (if org--blankp "\n\n"
"\n") :csep (if org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO"
:cbtrans "TODO") (list unordered (nil "")) org-tab-ind-state t) (e)
(funcall export-item e type depth)) ((nil "")) "\n")
  (format fmt (mapconcat (function (lambda (e) (funcall export-item e
type depth))) items (or (eval isep) "")))
  (let* ((type (car sub)) (items (cdr sub)) (fmt (concat (cond
(splicep "%s") ((eq type (quote ordered)) (concat (eval ostart) "%s"
(eval oend))) ((eq type (quote descriptive)) (concat (eval dstart)
"%s" (eval dend))) (t (concat (eval ustart) "%s" (eval uend)))) (eval
lsep)))) (format fmt (mapconcat (function (lambda (e) (funcall
export-item e type depth))) items (or (eval isep) ""))))
  (closure ((export-item closure ((export-sublist) (nobr) (cbtrans .
"TODO") (cboff . "TODO") (cbon . "DONE") (csep if org--blankp "\n\n"
"\n") (lsep) (isep if org--blankp "\n\n" "\n") (iend) (icount funcall
get-stars depth) (istart funcall get-stars depth) (ddend) (ddstart)
(dtend . " ") (dtstart . " ") (dend) (dstart) (uend) (ustart) (oend)
(ostart) (splicep . t) (p :splice t :dtstart " " :dtend " " :istart
(funcall get-stars depth) :icount (funcall get-stars depth) :isep (if
org--blankp "\n\n" "\n") :csep (if org--blankp "\n\n" "\n") :cbon
"DONE" :cboff "TODO" :cbtrans "TODO") (params :splice t :dtstart " "
:dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(list unordered (nil "")) org-tab-ind-state t) (item type depth) (let*
((counter (car-safe (prog1 item ...))) (fmt (concat (cond ... ... ...)
(eval iend))) (first (car item))) (cond ((string-match "\\[CBON\\]"
first) (setq first (replace-match cbon t t first))) ((string-match
"\\[CBOFF\\]" first) (setq first (replace-match cboff t t first)))
((string-match "\\[CBTRANS\\]" first) (setq first (replace-match
cbtrans t t first)))) (if nobr (progn (setq first
(org-list-item-trim-br first)))) (if (eq type (quote descriptive))
(progn (let* (... ... ...) (setq first ...)))) (setcar item first)
(format fmt (mapconcat (function (lambda ... ...)) item (or (eval
csep) ""))))) (export-sublist) (nobr) (cbtrans . "TODO") (cboff .
"TODO") (cbon . "DONE") (csep if org--blankp "\n\n" "\n") (lsep) (isep
if org--blankp "\n\n" "\n") (iend) (icount funcall get-stars depth)
(istart funcall get-stars depth) (ddend) (ddstart) (dtend . " ")
(dtstart . " ") (dend) (dstart) (uend) (ustart) (oend) (ostart)
(splicep . t) (p :splice t :dtstart " " :dtend " " :istart (funcall
get-stars depth) :icount (funcall get-stars depth) :isep (if
org--blankp "\n\n" "\n") :csep (if org--blankp "\n\n" "\n") :cbon
"DONE" :cboff "TODO" :cbtrans "TODO") (params :splice t :dtstart " "
:dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(list unordered (nil "")) org-tab-ind-state t) (sub depth) (let*
((type (car sub)) (items (cdr sub)) (fmt (concat (cond (splicep "%s")
((eq type ...) (concat ... "%s" ...)) ((eq type ...) (concat ... "%s"
...)) (t (concat ... "%s" ...))) (eval lsep)))) (format fmt (mapconcat
(function (lambda (e) (funcall export-item e type depth))) items (or
(eval isep) "")))))((unordered (nil "")) 0)
  funcall((closure ((export-item closure ((export-sublist) (nobr)
(cbtrans . "TODO") (cboff . "TODO") (cbon . "DONE") (csep if
org--blankp "\n\n" "\n") (lsep) (isep if org--blankp "\n\n" "\n")
(iend) (icount funcall get-stars depth) (istart funcall get-stars
depth) (ddend) (ddstart) (dtend . " ") (dtstart . " ") (dend) (dstart)
(uend) (ustart) (oend) (ostart) (splicep . t) (p :splice t :dtstart "
" :dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(params :splice t :dtstart " " :dtend " " :istart (funcall get-stars
depth) :icount (funcall get-stars depth) :isep (if org--blankp "\n\n"
"\n") :csep (if org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO"
:cbtrans "TODO") (list unordered (nil "")) org-tab-ind-state t) (item
type depth) (let* ((counter (car-safe (prog1 item ...))) (fmt (concat
(cond ... ... ...) (eval iend))) (first (car item))) (cond
((string-match "\\[CBON\\]" first) (setq first (replace-match cbon t t
first))) ((string-match "\\[CBOFF\\]" first) (setq first
(replace-match cboff t t first))) ((string-match "\\[CBTRANS\\]"
first) (setq first (replace-match cbtrans t t first)))) (if nobr
(progn (setq first (org-list-item-trim-br first)))) (if (eq type
(quote descriptive)) (progn (let* (... ... ...) (setq first ...))))
(setcar item first) (format fmt (mapconcat (function (lambda ... ...))
item (or (eval csep) ""))))) (export-sublist) (nobr) (cbtrans .
"TODO") (cboff . "TODO") (cbon . "DONE") (csep if org--blankp "\n\n"
"\n") (lsep) (isep if org--blankp "\n\n" "\n") (iend) (icount funcall
get-stars depth) (istart funcall get-stars depth) (ddend) (ddstart)
(dtend . " ") (dtstart . " ") (dend) (dstart) (uend) (ustart) (oend)
(ostart) (splicep . t) (p :splice t :dtstart " " :dtend " " :istart
(funcall get-stars depth) :icount (funcall get-stars depth) :isep (if
org--blankp "\n\n" "\n") :csep (if org--blankp "\n\n" "\n") :cbon
"DONE" :cboff "TODO" :cbtrans "TODO") (params :splice t :dtstart " "
:dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
(list unordered (nil "")) org-tab-ind-state t) (sub depth) (let*
((type (car sub)) (items (cdr sub)) (fmt (concat (cond (splicep "%s")
((eq type ...) (concat ... "%s" ...)) ((eq type ...) (concat ... "%s"
...)) (t (concat ... "%s" ...))) (eval lsep)))) (format fmt (mapconcat
(function (lambda (e) (funcall export-item e type depth))) items (or
(eval isep) ""))))) (unordered (nil "")) 0)
  (concat (funcall export-sublist list 0) "\n")
  (let* ((p params) (splicep (plist-get p :splice)) (ostart (plist-get
p :ostart)) (oend (plist-get p :oend)) (ustart (plist-get p :ustart))
(uend (plist-get p :uend)) (dstart (plist-get p :dstart)) (dend
(plist-get p :dend)) (dtstart (plist-get p :dtstart)) (dtend
(plist-get p :dtend)) (ddstart (plist-get p :ddstart)) (ddend
(plist-get p :ddend)) (istart (plist-get p :istart)) (icount
(plist-get p :icount)) (iend (plist-get p :iend)) (isep (plist-get p
:isep)) (lsep (plist-get p :lsep)) (csep (plist-get p :csep)) (cbon
(plist-get p :cbon)) (cboff (plist-get p :cboff)) (cbtrans (plist-get
p :cbtrans)) (nobr (plist-get p :nobr)) export-sublist (export-item
(function (lambda (item type depth) (let* ((counter ...) (fmt ...)
(first ...)) (cond (... ...) (... ...) (... ...)) (if nobr (progn
...)) (if (eq type ...) (progn ...)) (setcar item first) (format fmt
(mapconcat ... item ...)))))) (export-sublist (function (lambda (sub
depth) (let* ((type ...) (items ...) (fmt ...)) (format fmt (mapconcat
... items ...))))))) (concat (funcall export-sublist list 0) "\n"))
  org-list-to-generic((unordered (nil "")) (:splice t :dtstart " "
:dtend " " :istart (funcall get-stars depth) :icount (funcall
get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO"))
  (let* ((rule (cdr (assq (quote heading)
org-blank-before-new-entry))) (level (org-reduced-level (or
(org-current-level) 0))) (org--blankp (or (eq rule t) (and (eq rule
(quote auto)) (save-excursion (outline-previous-heading)
(org-previous-line-empty-p))))) (get-stars (function (lambda (d) (let
((oddeven-level ...)) (concat (make-string ... 42) " "))))))
(org-list-to-generic list (org-combine-plists (quote (:splice t
:dtstart " " :dtend " " :istart (funcall get-stars depth) :icount
(funcall get-stars depth) :isep (if org--blankp "\n\n" "\n") :csep (if
org--blankp "\n\n" "\n") :cbon "DONE" :cboff "TODO" :cbtrans "TODO"))
params)))
  org-list-to-subtree((unordered (nil "")))
  org-toggle-heading(nil)
  funcall-interactively(org-toggle-heading nil)
  call-interactively(org-toggle-heading)
  org-ctrl-c-star()
  funcall-interactively(org-ctrl-c-star)
  call-interactively(org-ctrl-c-star nil nil)
  command-execute(org-ctrl-c-star)


--
Kaushal Modi

Re: Lexical binding bug in org-list.el?

[Kaushal Modi]

Lesser steps to recreate this bug:

(1) In any buffer, do M-x org-mode

(2) Type: - item 1

(3) Do M-RET
Now you have
- item 1
-

(4) Now do C-c *
Boom!

Re: Lexical binding bug in org-list.el?

[Aaron Ecay]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

Hi Kaushal,

I can reproduce the bug, and you’re right about the cause.  I made the
attached patch, which seems to get the code back on its feet.  But I
just sort of fiddled with it until all the lexical scoping warnings from
the compiler went away; I have no idea whether it’s correct.

The org-list code is a mess, and I think we should hold off on converting
it to lexical scoping until it can be refactored in a more dedicated way.
Nonetheless I include the patch, in case it’s helpful to anyone.

Thanks for the report (and the very easy test case! :) ),

-- 
Aaron Ecay

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-draft-patch-to-fix-org-list.patch --]
[-- Type: text/x-diff, Size: 14913 bytes --]

From d4b3d0e9ec19d6c2bca8a53313c260b266437c00 Mon Sep 17 00:00:00 2001
From: Aaron Ecay <aaronecay@gmail.com>
Date: Fri, 6 Nov 2015 20:38:08 +0000
Subject: [PATCH] draft patch to fix org-list

---
 lisp/org-list.el | 328 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 153 insertions(+), 175 deletions(-)

diff --git a/lisp/org-list.el b/lisp/org-list.el
index 683a643..060fda3 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -2922,6 +2922,66 @@ ignores hidden links."
 \f
 ;;; Send and receive lists
 
+(defun org-list--get-text (beg end)
+  "Return text between BEG and END, trimmed, with checkboxes replaced."
+  (let ((text (org-trim (buffer-substring beg end))))
+    (if (string-match "\\`\\[\\([-X ]\\)\\]" text)
+	(replace-match
+	 (let ((box (match-string 1 text)))
+	   (cond
+	    ((equal box " ") "CBOFF")
+	    ((equal box "-") "CBTRANS")
+	    (t "CBON")))
+	 t nil text 1)
+      text)))
+
+(defun org-list--parse-item (e struct parents prevs)
+  "Return a list containing counter of item, if any, text and any sublist inside it."
+  (let ((start (save-excursion
+		 (goto-char e)
+		 (looking-at "[ \t]*\\S-+\\([ \t]+\\[@\\(start:\\)?\\([0-9]+\\|[a-zA-Z]\\)\\]\\)?[ \t]*")
+		 (match-end 0)))
+	;; Get counter number.  For alphabetic counter, get
+	;; its position in the alphabet.
+	(counter (let ((c (org-list-get-counter e struct)))
+		   (cond
+		    ((not c) nil)
+		    ((string-match "[A-Za-z]" c)
+		     (- (string-to-char (upcase (match-string 0 c)))
+			64))
+		    ((string-match "[0-9]+" c)
+		     (string-to-number (match-string 0 c))))))
+	(childp (org-list-has-child-p e struct))
+	(end (org-list-get-item-end e struct)))
+    ;; If item has a child, store text between bullet and
+    ;; next child, then recursively parse all sublists.  At
+    ;; the end of each sublist, check for the presence of
+    ;; text belonging to the original item.
+    (if childp
+	(let* ((children (org-list-get-children e struct parents))
+	       (body (list (org-list--get-text start childp))))
+	  (while children
+	    (let* ((first (car children))
+		   (sub (org-list-get-all-items first struct prevs))
+		   (last-c (car (last sub)))
+		   (last-end (org-list-get-item-end last-c struct)))
+	      (push (org-list--parse-sublist sub struct parents prevs) body)
+	      ;; Remove children from the list just parsed.
+	      (setq children (cdr (member last-c children)))
+	      ;; There is a chunk of text belonging to the
+	      ;; item if last child doesn't end where next
+	      ;; child starts or where item ends.
+	      (unless (= (or (car children) end) last-end)
+		(push (org-list--get-text last-end (or (car children) end))
+		      body))))
+	  (cons counter (nreverse body)))
+      (list counter (org-list--get-text start end)))))
+
+(defun org-list--parse-sublist (e struct parents prevs)
+  "Return a list whose car is list type and cdr a list of items' body."
+  (cons (org-list-get-list-type (car e) struct prevs)
+	(mapcar (lambda (x) (org-list--parse-item x struct parents prevs)) e)))
+
 (defun org-list-parse-list (&optional delete)
   "Parse the list at point and maybe DELETE it.
 
@@ -2956,77 +3016,10 @@ Point is left at list end."
 	 (parents (org-list-parents-alist struct))
 	 (top (org-list-get-top-point struct))
 	 (bottom (org-list-get-bottom-point struct))
-	 out
-	 (get-text
-	  (function
-	   ;; Return text between BEG and END, trimmed, with
-	   ;; checkboxes replaced.
-	   (lambda (beg end)
-	     (let ((text (org-trim (buffer-substring beg end))))
-	       (if (string-match "\\`\\[\\([-X ]\\)\\]" text)
-		   (replace-match
-		    (let ((box (match-string 1 text)))
-		      (cond
-		       ((equal box " ") "CBOFF")
-		       ((equal box "-") "CBTRANS")
-		       (t "CBON")))
-		    t nil text 1)
-		 text)))))
-	 (parse-sublist
-	  (function
-	   ;; Return a list whose car is list type and cdr a list of
-	   ;; items' body.
-	   (lambda (e)
-	     (cons (org-list-get-list-type (car e) struct prevs)
-		   (mapcar parse-item e)))))
-	 (parse-item
-	  (function
-	   ;; Return a list containing counter of item, if any, text
-	   ;; and any sublist inside it.
-	   (lambda (e)
-	     (let ((start (save-excursion
-			    (goto-char e)
-			    (looking-at "[ \t]*\\S-+\\([ \t]+\\[@\\(start:\\)?\\([0-9]+\\|[a-zA-Z]\\)\\]\\)?[ \t]*")
-			    (match-end 0)))
-		   ;; Get counter number.  For alphabetic counter, get
-		   ;; its position in the alphabet.
-		   (counter (let ((c (org-list-get-counter e struct)))
-			      (cond
-			       ((not c) nil)
-			       ((string-match "[A-Za-z]" c)
-				(- (string-to-char (upcase (match-string 0 c)))
-				   64))
-			       ((string-match "[0-9]+" c)
-				(string-to-number (match-string 0 c))))))
-		   (childp (org-list-has-child-p e struct))
-		   (end (org-list-get-item-end e struct)))
-	       ;; If item has a child, store text between bullet and
-	       ;; next child, then recursively parse all sublists.  At
-	       ;; the end of each sublist, check for the presence of
-	       ;; text belonging to the original item.
-	       (if childp
-		   (let* ((children (org-list-get-children e struct parents))
-			  (body (list (funcall get-text start childp))))
-		     (while children
-		       (let* ((first (car children))
-			      (sub (org-list-get-all-items first struct prevs))
-			      (last-c (car (last sub)))
-			      (last-end (org-list-get-item-end last-c struct)))
-			 (push (funcall parse-sublist sub) body)
-			 ;; Remove children from the list just parsed.
-			 (setq children (cdr (member last-c children)))
-			 ;; There is a chunk of text belonging to the
-			 ;; item if last child doesn't end where next
-			 ;; child starts or where item ends.
-			 (unless (= (or (car children) end) last-end)
-			   (push (funcall get-text
-					  last-end (or (car children) end))
-				 body))))
-		     (cons counter (nreverse body)))
-		 (list counter (funcall get-text start end))))))))
+	 out)
     ;; Store output, take care of cursor position and deletion of
     ;; list, then return output.
-    (setq out (funcall parse-sublist (org-list-get-all-items top struct prevs)))
+    (setq out (org-list--parse-sublist (org-list-get-all-items top struct prevs) struct parents prevs))
     (goto-char top)
     (when delete
       (delete-region top bottom)
@@ -3109,6 +3102,79 @@ for this list."
   "Trim line breaks in a list ITEM."
   (setq item (replace-regexp-in-string "\n +" " " item)))
 
+(defun org-list--export-item (item type depth plist)
+  "Export an item ITEM of type TYPE, at DEPTH.
+
+First string in item is treated in a special way as it can bring
+extra information that needs to be processed."
+  (let* ((counter (pop item))
+	 (istart (plist-get plist :istart))
+	 (istart-depth (funcall istart depth))
+	 (icount (plist-get plist :icount))
+	 (icount-depth (funcall icount depth))
+	 (fmt (concat
+	       (cond
+	 	((eq type 'descriptive)
+	 	 ;; Stick DTSTART to ISTART by
+	 	 ;; left-trimming the latter.
+	 	 (concat (or (and (string-match "[ \t\n\r]+\\'" istart-depth)
+				  (replace-match "" t t istart-depth))
+			     istart-depth)
+	 		 "%s" (plist-get plist :ddend)))
+	 	((and counter (eq type 'ordered))
+	 	 (concat icount-depth "%s"))
+	 	(t (concat istart-depth "%s")))
+	       (plist-get plist :iend)))
+	 (first (car item)))
+    ;; Replace checkbox if any is found.
+    (cond
+     ((string-match "\\[CBON\\]" first)
+      (setq first (replace-match (plist-get plist :cbon) t t first)))
+     ((string-match "\\[CBOFF\\]" first)
+      (setq first (replace-match (plist-get plist :cboff) t t first)))
+     ((string-match "\\[CBTRANS\\]" first)
+      (setq first (replace-match (plist-get plist :cbtrans) t t first)))
+     )
+    ;; Replace line breaks if required
+    (when (plist-get plist :nobr) (setq first (org-list-item-trim-br first)))
+    ;; Insert descriptive term if TYPE is `descriptive'.
+    (when (eq type 'descriptive)
+      (let* ((complete
+    	      (string-match "^\\(.*\\)[ \t]+::[ \t]*" first))
+    	     (term (if complete
+    		       (save-match-data
+    			 (org-trim (match-string 1 first)))
+    		     "???"))
+    	     (desc (if complete (substring first (match-end 0))
+    		     first)))
+    	(setq first (concat (plist-get plist :dtstart)
+    			    term
+    			    (plist-get plist :dtend)
+    			    (plist-get plist :ddstart)
+    			    desc))))
+    (setcar item first)
+    (format fmt
+    	    (mapconcat (lambda (e)
+    			 (if (stringp e) e
+    			   (org-list--export-sublist e (1+ depth) plist)))
+    		       item (or (plist-get plist :csep) "")))))
+
+(defun org-list--export-sublist (sub depth plist)
+  "Export sublist SUB at DEPTH."
+  (let* ((type (car sub))
+	 (items (cdr sub))
+	 (fmt (concat (cond
+		       ((plist-get plist :splicep) "%s")
+		       ((eq type 'ordered)
+			(concat (plist-get plist :ostart) "%s" (plist-get plist :oend)))
+		       ((eq type 'descriptive)
+			(concat (plist-get plist :dstart) "%s" (plist-get plist :dend)))
+		       (t (concat (plist-get plist :ustart) "%s" (plist-get plist :uend))))
+		      (plist-get plist :lsep))))
+    (format fmt (mapconcat (lambda (e)
+			     (org-list--export-item e type depth plist))
+			   items (or (plist-get plist :isep) "")))))
+
 (defun org-list-to-generic (list params)
   "Convert a LIST parsed through `org-list-parse-list' to other formats.
 Valid parameters PARAMS are:
@@ -3149,94 +3215,7 @@ item, and depth of the current sub-list, starting at 0.
 Obviously, `counter' is only available for parameters applying to
 items."
   (interactive)
-  (letrec ((p params)
-	   (splicep (plist-get p :splice))
-	   (ostart (plist-get p :ostart))
-	   (oend (plist-get p :oend))
-	   (ustart (plist-get p :ustart))
-	   (uend (plist-get p :uend))
-	   (dstart (plist-get p :dstart))
-	   (dend (plist-get p :dend))
-	   (dtstart (plist-get p :dtstart))
-	   (dtend (plist-get p :dtend))
-	   (ddstart (plist-get p :ddstart))
-	   (ddend (plist-get p :ddend))
-	   (istart (plist-get p :istart))
-	   (icount (plist-get p :icount))
-	   (iend (plist-get p :iend))
-	   (isep (plist-get p :isep))
-	   (lsep (plist-get p :lsep))
-	   (csep (plist-get p :csep))
-	   (cbon (plist-get p :cbon))
-	   (cboff (plist-get p :cboff))
-	   (cbtrans (plist-get p :cbtrans))
-	   (nobr (plist-get p :nobr))
-	   (export-item
-	    ;; Export an item ITEM of type TYPE, at DEPTH.  First
-	    ;; string in item is treated in a special way as it can
-	    ;; bring extra information that needs to be processed.
-	    (lambda (item type depth)
-	      (let* ((counter (pop item))
-		     (fmt (concat
-			   (cond
-			    ((eq type 'descriptive)
-			     ;; Stick DTSTART to ISTART by
-			     ;; left-trimming the latter.
-			     (concat (let ((s (eval istart)))
-				       (or (and (string-match "[ \t\n\r]+\\'" s)
-						(replace-match "" t t s))
-					   istart))
-				     "%s" (eval ddend)))
-			    ((and counter (eq type 'ordered))
-			     (concat (eval icount) "%s"))
-			    (t (concat (eval istart) "%s")))
-			   (eval iend)))
-		     (first (car item)))
-		;; Replace checkbox if any is found.
-		(cond
-		 ((string-match "\\[CBON\\]" first)
-		  (setq first (replace-match cbon t t first)))
-		 ((string-match "\\[CBOFF\\]" first)
-		  (setq first (replace-match cboff t t first)))
-		 ((string-match "\\[CBTRANS\\]" first)
-		  (setq first (replace-match cbtrans t t first))))
-		;; Replace line breaks if required
-		(when nobr (setq first (org-list-item-trim-br first)))
-		;; Insert descriptive term if TYPE is `descriptive'.
-		(when (eq type 'descriptive)
-		  (let* ((complete
-			  (string-match "^\\(.*\\)[ \t]+::[ \t]*" first))
-			 (term (if complete
-				   (save-match-data
-				     (org-trim (match-string 1 first)))
-				 "???"))
-			 (desc (if complete (substring first (match-end 0))
-				 first)))
-		    (setq first (concat (eval dtstart) term (eval dtend)
-					(eval ddstart) desc))))
-		(setcar item first)
-		(format fmt
-			(mapconcat (lambda (e)
-				     (if (stringp e) e
-				       (funcall export-sublist e (1+ depth))))
-				   item (or (eval csep) ""))))))
-	   (export-sublist
-	    (lambda (sub depth)
-	      ;; Export sublist SUB at DEPTH.
-	      (let* ((type (car sub))
-		     (items (cdr sub))
-		     (fmt (concat (cond
-				   (splicep "%s")
-				   ((eq type 'ordered)
-				    (concat (eval ostart) "%s" (eval oend)))
-				   ((eq type 'descriptive)
-				    (concat (eval dstart) "%s" (eval dend)))
-				   (t (concat (eval ustart) "%s" (eval uend))))
-				  (eval lsep))))
-		(format fmt (mapconcat (lambda (e)
-					 (funcall export-item e type depth))
-				       items (or (eval isep) "")))))))
-    (concat (funcall export-sublist list 0) "\n")))
+  (concat (org-list--export-sublist list 0 params) "\n"))
 
 (defun org-list-to-latex (list &optional _params)
   "Convert LIST into a LaTeX list.
@@ -3259,38 +3238,37 @@ syntax.  Return converted list as a string."
   (require 'ox-texinfo)
   (org-export-string-as list 'texinfo t))
 
+
+(defun org-list--get-stars (level d)
+  "Return the string for the heading, depending on depth D of
+current sub-list."
+  (let ((oddeven-level (+ level d 1)))
+    (concat (make-string (if org-odd-levels-only
+			     (1- (* 2 oddeven-level))
+			   oddeven-level)
+			 ?*)
+	    " ")))
+
 (defun org-list-to-subtree (list &optional params)
   "Convert LIST into an Org subtree.
 LIST is as returned by `org-list-parse-list'.  PARAMS is a property list
 with overruling parameters for `org-list-to-generic'."
-  (defvar get-stars) (defvar org--blankp)
   (let* ((rule (cdr (assq 'heading org-blank-before-new-entry)))
 	 (level (org-reduced-level (or (org-current-level) 0)))
 	 (org--blankp (or (eq rule t)
 		     (and (eq rule 'auto)
 			  (save-excursion
 			    (outline-previous-heading)
-			    (org-previous-line-empty-p)))))
-	 (get-stars ;FIXME: Can't rename without renaming it in org.el as well!
-	  (function
-	   ;; Return the string for the heading, depending on depth D
-	   ;; of current sub-list.
-	   (lambda (d)
-	     (let ((oddeven-level (+ level d 1)))
-	       (concat (make-string (if org-odd-levels-only
-					(1- (* 2 oddeven-level))
-				      oddeven-level)
-				    ?*)
-		       " "))))))
+			    (org-previous-line-empty-p))))))
     (org-list-to-generic
      list
      (org-combine-plists
-      '(:splice t
+      `(:splice t
         :dtstart " " :dtend " "
-        :istart (funcall get-stars depth)
-        :icount (funcall get-stars depth)
-        :isep (if org--blankp "\n\n" "\n")
-        :csep (if org--blankp "\n\n" "\n")
+        :istart (lambda (d) (org-list--get-stars ,level d))
+        :icount (lambda (d) (org-list--get-stars ,level d))
+        :isep (if ,org--blankp "\n\n" "\n")
+        :csep (if ,org--blankp "\n\n" "\n")
         :cbon "DONE" :cboff "TODO" :cbtrans "TODO")
       params))))
 
-- 
2.6.2

Re: Lexical binding bug in org-list.el?

[Kaushal Modi]

Thanks for working on this Aaron but M-x ediff-patch-file failed for me.

I am on the cce317 commit of org-mode
(http://orgmode.org/cgit.cgi/org-mode.git/commit/?id=265e5b9984818f76dea8f5147af0c4a939cf457b)

patching file /home/kmodi/e/elisp/org-mode/lisp_25_0/org-list.el
Hunk #1 succeeded at 2923 (offset 1 line).
Hunk #2 succeeded at 3017 (offset 1 line).
Hunk #3 succeeded at 3103 (offset 1 line).
Hunk #4 FAILED at 3215.
Hunk #5 FAILED at 3325.
2 out of 5 hunks FAILED -- saving rejects to file
/home/kmodi/e/elisp/org-mode/lisp_25_0/org-list.el.rej

Re: Lexical binding bug in org-list.el?

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> The org-list code is a mess, and I think we should hold off on converting
> it to lexical scoping until it can be refactored in a more dedicated
> way.

It seems a bit strong considering there's only one issue so far. I don't
think the code is a mess, but "Send and receive lists" part clearly
needs an overhaul.

> Nonetheless I include the patch, in case it’s helpful to anyone.

Lifting `org-list--get-text', `org-list--parse-item', etc. isn't
necessary. I just forgot a `letrec' in `org-list-parse-list'.

The problem comes from `org-list-to-substree'. `org-list-to-generic'
mechanism is not adequate for this function, which relied so far on
a kludge to get the job done (i.e., undocumented `eval'ing of :istart
and :icount values). Actually, `org-list-to-generic' in its current form
isn't adequate for anything. Even `org-list-to-*' functions do not use
it.

As a temporary fix, besides using `letrec' per above, we can
re-implement `org-list-to-subtree' without using `org-list-to-generic'.

Later, `org-list-to-*' could be re-implemented like `orgtbl-to-generic'.
Also, some parameters should be able to be set to a function generating
a string, which would be called with the current depth of the item. In
the process I think `org-list-parse-list' should be simplified, too, as
its current return value is useless (e.g., nothing uses [CBON] anymore
in the code base).

WDYT?


Regards,

-- 
Nicolas Goaziou

Re: Lexical binding bug in org-list.el?

[Aaron Ecay]

Hi Nicolas,

2015ko azaroak 7an, Nicolas Goaziou-ek idatzi zuen:
> 
> Hello,
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>> The org-list code is a mess, and I think we should hold off on converting
>> it to lexical scoping until it can be refactored in a more dedicated
>> way.
> 
> It seems a bit strong considering there's only one issue so far. I don't
> think the code is a mess, but "Send and receive lists" part clearly
> needs an overhaul.
> 
>> Nonetheless I include the patch, in case it’s helpful to anyone.
> 
> Lifting `org-list--get-text', `org-list--parse-item', etc. isn't
> necessary. 

I’ll defer to your judgment, of course, especially where it comes to
fixing the immediate issue.

On a broader scope, this is just one part of org that is written in this
style (one large let that defines many functions which call each other;
the body of the let is just a call into one of these functions).  This
isn’t idiomatic elisp (at least, I’ve never seen it outside org), and it
seems suboptimal for at least two reasons:
1. The interface between the functions isn’t well-defined – they could
   exchange information via arguments and/or by modifying variables
   introduced by their containing let.
2. It’s impossible to unit test the small let-bound functions.

In view of this, do you think it would be desirable in the long term to
refactor code like this into top-level functions?

> I just forgot a `letrec' in `org-list-parse-list'.

Oh ok.  I had never seen letrec before these recent lexical binding
changes, so I wasn’t familiar with how it could fix the problems.  But
I’m glad it can.

> [...]

I originally wrote individual responses to the points in the rest of
your email, but then I realized that it’s slightly more than a year
since we had an eerily similar discussion:
<http://mid.gmane.org/87d2anougv.fsf@gmail.com>.  I don’t think we
reached any conclusion then.  To avoid the same thing happening, I’m
going to try to step back and sketch the vision I have about this code
from a high-level perspective.

If it was up to me, there would be only three kinds of code touching
lists in org:
- code that manipulates org-elements objects and (where necessary)
  converts them to strings using the exporter
- a single function in ob-core that takes org-elements lists and converts
  them into a simple nested list format, for use as input to babel code
  blocks
- a single function in ob-core that is the inverse of the one above, for
  processing the output of code blocks

I think this implies:
- org-list-parse-list deprecated/removed
- org-list-to-generic deprecated/removed
- callers of these two functions updated to use org-elements/ox (with new
  helper functions introduced for this purpose if it seems necessary)

The simplicity gains from this would be:
- fewer functions in the public API of org (org-list-parse-list is replaced
  by preexisting org-elements functions, and org-list-to-generic by ox
  functions)
- hopefully less code to maintain (in terms of lines), though it remains
  to be seen how much or if at all these changes actually shrink the
  code base
- all org code manipulating lists uses a single format (org-elements).
  Babel code blocks are not org code (and often not in elisp), so they
  are the only thing that gets to use a different format
- the plist format controlling org-list-to-generic goes away

You said:

> Actually, `org-list-to-generic' in its current form isn't adequate for
> anything. Even `org-list-to-*' functions do not use it.

And

> we can re-implement `org-list-to-subtree' without using
> `org-list-to-generic'.

Which I think means we are on more or less the same page about
org-list-to-generic.  You also said:

> `org-list-parse-list' should be simplified, too, as its current return
> value is useless (e.g., nothing uses [CBON] anymore in the code base).

Here I’m more radical than you: since the current return value is useless
(I agree) and hardly used anywhere, my conclusion is to just get rid of
the function, rather than trying to invent a way in which it could be
useful.

I hope that having laid it out in this way helps to understand what I
have in mind.  I’d of course be very glad to hear what your thoughts
are, and hopefully we’ll be able to work out how to proceed.

-- 
Aaron Ecay

Re: Lexical binding bug in org-list.el?

[Nicolas Goaziou]

Aaron Ecay <aaronecay@gmail.com> writes:

> On a broader scope, this is just one part of org that is written in this
> style (one large let that defines many functions which call each other;
> the body of the let is just a call into one of these functions).  This
> isn’t idiomatic elisp (at least, I’ve never seen it outside org), and it
> seems suboptimal for at least two reasons:
> 1. The interface between the functions isn’t well-defined – they could
>    exchange information via arguments and/or by modifying variables
>    introduced by their containing let.

They are mutually recursive functions. I don't think that is
un-idiomatic in the Lisp world. I don't think why the reason above would
make them sub-optimal, too.

> 2. It’s impossible to unit test the small let-bound functions.

Why would you want to unit test them? They are an implementation detail.
Only `org-list-parse-list' should be tested here.

> In view of this, do you think it would be desirable in the long term to
> refactor code like this into top-level functions?

No, I don't. I see no reason to pollute name space with these very
specific functions.

> If it was up to me, there would be only three kinds of code touching
> lists in org:
> - code that manipulates org-elements objects and (where necessary)
>   converts them to strings using the exporter
> - a single function in ob-core that takes org-elements lists and converts
>   them into a simple nested list format, for use as input to babel code
>   blocks
> - a single function in ob-core that is the inverse of the one above, for
>   processing the output of code blocks
>
> I think this implies:
> - org-list-parse-list deprecated/removed
> - org-list-to-generic deprecated/removed
> - callers of these two functions updated to use org-elements/ox (with new
>   helper functions introduced for this purpose if it seems necessary)
>
> The simplicity gains from this would be:
> - fewer functions in the public API of org (org-list-parse-list is replaced
>   by preexisting org-elements functions, and org-list-to-generic by ox
>   functions)
> - hopefully less code to maintain (in terms of lines), though it remains
>   to be seen how much or if at all these changes actually shrink the
>   code base
> - all org code manipulating lists uses a single format (org-elements).
>   Babel code blocks are not org code (and often not in elisp), so they
>   are the only thing that gets to use a different format
> - the plist format controlling org-list-to-generic goes away

Like for radio tables, radio lists are meant to be used in foreign
buffers, i.e., buffers not in Org mode. They have almost the same
requirements as Babel code blocks, which explains why they share the
same representation. The same happens with radio tables, which provide
their own representation to Babel, through `org-table-to-lisp'. If only
for symmetry, Org List should be the provider of any list
representation.

Moreover, `org-table-to-generic' is a powerful and flexible thin wrapper
around Org Export. In particular, `org-table-to-latex' is quite
different from calling `org-latex-export-to-latex' on an Org list.
Similarly, `org-list-to-generic' could be implemented as such, and,
therefore, may not be discarded too easily.

I'm not married to radio lists, and I wouldn't mind them being removed,
but, if they are to be kept, they should at least be implemented
correctly, not left in their current dumbed down state. Unfortunately,
if I had to guess, I'd say that radio lists have no user, unlike to
radio tables (which have their own video on a popular site).

My take on the problem at hand is

  - change `org-list-parse-list' to provide a simpler output and update
    Babel should to use that new output.

  - re-implement `org-list-to-subtree' using directly Elements, or even
    string massaging.

  - start a poll on the ML to guess the potential user base for radio
    lists. Depending on the results, we might either deprecate the whole
    thing, as you suggest, or implement in a more powerful and clean
    way. Note that first item implies `org-list-to-generic' should be
    updated to handle new output for `org-list-parse-list'.


Regards,

Re: Lexical binding bug in org-list.el?

[Aaron Ecay]

Hi Nicolas,

2015ko azaroak 7an, Nicolas Goaziou-ek idatzi zuen:
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>> On a broader scope, this is just one part of org that is written in this
>> style (one large let that defines many functions which call each other;
>> the body of the let is just a call into one of these functions).  This
>> isn’t idiomatic elisp (at least, I’ve never seen it outside org), and it
>> seems suboptimal for at least two reasons:
>> 1. The interface between the functions isn’t well-defined – they could
>> exchange information via arguments and/or by modifying variables
>> introduced by their containing let.
> 
> They are mutually recursive functions. I don't think that is
> un-idiomatic in the Lisp world. I don't think why the reason above would
> make them sub-optimal, too.

I guess it’s a scheme-ism.  <http://cl-su-ai.lisp.se/msg04741.html>.
Grepping for it in emacs sources, I see 4 uses in vc-* libraries, 1 in
octave, and 1 in emacs-lisp mode.  cl-labels is also implemented in
terms of letrec.  Org has 16 uses so far, all of them introduced since
the lexical binding change (or in any case since the last merge with
emacs).

I’m not saying we shouldn’t use it just because it’s rare, or a
scheme-ism.  Just satisfying my own curiosity out loud.  :)

> 
>> 2. It’s impossible to unit test the small let-bound functions.
> 
> Why would you want to unit test them? They are an implementation detail.
> Only `org-list-parse-list' should be tested here.

I don’t think we should unit-test only public API functions.  Indeed, I
find it easier to write unit tests for small functions, since I don’t
have to construct a lot of extra context to exercise all the corner
cases.

> 
>> In view of this, do you think it would be desirable in the long term to
>> refactor code like this into top-level functions?
> 
> No, I don't. I see no reason to pollute name space with these very
> specific functions.

I’m not sure I get it – interned symbols are cheap, and with the new-ish
double-dash convention for private functions users/downstream developers
can be pretty sure they don’t need to worry about them.

If it’s for us as org devs, I think the benefits outweigh the drawback
of having the extra function names to sift through in code completion
popups (which is the only one I can think of).  But opinions can differ,
and maybe I haven’t considered all the potential negatives.

[...]

> My take on the problem at hand is
> 
>   - change `org-list-parse-list' to provide a simpler output and update
>     Babel should to use that new output.
> 
>   - re-implement `org-list-to-subtree' using directly Elements, or even
>     string massaging.

OK, these both sound do-able.

> 
>   - start a poll on the ML to guess the potential user base for radio
>     lists. Depending on the results, we might either deprecate the whole
>     thing, as you suggest, or implement in a more powerful and clean
>     way. Note that first item implies `org-list-to-generic' should be
>     updated to handle new output for `org-list-parse-list'.

Given that no one’s ever asked for better radio lists despite us
advertising the feature and its unflattering comparison with more
powerful radio tables, I doubt there will be any interest.  But it’s
worth asking.  Should one of us start a new thread to ask this question?

-- 
Aaron Ecay

Re: Lexical binding bug in org-list.el?

[Nicolas Goaziou]

Aaron Ecay <aaronecay@gmail.com> writes:

> I don’t think we should unit-test only public API functions.

I didn't say that. However, I wouldn't write tests for "--" functions.

> Indeed, I find it easier to write unit tests for small functions,
> since I don’t have to construct a lot of extra context to exercise all
> the corner cases.

That's not my point. However I'd rather spent my time elsewhere than
writing tests for worker functions.

>>> In view of this, do you think it would be desirable in the long term to
>>> refactor code like this into top-level functions?
>> 
>> No, I don't. I see no reason to pollute name space with these very
>> specific functions.
>
> I’m not sure I get it – interned symbols are cheap, and with the new-ish
> double-dash convention for private functions users/downstream developers
> can be pretty sure they don’t need to worry about them.

I see no reason to define at top level a worker function with a very
limited scope. Much like let bindings I find it much clearer to keep the
scope of a function as close as possible to its real use, as long as the
function isn't too large. If one day, it is deemed useful in another
function, then it can be lifted to top level.

Anyway, this we're really nitpicking. There's nothing fundamentally
wrong in either choice.

>>   - change `org-list-parse-list' to provide a simpler output and update
>>     Babel should to use that new output.

So the new output could be

  plain-list = (type item+)
  type = ordered | unordered | descriptive
  item = ((text|plain-list)+)

Thus,

  1. first item
     + sub-item one
     + [X] sub-item two
     more text in first item
  2. [@3] last item

is parsed as

  (ordered
   ("first item"
    (unordered
     ("sub-item one")
     ("[X] sub-item two"))
    "more text in first item")
   ("[@3] last item"))

The slightly tricky part is taking care of the indentation when text
contains more than one line.

Also, for completeness, a descriptive list, e.g.,

  - tag :: text

would be simply parsed as

  (descriptive ("tag :: text"))

The objective is to make this syntax easy to create programmatically,
e.g. for Babel. Making something out of it would be the goal of
`org-list-to-generic'.

WDYT?

>>   - re-implement `org-list-to-subtree' using directly Elements, or even
>>     string massaging.
>
> OK, these both sound do-able.

Meanwhile, I fixed the current issue. It is as ugly as before, but at
least, no error is thrown anymore.

>>   - start a poll on the ML to guess the potential user base for radio
>>     lists. Depending on the results, we might either deprecate the whole
>>     thing, as you suggest, or implement in a more powerful and clean
>>     way. Note that first item implies `org-list-to-generic' should be
>>     updated to handle new output for `org-list-parse-list'.

Having slept over the idea, I think we could simply update
`org-list-to-generic' to be more robust and be done with it. I can
implement it in a couple of hours (and debug it in a couple of
months...).


Regards,

Re: Lexical binding bug in org-list.el?

[Aaron Ecay]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]

Hi Nicolas,

2015ko azaroak 8an, Nicolas Goaziou-ek idatzi zuen:

[...]

> Anyway, this we're really nitpicking. There's nothing fundamentally
> wrong in either choice.

You’re right, and I’m not trying to be quarrelsome.  At least for me,
it’s helpful to understand the perspective on things like this, so I can
try to model what is normative guidance and what is not.  Thanks for
humoring me.

> 
>>> - change `org-list-parse-list' to provide a simpler output and update
>>> Babel should to use that new output.
> 
> So the new output could be

[...] details elided

> 
> WDYT?

LGTM.  I’ve probably met my quota of org-related fun for the day (see
below...), but implementing this in terms of elements will be my next
org-list related task.

> 
>>> - re-implement `org-list-to-subtree' using directly Elements, or even
>>> string massaging.

It’s obvious to me that string-massaging causes friction in parts of
org’s code, so I’d like to try a different approach here.  The attached
patch makes a stab at doing the reimplementation in terms of elements.
I think it came out rather nicely, but I’d really value hearing your
opinion on it.

It’s very lightly tested so far.  I basically just used the following
snippet as a test case: put it in an org-mode buffer, put your cursor
somewhere inside the list, and M-: (org-list-to-subtree2)

- foo
- *bar*
  blorfle
- [X] baz
  - quux
  - 123

> Having slept over the idea, I think we could simply update
> `org-list-to-generic' to be more robust and be done with it. I can
> implement it in a couple of hours (and debug it in a couple of
> months...).

OK.  Don’t hesitate to ask if there’s some way we can help, of course.

Thanks,

-- 
Aaron Ecay

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-draft-implementation-of-org-list-to-subtree-in-terms.patch --]
[-- Type: text/x-diff, Size: 4710 bytes --]

From 2c41ae7704c133086a772b8651a1c3cd67feab78 Mon Sep 17 00:00:00 2001
From: Aaron Ecay <aaronecay@gmail.com>
Date: Sun, 8 Nov 2015 19:37:22 +0000
Subject: [PATCH] draft implementation of org-list-to-subtree in terms of
 org-element

---
 lisp/org-list.el | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/lisp/org-list.el b/lisp/org-list.el
index 19d5b03..1612e4e 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -3263,6 +3263,110 @@ syntax.  Return converted list as a string."
   (require 'ox-texinfo)
   (org-export-string-as list 'texinfo t))
 
+(defun org-list--partial-parse-contents (parse)
+  "Get the actual contents of a partial org-mode parse.
+
+Specifically, when parsing a piece of text smaller than a
+headline, `org-element-parse-buffer' wraps its result with a
+dummy `section' element, as well as the standard `org-data'
+wrapper.  This function removes these, returning a list of
+org-elements.
+
+TODO: maybe this needs a more general name."
+  (org-element-contents
+   ;; strip the org-data element
+   (nth 0 (org-element-contents
+	   ;; and the section element
+	   parse))))
+
+(defun org-list--split-first-line (contents)
+  "Remove the first line of text from an org-element item.
+
+CONTENTS are the contents of the item org-element: at least a
+paragraph followed by zero or more other elements.
+
+Returns a cons of the first textual line and a list of
+org-elements representing the structure of the item minus this
+line.
+
+TODO: is the first daughter of an item always a paragraph?"
+  (let ((graf (nth 0 contents)))
+    (unless (eq (org-element-type graf) 'paragraph)
+      (error "`org-list--split-first-line' got confused"))
+    (goto-char (org-element-property :begin graf))
+    (let* ((eol (point-at-eol))
+	   (end (org-element-property :end graf))
+	   (first-line (buffer-substring-no-properties (point) eol)))
+      (if (> (1+ eol) end)
+	  ;; One line paragraph: it becomes the entirety of the
+	  ;; headline, and we remove it from contents
+	  (setq contents (cdr contents))
+	;; Multi-line paragraph: narrow the buffer to lines 2-N, parse
+	;; them, and set them as the contents of the paragraph.
+	(save-restriction
+	  (widen)
+	  (narrow-to-region (1+ eol) end)
+	  (org-element-set-contents graf
+				    (org-list--partial-parse-contents
+				     ;; TODO: We're playing a trick on
+				     ;; the parser here.  AFAICT, the
+				     ;; parse does not rely on the
+				     ;; cache.  But maybe we should
+				     ;; let org-element-use-cache to
+				     ;; nil around this call, in case
+				     ;; that changes in the future.
+				     (org-element-parse-buffer)))))
+      (cons first-line contents))))
+
+(defun org-list--item-to-headline (item level)
+  "Convert an org-element list item to a headline.
+
+The first line of the list item becomes the "
+  (unless (eq (car item) 'item)
+    (error "`org-list--item-to-headline' expects an item argument"))
+  (let* ((r (org-list--split-first-line (org-element-contents item)))
+	 (title (car r))
+	 (other-contents (cdr r)))
+    (list 'headline
+	  `(:level ,level
+		   ,@(when (eq (org-element-property :checkbox item) 'on)
+		       (list :todo-keyword
+			     ;; TODO: how to fish the approporiate
+			     ;; value out of org-todo-keywords?
+			     "TODO"))
+		   :title ,title)
+	  (mapcar (lambda (x) (if (eq (org-element-type x) 'plain-list)
+				  (org-list--to-headlines x (1+ level))
+				x))
+		  other-contents))))
+
+(defun org-list--to-headlines (list level)
+  (unless (eq (car list) 'plain-list)
+    (error "`org-list-to-subtree' expects a plain-list argument"))
+  (mapcar (lambda (x) (org-list--item-to-headline x level))
+	  (org-element-contents list)))
+
+(defun org-list-to-subtree2 ()
+  (let* ((e (org-element-at-point))
+	 (l (org-element-lineage e))
+	 (list (cl-find-if (lambda (x) (eq (org-element-type x) 'plain-list))
+			   (nreverse l)))
+	 (level (org-reduced-level (or (org-current-level) 0)))
+	 (begin (org-element-property :begin list))
+	 (end (org-element-property :end list))
+	 (parse (save-restriction
+		  (widen)
+		  (narrow-to-region begin end)
+		  (org-element-parse-buffer)))
+	 (new-subtree (org-list--to-headlines
+		       (nth 0 (org-list--partial-parse-contents parse))
+		       level)))
+    (goto-char end)
+    ;; Don't eat the blank lines after the list.
+    (skip-chars-backward " \n\t\f")
+    (delete-region begin (point))
+    (insert (org-element-interpret-data new-subtree))))
+
 (defun org-list-to-subtree (list &optional params)
   "Convert LIST into an Org subtree.
 LIST is as returned by `org-list-parse-list'.  PARAMS is a property list
-- 
2.6.2

Re: Lexical binding bug in org-list.el?

[Kaushal Modi]

Thanks for fixing this.

Re: Lexical binding bug in org-list.el?

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> LGTM.  I’ve probably met my quota of org-related fun for the day (see
> below...), but implementing this in terms of elements will be my next
> org-list related task.

What is "this"?

Unfortunately org-list and Elements do not play nicely with each other
at the moment, because of org-struct, i.e., because of lists outside Org
buffers. Hence the `org-list-struct' and `org-element--list-struct'
duplication.

> It’s very lightly tested so far.  I basically just used the following
> snippet as a test case: put it in an org-mode buffer, put your cursor
> somewhere inside the list, and M-: (org-list-to-subtree2)

OK. Some comments follow.

> OK.  Don’t hesitate to ask if there’s some way we can help, of course.

I'll send a patch once I have the time to do this (depending on the bug
reports). I'd appreciate if you could update Babel accordingly.

> +(defun org-list--partial-parse-contents (parse)
> +  "Get the actual contents of a partial org-mode parse.
> +
> +Specifically, when parsing a piece of text smaller than a
> +headline, `org-element-parse-buffer' wraps its result with a
> +dummy `section' element, as well as the standard `org-data'
> +wrapper.  This function removes these, returning a list of
> +org-elements.
> +
> +TODO: maybe this needs a more general name."
> +  (org-element-contents
> +   ;; strip the org-data element
> +   (nth 0 (org-element-contents
> +	   ;; and the section element
> +	   parse))))

Actually, I often wonder if `section' should appear at all in the parse
tree. I mean, it is an important feature for export, but it could be
added automatically at export time (i.e., in `org-export-as', like many
other tree transforms) without cluttering the original parse tree.

Also, here, you probably really want

  (org-element-map parse org-element-all-elements #'identity nil t)

since you're going to handle a single element anyway.

> +(defun org-list--split-first-line (contents)
> +  "Remove the first line of text from an org-element item.
> +
> +CONTENTS are the contents of the item org-element: at least a
> +paragraph followed by zero or more other elements.
> +
> +Returns a cons of the first textual line and a list of
> +org-elements representing the structure of the item minus this
> +line.
> +
> +TODO: is the first daughter of an item always a paragraph?"

Absolutely not. E.g.,

   - 

     | a |

   - item 2


> +  (let ((graf (nth 0 contents)))
> +    (unless (eq (org-element-type graf) 'paragraph)
> +      (error "`org-list--split-first-line' got confused"))

See above.

> +    (goto-char (org-element-property :begin graf))
> +    (let* ((eol (point-at-eol))

Use the original! -> `line-end-position'. This one is a compatibility
alias for XEmacs.

> +	   (end (org-element-property :end graf))
> +	   (first-line (buffer-substring-no-properties (point) eol)))
> +      (if (> (1+ eol) end)
> +	  ;; One line paragraph: it becomes the entirety of the
> +	  ;; headline, and we remove it from contents
> +	  (setq contents (cdr contents))
> +	;; Multi-line paragraph: narrow the buffer to lines 2-N, parse
> +	;; them, and set them as the contents of the paragraph.
> +	(save-restriction
> +	  (widen)
> +	  (narrow-to-region (1+ eol) end)
> +	  (org-element-set-contents graf
> +				    (org-list--partial-parse-contents
> +				     ;; TODO: We're playing a trick on
> +				     ;; the parser here.  AFAICT, the
> +				     ;; parse does not rely on the
> +				     ;; cache.  But maybe we should
> +				     ;; let org-element-use-cache to
> +				     ;; nil around this call, in case
> +				     ;; that changes in the future.
> +				     (org-element-parse-buffer)))))

It shouldn't change. Also, there is `org-element-copy' if you need to
alter an element obtained through `org-element-at-point'.

> +      (cons first-line contents))))
> +
> +(defun org-list--item-to-headline (item level)
> +  "Convert an org-element list item to a headline.
> +
> +The first line of the list item becomes the "
> +  (unless (eq (car item) 'item)
> +    (error "`org-list--item-to-headline' expects an item argument"))
> +  (let* ((r (org-list--split-first-line (org-element-contents item)))
> +	 (title (car r))
> +	 (other-contents (cdr r)))
> +    (list 'headline
> +	  `(:level ,level
> +		   ,@(when (eq (org-element-property :checkbox item) 'on)
> +		       (list :todo-keyword
> +			     ;; TODO: how to fish the approporiate
> +			     ;; value out of org-todo-keywords?
> +			     "TODO"))

I'd try

  (car org-not-done-keywords)

However, it returns nil if we're parsing a non-Org buffer.

> +		   :title ,title)
> +	  (mapcar (lambda (x) (if (eq (org-element-type x) 'plain-list)
> +				  (org-list--to-headlines x (1+ level))
> +				x))
> +		  other-contents))))
> +
> +(defun org-list--to-headlines (list level)
> +  (unless (eq (car list) 'plain-list)
> +    (error "`org-list-to-subtree' expects a plain-list argument"))
> +  (mapcar (lambda (x) (org-list--item-to-headline x level))
> +	  (org-element-contents list)))
> +
> +(defun org-list-to-subtree2 ()
> +  (let* ((e (org-element-at-point))
> +	 (l (org-element-lineage e))
> +	 (list (cl-find-if (lambda (x) (eq (org-element-type x) 'plain-list))
> +			   (nreverse l)))

Also,

  ;; Find the top-most plain-list containing element at point.
  (org-element-map (nreverse l) 'plain-list #'identity nil t)

> +	 (level (org-reduced-level (or (org-current-level) 0)))
> +	 (begin (org-element-property :begin list))
> +	 (end (org-element-property :end list))
> +	 (parse (save-restriction
> +		  (widen)
> +		  (narrow-to-region begin end)
> +		  (org-element-parse-buffer)))
> +	 (new-subtree (org-list--to-headlines
> +		       (nth 0 (org-list--partial-parse-contents parse))
> +		       level)))
> +    (goto-char end)
> +    ;; Don't eat the blank lines after the list.
> +    (skip-chars-backward " \n\t\f")

Why "\f"?

> +    (delete-region begin (point))
> +    (insert (org-element-interpret-data new-subtree))))

Note that current implementation is slightly smarter since it handles
`org-blank-before-new-entry'.

One issue here is that we are going to duplicate some code since
`org-list-parse-list' is going to do slightly the same but with
a different representation.


Regards,

-- 
Nicolas Goaziou