Fix function definition highlighting #563

phy1729 · 2018-10-21T20:03:55Z

Fixes #223

Adds new function-definition style; thoughts on what that should be by default?

danielshahaf

👍

It's so easy now that we have the array that I wonder why we didn't do it before.

Some minor suggestions and a whole list of test cases for your enjoyment. :)

danielshahaf · 2018-10-21T20:55:17Z

highlighters/main/main-highlighter.zsh

  # - :regular:    "Not a command word", and command delimiters are permitted.
  #                Mainly used to detect premature termination of commands.
  # - :always:     The word 'always' in the «{ foo } always { bar }» syntax.
+  # - :function:   Function names after function or before ()


Quote the reserved words function and () for clarity.

danielshahaf · 2018-10-21T20:56:39Z

highlighters/main/main-highlighter.zsh

     # try-always construct
     style=reserved-word # de facto a reserved word, although not de jure
     next_word=':start:'
+   elif [[ $this_word == :function: ]]; then


Most state checks use asterisks before/after the colon. Shouldn't this do the same? (I suspect it should, but even if not it definitely deserves a comment explaining the exception.)

danielshahaf · 2018-10-21T21:13:10Z

docs/highlighters/main.md

 * `dollar-double-quoted-argument` - parameter expansion inside double quotes (`$foo` inside `""`)
 * `back-double-quoted-argument` -  backslash escape sequences inside double-quoted arguments (`\"` in `"foo\"bar"`)
 * `back-dollar-quoted-argument` -  backslash escape sequences inside dollar-quoted arguments (`\x` in `$'\x48'`)
+* `function-definition` - function name(s) when defining a function (`f` and `g` in `f g () { : }`)


Test cases (written before I read the diff):

function { pwd } — anonymous function

function pwd — defines a function named pwd, not runs that command

function >/dev/null { pwd } — anonymous function

function f >/dev/null { pwd } — named function with sticky redirection

function >/dev/null f { pwd } — syntax error (!)

The above also support an optional () after the function names and before the brace.

() {} — anonymous function

() pwd — anonymous function

() () () pwd — anonymous function for calling tor

The above also come in named variants.

function () () pwd — two anonymous functions

function f () () pwd — named function whose body is an anonymous function

function () f () pwd — anonymous function that defines a function named f

function () () f pwd — two anonymous functions, f is in command position (unknown-token)

Then there's the MULTI_FUNC_DEF variants of all these.

Then there's

unsetopt multifuncdef; function f g h { pwd } — creates a function named f. Probably a zsh bug, actually, but shouldn't we highlight g and h as errors nevertheless? (I saw the test for the non-function-syntax variant of this.)

Question: In () pwd, should the () be highlighted as a command position? What about function { pwd }?

Another case (github didn't let me edit comment after I pressed the green button):

% function f pwd % f

Fun fact: function >/dev/null f { pwd } is only a syntax error when right_brace_is_recognised_everywhere is true. The redirection makes it an anonymous function that executes f. The wonders of zsh's grammar.

What should the newline be highlighted as?

How about highlighting it the same way as the newlines in

for 1 in foo bar baz do echo $1 done

?

danielshahaf · 2018-10-21T21:14:42Z

highlighters/main/main-highlighter.zsh

                           this_word=':start::regular:'
+                           ;;
+                        ('function')
+                           next_word=:function:


Minor: I usually quote ':foo:' tokens, partly for Vim syntax highlighting purposes. We should probably be consistent one way or the other.

Ah, I was wondering why many were quoted. I'll match.

danielshahaf · 2018-10-21T21:15:45Z

highlighters/main/main-highlighter.zsh

-                 fi
+        '()')
+                 # Function definition was handled above
+                 style=unknown-token


When can this happen? f g () with nomultifuncdef, function "foo'bar" (), …?

or after a redirection or in an array assignment.

What about empty case patterns? case foo in (bar) ;; esac → case foo in () ;; esac

Right now it's highlighted as reserved-word which isn't correct either. We'll probably need another state to handle case correctly.

With this branch the () in case is highlighted as unknown-token — which is correct, since it's a syntax error. Let's discuss case on #406.

danielshahaf · 2018-10-21T21:21:31Z

highlighters/main/main-highlighter.zsh

-     if (( ${+precommand_options[$arg]} )) && _zsh_highlight_main__is_runnable $arg; then
+     if [[ $res != reserved && ( $args[1] == '()' ||
+           # TODO: Function names can be absurd; this handles the more common cases without invoking Cthulhu.
+           ( $zsyh_user_options[multifuncdef] == on && $args[(r)*[^[:alnum:]_-]*] == '()' ) ) ]]; then


I wonder if we could do something along the line of "after function, everything that isn't a token (&&, >, etc) or the reserved words { or () is a function name? Haven't looked at Src/parse.c of this.

function already has that in effect. This branch only effects words before ().

Sorry, that's what I meant. "If one of the following words is (), and it's in the same simple command as the current word, then all intervening words [other than redirections] are function names" or something like this. Basically, we know that "everything from the start of the command to the () token is function names", with some exceptions (noglob, redirections, …), so I wonder if a blacklist might be better than a whitelist.

It would probably be a better approach, but I'm not sure how to keep it fast or how to enumerate all of the invalid function names.

(Aside noglob () pwd is a valid function definition.)

Fixes zsh-users#223 zsh-users#237

danielshahaf · 2020-02-28T22:44:12Z

@phy1729 This is milestoned 0.8.0. Do you intend to work on it for the 0.8.0 release? If yes, great; if not, could you re-milestone it appropriately? Cheers. If it's waiting on me, let me know!

danielshahaf · 2020-03-15T13:17:38Z

(Assigning until the question is answered. Feel free to unassign yourself if needed.)

danielshahaf · 2020-05-22T02:35:37Z

Triage: Bumping to the next milestone due to feedback timeout.

danielshahaf reviewed Oct 21, 2018

View reviewed changes

phy1729 force-pushed the function-definition branch from 4433121 to c2114f1 Compare October 22, 2018 00:51

danielshahaf mentioned this pull request Oct 22, 2018

Highlight named fd redirections #564

Merged

phy1729 force-pushed the function-definition branch from c2114f1 to 35d7f8c Compare October 23, 2018 04:14

main: Move in_redirection to its own branch

4a7462f

phy1729 force-pushed the function-definition branch from 35d7f8c to cd64662 Compare October 23, 2018 04:27

phy1729 added this to the 0.7.0 milestone Oct 23, 2018

main: Fix function definition highlighting

64b13f5

Fixes zsh-users#223 zsh-users#237

phy1729 force-pushed the function-definition branch from cd64662 to 64b13f5 Compare October 23, 2018 04:40

danielshahaf modified the milestones: 0.7.0, 0.8.0 Jun 16, 2019

phy1729 force-pushed the master branch from a02355a to 7fd44bc Compare February 25, 2020 13:53

danielshahaf assigned phy1729 Mar 15, 2020

danielshahaf modified the milestones: 0.8.0, 0.9.0 May 22, 2020

Fix function definition highlighting #563

Are you sure you want to change the base?

Fix function definition highlighting #563

Uh oh!

Conversation

phy1729 commented Oct 21, 2018

Uh oh!

danielshahaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielshahaf commented Feb 28, 2020

Uh oh!

danielshahaf commented Mar 15, 2020

Uh oh!

danielshahaf commented May 22, 2020

Uh oh!

Uh oh!