Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fontifying of variable names in declarations not working correctly #1752

Open
fnJeff opened this issue Nov 11, 2021 · 6 comments
Open

Fontifying of variable names in declarations not working correctly #1752

fnJeff opened this issue Nov 11, 2021 · 6 comments

Comments

@fnJeff
Copy link

fnJeff commented Nov 11, 2021

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

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
Copy link
Member

wsnyder commented Nov 11, 2021

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
Copy link
Author

fnJeff commented Nov 12, 2021

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
Copy link
Member

wsnyder commented Nov 12, 2021

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
Copy link
Contributor

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

image


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

kaushalmodi added a commit to kaushalmodi/verilog-mode that referenced this issue Jun 16, 2022
@wsnyder
Copy link
Member

wsnyder commented Jun 17, 2022

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

@gmlarumbe
Copy link
Contributor

Hi all,

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

verilog_font_lock

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:

verilog_font_lock_2

kaushalmodi added a commit to kaushalmodi/verilog-mode that referenced this issue May 4, 2023
kaushalmodi added a commit to kaushalmodi/.emacs.d that referenced this issue May 5, 2023
It used a verilog-declaration-re-2-no-macro variable which doesn't
exist upstream any more.

Ref: veripool/verilog-mode#1752
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants