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

Simplify and correct angle bracket propertizing and macro argument detection. #467

Merged
merged 1 commit into from Dec 20, 2022

Conversation

jimblandy
Copy link
Contributor

Fixes #465.

When rust-syntax-propertize uses rust-macro-scopes to find ranges of text that are macro arguments, it ends up inadvertently poisoning the syntax-ppss cache by applying it to text that doesn't have the necessary syntax-table properties applied yet - the very job that rust-syntax-propertize is trying to do.

However, rust-macro-scopes does much more work than necessary. Rather than producing a list of ranges of macro arguments, we can just use the list of enclosing opening parens provided by syntax-ppss, checking each paren to see if it seems to be a macro or macro_rules call.

We have to keep syntax-ppss's cache accurate for other reasons anyway, so we might as well just use its data, rather than introducing another cache of our own - especially a problematic one (see #465).

  • rust-mode.el (rust-in-macro): Consult syntax-ppss's list of enclosing parens, rather than using rust-macro-scope. Remove optional arguments, which were only used by tests.
    (rust-macro-scopes, rust-macro-scope): Delete. Now we just use syntax-ppss's internal cache.
    (rust-syntax-propertize): Don't bind rust-macro-scopes.
    (rust-looking-back-macro-rules): New function.
    (rust-looking-back-macro): Support a space between macro name and !, by consulting rust-expression-introducers.
    (rust-expression-introducers): New constant. Use in rust-looking-back-macro and rust-is-in-expression-context.
    (rust-is-in-expression-context): Use rust-expression-introducers.
    (rust-looking-back-ident): Don't use looking-back. We've already moved to the correct spot for looking-at, within a save-excursion.
  • rust-mode-tests.el: Update tests.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brotzeit (or someone else) soon.

Please see the contribution instructions for more information.

…tection.

Fixes rust-lang#465.

When `rust-syntax-propertize` uses `rust-macro-scopes` to find
ranges of text that are macro arguments, it ends up inadvertently
poisoning the `syntax-ppss` cache by applying it to text that doesn't
have the necessary `syntax-table` properties applied yet - the very
job that `rust-syntax-propertize` is trying to do.

However, `rust-macro-scopes` does much more work than necessary.
Rather than producing a list of ranges of macro arguments, we can just
use the list of enclosing opening parens provided by syntax-ppss,
checking each paren to see if it seems to be a macro or `macro_rules`
call.

We have to keep syntax-ppss's cache accurate for other reasons anyway,
so we might as well just use its data, rather than introducing another
cache of our own - especially a problematic one (see rust-lang#465).

* rust-mode.el (rust-in-macro): Consult `syntax-ppss`'s list of enclosing
parens, rather than using `rust-macro-scope`. Remove optional arguments, which
were only used by tests.
(rust-macro-scopes, rust-macro-scope): Delete. Now we just use `syntax-ppss`'s
internal cache.
(rust-syntax-propertize): Don't bind `rust-macro-scopes`.
(rust-looking-back-macro-rules): New function.
(rust-looking-back-macro): Support a space between macro name and `!`, by
consulting `rust-expression-introducers`.
(rust-expression-introducers): New constant. Use in `rust-looking-back-macro`
and `rust-is-in-expression-context`.
(rust-is-in-expression-context): Use `rust-expression-introducers`.
(rust-looking-back-ident): Don't use `looking-back`. We've already moved to the
correct spot for `looking-at`, within a `save-excursion`.
* rust-mode-tests.el: Update tests.
@brotzeit
Copy link
Contributor

@phillord I remember that you improved the code some time ago. Can you take a look ?

@brotzeit
Copy link
Contributor

Ok let's give it a try, sorry for the delay.

@brotzeit brotzeit merged commit 07c4a3a into rust-lang:master Dec 20, 2022
@jimblandy
Copy link
Contributor Author

In a different bug you said:

Unfortunately we lack users who want to improve rust-mode.

Is rust-lang/rust-mode looking for maintainters? I'm probably qualified.

@jimblandy
Copy link
Contributor Author

Oops. @brotzeit ^

@brotzeit
Copy link
Contributor

Is rust-lang/rust-mode looking for maintainters? I'm probably qualified.

Yeah, thanks! Looking for new people for quite some time ;)

However that's an interesting timing because it seems they just merged a rust treesitter mode into emacs. I even thought about asking the emacs devs if we should merge the rust-mode features, that can't be ported to treesitter, into emacs. I know that treesitter is quite new and that we have to maintain rust-mode for years even if we decide to do this, but if the community wants rust support in emacs maybe that's the way to go.

Another possibility would be to keep maintaining the code here and add another mode that derives from the new treesitter mode in emacs29. What do you think ?

@brotzeit
Copy link
Contributor

And I can't add you as member, I have to ask an owner of the rust-lang organization.

@phillord
Copy link
Contributor

Didn't know about treesitter, but it looks pretty nice and may well be the way to go. Partial parsing in Emacs-lisp cannot carry on for ever, I think.

If the treesitter mode fulfils all that rust-mode does, I'd be inclined to be hard nosed about it. rust-mode could move to maintenance only with the intention of deprecation once Emacs-29 comes out. Where that would leave rustic I am not quite sure.

Just my thoughts; I fear I am unlikely to contribute much, other than the occasional bug fix, so take it with a pinch of salt.

@brotzeit
Copy link
Contributor

brotzeit commented Dec 21, 2022

If the treesitter mode fulfils all that rust-mode does

It doesn't.

Where that would leave rustic I am not quite sure.

I'm not even sure that the emacs devs accept the additional code of rust-mode, and there is quite some code that can't be handled by treesitter. rustic has lots of dependencies and lots of additional code.

My plan was to add another rust-mode that derives from the derives treesitter mode. It could be used instead of the current rust-mode by using an option(for now) if emacs29+ is used. As soon as it can be considered stable I would make it the default. This way we wouldn't even need to apply any changes to rustic because it would just depend on the new rust-mode.

@phillord
Copy link
Contributor

Sorry, I was not clear. I meant, if in the some hypothetical future, the tree-sitter mode could fulfil all that rust-mode does.

But, yes, your way seems much the same. Any one using rustic would, at some point, magically flip from being rust-mode to using tree-sitter mode (or your derived mode). Anyone using rust-mode directly would have to choose to do this explicitly I guess.

I would guess that the code would be accepted into the Emacs core, but it will depend on copyright waivers. The other question is whether the release cycle of Rust and any rust-mode are okay will the relatively slow Emacs release cadence.

@brotzeit
Copy link
Contributor

Sorry, I was not clear. I meant, if in the some hypothetical future, the tree-sitter mode could fulfil all that rust-mode does.

I understood you correctly ;)

The other question is whether the release cycle of Rust and any rust-mode are okay will the relatively slow Emacs release cadence.

I don't think that's an issue as we barely apply changes due to rust releases.

@brotzeit
Copy link
Contributor

@jimblandy let's see if this works =)

@jimblandy
Copy link
Contributor Author

I don't know enough about treesitter yet to really say whether it makes sense as an implementation strategy for a full-featured Rust mode. It might be possible to have a single rust-mode.el that uses treesitter where available, and falls back to other mechanisms otherwise.

The part of the engine patched by this PR is a hybrid of parse-partial-sexp and manual annotation of difficult parts of the syntax. Unless treesitter can handle Rust directly, we'll need to keep on using hybrid techniques.

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.

rust-syntax-propertize assumes text already has the properties it is responsible for placing
4 participants