Fix ancient defect and dependency complications related to having two rust-mode implementations #534
Conversation
That was always the intention, but the cleanup code was always
placed outside the unwind forms.
lib/rust-mode/rust-rustfmt.el:60:12: Warning: ‘unwind-protect’
without unwind forms
rust-mode-treesitter.el
Outdated
| @@ -5,6 +5,9 @@ | |||
|
|
|||
| ;;; Code: | |||
|
|
|||
| (defvar rust-before-save-hook) | |||
There was a problem hiding this comment.
Shouldn't this come from rust-common.el ?
There was a problem hiding this comment.
Yes, but it seems you forgot to explicitly require that. It appears that, for some reason, which escapes me now, I assumed there was a reason why this library doesn't require any other libraries.
There was a problem hiding this comment.
But rust-mode.el does require it:
Line 17 in bd7d8ed
There was a problem hiding this comment.
But rust-mode-treesitter.el does not require rust-mode or any other library.
There was a problem hiding this comment.
That's because rust-mode.el requires rust-mode-treesitter.el:
Line 74 in bd7d8ed
There was a problem hiding this comment.
And that is why we need to declare this variable. When we compile rust-mode-treesitter.el, the compiler doesn't know that at runtime, rust-mode-treesitter is loaded because it is required in rust-mode, and that it is assumed the user (or some third-party code) does not load/require rust-mode-treesitter directly. So we have to tell the compiler about it, so it can stop being alarmist about it, and so that we don't get used to it outputting warnings and thus end up ignoring them.
However, I don't think that is right way to do it. This mode does depend other parts of this package, so it should depend on them using require. This is complicated by the fact that you also want to load this file, whenever rust-mode is loaded (modulo some conditions, that are not relevant to this discussion).
You can have it both ways, by moving (require 'rust-mode-treesitter) below (provide 'rust-mode) (as is already being done for (require 'rust-utils)).
There was a problem hiding this comment.
rust-prog-mode should also require rust-mode.
There was a problem hiding this comment.
There was a problem hiding this comment.
I've pushed an update.
There was a problem hiding this comment.
Thanks, I did a quick testing and this it looks good. I will use it throughout the day and see if any issue pops up.
"rust-prog-mode.el" and "rust-mode-treesitter.el" provide competing implementations of `rust-mode'. Both implementations depend on code in "rust-mode.el", and thus must require that. Doing that is complicated by the fact that "rust-mode.el" loads one of these libraries, depending on `rust-mode-treesitter-derive's value. Address this conflict by: 1. Requiring feature `rust-mode' in the two libraries that implement the `rust-mode' major-mode and that use things defined in "rust-mode.el". 2. Moving the require forms for these two libraries in "rust-mode.el", below the `provide' form for `rust-mode'.
tarsius
changed the title
|
@jroimartin / @condy0919 Can you also test drive this PR since you have had related issues previously ? |
My pleasure. |
@psibi it seems to work fine on my side. |
| ;;;###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)) |
There was a problem hiding this comment.
Emacs > 29.1 can be compiled without tree sitter feature.
We should maybe replace this with (functionp 'treesit-parser-p), or something similar ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
And also the if check in rust-mode-treesitter.el
There was a problem hiding this comment.
Yeah, agree. I have opened an issue to track this: #537
|
I'm merging this since I have seen no objections from others. Thanks @tarsius for the PR! Thanks @condy0919 and @jroimartin for the additional testing. |
|
You're welcome! |
and