Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> +                                            ;; Don't include all symbols
> +                                            ;; (bug#16646).
> +                                            (lambda (sym)
> +                                              (or (boundp sym)
> +                                                  (fboundp sym)
> +                                                  (symbol-plist sym)))
> +                                            'strict))

Shouldn't we also filter out functions (and maybe the symbols of the
third kind), depending on whether there's a quote before BEG?

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Stefan Monnier]

>> +                                            ;; Don't include all symbols
>> +                                            ;; (bug#16646).
>> +                                            (lambda (sym)
>> +                                              (or (boundp sym)
>> +                                                  (fboundp sym)
>> +                                                  (symbol-plist sym)))
>> +                                            'strict))

> Shouldn't we also filter out functions (and maybe the symbols of the
> third kind), depending on whether there's a quote before BEG?

Yes, except that "a quote before BEG" is not sufficient.
We could/should also check if one of the parent open-parens is prefixed
with a quote or a backquote, and only allow boundp if not.


        Stefan

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > Shouldn't we also filter out functions (and maybe the symbols of the
> > third kind), depending on whether there's a quote before BEG?
>
> Yes, except that "a quote before BEG" is not sufficient.
> We could/should also check if one of the parent open-parens is prefixed
> with a quote or a backquote, and only allow boundp if not.

What about macro calls?  E.g. (defadvice make-frame| ) - there's no
quoting involved.

Michael.

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Stefan Monnier]

> What about macro calls?  E.g. (defadvice make-frame| ) - there's no
> quoting involved.

Indeed, there can be other cases.


        Stefan

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Dmitry Gutov]

On 07.02.2014 00:00, Stefan Monnier wrote:
> Yes, except that "a quote before BEG" is not sufficient.
> We could/should also check if one of the parent open-parens is prefixed
> with a quote or a backquote, and only allow boundp if not.

Ok to install?

This doesn't handle macros, but most of them don't try to obfuscate 
whether a given form is evaluated or not, at least well-behaving ones.

`defadvice', ideally, would have to be handled specially anyway, to 
limit completions to functions.


=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2014-02-10 01:34:22 +0000
+++ lisp/emacs-lisp/lisp.el	2014-02-11 03:36:20 +0000
@@ -752,6 +752,20 @@
                       (mapcar #'symbol-name (lisp--local-variables))))))
           lastvars)))))

+(defun lisp--form-quoted-p ()
+  "Return non-nil if the form after point is not evaluated.
+It can be quoted, or be inside a quoted form.
+This function moves point."
+  ;; FIXME: Do some macro expansion maybe.
+  (or (eq (char-after) ?\[)
+      (progn
+        (skip-chars-backward " ")
+        (memq (char-before) '(?' ?`)))
+      (and (not (eq (char-before) ?,))
+           (ignore-errors
+             (up-list -1)
+             (lisp--form-quoted-p)))))
+
  ;; FIXME: Support for Company brings in features which straddle eldoc.
  ;; We should consolidate this, so that major modes can provide all that
  ;; data all at once:
