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

[Eric Ludlam <ericludlam@gmail.com>]

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
>
>