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.

Sign up for GitHub

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

Jump to bottom

Fix ancient defect and dependency complications related to having two rust-mode implementations #534

Merged
merged 2 commits into from Mar 29, 2024
Merged

Fix ancient defect and dependency complications related to having two rust-mode implementations #534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ce4f8e
rust--format-call: Delete file in case of error
tarsius Mar 16, 2024
7c5de03
Fix dependencies between rust-mode implementations
tarsius Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions rust-mode-treesitter.el
View file
Open in desktop
Expand Up @@ -5,12 +5,13 @@

;;; Code:

(require 'rust-mode)

;; Do not compile or load on Emacs releases that don't support
;; this. See https://github.com/rust-lang/rust-mode/issues/520.
(when (version<= "29.1" emacs-version)
;; We have the when macro because of
;; https://github.com/rust-lang/rust-mode/issues/520
(require 'treesit)
(require 'rust-ts-mode)
(require 'rust-common)

(define-derived-mode rust-mode rust-ts-mode "Rust"
"Major mode for Rust code.
Expand Down
10 changes: 6 additions & 4 deletions rust-mode.el
View file
Open in desktop
Expand Up @@ -71,17 +71,19 @@ instead of `prog-mode'. This option requires emacs29+."
map)
"Keymap for Rust major mode.")

(if (and (version<= "29.1" emacs-version) rust-mode-treesitter-derive)
(require 'rust-mode-treesitter)
(require 'rust-prog-mode))

;;;###autoload
(autoload 'rust-mode "rust-mode" "Major mode for Rust code." t)

;;;###autoload
(add-to-list 'auto-mode-alist '("\\.rs\\'" . rust-mode))

(provide 'rust-mode)

(if (and rust-mode-treesitter-derive
(version<= "29.1" emacs-version))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emacs > 29.1 can be compiled without tree sitter feature.
We should maybe replace this with (functionp 'treesit-parser-p), or something similar ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why this is opt in feature, by default rust-mode-treesiter-derive is nil.

We should maybe replace this with (functionp 'treesit-parser-p), or something similar ?

Even then it should be opt-in IMO (atleast for a couple of years) and that already introduces a new defcustom variable. Also, in my experience I have found treesit-parser-p not reliable where during startup it returned nil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we should replace (version<= "29.1" emacs-version) with (functionp 'treesit-parser-p).

Also, in my experience I have found treesit-parser-p not reliable where during startup it returned nil.

(functionp 'treesit-parser-p) doesn't call treesit-parser-p, it will just return nil when emacs has not been compiled with treesit (--without-tree-sitter flag at compile time)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we should replace (version<= "29.1" emacs-version) with (functionp 'treesit-parser-p).

Ah, I see what you mean. Probably you can open a separate PR for it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also the if check in rust-mode-treesitter.el

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree. I have opened an issue to track this: #537

(require 'rust-mode-treesitter)
(require 'rust-prog-mode))

(require 'rust-utils)

;;; rust-mode.el ends here
3 changes: 2 additions & 1 deletion rust-prog-mode.el
View file
Open in desktop
Expand Up @@ -4,7 +4,8 @@
;; rust-mode code deriving from prog-mode instead of rust-ts-mode

;;; Code:
(require 'rust-common)

(require 'rust-mode)

(defvar electric-pair-inhibit-predicate)
(defvar electric-pair-skip-self)
Expand Down
4 changes: 2 additions & 2 deletions rust-rustfmt.el
View file
Open in desktop
Expand Up @@ -87,8 +87,8 @@
(insert-file-contents tmpf)
(rust--format-fix-rustfmt-buffer (buffer-name buf))
(error "Rustfmt failed, see %s buffer for details"
rust-rustfmt-buffername))))
(delete-file tmpf))))))
rust-rustfmt-buffername)))
(delete-file tmpf)))))))

;; Since we run rustfmt through stdin we get <stdin> markers in the
;; output. This replaces them with the buffer name instead.
Expand Down