@@ -835,12 +849,16 @@
                             lisp--local-variables-completion-table
                             (apply-partially 
#'completion-table-with-predicate
                                              obarray
-                                            ;; Don't include all symbols
-                                            ;; (bug#16646).
-                                            (lambda (sym)
-                                              (or (boundp sym)
-                                                  (fboundp sym)
-                                                  (symbol-plist sym)))
+                                            (if (save-excursion
+                                                  (goto-char beg)
+                                                  (lisp--form-quoted-p))
+                                                ;; Don't include all 
symbols
+                                                ;; (bug#16646).
+                                                (lambda (sym)
+                                                  (or (boundp sym)
+                                                      (fboundp sym)
+                                                      (symbol-plist sym)))
+                                              #'boundp)
                                              'strict))
                        :annotation-function
                        (lambda (str) (if (fboundp (intern-soft str)) " 
<f>"))

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Michael Heerdegen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> This doesn't handle macros, but most of them don't try to obfuscate
> whether a given form is evaluated or not, at least well-behaving ones.

Whether a macro evaluates arguments isn't a question of obfuscation.
Depending on circumstances, it's a necessity.

> `defadvice', ideally, would have to be handled specially anyway, to 
> limit completions to functions.
>
>
> === modified file 'lisp/emacs-lisp/lisp.el'
> --- lisp/emacs-lisp/lisp.el	2014-02-10 01:34:22 +0000
> +++ lisp/emacs-lisp/lisp.el	2014-02-11 03:36:20 +0000
> @@ -752,6 +752,20 @@
>                        (mapcar #'symbol-name (lisp--local-variables))))))
>            lastvars)))))
>
> +(defun lisp--form-quoted-p ()
> +  "Return non-nil if the form after point is not evaluated.
> +It can be quoted, or be inside a quoted form.
> +This function moves point."
> +  ;; FIXME: Do some macro expansion maybe.
> +  (or (eq (char-after) ?\[)
> +      (progn
> +        (skip-chars-backward " ")
> +        (memq (char-before) '(?' ?`)))
> +      (and (not (eq (char-before) ?,))
> +           (ignore-errors
> +             (up-list -1)
> +             (lisp--form-quoted-p)))))
> +
>   ;; FIXME: Support for Company brings in features which straddle eldoc.
>   ;; We should consolidate this, so that major modes can provide all that
>   ;; data all at once:
> @@ -835,12 +849,16 @@
>                              lisp--local-variables-completion-table
>                              (apply-partially 
> #'completion-table-with-predicate
>                                               obarray
> -                                            ;; Don't include all symbols
> -                                            ;; (bug#16646).
> -                                            (lambda (sym)
> -                                              (or (boundp sym)
> -                                                  (fboundp sym)
> -                                                  (symbol-plist sym)))
> +                                            (if (save-excursion
> +                                                  (goto-char beg)
> +                                                  (lisp--form-quoted-p))
> +                                                ;; Don't include all 
> symbols
> +                                                ;; (bug#16646).
> +                                                (lambda (sym)
> +                                                  (or (boundp sym)
> +                                                      (fboundp sym)
> +                                                      (symbol-plist sym)))
> +                                              #'boundp)
>                                               'strict))
>                         :annotation-function
>                         (lambda (str) (if (fboundp (intern-soft str)) " 
> <f>"))

I really don't think this is a good idea.  This would break completion
inside macros.  In any case, there are symbols I want to complete that
are not boundp, like keywords, tags, faces etc, also when they appear in
quoted structures.  Generally it's impossible to say how a symbol will
be used in LISP.  `lisp--form-quoted-p' does only work inside balanced
parentheses.  IMHO, having some false positives is less problematic than
breaking completion in some cases.

Maybe consider making the behavior customizable, so everybody can get
what he wants.

Michael.

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Dmitry Gutov]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Whether a macro evaluates arguments isn't a question of obfuscation.
> Depending on circumstances, it's a necessity.

When a macro doesn't evaluate an argument, most of the time it's to
introduce a new local var, or define one or several globals.

IOW, please provide an example of a popular macro that isn't `defadvice'
where this will be a problem.

> I really don't think this is a good idea.  This would break completion
> inside macros.

Only in some, ill-behaving ones.

> In any case, there are symbols I want to complete that
> are not boundp, like keywords, tags, faces etc, also when they appear in
> quoted structures.

Have you tried the patch? When you're inside a quoted structure, any
kinds of meaningful symbols should be offered as completions.

> `lisp--form-quoted-p' does only work inside balanced
> parentheses.

Example?

I don't see any problems if some parens are missing after point.

> IMHO, having some false positives is less problematic than
> breaking completion in some cases.

"Some"? Emacs has about as much functions as variables, not to mention
faces, features, etc.

On average, in non-quoted forms more than half of completions are false
positives.

> Maybe consider making the behavior customizable, so everybody can get
> what he wants.

Maybe. But see the beginning of this message.

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Michael Heerdegen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
> > Whether a macro evaluates arguments isn't a question of obfuscation.
> > Depending on circumstances, it's a necessity.
>
> When a macro doesn't evaluate an argument, most of the time it's to
> introduce a new local var, or define one or several globals.
>
> IOW, please provide an example of a popular macro that isn't `defadvice'
> where this will be a problem.

callf, callf2, defsetf.  Dunno if callf is popular enough, but I use it
very frequently.  And `function', as Stefan already mentioned.

> Have you tried the patch? When you're inside a quoted structure, any
> kinds of meaningful symbols should be offered as completions.

I had to fix some wrong line breaks in the patch before applying.  But
yes, quoted structures work as expected.

> > `lisp--form-quoted-p' does only work inside balanced
> > parentheses.
>
> Example?

Indeed, it works - my fault, I had only overflown the patch.

A situation where your patch doesn't work so well is in
quoted structures that actually are evaluated, like in `eval' or
`eval-after-load'.  Here, completion is like before.

But after thinking more about it, situations where your patch is holding
back valid completions are quite rare in practice, so I can live with
that change.  It would be good if the cl-callf2? case would work,
however.


Thanks,

Michael.

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Dmitry Gutov]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> callf, callf2, defsetf.  Dunno if callf is popular enough, but I use it
> very frequently.  And `function', as Stefan already mentioned.

Thanks. I've never used them, so far.

> A situation where your patch doesn't work so well is in
> quoted structures that actually are evaluated, like in `eval' or
> `eval-after-load'.  Here, completion is like before.

We might add special handling for `eval-after-load' eventually, but
`eval' should be more rare: I'd expect that most of the time it's passed
a variable, not a straight quoted form. Although yes, mutual canceling
of `eval' and one quote shouldn't be too hard to implement.

> But after thinking more about it, situations where your patch is holding
> back valid completions are quite rare in practice, so I can live with
> that change.  It would be good if the cl-callf2? case would work,
> however.

Ok, good. See the updated patch (in another email).

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Michael Heerdegen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> We might add special handling for `eval-after-load' eventually, but
> `eval' should be more rare: I'd expect that most of the time it's passed
> a variable, not a straight quoted form. Although yes, mutual canceling
> of `eval' and one quote shouldn't be too hard to implement.

I agree.  And `eval-after-load' has more or less been obsoleted by
`with-eval-after-load', where no quoting is necessary.

> Ok, good. See the updated patch (in another email).

Thanks, I'll have a look.

Michael.

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Stefan Monnier]

> We might add special handling for `eval-after-load' eventually, but

No: people should be moving to with-eval-after-load which does not
suffer from such problems.

> `eval' should be more rare: I'd expect that most of the time it's passed
> a variable, not a straight quoted form.  Although yes, mutual canceling
> of `eval' and one quote shouldn't be too hard to implement.

Again, probably undesirable, since the use of `eval' should not be encouraged.


        Stefan

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Dmitry Gutov]

On 13.02.2014 15:33, Stefan Monnier wrote:
>> We might add special handling for `eval-after-load' eventually, but
>
> No: people should be moving to with-eval-after-load which does not
> suffer from such problems.
>
>> `eval' should be more rare: I'd expect that most of the time it's passed
>> a variable, not a straight quoted form.  Although yes, mutual canceling
>> of `eval' and one quote shouldn't be too hard to implement.
>
> Again, probably undesirable, since the use of `eval' should not be encouraged.

Even better. I wasn't keen on adding these tweaks myself.

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Stefan Monnier]

>> Yes, except that "a quote before BEG" is not sufficient.
>> We could/should also check if one of the parent open-parens is prefixed
>> with a quote or a backquote, and only allow boundp if not.
> Ok to install?

Let's keep it for after 24.4.  It's clearly a new feature, and I don't
see any hurry to implement it.

> +(defun lisp--form-quoted-p ()
> +  "Return non-nil if the form after point is not evaluated.
> +It can be quoted, or be inside a quoted form.
> +This function moves point."
> +  ;; FIXME: Do some macro expansion maybe.
> +  (or (eq (char-after) ?\[)
> +      (progn
> +        (skip-chars-backward " ")
> +        (memq (char-before) '(?' ?`)))
> +      (and (not (eq (char-before) ?,))
> +           (ignore-errors
> +             (up-list -1)
> +             (lisp--form-quoted-p)))))

I think we'd want to use (nth 9 (syntax-ppss)) rather than up-list (it's
got the start pos of all enclosing lists in ).  Using syntax-ppss should
also help us handle the case we're inside a comment/string.

