Fontifying of variable names in declarations not working correctly

[fnJeff]

I'm not sure how no one has brought this up yet, but it seems that something is definitely wrong with the code to recognize and fontify variable names in declarations.

And while I'm here, it would be really nice to be able to disable fontifying the variable names in declarations without having to locate and comment out the code in the LISP as I did below to make it stop doing this.

  (setq verilog-font-lock-keywords-1
    (append verilog-font-lock-keywords
        (list
         ;; Fontify module definitions
         (list
                 "\\<\\(\\(macro\\|connect\\)?module\\|primitive\\|class\\|program\\|interface\\|package\\|task\\)\\>\\s-*\\(\\sw+\\)"
          '(1 font-lock-keyword-face)
          '(3 font-lock-function-name-face prepend))
         ;; Fontify function definitions
         (list
          (concat "\\<function\\>\\s-+\\(integer\\|real\\(time\\)?\\|time\\)\\s-+\\(\\sw+\\)" )
                  '(1 font-lock-keyword-face)
                  '(3 font-lock-constant-face prepend))
         '("\\<function\\>\\s-+\\(\\[[^]]+\\]\\)\\s-+\\(\\sw+\\)"
           (1 font-lock-keyword-face)
           (2 font-lock-constant-face append))
         '("\\<function\\>\\s-+\\(\\sw+\\)"
           1 'font-lock-constant-face append)
;;                 ;; Fontify variable names in declarations
;;                 (list
;;                  verilog-declaration-re
;;                  (list
;;                   ;; Anchored matcher (lookup Search-Based Fontification)
;;                   'verilog-declaration-varname-matcher
;;                   ;; Pre-form for this anchored matcher:
;;                   ;; First, avoid declaration keywords written in comments,
;;                   ;; which can also trigger this anchor.
;;                   '(if (not (verilog-in-comment-p))
;;                        (verilog-single-declaration-end verilog-highlight-max-lookahead)
;;                      (point)) ;; => current declaration statement is of 0 length
;;                   nil ;; Post-form: nothing to be done
;;                   '(0 font-lock-variable-name-face t t)))
                )))

module debug_fontify_variable_name_in_declaration;

   logic              correct_0;
   logic signed [1:0] incorrect_0;
   logic              correct_1;
   logic signed [1:0] incorrect_1;

   typedef enum       logic [1:0]
                      {STATE_0,
                       STATE_1,
                       STATE_2,
                       STATE_3} t_incorrect_2;
   t_incorrect_2 incorrect_3;

endmodule

You can see in the screenshot below that it is incorrectly fontifying several things incorrectly instead of the variable names.

Debug info:

Emacs : GNU Emacs 27.1 (build 1, x86_64-redhat-linux-gnu, GTK+ Version 3.14.13) of 2020-12-17 Package: verilog-mode v2021-10-14-797711e-vpo-GNU

current state: (setq verilog-active-low-regexp nil verilog-after-save-font-hook nil verilog-align-ifelse nil verilog-assignment-delay "" verilog-auto-arg-sort nil verilog-auto-declare-nettype nil verilog-auto-delete-trailing-whitespace nil verilog-auto-endcomments nil verilog-auto-hook nil verilog-auto-ignore-concat nil verilog-auto-indent-on-newline t verilog-auto-inout-ignore-regexp nil verilog-auto-input-ignore-regexp nil verilog-auto-inst-column 40 verilog-auto-inst-dot-name nil verilog-auto-inst-interfaced-ports nil verilog-auto-inst-param-value nil verilog-auto-inst-sort nil verilog-auto-inst-template-numbers nil verilog-auto-inst-vector 'unsigned verilog-auto-lineup 'declarations verilog-auto-newline nil verilog-auto-output-ignore-regexp nil verilog-auto-read-includes nil verilog-auto-reset-blocking-in-non t verilog-auto-reset-widths t verilog-auto-save-policy nil verilog-auto-sense-defines-constant nil verilog-auto-sense-include-inputs nil verilog-auto-star-expand t verilog-auto-star-save nil verilog-auto-template-warn-unused nil verilog-auto-tieoff-declaration "wire" verilog-auto-tieoff-ignore-regexp nil verilog-auto-unused-ignore-regexp nil verilog-auto-wire-type "logic" verilog-before-auto-hook nil verilog-before-delete-auto-hook nil verilog-before-getopt-flags-hook nil verilog-before-save-font-hook nil verilog-cache-enabled t verilog-case-fold t verilog-case-indent 3 verilog-cexp-indent 3 verilog-compiler "verilog " verilog-coverage "echo 'No verilog-coverage set, see \"M-x describe-variable verilog-coverage\"'" verilog-delete-auto-hook nil verilog-getopt-flags-hook nil verilog-highlight-grouping-keywords t verilog-highlight-includes t verilog-highlight-modules t verilog-highlight-translate-off nil verilog-indent-begin-after-if t verilog-indent-declaration-macros nil verilog-indent-level 3 verilog-indent-level-behavioral 3 verilog-indent-level-declaration 3 verilog-indent-level-directive 1 verilog-indent-level-module 3 verilog-indent-lists t verilog-library-directories '(".") verilog-library-extensions '(".v" ".va" ".sv") verilog-library-files nil verilog-library-flags '("") verilog-linter "echo 'No verilog-linter set, see \"M-x describe-variable verilog-linter\"'" verilog-minimum-comment-distance 10 verilog-mode-hook 'verilog-set-compile-command verilog-mode-release-emacs t verilog-mode-version "2021-10-14-797711e-vpo-GNU" verilog-preprocessor "verilator -E FLAGS FILE" verilog-simulator "echo 'No verilog-simulator set, see \"M-x describe-variable verilog-simulator\"'" verilog-tab-always-indent t verilog-tab-to-comment nil verilog-typedef-regexp "^\(t.+\|sm[^_]+\)$" verilog-warn-fatal nil )

[wsnyder]

Agree it's a bit off. Note, unfortunately, highlighting issues for the most part are only fixed when someone can provide a pull request.

[fnJeff]

Sorry for my git ignorance. Is this something I need to do myself? Or someone that sees this issue will create the pull request?

[wsnyder]

Someone else might come along and fix it eventually, but best bet is to fix it yourself. Perhaps a better way I could have said it is for font-lock I didn't write that code, and we don't currently have a maintainer.

[kaushalmodi]

This seems to fix at least the fontification when signed is in the type declaration:

diff --git a/verilog-mode.el b/verilog-mode.el
index 1102014..e791c81 100644
--- a/verilog-mode.el
+++ b/verilog-mode.el
@@ -3372,7 +3372,7 @@ See also `verilog-font-lock-extra-types'.")
           1 'font-lock-constant-face append)
                  ;; Fontify variable names in declarations
                  (list
-                  verilog-declaration-re
+                  verilog-declaration-re-2-no-macro ;declaration type with optional (un)signed keyword and range
                   (list
                    ;; Anchored matcher (lookup Search-Based Fontification)
                    'verilog-declaration-varname-matcher

Test snippet

module debug_fontify_variable_name_in_declaration;

   logic              correct_0;
   logic signed [1:0] now_correct_0;
   logic              correct_1;
   logic signed [1:0] now_correct_1;

   typedef enum       logic [1:0]
                      {STATE_0,
                       STATE_1,
                       STATE_2,
                       STATE_3} t_incorrect_2;
   t_incorrect_2 incorrect_3;

endmodule

Outcome

---

@wsnyder Does this one-line change look good enough for a PR?

[wsnyder]

@gmlarumbe not sure if you've looked at this area, what do you think?

[gmlarumbe]

Hi all,

This is what I get after applying modi's patch:

I believe that modi's patch fixes the incorrect_0/incorrect_1 case without breaking anything. However, modi's screenshot is slightly different from mine and fnJeff's. @kaushalmodi , do you use some additional custom font-lock related code that happens to colorize logic/STATE_0/STATE_1/STATE_2 on t_incorrect_2 declaration?

It seems that both verilog-declaration-re and verilog-declaration-re-2-no-macro do not correctly match the declaration of variable t_incorrect_2, considering logic instead of t_incorrect_2 as the variable name. Updating these regexps to also cover this enum case with newlines does not seem straightforward though.

I would go ahead with the PR, but I would also add something else to avoid overriding proper font-lock of logic keyword on typedef enums:

diff --git a/verilog-mode.el b/verilog-mode.el
index 1102014..2b90c7e 100644
--- a/verilog-mode.el
+++ b/verilog-mode.el
@@ -3383,7 +3383,7 @@ See also `verilog-font-lock-extra-types'.")
                         (verilog-single-declaration-end verilog-highlight-max-lookahead)
                       (point)) ;; => current declaration statement is of 0 length
                    nil ;; Post-form: nothing to be done
-                   '(0 font-lock-variable-name-face t t)))
+                   '(0 font-lock-variable-name-face nil t)))
                 )))

This is the result: