[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: New tree-sitter mode: bison-ts-mode

From: BTuin
Subject: Re: New tree-sitter mode: bison-ts-mode
Date: Fri, 22 Sep 2023 16:53:08 +0200
User-agent: Mozilla Thunderbird

Le 22/09/2023 à 09:38, Philip Kaludercic a écrit :
"Augustin Chéneau (BTuin)" <btuin@mailo.com> writes:

A few comments on the proposed file:

;;; bison-ts-mode --- Tree-sitter mode for Bison -*- lexical-binding: t; -*-

Could you add the usual header information here?

;; Copyright (C) 2022-2023 Free Software Foundation, Inc.

--8<---------------cut here---------------start------------->8---
;; Author: Augustin Chéneau <btuin@mailo.com>

;; This file is part of GNU Emacs.

;; GNU Emacs is free software: you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; GNU Emacs is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
--8<---------------cut here---------------end--------------->8---

;;; Commentary:

;; This is a mode based on tree-sitter for Bison and Yacc files, tools to 
generate parsers.

Shouldn't you mention what tree sitter grammar is being being used here?

;;; Code:

(require 'treesit)
(require 'c-ts-common)

(declare-function treesit-parser-create "treesit.c")
(declare-function treesit-induce-sparse-tree "treesit.c")
(declare-function treesit-node-child-by-field-name "treesit.c")
(declare-function treesit-search-subtree "treesit.c")
(declare-function treesit-node-parent "treesit.c")
(declare-function treesit-node-next-sibling "treesit.c")
(declare-function treesit-node-type "treesit.c")
(declare-function treesit-node-child "treesit.c")
(declare-function treesit-node-end "treesit.c")
(declare-function treesit-node-start "treesit.c")
(declare-function treesit-node-string "treesit.c")
(declare-function treesit-query-compile "treesit.c")
(declare-function treesit-query-capture "treesit.c")
(declare-function treesit-parser-add-notifier "treesit.c")
(declare-function treesit-parser-buffer "treesit.c")
(declare-function treesit-parser-list "treesit.c")

(defgroup bison nil

bison or bison-ts?

As far as I know, no group is explicitly reserved for tree-sitter. rust-ts-mode uses the group "rust", java-ts-mode uses "java", ruby-ts-mode uses "ruby"...

   "Support for Bison and Yacc."

Shouldn't tree-sitter be mentioned here?

Same as above.

   :group 'languages)

(defcustom bison-ts-mode-indent-offset 2
   "Number of spaces for each indentation step in `bison-ts-mode'.
It has no effect in the epilogue part of the file."
   :version "30.1"
   :type 'integer
   :safe 'integerp
   :group 'bison)

(defcustom bison-ts-mode-autodetect-language t
   "Search for a %language directive in the file at initialization.
Changing the value of this directive in the file requires to reload the mode to
be effective.  If `bison-ts-mode-buffer-language' is set by a file-local
  variable, the auto-detection is not run."
   :version "30.1"
   :type 'boolean
   :safe 'boolean
   :group 'bison)

(defvar-local bison-ts-mode-embedded-language nil
   "Embedded language in Bison buffer.")

(defun bison-ts-mode--merge-feature-lists (l1 l2)
   "Merge the lists of lists L1 and L2.
The first sublist of L1 is merged with the first sublist of L2 and so on.
L1 and L2 don't need to have the same size."
   (let ((res ()))
     (while (or l1 l2)
       (setq res (push (append (car l1) (car l2)) res))
       (setq l1 (cdr l1) l2 (cdr l2)))
     (nreverse res)))

Is this a generic function that should be extracted into some common file?

It probably should, it could be useful for other *-ts-mode that want to mix multiple languages, unless there already is such a function in Elisp.

(defun bison-ts-mode--find-language-in-buffer (&optional buffer)
   "Find and return the language set by the Bison directive %language.
If BUFFER is set, search in this buffer, otherwise search in the current
     (when buffer
       (switch-to-buffer buffer))

Or rather?

   (with-current-buffer (or buffer (current-buffer))

     (goto-char (point-min))
     (let ((pos-end
              bol (0+ blank) "%language" (0+ blank) "\"" (1+ (in alpha "+")) 
           (pos-beg nil))
       (when pos-end

Using when-let might be nice here.

         (goto-char (1- pos-end))
         (setq pos-beg (1+ (search-backward "\"" nil t)))
         (buffer-substring-no-properties pos-beg (1- pos-end))))))

I'd use a single regular expression here with a group, then extract the
right information using `match-string'.

Nice, I didn't know it was possible.

(defun bison-ts-mode--detect-language (&optional buffer)
   "Dectect the embedded language in a Bison buffer.
Known languages are C, C++, D, and Java, but D is not supported as there is
no support for tree-sitter D in Emacs yet.
If BUFFER is set, search in this buffer, otherwise search in the current
   (if-let ((str (bison-ts-mode--find-language-in-buffer buffer)))
       (pcase (downcase str)
         ("c" 'c)
         ("c++" 'cpp)
         ("d" (progn (message "D language not yet supported") nil))

Each pcase case has an implicit progn.

         ("java" 'java))

Use when-let to avoid this progn.

        "bison-ts-mode: %%language specification not found or invalid, defaulting to 

Is it necessary to prefix the message with the major mode name?

If feared it would be a bit cryptic without. Anyway I modified this function to only display a message if "%language" is present but the language is invalid, so it probably isn't necessary anymore.

(defun bison-ts-mode--language-at-point-function (position)
   "Return the language at POSITION."
   (let* ((node (treesit-node-at position 'bison)))
      let is enough

     (if (equal (treesit-node-type node)

There is no need to break the line here.


(defun bison-ts-mode--font-lock-settings (language)
   "Return the font-lock settings for Bison.
LANGUAGE should be set to \\='bison."
    :language language
    :feature 'bison-comment
    '((comment) @font-lock-comment-face)

    :language language
    :feature 'bison-declaration
    '((declaration_name) @font-lock-keyword-face)

    :language language
    :feature 'bison-type
    '((type) @font-lock-type-face)

    :language language
    :feature 'bison-grammar-rule-usage
    '((grammar_rule_identifier) @font-lock-variable-use-face)

    :language language
    :feature 'bison-grammar-rule-declaration
    '((grammar_rule (grammar_rule_declaration)

    :language language
    :feature 'bison-string
    :override t
    '((string) @font-lock-string-face)

    :language language
    :feature 'bison-literal
    :override t
    '((char_literal) @font-lock-keyword-face
      (number_literal) @font-lock-number-face)

    :language language
    :feature 'bison-directive-grammar-rule
    :override t
    '((grammar_rule (directive) @font-lock-keyword-face))

    :language language
    :feature 'bison-operator
    :override t
    '(["|"] @font-lock-operator-face)

    :language language
    :feature 'bison-delimiter
    :override t
    '([";"] @font-lock-delimiter-face)))

(defvar bison-ts-mode--font-lock-feature-list
   '(( bison-comment bison-declaration bison-type
       bison-grammar-rule-usage bison-grammar-rule-declaration
       bison-string bison-literal bison-directive-grammar-rule
       bison-operator bison-delimiter)))

(defun bison-ts-mode--bison-matcher-action (root-name)
   "Treesit matcher to check if NODE at BOL is not located in the epilogue.
ROOT-NAME is the highest-level node of the embedded language."
   (lambda (node _parent bol &rest _)
     (if (equal (treesit-node-type (treesit-node-parent node)) root-name)
         (let* ((bison-node (treesit-node-at bol 'bison)))
            here again, let is enough

            (if (equal
                 (treesit-node-parent(treesit-node-parent bison-node))) 

Though you could bind the (treesit-node-type ...) expression under the
above let.


Why (if foo t nil) when foo would do the same job (equal only returns
nil and t, so normalising the value isn't even necessary).

Because I was stupid.

(defun bison-ts-mode--bison-matcher-not-epilogue (root-name)
   "Treesit matcher to check if NODE at BOL is not located in the epilogue.
ROOT-NAME is the highest-level node of the embedded language."
   (lambda (node _parent bol &rest _)
     (if (equal (treesit-node-type (treesit-node-parent node)) root-name)
         (let* ((bison-node (treesit-node-at bol 'bison)))
           (if (equal (treesit-node-type (treesit-node-parent bison-node)) 

Am I missing something, or couldn't these two functions be merged if you
give them a third argument NODE-TYPE and pass it "action" or "epilogue".

No, bison-ts-mode--bison-matcher-action checks if the _grandparent_ is an "action" node, while bison-ts-mode--bison-matcher-not-epilogue checks if the _parent_ is an "epilogue" node.

(defun bison-ts-mode--bison-parent (_node _parent bol &rest _)
   "Get the parent of the bison node at BOL."
   (treesit-node-start (treesit-node-parent (treesit-node-at bol 'bison))))

(defun bison-ts-mode--indent-rules ()
   "Indent rules supported by `bison-ts-mode'."
         `(((node-is "^declaration$")
            column-0 0)
           ((and (parent-is "^declaration$")
                 (not (node-is "^code_block$")))
            column-0 2)
           ((and (parent-is "comment") c-ts-common-looking-at-star)
            c-ts-common-comment-start-after-first-star -1)
           ((parent-is "comment") prev-adaptive-prefix 0)

           ;; Opening and closing brackets "{}" of declarations
           ((and (parent-is "^declaration$")
                 (node-is "^code_block$"))
            column-0 0)
           ((and (n-p-gp "}" "" "^declaration$"))
            column-0 0)
           ((parent-is "^declaration$") parent 2)
           ((node-is "^grammar_rule$") column-0 0)
             (parent-is "^grammar_rule$")
             (node-is ";"))
            column-0 bison-ts-mode-indent-offset)
           ((and (parent-is "^grammar_rule$")
                 (node-is "|"))
            column-0 bison-ts-mode-indent-offset)
           ((and (parent-is "^grammar_rule$")
                 (not (node-is "^grammar_rule_declaration$"))
                 (not (node-is "^action$")))
            column-0 ,(+ bison-ts-mode-indent-offset 2))
             (node-is "^action$")
             (node-is "^}$"))
            column-0 12)
           ;; Set '%%' at the beginning of the line
             (and (parent-is "^grammar_rules_section$")
                  (node-is "%%"))
             (node-is "^grammar_rules_section$"))
            column-0 0)
           (no-node parent-bol 0))))
     `((bison . ,common)
       ;; Import and override embedded languages rules to add an offset
       ,(pcase bison-ts-mode-embedded-language
          ('c `(c
                ((bison-ts-mode--bison-matcher-action "translation_unit")
                 bison-ts-mode--bison-parent ,bison-ts-mode-indent-offset)
                ((bison-ts-mode--bison-matcher-not-epilogue "translation_unit")
                 column-0 ,bison-ts-mode-indent-offset)
                ,@(alist-get 'c (c-ts-mode--get-indent-style 'c))))
          ('cpp `(cpp
                  ((bison-ts-mode--bison-matcher-action "translation_unit")
                   bison-ts-mode--bison-parent ,bison-ts-mode-indent-offset)
                   parent-0 ,bison-ts-mode-indent-offset)
                  ,@(alist-get 'cpp (c-ts-mode--get-indent-style 'cpp))))
          ('java `(java
                   ((bison-ts-mode--bison-matcher-action "program")
                    bison-ts-mode--bison-parent ,bison-ts-mode-indent-offset)
                   ((bison-ts-mode--bison-matcher-not-epilogue "program")
                    column-0 ,bison-ts-mode-indent-offset)

(define-derived-mode bison-ts-mode prog-mode "Bison"
   "A mode for Bison."

Also, mentioning tree-sitter seems like something worth doing.

   (when (treesit-ready-p 'bison)
     (when (not bison-ts-mode-embedded-language)
       (setq bison-ts-mode-embedded-language (bison-ts-mode--detect-language)))

     ;; Require only if needed, to avoid warnings if a grammar is not
        ;; installed but not used.
     (pcase bison-ts-mode-embedded-language

Would a `pcase-exhaustive' be appropriate here?

No, the language D is recognized but not supported yet in Emacs, so if this language is detected it will not configure anything.

       ('c (require 'c-ts-mode))
       ('cpp (require 'c-ts-mode))
       ('java (require 'java-ts-mode)))

     (setq-local treesit-font-lock-settings
                 (append (bison-ts-mode--font-lock-settings 'bison)
                         (pcase bison-ts-mode-embedded-language
                           ('c (c-ts-mode--font-lock-settings 'c))
                           ('cpp (c-ts-mode--font-lock-settings 'cpp))
                           ('java java-ts-mode--font-lock-settings))))

     (setq-local treesit-font-lock-feature-list
                 (if bison-ts-mode-embedded-language
                      (pcase bison-ts-mode-embedded-language
                        ('c c-ts-mode--feature-list)
                        ('cpp c-ts-mode--feature-list)
                        ('java java-ts-mode--feature-list)))

     (setq-local treesit-simple-imenu-settings
                    (lambda (node) (substring-no-properties (treesit-node-text 


     (setq-local treesit-simple-indent-rules

     (setq-local treesit-language-at-point-function 

     (when bison-ts-mode-embedded-language
       (setq-local treesit-range-settings
                    :embed bison-ts-mode-embedded-language
                    :host 'bison
                    :local t
                    '((embedded_code) @capture))))


(provide 'bison-ts-mode)
;;; bison-ts-mode.el ends here

Sorry for the number of comments, but there has been a discussion on the
code-quality of tree-sitter major modes that has been less than optimal,
so I hope that your contribution could help raise the bar.

No problem, thank you for your review!
You can find the updated mode attached.

I've added `bison-ts-mode-embedded-language' to `safe-local-variable' with:

(defvar-local bison-ts-mode-embedded-language nil
  "Embedded language in Bison buffer.
Supported values are `c', `cpp', and `java'.")
(put 'bison-ts-mode-embedded-language 'safe-local-variable 'symbolp)

By the way I have sent the mail for the copyright assignment.

Attachment: bison-ts-mode.el
Description: Text Data

reply via email to

[Prev in Thread] Current Thread [Next in Thread]