> `defadvice', ideally, would have to be handled specially anyway, to limit
> completions to functions.

Yes.  And `function' (aka #') should only complete functions as well.


        Stefan

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Let's keep it for after 24.4.  It's clearly a new feature, and I don't
> see any hurry to implement it.

Ok, you're the boss. I'd consider it mostly a bugfix, though. At least
the initial version.

> I think we'd want to use (nth 9 (syntax-ppss)) rather than up-list (it's
> got the start pos of all enclosing lists in ).  Using syntax-ppss should
> also help us handle the case we're inside a comment/string.

Done. I'm assuming you mean that when we're inside non-code area, we're
always "quoted".

>> `defadvice', ideally, would have to be handled specially anyway, to limit
>> completions to functions.
>
> Yes.  And `function' (aka #') should only complete functions as well.

And done.


=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2014-02-10 05:50:16 +0000
+++ lisp/emacs-lisp/lisp.el	2014-02-13 05:21:15 +0000
@@ -762,6 +762,46 @@
                      (mapcar #'symbol-name (lisp--local-variables))))))
          lastvars)))))
 
+(defun lisp--expect-function-p (pos)
+  "Return non-nil if the symbol at point is expected to be a function."
+  (or
+   (and (eq (char-before pos) ?')
+        (eq (char-before (1- pos)) ?#))
+   (save-excursion
+     (let ((parent (nth 1 (syntax-ppss pos))))
+       (when parent
+         (goto-char parent)
+         (and
+          (looking-at (concat "(\\(cl-\\)?"
+                              (regexp-opt '("function" "defadvice"
+                                            "callf" "callf2"
+                                            "defsetf"))
+                              "[ \t\r\n]+"))
+          (eq (match-end 0) pos)))))))
+
+(defun lisp--form-quoted-p (pos)
+  "Return non-nil if the form at POS is not evaluated.
+It can be quoted, or be inside a quoted form.
+This function moves point."
+  ;; FIXME: Do some macro expansion maybe.
+  (save-excursion
+    (let ((state (syntax-ppss pos)))
+      (or (nth 8 state)   ; Code inside strings usually isn't evaluated.
+          ;; FIXME: The 9th element is undocumented.
+          (let ((nesting (cons (point) (reverse (nth 9 state))))
+                res)
+            (while (and nesting (not res))
+              (goto-char (pop nesting))
+              (cond
+               ((or (eq (char-after) ?\[)
+                    (progn
+                      (skip-chars-backward " ")
+                      (memq (char-before) '(?' ?`))))
+                (setq res t))
+               ((eq (char-before) ?,)
+                (setq nesting nil))))
+            res)))))
+
 ;; FIXME: Support for Company brings in features which straddle eldoc.
 ;; We should consolidate this, so that major modes can provide all that
 ;; data all at once:
@@ -841,22 +881,41 @@
                 ;; use it to provide a more specific completion table in some
                 ;; cases.  E.g. filter out keywords that are not understood by
                 ;; the macro/function being called.
-                (list nil (completion-table-merge
-                           lisp--local-variables-completion-table
-                           (apply-partially #'completion-table-with-predicate
-                                            obarray
-                                            ;; Don't include all symbols
-                                            ;; (bug#16646).
-                                            (lambda (sym)
-                                              (or (boundp sym)
-                                                  (fboundp sym)
-                                                  (symbol-plist sym)))
-                                            'strict))
+                (cond
+                 ((lisp--expect-function-p beg)
+                  (list nil obarray
+                        :predicate #'fboundp
+                        :company-doc-buffer #'lisp--company-doc-buffer
+                        :company-docsig #'lisp--company-doc-string
+                        :company-location #'lisp--company-location))
+                 ((lisp--form-quoted-p beg)
+                  (list nil (completion-table-merge
+                             ;; FIXME: Is this table useful for this case?
+                             lisp--local-variables-completion-table
+                             (apply-partially #'completion-table-with-predicate
+                                              obarray
+                                              ;; Don't include all symbols
+                                              ;; (bug#16646).
+                                              (lambda (sym)
+                                                (or (boundp sym)
+                                                    (fboundp sym)
+                                                    (symbol-plist sym)))
+                                              'strict))
                       :annotation-function
                       (lambda (str) (if (fboundp (intern-soft str)) " <f>"))
                       :company-doc-buffer #'lisp--company-doc-buffer
                       :company-docsig #'lisp--company-doc-string
-                      :company-location #'lisp--company-location)
+                      :company-location #'lisp--company-location))
+                 (t
+                  (list nil (completion-table-merge
+                             lisp--local-variables-completion-table
+                             (apply-partially #'completion-table-with-predicate
+                                              obarray
+                                              #'boundp
+                                              'strict))
+                        :company-doc-buffer #'lisp--company-doc-buffer
+                        :company-docsig #'lisp--company-doc-string
+                        :company-location #'lisp--company-location)))
               ;; Looks like a funcall position.  Let's double check.
               (save-excursion
                 (goto-char (1- beg))

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Michael Heerdegen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> And done.

Works for me after doing some quick tests.  Question: The doc of
`lisp--form-quoted-p' says that "This function moves point.", but the
whole body is wrapped in a `save-excursion' ...?

Regards,

Michael.

Re: trunk r116285: * lisp/emacs-lisp/lisp.el (lisp-completion-at-point): Symbols don't start

[Dmitry Gutov]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Question: The doc of
> `lisp--form-quoted-p' says that "This function moves point.", but the
> whole body is wrapped in a `save-excursion' ...?

Thanks, that was a leftover from the previous patch, now fixed locally.