completion-at-point + semantic : erroneous error

completion-at-point + semantic : erroneous error

[Eric Ludlam]

Hi all,

I'm updating from Emacs 24 w/ CEDET from sourceforge to Emacs from git, 
using built-in cedet, and I encountered an unexpected error.  A 
simplified reproduction step is:

emacs -q
M-x semantic-mode
; visit a file supported by semantic, such as a C file
; put cursor in a blank space
M-x completion-at-point

It will error with: "Nothing to complete".

The underlying completion function has always issued this error, but the 
way it was brought into completion-at-point causes it to error mid way. 
I'm not completely familiar with completion-at-point behavior, but my 
assumption is it terminates navigating the list of completion functions.

This also showed up when using company-mode which seems to use the same 
underlying function list by default.  It is also especially annoying if 
'debug-on-error' is on with company mode, as the stack just keeps 
popping up when typing innocuous things.

The list of items in the list of completion functions include 
`semantic-analyze-completion-at-point-function' and a couple others.  I 
believe these functions, specific to completion-at-point, need to wrap 
their call the the underlying `semantic-analyze-possible-completions' so 
that errors aren't thrown, and it doesn't get in the way when debugging 
something completely different.

The below patch solved the problem for me.

Thanks
Eric


diff --git a/lisp/cedet/semantic/analyze.el b/lisp/cedet/semantic/analyze.el
index 6851ad556a..4e2d9d8728 100644
--- a/lisp/cedet/semantic/analyze.el
+++ b/lisp/cedet/semantic/analyze.el
@@ -827,7 +827,9 @@ semantic-analyze-completion-at-point-function
  This function can be used by `completion-at-point-functions'."
    (when (semantic-active-p)
      (let* ((ctxt (semantic-analyze-current-context))
-           (possible (semantic-analyze-possible-completions ctxt)))
+           (possible (condition-case nil
+                         (semantic-analyze-possible-completions ctxt)
+                       (error nil))))

        ;; The return from this is either:
        ;; nil - not applicable here.
@@ -846,7 +848,9 @@ semantic-analyze-notc-completion-at-point-function
  This function can be used by `completion-at-point-functions'."
    (when (semantic-active-p)
      (let* ((ctxt (semantic-analyze-current-context))
-           (possible (semantic-analyze-possible-completions ctxt 'no-tc)))
+           (possible (condition-case nil
+                         (semantic-analyze-possible-completions ctxt 
'no-tc)
+                       (error nil))))

        (when possible
          (list (car (oref ctxt bounds))
@@ -862,8 +866,10 @@ 
semantic-analyze-nolongprefix-completion-at-point-function
  This function can be used by `completion-at-point-functions'."
    (when (semantic-active-p)
      (let* ((ctxt (semantic-analyze-current-context))
-           (possible (semantic-analyze-possible-completions
-                      ctxt 'no-tc 'no-longprefix)))
+           (possible (condition-case nil
+                         (semantic-analyze-possible-completions
+                          ctxt 'no-tc 'no-longprefix)
+                       (error nil))))

        (when possible
          (list (car (oref ctxt bounds))

Re: completion-at-point + semantic : erroneous error

[Stefan Monnier]

> emacs -q
> M-x semantic-mode
> ; visit a file supported by semantic, such as a C file
> ; put cursor in a blank space
> M-x completion-at-point

The backtrace shows that the error is signaled by

semantic-analyze-possible-completions called from
semantic-analyze-completion-at-point-function.  I'm not sufficiently
familiar with the way define-overloadable-function works to be sure what
to do here, but the patch below might be a start (it seems to fix the
immediate problem you're reporting here, but I see other places where
we signal this error).

Arguably, semantic-analyze-possible-completions should never be called
interactively nowadays (should always go through
completion-at-point-functions), so there are likely some cleanups that
could be done.


        Stefan


PS: It would also be good to replace uses of
define-overloadable-function with cl-defmethod, although the mapping
from one to the other is not immediate.


diff --git a/lisp/cedet/semantic/analyze/complete.el b/lisp/cedet/semantic/analyze/complete.el
index b471c0d1a1..ea9bf9111b 100644
--- a/lisp/cedet/semantic/analyze/complete.el
+++ b/lisp/cedet/semantic/analyze/complete.el
@@ -93,7 +93,8 @@ semantic-analyze-possible-completions
 			  context
 			(semantic-analyze-current-context context)))
 	     (ans (if (not context)
-		      (error "Nothing to complete")
+	              (when (called-interactively-p 'any)
+		        (error "Nothing to complete"))
 		    (:override))))
 	;; If interactive, display them.
 	(when (called-interactively-p 'any)

Re: completion-at-point + semantic : erroneous error

[Stefan Monnier]

> PS: It would also be good to replace uses of
> define-overloadable-function with cl-defmethod, although the mapping
> from one to the other is not immediate.

The patch below is an attempt to do that for
semantic-analyze-possible-completions.  I'd be interested to hear your
opinion on this (especially the with-mode-local part).


        Stefan


diff --git a/lisp/cedet/semantic/analyze/complete.el b/lisp/cedet/semantic/analyze/complete.el
index b471c0d1a1..193c975c47 100644
--- a/lisp/cedet/semantic/analyze/complete.el
+++ b/lisp/cedet/semantic/analyze/complete.el
@@ -63,7 +63,7 @@ semantic-analyze-tags-of-class-list
 ;;; MAIN completion calculator
 ;;
 ;;;###autoload
-(define-overloadable-function semantic-analyze-possible-completions (context &rest flags)
+(cl-defgeneric semantic-analyze-possible-completions (context &optional flags)
   "Return a list of semantic tags which are possible completions.
 CONTEXT is either a position (such as point), or a precalculated
 context.  Passing in a context is useful if the caller also needs
@@ -83,7 +83,9 @@ semantic-analyze-possible-completions
   * Argument to a function with type constraints.
 When called interactively, displays the list of possible completions
 in a buffer."
-  (interactive "d")
+  (semantic-analyze-possible-completions-default context flags))
+
+(cl-defmethod semantic-analyze-possible-completions :around (context &rest flags)
   ;; In theory, we don't need the below since the context will
   ;; do it for us.
   ;;(semantic-refresh-tags-safe)
@@ -93,8 +95,9 @@ semantic-analyze-possible-completions
 			  context
 			(semantic-analyze-current-context context)))
 	     (ans (if (not context)
-		      (error "Nothing to complete")
-		    (:override))))
+	              (when (called-interactively-p 'any)
+		        (error "Nothing to complete"))
+		    (cl-call-next-method))))
 	;; If interactive, display them.
 	(when (called-interactively-p 'any)
 	  (with-output-to-temp-buffer "*Possible Completions*"
diff --git a/lisp/cedet/semantic/bovine/make.el b/lisp/cedet/semantic/bovine/make.el
index 3676c6972f..dbfa060b5d 100644
--- a/lisp/cedet/semantic/bovine/make.el
+++ b/lisp/cedet/semantic/bovine/make.el
@@ -32,9 +32,6 @@
 (require 'semantic/analyze)
 (require 'semantic/dep)
 
-(declare-function semantic-analyze-possible-completions-default
-		  "semantic/analyze/complete")
-
 ;;; Code:
 (define-lex-analyzer semantic-lex-make-backslash-no-newline
   "Detect and create a beginning of line token (BOL)."
@@ -174,13 +171,13 @@ semantic-format-tag-uml-prototype
 This is the same as a regular prototype."
   (semantic-format-tag-prototype tag parent color))
 
-(define-mode-local-override semantic-analyze-possible-completions
-  makefile-mode (context)
+(cl-defmethod semantic-analyze-possible-completions
+  (context &context (major-mode makefile-mode) &rest _)
   "Return a list of possible completions in a Makefile.
 Uses default implementation, and also gets a list of filenames."
   (require 'semantic/analyze/complete)
   (with-current-buffer (oref context buffer)
-    (let* ((normal (semantic-analyze-possible-completions-default context))
+    (let* ((normal (cl-call-next-method))
 	   (classes (oref context prefixclass))
 	   (filetags nil))
       (when (memq 'filename classes)
diff --git a/lisp/cedet/semantic/grammar.el b/lisp/cedet/semantic/grammar.el
index 813580ba6c..6767cb10fc 100644
--- a/lisp/cedet/semantic/grammar.el
+++ b/lisp/cedet/semantic/grammar.el
@@ -1914,13 +1914,15 @@ semantic-analyze-current-context
 
       context-return)))
 
-(define-mode-local-override semantic-analyze-possible-completions
-  semantic-grammar-mode (context)
+(cl-defmethod semantic-analyze-possible-completions
+  (context &context (major-mode semantic-grammar-mode) &rest _)
   "Return a list of possible completions based on CONTEXT."
   (require 'semantic/analyze/complete)
   (if (semantic-grammar-in-lisp-p)
+      ;; FIXME: Does the `let' make the `with-mode-local' redundant here?
       (with-mode-local emacs-lisp-mode
-	(semantic-analyze-possible-completions context))
+	(let ((major-mode 'emacs-lisp-mode))
+	  (semantic-analyze-possible-completions context)))
     (with-current-buffer (oref context buffer)
       (let* ((prefix (car (oref context prefix)))
 	     (completetext (cond ((semantic-tag-p prefix)
diff --git a/lisp/cedet/semantic/texi.el b/lisp/cedet/semantic/texi.el
index 3a0050b920..510c4ea043 100644
--- a/lisp/cedet/semantic/texi.el
+++ b/lisp/cedet/semantic/texi.el
@@ -412,8 +412,8 @@ semantic-texi-command-completion-list
 	  )
   "List of commands that we might bother completing.")
 
-(define-mode-local-override semantic-analyze-possible-completions
-  texinfo-mode (context)
+(cl-defmethod semantic-analyze-possible-completions
+  (context &context (major-mode texinfo-mode) &rest _)
   "List smart completions at point.
 Since texinfo is not a programming language the default version is not
 useful.  Instead, look at the current symbol.  If it is a command

Re: completion-at-point + semantic : erroneous error

[Eric Ludlam]

On 10/11/19 1:34 PM, Stefan Monnier wrote:
>> PS: It would also be good to replace uses of
>> define-overloadable-function with cl-defmethod, although the mapping
>> from one to the other is not immediate.
> The patch below is an attempt to do that for
> semantic-analyze-possible-completions.  I'd be interested to hear your
> opinion on this (especially the with-mode-local part).

Thanks for looking into this Stefan.

For the part of the patch that only issues the error if called 
interactively, that makes sense to me.  I used that fcn interactively 
when debugging the tool, or debugging new language support, but most 
users probably won't.

I don't know enough about how far and wide it is used these days to know 
if that error is needed by anyone.  I would suspect not, since returning 
empty happens for other reasons too.


For the part of the patch replacing `define-overloadable-function' with 
methods, I was surprised by the &context syntax.  I've been away from 
emacs devel too long to be familiar with it, but the doc on it was 
helpful. mode-local.el was always a hack to overcome the mess that was 
managing behavior changes between modes while providing useful base 
functionality.  Updating to methods using &context seems like a fine way 
to solve the same problem.

I went back to mode-local to remind myself about it.  One of the things 
it handled was derived modes.  I vaguely recall this being important for 
Makefiles as there are several makefile-mode variants? (looks in 
make-mode.el) - yes, there are a bunch.  Not sure what a "&context 
(major-mode ...) " does to know if it is derived from a parent mode or 
not.  There is a mode-local-equivalent-mode-p predicate for handling that.

Also in your patch is a TODO about `with-mode-local'.  That 
with-mode-local did more than just allow the next method call to 
dispatch correctly.  It also sets up mode local variables, and other 
mode-local dispatchers for anything the dispatched call might need.  In 
this case, the completion call uses lots of other mode local dispatched 
functions.  with-mode-local doesn't set the major-mode though.  Since 
all of CEDET is presumably still using mode-local features, you will 
need both.


I hope this helps.

Eric


>
>          Stefan
>
>
> diff --git a/lisp/cedet/semantic/analyze/complete.el b/lisp/cedet/semantic/analyze/complete.el
> index b471c0d1a1..193c975c47 100644
> --- a/lisp/cedet/semantic/analyze/complete.el
> +++ b/lisp/cedet/semantic/analyze/complete.el
> @@ -63,7 +63,7 @@ semantic-analyze-tags-of-class-list
>   ;;; MAIN completion calculator
>   ;;
>   ;;;###autoload
> -(define-overloadable-function semantic-analyze-possible-completions (context &rest flags)
> +(cl-defgeneric semantic-analyze-possible-completions (context &optional flags)
>     "Return a list of semantic tags which are possible completions.
>   CONTEXT is either a position (such as point), or a precalculated
>   context.  Passing in a context is useful if the caller also needs
> @@ -83,7 +83,9 @@ semantic-analyze-possible-completions
>     * Argument to a function with type constraints.
>   When called interactively, displays the list of possible completions
>   in a buffer."
> -  (interactive "d")
> +  (semantic-analyze-possible-completions-default context flags))
> +
> +(cl-defmethod semantic-analyze-possible-completions :around (context &rest flags)
>     ;; In theory, we don't need the below since the context will
>     ;; do it for us.
>     ;;(semantic-refresh-tags-safe)
> @@ -93,8 +95,9 @@ semantic-analyze-possible-completions
>   			  context
>   			(semantic-analyze-current-context context)))
>   	     (ans (if (not context)
> -		      (error "Nothing to complete")
> -		    (:override))))
> +	              (when (called-interactively-p 'any)
> +		        (error "Nothing to complete"))
> +		    (cl-call-next-method))))
>   	;; If interactive, display them.
>   	(when (called-interactively-p 'any)
>   	  (with-output-to-temp-buffer "*Possible Completions*"
> diff --git a/lisp/cedet/semantic/bovine/make.el b/lisp/cedet/semantic/bovine/make.el
> index 3676c6972f..dbfa060b5d 100644
> --- a/lisp/cedet/semantic/bovine/make.el
> +++ b/lisp/cedet/semantic/bovine/make.el
> @@ -32,9 +32,6 @@
>   (require 'semantic/analyze)
>   (require 'semantic/dep)
>   
> -(declare-function semantic-analyze-possible-completions-default
> -		  "semantic/analyze/complete")
> -
>   ;;; Code:
>   (define-lex-analyzer semantic-lex-make-backslash-no-newline
>     "Detect and create a beginning of line token (BOL)."
> @@ -174,13 +171,13 @@ semantic-format-tag-uml-prototype
>   This is the same as a regular prototype."
>     (semantic-format-tag-prototype tag parent color))
>   
> -(define-mode-local-override semantic-analyze-possible-completions
> -  makefile-mode (context)
> +(cl-defmethod semantic-analyze-possible-completions
> +  (context &context (major-mode makefile-mode) &rest _)
>     "Return a list of possible completions in a Makefile.
>   Uses default implementation, and also gets a list of filenames."
>     (require 'semantic/analyze/complete)
>     (with-current-buffer (oref context buffer)
> -    (let* ((normal (semantic-analyze-possible-completions-default context))
> +    (let* ((normal (cl-call-next-method))
>   	   (classes (oref context prefixclass))
>   	   (filetags nil))
>         (when (memq 'filename classes)
> diff --git a/lisp/cedet/semantic/grammar.el b/lisp/cedet/semantic/grammar.el
> index 813580ba6c..6767cb10fc 100644
> --- a/lisp/cedet/semantic/grammar.el
> +++ b/lisp/cedet/semantic/grammar.el
> @@ -1914,13 +1914,15 @@ semantic-analyze-current-context
>   
>         context-return)))
>   
> -(define-mode-local-override semantic-analyze-possible-completions
> -  semantic-grammar-mode (context)
> +(cl-defmethod semantic-analyze-possible-completions
> +  (context &context (major-mode semantic-grammar-mode) &rest _)
>     "Return a list of possible completions based on CONTEXT."
>     (require 'semantic/analyze/complete)
>     (if (semantic-grammar-in-lisp-p)
> +      ;; FIXME: Does the `let' make the `with-mode-local' redundant here?
>         (with-mode-local emacs-lisp-mode
> -	(semantic-analyze-possible-completions context))
> +	(let ((major-mode 'emacs-lisp-mode))
> +	  (semantic-analyze-possible-completions context)))
>       (with-current-buffer (oref context buffer)
>         (let* ((prefix (car (oref context prefix)))
>   	     (completetext (cond ((semantic-tag-p prefix)
> diff --git a/lisp/cedet/semantic/texi.el b/lisp/cedet/semantic/texi.el
> index 3a0050b920..510c4ea043 100644
> --- a/lisp/cedet/semantic/texi.el
> +++ b/lisp/cedet/semantic/texi.el
> @@ -412,8 +412,8 @@ semantic-texi-command-completion-list
>   	  )
>     "List of commands that we might bother completing.")
>   
> -(define-mode-local-override semantic-analyze-possible-completions
> -  texinfo-mode (context)
> +(cl-defmethod semantic-analyze-possible-completions
> +  (context &context (major-mode texinfo-mode) &rest _)
>     "List smart completions at point.
>   Since texinfo is not a programming language the default version is not
>   useful.  Instead, look at the current symbol.  If it is a command
>
>

Re: completion-at-point + semantic : erroneous error

[Stefan Monnier]

> For the part of the patch replacing `define-overloadable-function' with
> methods, I was surprised by the &context syntax.  I've been away from emacs
> devel too long to be familiar with it, but the doc on it was
> helpful. mode-local.el was always a hack to overcome the mess that was
> managing behavior changes between modes while providing useful base
> functionality.  Updating to methods using &context seems like a fine way to
> solve the same problem.
>
> I went back to mode-local to remind myself about it.  One of the things it
> handled was derived modes. 

The `derived-mode` specializer I used in the patch for the &context part
correctly handles derived modes, as the name suggests.  It doesn't pay
attention to mode-local's `mode-local-parent` property, of course, only
to the `derived-mode-parent` property.

[ BTW: I'd like to remove the `mode-local-parent` property.
  AFAICT it's only ever set by set-mode-local-parent which is only used by
  define-child-mode which is used as follows:

    bovine/c.el:(define-child-mode c++-mode c-mode
    bovine/el.el:(define-child-mode lisp-mode emacs-lisp-mode
    html.el:(define-child-mode html-helper-mode html-mode
    wisent/javascript.el:(define-child-mode js-mode javascript-mode)
    wisent/javascript.el:(define-child-mode js-mode javascript-mode)
    wisent/python.el:(define-child-mode python-2-mode python-mode "Python 2 mode")
    wisent/python.el:(define-child-mode python-3-mode python-mode "Python 3 mode")

  I suspect these could be replaced with other things.  WDYT? ]

> Not sure what a "&context (major-mode ...) " does to know if it is
> derived from a parent mode or not. 

It looks at the derived-mode-parent property.

> Also in your patch is a TODO about `with-mode-local'.  That with-mode-local
> did more than just allow the next method call to dispatch correctly.  It
> also sets up mode local variables, and other mode-local dispatchers for
> anything the dispatched call might need.  In this case, the completion call
> uses lots of other mode local dispatched functions.  with-mode-local doesn't
> set the major-mode though.  Since all of CEDET is presumably still using
> mode-local features, you will need both.

Yes.  More importantly, I see that additionally to
define-mode-local-override some functions are overloaded on
a buffer-local basis via semantic-install-function-overrides.
This doesn't fit well with the cl-defmethod system.  I can probably get
rid of semantic-install-function-overrides, but do you happen to
know if some functions are overloaded on a buffer-local basis from
elsewhere than semantic-install-function-overrides?


        Stefan

Re: completion-at-point + semantic : erroneous error

[Eric Ludlam]

On 10/24/19 4:17 PM, Stefan Monnier wrote:
>> I went back to mode-local to remind myself about it.  One of the things it
>> handled was derived modes.
> The `derived-mode` specializer I used in the patch for the &context part
> correctly handles derived modes, as the name suggests.  It doesn't pay
> attention to mode-local's `mode-local-parent` property, of course, only
> to the `derived-mode-parent` property.
>
> [ BTW: I'd like to remove the `mode-local-parent` property.
>    AFAICT it's only ever set by set-mode-local-parent which is only used by
>    define-child-mode which is used as follows:
>
>      bovine/c.el:(define-child-mode c++-mode c-mode
>      bovine/el.el:(define-child-mode lisp-mode emacs-lisp-mode
>      html.el:(define-child-mode html-helper-mode html-mode
>      wisent/javascript.el:(define-child-mode js-mode javascript-mode)
>      wisent/javascript.el:(define-child-mode js-mode javascript-mode)
>      wisent/python.el:(define-child-mode python-2-mode python-mode "Python 2 mode")
>      wisent/python.el:(define-child-mode python-3-mode python-mode "Python 3 mode")
>
>    I suspect these could be replaced with other things.  WDYT? ]

It sounds like a goal is to slowly remove mode-local.  If there is a 
better official way to do the same thing that seems fine with me.

For this specific item, I'm curious what the alternative might be.  The 
obvious solution I can think of is making all the assignments for 
functions and variables to all relevant modes, which feels error prone.  
This was a way to specify similar modes for all overrides for this tool.

>
>> Also in your patch is a TODO about `with-mode-local'.  That with-mode-local
>> did more than just allow the next method call to dispatch correctly.  It
>> also sets up mode local variables, and other mode-local dispatchers for
>> anything the dispatched call might need.  In this case, the completion call
>> uses lots of other mode local dispatched functions.  with-mode-local doesn't
>> set the major-mode though.  Since all of CEDET is presumably still using
>> mode-local features, you will need both.
> Yes.  More importantly, I see that additionally to
> define-mode-local-override some functions are overloaded on
> a buffer-local basis via semantic-install-function-overrides.
> This doesn't fit well with the cl-defmethod system.  I can probably get
> rid of semantic-install-function-overrides, but do you happen to
> know if some functions are overloaded on a buffer-local basis from
> elsewhere than semantic-install-function-overrides?

I'm not sure.  David Engster did most of the work on mode-local. There 
used to be the primitive semantic- only version you found that he 
wrapped up in mode-local.  Looking at this in retrospect, I'm not sure 
why the functions installed with semantic-install-function-overrides 
weren't done using mode-local more directly.  If they were converted, 
then semantic-install-function-overrides could be removed.

On a side note, I was testing your patch that started this thread by 
converting more tests from CEDET on sourceforge to be part of Emacs.  It 
has test files from a broader range of modes.  It doesn't test all the 
different overrides and modes, but if a goal is to factor mode-local 
out, it could more definitively answer if any parsing infrastructure is 
broken given some of these proposed changes.  I'll try and get it 
wrapped up and ready soon.

Eric

Re: completion-at-point + semantic : erroneous error

[Stefan Monnier]

>>> I went back to mode-local to remind myself about it.  One of the things it
>>> handled was derived modes.
>> The `derived-mode` specializer I used in the patch for the &context part
>> correctly handles derived modes, as the name suggests.  It doesn't pay
>> attention to mode-local's `mode-local-parent` property, of course, only
>> to the `derived-mode-parent` property.
>>
>> [ BTW: I'd like to remove the `mode-local-parent` property.
>>    AFAICT it's only ever set by set-mode-local-parent which is only used by
>>    define-child-mode which is used as follows:
>>
>>      bovine/c.el:(define-child-mode c++-mode c-mode
>>      bovine/el.el:(define-child-mode lisp-mode emacs-lisp-mode
>>      html.el:(define-child-mode html-helper-mode html-mode
>>      wisent/javascript.el:(define-child-mode js-mode javascript-mode)
>>      wisent/javascript.el:(define-child-mode js-mode javascript-mode)
>>      wisent/python.el:(define-child-mode python-2-mode python-mode "Python 2 mode")
>>      wisent/python.el:(define-child-mode python-3-mode python-mode "Python 3 mode")
>>
>>    I suspect these could be replaced with other things.  WDYT? ]
>
> It sounds like a goal is to slowly remove mode-local.

Yes and no: I'd like to remove the duplication that it entails.
E.g. I think defmethod's &context has made mode-local's
overloadable-functions largely redundant, so I think it would be good to
remove those overloadable-functions.

I haven't looked at the mode-local-variable part of mode-local.el, so
I don't plan on removing any of it for now, tho I think that if it
stays, it would be good to better integrate it into the rest of Emacs.

> If there is a better official way to do the same thing that seems fine
> with me.

My hope is that defmethod's &context covers those needs and that "it
seems fine" to you.  Don't know if it's the case.

> For this specific item, I'm curious what the alternative might be.  The
> obvious solution I can think of is making all the assignments for 
> functions and variables to all relevant modes, which feels error prone. 
> This was a way to specify similar modes for all overrides for this tool.

W.r.t the `mode-local-parent` property, it looks pretty ad-hoc (not to
say hackish): why not set `derived-mode-parent` instead?  Of course, the
right way to set it is to change the mode so it sets it via
`define-derived-mode`.  Otherwise you're in "it's kind of a child but
not really" territory.

BTW, regarding the above uses of define-child-mode, they've been reduced
down to just:

    bovine/c.el:  (define-child-mode c++-mode c-mode
    bovine/el.el:  (define-child-mode lisp-mode emacs-lisp-mode

I think the `lisp-mode` one is an error: lisp-mode is supposed to be for
common-lisp, which is clearly not a child of emacs-lisp-mode.
This said, AFAIK noone uses lisp-mode, everyone uses some other mode for
common-lisp, either the one from SLIME or the one from SLY.

The one for `c++-mode` is more tricky: I guess one could change cc-mode
to make c++-mode derive from c-mode instead of prog-mode, but that would
make it run c-mode-hook which some users might not like.  Maybe we
should have a c-base-mode from which both c-mode and c++-mode derive?
This question is of course largely irrelevant since Alan will likely
never accept any such change in cc-mode.el.  But I think it would be
perfectly fine to make define-child-mode set the derived-mode-parent
property in this particular case.

> I'm not sure.  David Engster did most of the work on mode-local. There used
> to be the primitive semantic- only version you found that he wrapped up in
> mode-local.  Looking at this in retrospect, I'm not sure why the functions
> installed with semantic-install-function-overrides weren't done using
> mode-local more directly.  If they were converted, then
> semantic-install-function-overrides could be removed.

OK.  I'm not sufficiently familiar with the code to see how it can be
changed to use define-overloadable-function instead of
semantic-install-function-overrides, but I'll try and find out.

> On a side note, I was testing your patch that started this thread by
> converting more tests from CEDET on sourceforge to be part of Emacs.  It 
> has test files from a broader range of modes.  It doesn't test all the
> different overrides and modes, but if a goal is to factor mode-local 
> out, it could more definitively answer if any parsing infrastructure is
> broken given some of these proposed changes.  I'll try and get it 
> wrapped up and ready soon.

That would be great, yes,


        Stefan

Re: completion-at-point + semantic : erroneous error

[Eric Ludlam]

On 10/27/19 4:38 PM, Stefan Monnier wrote:
>>
>> It sounds like a goal is to slowly remove mode-local.
> Yes and no: I'd like to remove the duplication that it entails.
> E.g. I think defmethod's &context has made mode-local's
> overloadable-functions largely redundant, so I think it would be good to
> remove those overloadable-functions.
>
> I haven't looked at the mode-local-variable part of mode-local.el, so
> I don't plan on removing any of it for now, tho I think that if it
> stays, it would be good to better integrate it into the rest of Emacs.
>
>> If there is a better official way to do the same thing that seems fine
>> with me.
> My hope is that defmethod's &context covers those needs and that "it
> seems fine" to you.  Don't know if it's the case.

Heh, sure.  The defmethod technique solves the same core problem, but 
has the added benefit of being clearer by not depending on a magic 
-default implementation somewhere.  It has been long enough I don't 
recall all the variants out there.

>> For this specific item, I'm curious what the alternative might be.  The
>> obvious solution I can think of is making all the assignments for
>> functions and variables to all relevant modes, which feels error prone.
>> This was a way to specify similar modes for all overrides for this tool.
> W.r.t the `mode-local-parent` property, it looks pretty ad-hoc (not to
> say hackish): why not set `derived-mode-parent` instead?  Of course, the
> right way to set it is to change the mode so it sets it via
> `define-derived-mode`.  Otherwise you're in "it's kind of a child but
> not really" territory.
>
> BTW, regarding the above uses of define-child-mode, they've been reduced
> down to just:
>
>      bovine/c.el:  (define-child-mode c++-mode c-mode
>      bovine/el.el:  (define-child-mode lisp-mode emacs-lisp-mode
>
> I think the `lisp-mode` one is an error: lisp-mode is supposed to be for
> common-lisp, which is clearly not a child of emacs-lisp-mode.
> This said, AFAIK noone uses lisp-mode, everyone uses some other mode for
> common-lisp, either the one from SLIME or the one from SLY.
>
> The one for `c++-mode` is more tricky: I guess one could change cc-mode
> to make c++-mode derive from c-mode instead of prog-mode, but that would
> make it run c-mode-hook which some users might not like.  Maybe we
> should have a c-base-mode from which both c-mode and c++-mode derive?
> This question is of course largely irrelevant since Alan will likely
> never accept any such change in cc-mode.el.  But I think it would be
> perfectly fine to make define-child-mode set the derived-mode-parent
> property in this particular case.

This part of the mode-local system was put together to workaround the 
fact that we weren't modifying Emacs itself, and sometimes wanted to 
support multiple versions, and support other random modes not in Emacs.  
The thing with c/c++ is a prime example.  If we externally define c++ as 
a child of c, will that break anything else?

>> I'm not sure.  David Engster did most of the work on mode-local. There used
>> to be the primitive semantic- only version you found that he wrapped up in
>> mode-local.  Looking at this in retrospect, I'm not sure why the functions
>> installed with semantic-install-function-overrides weren't done using
>> mode-local more directly.  If they were converted, then
>> semantic-install-function-overrides could be removed.
> OK.  I'm not sufficiently familiar with the code to see how it can be
> changed to use define-overloadable-function instead of
> semantic-install-function-overrides, but I'll try and find out.
>
I can take a crack at this.  I'd like to figure out the test issues 
first though.

Eric

Re: completion-at-point + semantic : erroneous error

[Eric Ludlam]

On 10/11/19 1:34 PM, Stefan Monnier wrote:
>> PS: It would also be good to replace uses of
>> define-overloadable-function with cl-defmethod, although the mapping
>> from one to the other is not immediate.
> The patch below is an attempt to do that for
> semantic-analyze-possible-completions.  I'd be interested to hear your
> opinion on this (especially the with-mode-local part).
>
Hi Stefan,


As a followup, I started using the shorter version of your patch, and 
found there are additional errors thrown from 
semantic-analyze-possible-completions-default (what will be a method in 
your patch).  I had to eliminate those errors to fully clean up the 
original problem that started this thread.

I used catch/throw to solve the problem (see below: patch is against the 
original semantic/analyze/complete.el, not your patched version with 
methods.)

As an added wrinkle on the `should it be interactive' front, there is a 
keybinding in semantic.el for semantic-analyze-possible-completions that 
would need to be dealt with.


Eric



diff --git a/lisp/cedet/semantic/analyze/complete.el 
b/lisp/cedet/semantic/analyze/complete.el
index b471c0d1a1..b8820b927f 100644
--- a/lisp/cedet/semantic/analyze/complete.el
+++ b/lisp/cedet/semantic/analyze/complete.el
@@ -93,7 +93,8 @@ semantic-analyze-possible-completions
                context
              (semantic-analyze-current-context context)))
           (ans (if (not context)
-              (error "Nothing to complete")
+              (when (called-interactively-p 'any)
+                        (error "Nothing to complete"))
              (:override))))
      ;; If interactive, display them.
      (when (called-interactively-p 'any)
@@ -132,155 +133,159 @@ semantic-analyze-possible-completions-default
       (do-unique (not (memq 'no-unique flags)))
       )

-    (when (not do-longprefix)
-      ;; If we are not doing the long prefix, shorten all the key
-      ;; elements.
-      (setq prefix (list (car (reverse prefix)))
-        prefixtypes nil))
-
-    ;; Calculate what our prefix string is so that we can
-    ;; find all our matching text.
-    (setq completetext (car (reverse prefix)))
-    (if (semantic-tag-p completetext)
-    (setq completetext (semantic-tag-name completetext)))
-
-    (if (and (not completetext) (not desired-type))
-    (error "Nothing to complete"))
-
-    (if (not completetext) (setq completetext ""))
-
-    ;; This better be a reasonable type, or we should fry it.
-    ;; The prefixtypes should always be at least 1 less than
-    ;; the prefix since the type is never looked up for the last
-    ;; item when calculating a sequence.
-    (setq completetexttype (car (reverse prefixtypes)))
-    (when (or (not completetexttype)
-          (not (and (semantic-tag-p completetexttype)
-            (eq (semantic-tag-class completetexttype) 'type))))
-      ;; What should I do here?  I think this is an error condition.
-      (setq completetexttype nil)
-      ;; If we had something that was a completetexttype but it wasn't
-      ;; valid, then express our dismay!
-      (when (> (length prefix) 1)
-    (let* ((errprefix (car (cdr (reverse prefix)))))
-      (error "Cannot find types for `%s'"
-         (cond ((semantic-tag-p errprefix)
-            (semantic-format-tag-prototype errprefix))
-               (t
-            (format "%S" errprefix)))))
-    ))
-
-    ;; There are many places to get our completion stream for.
-    ;; Here we go.
-    (if completetexttype
-
-    (setq c (semantic-find-tags-for-completion
-         completetext
-         (semantic-analyze-scoped-type-parts completetexttype scope)
-         ))
-
-      ;; No type based on the completetext.  This is a free-range
-      ;; var or function.  We need to expand our search beyond this
-      ;; scope into semanticdb, etc.
-      (setq c (nconc
-           ;; Argument list and local variables
-           (semantic-find-tags-for-completion completetext localvar)
-           ;; The current scope
-           (semantic-find-tags-for-completion completetext (when scope 
(oref scope fullscope)))
-           ;; The world
-           (semantic-analyze-find-tags-by-prefix completetext))
-        )
-      )
-
-    (let ((loopc c)
-      (dtname (semantic-tag-name desired-type)))
-
-      ;; Save off our first batch of completions
-      (setq origc c)
-
-      ;; Reset c.
-      (setq c nil)
-
-      ;; Loop over all the found matches, and categorize them
-      ;; as being possible features.
-      (while (and loopc do-typeconstraint)
-
-    (cond
-     ;; Strip operators
-     ((semantic-tag-get-attribute (car loopc) :operator-flag)
-      nil
-      )
-
-     ;; If we are completing from within some prefix,
-     ;; then we want to exclude constructors and destructors
-     ((and completetexttype
-           (or (semantic-tag-get-attribute (car loopc) :constructor-flag)
-           (semantic-tag-get-attribute (car loopc) :destructor-flag)))
-      nil
-      )
-
-     ;; If there is a desired type, we need a pair of restrictions
-     (desired-type
+    (catch 'cant-complete
+      (when (not do-longprefix)
+        ;; If we are not doing the long prefix, shorten all the key
+        ;; elements.
+        (setq prefix (list (car (reverse prefix)))
+          prefixtypes nil))
+
+      ;; Calculate what our prefix string is so that we can
+      ;; find all our matching text.
+      (setq completetext (car (reverse prefix)))
+      (if (semantic-tag-p completetext)
+      (setq completetext (semantic-tag-name completetext)))
+
+      (if (and (not completetext) (not desired-type))
+          (throw 'cant-complete nil)
+      ;;(error "Nothing to complete")
+        )
+
+      (if (not completetext) (setq completetext ""))
+
+      ;; This better be a reasonable type, or we should fry it.
+      ;; The prefixtypes should always be at least 1 less than
+      ;; the prefix since the type is never looked up for the last
+      ;; item when calculating a sequence.
+      (setq completetexttype (car (reverse prefixtypes)))
+      (when (or (not completetexttype)
+            (not (and (semantic-tag-p completetexttype)
+              (eq (semantic-tag-class completetexttype) 'type))))
+        ;; What should I do here?  I think this is an error condition.
+        (setq completetexttype nil)
+        ;; If we had something that was a completetexttype but it wasn't
+        ;; valid, then express our dismay!
+        (when (> (length prefix) 1)
+          (throw 'cant-complete nil)
+;;;      (let* ((errprefix (car (cdr (reverse prefix)))))
+;;;        (error "Cannot find types for `%s'"
+;;;           (cond ((semantic-tag-p errprefix)
+;;;              (semantic-format-tag-prototype errprefix))
+;;;                 (t
+;;;              (format "%S" errprefix)))))
+      ))
+
+      ;; There are many places to get our completion stream for.
+      ;; Here we go.
+      (if completetexttype
+
+      (setq c (semantic-find-tags-for-completion
+           completetext
+           (semantic-analyze-scoped-type-parts completetexttype scope)
+           ))
+
+        ;; No type based on the completetext.  This is a free-range
+        ;; var or function.  We need to expand our search beyond this
+        ;; scope into semanticdb, etc.
+        (setq c (nconc
+             ;; Argument list and local variables
+             (semantic-find-tags-for-completion completetext localvar)
+             ;; The current scope
+             (semantic-find-tags-for-completion completetext (when 
scope (oref scope fullscope)))
+             ;; The world
+             (semantic-analyze-find-tags-by-prefix completetext))
+          )
+        )
+
+      (let ((loopc c)
+        (dtname (semantic-tag-name desired-type)))
+
+        ;; Save off our first batch of completions
+        (setq origc c)
+
+        ;; Reset c.
+        (setq c nil)
+
+        ;; Loop over all the found matches, and categorize them
+        ;; as being possible features.
+        (while (and loopc do-typeconstraint)

        (cond
-       ;; Ok, we now have a completion list based on the text we found
-       ;; we want to complete on.  Now filter that stream against the
-       ;; type we want to search for.
-       ((string= dtname (semantic-analyze-type-to-name 
(semantic-tag-type (car loopc))))
-        (setq c (cons (car loopc) c))
+       ;; Strip operators
+       ((semantic-tag-get-attribute (car loopc) :operator-flag)
+        nil
          )

-       ;; Now anything that is a compound type which could contain
-       ;; additional things which are of the desired type
-       ((semantic-tag-type (car loopc))
-        (let ((att (semantic-analyze-tag-type (car loopc) scope))
-        )
-          (if (and att (semantic-tag-type-members att))
-          (setq c (cons (car loopc) c))))
+       ;; If we are completing from within some prefix,
+       ;; then we want to exclude constructors and destructors
+       ((and completetexttype
+             (or (semantic-tag-get-attribute (car loopc) :constructor-flag)
+             (semantic-tag-get-attribute (car loopc) :destructor-flag)))
+        nil
          )

-       ) ; cond
-      ); desired type
-
-     ;; No desired type, no other restrictions.  Just add.
-     (t
-      (setq c (cons (car loopc) c)))
-
-     ); cond
-
-    (setq loopc (cdr loopc)))
-
-      (when desired-type
-    ;; Some types, like the enum in C, have special constant values that
-    ;; we could complete with.  Thus, if the target is an enum, we can
-    ;; find possible symbol values to fill in that value.
-    (let ((constants
-           (semantic-analyze-type-constants desired-type)))
-      (if constants
-          (progn
-        ;; Filter
-        (setq constants
-              (semantic-find-tags-for-completion
-               completetext constants))
-        ;; Add to the list
-        (setq c (nconc c constants)))
-        )))
-      )
-
-    (when desired-class
-      (setq c (semantic-analyze-tags-of-class-list c desired-class)))
-
-    (if do-unique
-    (if c
-        ;; Pull out trash.
-        ;; NOTE TO SELF: Is this too slow?
-        (setq c (semantic-unique-tag-table-by-name c))
-      (setq c (semantic-unique-tag-table-by-name origc)))
-      (when (not c)
-    (setq c origc)))
-
-    ;; All done!
-    c))
+       ;; If there is a desired type, we need a pair of restrictions
+       (desired-type
+
+        (cond
+         ;; Ok, we now have a completion list based on the text we found
+         ;; we want to complete on.  Now filter that stream against the
+         ;; type we want to search for.
+         ((string= dtname (semantic-analyze-type-to-name 
(semantic-tag-type (car loopc))))
+          (setq c (cons (car loopc) c))
+          )
+
+         ;; Now anything that is a compound type which could contain
+         ;; additional things which are of the desired type
+         ((semantic-tag-type (car loopc))
+          (let ((att (semantic-analyze-tag-type (car loopc) scope))
+            )
+            (if (and att (semantic-tag-type-members att))
+            (setq c (cons (car loopc) c))))
+          )
+
+         )                          ; cond
+        )                           ; desired type
+
+       ;; No desired type, no other restrictions.  Just add.
+       (t
+        (setq c (cons (car loopc) c)))
+
+       )                            ; cond
+
+      (setq loopc (cdr loopc)))
+
+        (when desired-type
+      ;; Some types, like the enum in C, have special constant values that
+      ;; we could complete with.  Thus, if the target is an enum, we can
+      ;; find possible symbol values to fill in that value.
+      (let ((constants
+             (semantic-analyze-type-constants desired-type)))
+        (if constants
+            (progn
+          ;; Filter
+          (setq constants
+                (semantic-find-tags-for-completion
+                 completetext constants))
+          ;; Add to the list
+          (setq c (nconc c constants)))
+          )))
+        )
+
+      (when desired-class
+        (setq c (semantic-analyze-tags-of-class-list c desired-class)))
+
+      (if do-unique
+      (if c
+          ;; Pull out trash.
+          ;; NOTE TO SELF: Is this too slow?
+          (setq c (semantic-unique-tag-table-by-name c))
+        (setq c (semantic-unique-tag-table-by-name origc)))
+        (when (not c)
+      (setq c origc)))
+
+      ;; All done!
+      c)))

  (provide 'semantic/analyze/complete)

Re: completion-at-point + semantic : erroneous error

[Stefan Monnier]

> As a followup, I started using the shorter version of your patch, and found
> there are additional errors thrown from
> semantic-analyze-possible-completions-default (what will be a method in 
> your patch).  I had to eliminate those errors to fully clean up the original
> problem that started this thread.

[ Sorry for taking so long to get back to it.  ]

After fighting for a while with your patch (it was full of NBSP chars
and wrapped lines) and then looking at it some more, I decided to
install the patch below instead which replaces the `catch` with
a `with-demoted-errors` (and places it in the caller instead).
Maybe those errors should indeed all be replaced by silently returning
nil, but the code currently goes through the trouble of building
more precise error messages, so I felt like maybe we should preserve
that info.


        Stefan


diff --git a/lisp/cedet/semantic/analyze/complete.el b/lisp/cedet/semantic/analyze/complete.el
index b471c0d1a1..b473ade159 100644
--- a/lisp/cedet/semantic/analyze/complete.el
+++ b/lisp/cedet/semantic/analyze/complete.el
@@ -93,8 +93,10 @@ semantic-analyze-possible-completions
 			  context
 			(semantic-analyze-current-context context)))
 	     (ans (if (not context)
-		      (error "Nothing to complete")
-		    (:override))))
+		      (when (called-interactively-p 'any)
+			(error "Nothing to complete"))
+		    (with-demoted-errors "%S"
+		      (:override)))))
 	;; If interactive, display them.
 	(when (called-interactively-p 'any)
 	  (with-output-to-temp-buffer "*Possible Completions*"

Re: completion-at-point + semantic : erroneous error

[Eric Ludlam]

On 10/23/19 4:18 PM, Stefan Monnier wrote:
>> As a followup, I started using the shorter version of your patch, and found
>> there are additional errors thrown from
>> semantic-analyze-possible-completions-default (what will be a method in
>> your patch).  I had to eliminate those errors to fully clean up the original
>> problem that started this thread.
> [ Sorry for taking so long to get back to it.  ]
>
> After fighting for a while with your patch (it was full of NBSP chars
> and wrapped lines) and then looking at it some more, I decided to
> install the patch below instead which replaces the `catch` with
> a `with-demoted-errors` (and places it in the caller instead).
> Maybe those errors should indeed all be replaced by silently returning
> nil, but the code currently goes through the trouble of building
> more precise error messages, so I felt like maybe we should preserve
> that info.
>
Thanks Stefan, and sorry about the bum patch.  I'll do attachments in 
the future.

Your patch seems like a good start.

One of the things that was driving me to fix this this is that I 
typically have debug-on-error on by default, so having this function run 
all the time via company-mode was disruptive.  It looks like 
with-demoted-errors will continue to allow debug-on-error to stop, which 
would be useful for debugging a real problem.

That said, I think it is OK to move forward with the simple version of 
your patch as it will help anyone trying to do the same thing w/out 
debug-on-error.

Eric

Re: completion-at-point + semantic : erroneous error

[Stefan Monnier]

> One of the things that was driving me to fix this this is that I typically
> have debug-on-error on by default, so having this function run all the time
> via company-mode was disruptive.  It looks like with-demoted-errors will
> continue to allow debug-on-error to stop, which would be useful for
> debugging a real problem.

Ah, right.  I too have debug-on-error non-nil of course.
The `error`s should be turned into `user-error`s to avoid this problem.

Tho of course, if the corresponding messages are not useful, returning
nil is a better choice, indeed.


        Stefan

Re: completion-at-point + semantic : erroneous error

[Eric Ludlam]

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

On 10/23/19 4:18 PM, Stefan Monnier wrote:
>> As a followup, I started using the shorter version of your patch, and found
>> there are additional errors thrown from
>> semantic-analyze-possible-completions-default (what will be a method in
>> your patch).  I had to eliminate those errors to fully clean up the original
>> problem that started this thread.
> [ Sorry for taking so long to get back to it.  ]
>
> After fighting for a while with your patch (it was full of NBSP chars
> and wrapped lines) and then looking at it some more, I decided to
> install the patch below instead which replaces the `catch` with
> a `with-demoted-errors` (and places it in the caller instead).
> Maybe those errors should indeed all be replaced by silently returning
> nil, but the code currently goes through the trouble of building
> more precise error messages, so I felt like maybe we should preserve
> that info.

Hi Stefan,

I pulled down the latest, and ran against a more extensive test suite 
I'm working on.  I found that the new version is always pushing down the 
optional 'flags input to overrides, which makes sense.  Several override 
methods don't accommodate this.  The attached patch adds that optional 
'flags' input to fix the error and allow the make, texi, and grammar 
modes run their specialization.

> Ah, right.  I too have debug-on-error non-nil of course.
> The `error`s should be turned into `user-error`s to avoid this problem.

> Tho of course, if the corresponding messages are not useful, returning
> nil is a better choice, indeed.

user-error sounds like the shortest path here.  The original purpose of 
the error was to provide a fine-grained reason for why something may 
have failed to complete.  I think most completion engines don't care 
about that.

Thanks

Eric




[-- Attachment #2: optflags.patch --]
[-- Type: text/x-patch, Size: 1761 bytes --]

diff --git a/lisp/cedet/semantic/bovine/make.el b/lisp/cedet/semantic/bovine/make.el
index 3676c6972f..01a15b8232 100644
--- a/lisp/cedet/semantic/bovine/make.el
+++ b/lisp/cedet/semantic/bovine/make.el
@@ -175,7 +175,7 @@ semantic-format-tag-uml-prototype
   (semantic-format-tag-prototype tag parent color))
 
 (define-mode-local-override semantic-analyze-possible-completions
-  makefile-mode (context)
+  makefile-mode (context &rest flags)
   "Return a list of possible completions in a Makefile.
 Uses default implementation, and also gets a list of filenames."
   (require 'semantic/analyze/complete)
diff --git a/lisp/cedet/semantic/grammar.el b/lisp/cedet/semantic/grammar.el
index 813580ba6c..e1337b3eb9 100644
--- a/lisp/cedet/semantic/grammar.el
+++ b/lisp/cedet/semantic/grammar.el
@@ -1915,7 +1915,7 @@ semantic-analyze-current-context
       context-return)))
 
 (define-mode-local-override semantic-analyze-possible-completions
-  semantic-grammar-mode (context)
+  semantic-grammar-mode (context &rest flags)
   "Return a list of possible completions based on CONTEXT."
   (require 'semantic/analyze/complete)
   (if (semantic-grammar-in-lisp-p)
diff --git a/lisp/cedet/semantic/texi.el b/lisp/cedet/semantic/texi.el
index 73f0e734f3..0fdc9ee8bc 100644
--- a/lisp/cedet/semantic/texi.el
+++ b/lisp/cedet/semantic/texi.el
@@ -413,7 +413,7 @@ semantic-texi-command-completion-list
   "List of commands that we might bother completing.")
 
 (define-mode-local-override semantic-analyze-possible-completions
-  texinfo-mode (context)
+  texinfo-mode (context &rest flags)
   "List smart completions at point.
 Since texinfo is not a programming language the default version is not
 useful.  Instead, look at the current symbol.  If it is a command

Re: completion-at-point + semantic : erroneous error

[Stefan Monnier]

> As an added wrinkle on the `should it be interactive' front, there is
>  a keybinding in semantic.el for semantic-analyze-possible-completions that 
> would need to be dealt with.

Indeed.  What is it supposed to do, exactly?
Does `completion-help-at-point` provide the same functionality, or is it
meant for something different?


        Stefan

Re: completion-at-point + semantic : erroneous error

[Eric Ludlam]

On 10/23/19 4:22 PM, Stefan Monnier wrote:
>> As an added wrinkle on the `should it be interactive' front, there is
>>   a keybinding in semantic.el for semantic-analyze-possible-completions that
>> would need to be dealt with.
> Indeed.  What is it supposed to do, exactly?
> Does `completion-help-at-point` provide the same functionality, or is it
> meant for something different?
>
I think it is there b/c I used to execute that command a lot when 
something wasn't working right as a debugging tool.  The keybinding 
could be removed.

Eric