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

Fix rust-mode lazy loading #530

Merged
merged 1 commit into from Mar 12, 2024

Conversation

jroimartin
Copy link
Contributor

@jroimartin jroimartin commented Mar 8, 2024

This PR fixes the following error that happends when opening a .rs
file:

File mode specification error: (void-function rust-mode)

It conditionally autoloads the proper rust-mode version depending on
the user environment.

Fixes #528

@psibi
Copy link
Member

psibi commented Mar 8, 2024

Thanks, @jroimartin can you see why the tests are failing ?

@jroimartin
Copy link
Contributor Author

Sure, I'm working on it.

@jroimartin jroimartin force-pushed the fix-rust-mode-autoload branch 2 times, most recently from 74f7728 to b9baa86 Compare March 8, 2024 10:55
@jroimartin
Copy link
Contributor Author

jroimartin commented Mar 8, 2024

@psibi PTAL. I think this is the smallest required change. On the flip side, it is a bit tricky. But having two major modes with the same name and loading them conditionally seems problematic. So, I opted for allowing the stub function (called rust-mode) to be redefined when the actual mode gets loaded.

@psibi
Copy link
Member

psibi commented Mar 8, 2024

Thanks, LGTM. @condy0919 Can you see if it works in your setup too ? I will test drive this PR too.

@condy0919
Copy link
Contributor

Thanks, LGTM. @condy0919 Can you see if it works in your setup too ? I will test drive this PR too.

Sorry, I'm in a touring🥺 can't help that

@jroimartin
Copy link
Contributor Author

No rush. There is a valid workaround which is just adding (require 'rust-mode) to init.el. It is just that it is nice to support lazy loading 🙂 This change also make rust-mode just work for people using plain package.el.

@psibi
Copy link
Member

psibi commented Mar 8, 2024

No worries, I'm also going to be on travel for the next two days. I will test this out more properly after that.

rust-mode.el Outdated
(require 'rust-mode-treesitter)
(require 'rust-prog-mode))
;;;###autoload
(defun rust-mode ()
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this function conflict with the rust-mode function that is exposed by the rust-prog-mode / rust-treesitter-mode ?

Copy link
Member

Choose a reason for hiding this comment

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

Infact, give that you are calling rust-mode in the body of this function - it makes me more nervous. Do you think we can rename it to a different function instead, probably rust-mode-load or something like that ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@psibi yeah, it is a bit hacky...

The thing is that we want to autoload rust-mode.el when the user calls (rust-mode) and then select the actual major mode. However, both versions of the major mode (rust-prog-mode and rust-treesitter-mode) are called rust-mode and which one to choose depends on the environment (the emacs version and the value of rust-mode-treesitter-derive). So, I don't know how to autoload one or the other conditionally.

Also, we cannot change the name of the major modes (and autoload both) because the hooks are named after them and other extensions expect these modes to be called rust-mode.

So, this PR autoloads a stub called rust-mode that is redefined by the specific call to define-derived-mode. Thus, the following calls to rust-mode execute the rust-mode function corresponding to the actual major mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

probably rust-mode-load or something like that ?

Or rust-mode-maybe, it's mentioned in the corresponding issue. But using another name may break users' config as many people manually do

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

This PR fixes the following error that happends when opening a .rs
file:

	File mode specification error: (void-function rust-mode)

It conditionally autoloads the proper rust-mode version depending on
the user environment.

Fixes rust-lang#528
@jroimartin
Copy link
Contributor Author

@psibi @condy0919 what do you think of db7d086? This option does not require messing up with the rust-mode function definition.

@condy0919
Copy link
Contributor

@psibi @condy0919 what do you think of db7d086? This option does not require messing up with the rust-mode function definition.

This solution comes to mind at first, and the trick is used in the evil-exchange package.

@psibi
Copy link
Member

psibi commented Mar 12, 2024

I did some testing and this seems to work fine. @jroimartin Thanks for the PR. @condy0919 Thanks for testing and review.

@psibi psibi merged commit 825a37d into rust-lang:master Mar 12, 2024
14 checks passed
@jroimartin jroimartin deleted the fix-rust-mode-autoload branch March 12, 2024 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File mode specification error: (void-function rust-mode)
3 participants