emacs-ess / ESS

Emacs Speaks Statistics: ESS
https://ess.r-project.org/
GNU General Public License v3.0
613 stars 160 forks source link

shorthand function, paren-matching, and highlighting #1278

Closed r2evans closed 2 months ago

r2evans commented 4 months ago

R-4.1.0 added shorthand notation for function definition.

fun <- function(x, y) x + y

fun <- \(x, y) x + y

In R source file buffers, paren-matching works in both, but highlighting is incomplete in the shorthand:

image

image

In the R inferior process itself, shorthand notation support is mixed.

Is there a way to:


(emacs-version)
"GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38, cairo version 1.17.8)
 of 2023-09-02, modified by Debian"
(ess-version)
"ess-version:  [elpa: 20240122.1720] (loaded from /home/r2/.emacs.d/elpa/ess-20240122.1720/)"
ess-style
RStudio
mmaechler commented 4 months ago

Yes, Emacs lisp is a very complete language (etc ..), hence indeed there must be a way to ...

But no, I would not know how much work such a change would entail (and personally almost always prefer the non-shorthand notation), but we'd surely be grateful for small & well-tested patches ("small" because we cannot afford maintaining too complex code and because I believe it "must be possible" relatively easily with all the emacs lisp core utility functionality).

r2evans commented 4 months ago

Thank you @mmaechler , I understand that the regex involved would be rather more-complicated. Personally I like the notion of the shorthand function notation but not the use of the backslash in it (though I don't have a ready-replacement). If I get time I'll work on a PR, but ... unlikely to be soon. Feel free to close if you like. Thank you again!

maxecharel commented 4 months ago

Hi @r2evans, I took the liberty to submit a PR related to this issue. I've tried to mimic the 'interface' already implemented for function keywords in order to make the solution scalable, but in case this PR does not go through and you're interested in a 'quick fix' for font-locking, I have a more 'hardcoded' solution involving a slight modification of ess-r--find-fl-keyword. @mmaechler do not hesitate to come back to me if you prefer such an hardcoded solution to the one included in the current PR, I could modify the PR accordingly.
Thanks for the great work with ESS!

r2evans commented 4 months ago

I appreciate it, @maxecharel !

And now I'm confused ... as I was about to try the PR/patch, my emacs no longer missed the paren-matching shown in my original comment here. Namely, yanking or typing both forms into both .R source files and the R buffer itself will correctly paren-match. Granted, highlighting is missing on the abbreviation-function form, but paren matching works ... same emacs, same ESS, no changes that I can recall ... (smh)

I have recently restarted emacs, I wonder if something else in the buffer causes it to mis-identify. I'll see if I can improve the regex, as the R inferior buffer can certainly get "confusing" from a regex viewpoint.

The highlighting would be a nice thing, though admittedly aesthetic.

maxecharel commented 4 months ago

glad it brings at least some aesthetic improvement :) Also note that the PR includes font-locking for function names (I mean, LHS of the assignment operator when defining a function) and recognition of the shorthand notation as a function pattern (e.g. it is now recognized by ess-beginning-of-function). But I agree, putting the latter aside, it is mostly cosmetic stuff^^

In any case, I am glad the issue has been solved by restarting Emacs :)

r2evans commented 4 months ago

@maxecharel I am really not happy when the issue is not reproducible ... that means something else is going on, and admittedly it's likely a regex thing in a long-running ess inferior buffer. So, mostly "not happy" that it is inconsistent.

I suggest though that perhaps anything in your PR that is not related to font-lock should probably be removed, as it may be fixing something that is not broken. I haven't look at the PR in depth, but simpler is always better, and I'm sure the maintainers would prefer not making changes that aren't justified by palpable/reproducible bugs. (Either that or come up with a more reproducible example than I did in the original post.)

maxecharel commented 4 months ago

Thanks for the advice, @r2evans . I think there is a slight misunderstanding, and maybe I should remove the reference to this issue, which, aside from the font-locking part, appears to be only related to your configuration (or to some 'live' setting of some buffer-local variables, you know better).

Basically, in the PR I do not really try to 'fix' things: I rather added some support for the shorthand notation (which, as you highlighted, can now be used as a way to define a function in R), including font-locking that was not there (and that you mentioned in this issue, wrt that matter it is a 'fix'). But I agree, the maintainers may not be happy with my PR, even if the modifications it introduces are minor. Let's wait for some feedback; if negative, I will modify the PR or simply remove it depending on the feedback.

Concerning the paren-matching problem, with a minimal configuration:

paren-matching_with_minimal_config (obviously, here iESS and ESS is a clone of the main branch without the modifs of my PR)

So, at least on my side, no problem (minimal config dedicated to this issue, Emacs 29.2 built from source, clone of current ESS).

BTW, the config I used:

(setq inhibit-startup-message t)
(setq initial-scratch-message nil)
(load-theme 'modus-vivendi)

(add-to-list 'load-path "./ESS_original/ESS/lisp/")

(require 'ess-mode) 
(require 'ess-r-mode)

(setq ess-ask-for-ess-directory nil) 

plus 4 lines of windmove config because hey, muscle memory. It may be a silly question, but have you tried to reproduce the paren-matching part of your issue with a minimal config?

Best,

M

edit: 'let's wait', not 'let's way'

r2evans commented 4 months ago

Gotcha, sorry ... after I saw that I could not repro my own OP, I paused on importing your code, so I didn't get a full look at it. Sorry about my miscommunication.

Repro with minimal config: I do it now and I cannot reproduce the OP. I can also no longer repro the paren-matching problem with "full" config, either.

The font-lock seems to work for me.

r2evans commented 3 months ago

Okay, as an update to prove I'm not going crazy ... not sure it helps a ton, but ...

Two R sessions in the same emacs:

image

image

(There is nothing after the \(z).)

maxecharel commented 3 months ago

Interesting; I guess it's your full config, not a minimal one? Any potentially relevant difference in terms of buffer-local variables and local minor modes? (edit: I mean between the two 'sessions')

r2evans commented 3 months ago

What I've not shown there is that the first R buffer only has 1700 or so lines of history, whereas the second has currently well over 10000 lines of history.

There is something in previous output (perhaps errors or warnings with unbalanced parens/braces) that is causing the regex to fail. Case in point: when I see that the paren-matching is incorrect (second image), I confirmed my suspicion by deleting all of the history in the current buffer and trying the paren-matching again, at which point the matching works correctly.

Unfortunately, I just deleted too many lines and was unable to undo the deletion, so I cannot at this time repeat the troubleshooting with fewer lines deleted to see what is problematic. As I find the problem later, I'll re-attempt to figure out what kind of text is causing the regex to fail.