[RFC] Lua error message better formatting #7969

AlexandreDubray · 2018-02-05T11:53:23Z

reference #7388

_emsg is the same as emsg but has additionnal argument which is a pointer to a function that is used to return the value

ZyX-I · 2018-02-05T21:35:29Z

src/nvim/message.c

+ *
+ * return TRUE if wait_return not called
+ */
+int emsg(const char_u *s_)


No need to have const char *s = (const char *)s_; line above as well as underscore here: underscore is only because of the existence of the line and line is there only because I did not feel like refactoring lots of emsg callers which use char_u.

Ah, and here still is underscore.

Oh god, I must be blind. Sorry for that, I will change it immediately so I will not forget anymore !

ZyX-I · 2018-02-05T21:37:24Z

src/nvim/message.c

- * return TRUE if wait_return not called
- */
-int emsg(const char_u *s_)
+static int _emsg(const char *s_, int (*msg_attr)(const char *s, const int attr))


It is bad idea to mask symbols; also need typedef, AFAIR C declarations generator intentionally cannot swallow that.

I made the typedef in a commit but I did not understood the "mask symbol" part (I mean, technically what does that mean? I'm sorry, my knowledge of C is not that advanced).

It means that you should not define a symbol in some inner scope which is the same as a symbol in an outer scope. E.g. in

int i = 0; { int i = 1; printf("%d\n", i); } printf("%d\n", i);

second i masks the first one. msg_attr is a function defined in compilation unit scope, and at the same time an argument to _emsg which means that it is redefined in _emsg functions scope.

Indeed it was not smart, thanks for the explanation!

The added function behaves like the non-echo function but display message in a echo-style way (i.e. tab and newline are preserved)

ZyX-I · 2018-02-05T23:04:08Z

src/nvim/message.c

@@ -54,6 +54,8 @@ struct msgchunk_S {
  char_u sb_text[1];            /* text to be displayed, actually longer */
 };

+typedef int (*fct_msg_attr)(const char *s, const int attr);


New types should be CamelCase.

ZyX-I · 2018-02-05T23:05:18Z

src/nvim/message.c

+msg_echo_attr_keep(
+    const char *s,
+    const int attr,
+    int keep


You must not put arguments completely on separate lines unless you absolutely have to. Neither separate type from function name or closing parenthesis from the last argument.

ZyX-I · 2018-02-05T23:06:02Z

src/nvim/message.c

+    const int attr,
+    int keep
+)
+  FUNC_ATTR_NONNULL_ARG(1)


FUNC_ATTR_NONNULL_ALL may be used when all pointer arguments are non-NULL. Non-pointer ones do not matter.

ZyX-I · 2018-02-05T23:07:19Z

src/nvim/message.c

@@ -577,9 +666,9 @@ static int _emsg(const char *s_, int (*msg_attr)(const char *s, const int attr))
 */
 int emsg(const char_u *s_)
 {
-  return _emsg((const char *)s_, &msg_attr);
+  fct_msg_attr f = &msg_attr;


Do not create temporary variable here, code should work as it worked.

ZyX-I · 2018-02-05T23:12:03Z

src/nvim/message.c

+  buf = (char *)msg_strtrunc((char_u *)s, false);
+  if (buf != NULL) {
+    s = buf;
+  }


Too much copy-paste.

ZyX-I · 2018-02-05T23:12:36Z

src/nvim/message.c

+
+  char * spec_char = "\t\n\r";
+
+  char * next_spec = (char *)s;


There should not be space after asterisk.

And next_spec must be const char *. Do not cast to char * until needed.

I.e. until going to legacy (unrefactored) msg_outtrans[_len]_attr.

ZyX-I · 2018-02-05T23:14:28Z

src/nvim/message.c

+    s = buf;
+  }
+
+  char * spec_char = "\t\n\r";


Do not make compiler work harder, strpbrk argument better be a literal, either just literal or macros. But since you are using this list in a single place just put literal there.

ZyX-I · 2018-02-05T23:17:13Z

src/nvim/message.c

+  char * next_spec = (char *)s;
+
+  while (next_spec != NULL) {
+    next_spec = (char *)vim_strpbrk(s, spec_char);


You just added a bunch of useless casts here (some hidden by vim_strpbrk macros), don’t use vim_strpbrk, use strpbrk directly.

ZyX-I · 2018-02-05T23:23:14Z

src/nvim/message.c

@@ -606,6 +713,19 @@ static bool emsgfv(const char *fmt, va_list ap)
  return emsg((const char_u *)errbuf);
 }

+static bool emsgfv_echo(const char *fmt, va_list ap)


This function is short and not used anywhere, but in emsgf_echo, so should not exist.

ZyX-I · 2018-02-05T23:29:27Z

src/nvim/message.c

@@ -572,7 +653,21 @@ int emsg(const char_u *s_)

  // Display the error message itself.
  msg_nowait = false;  // Wait for this msg.
-  return msg_attr(s, attr);
+  return ret_fct(s, attr);


ret_fct is a strange name. I would go with something like display_msg.

AlexandreDubray · 2018-02-06T20:28:25Z

@ZyX-I I think I've modified everything you pointed out. Still I'm not sure about the "too much copy-paste" comment. Let me know if I need to modify anything :-)

ZyX-I · 2018-02-06T21:28:26Z

src/nvim/message.c

+}
+
+int
+msg_echo_attr_keep(const char *s, const int attr, int keep)


You are still separating type from function name. Correct examples of formatting are new files like src/nvim/eval/typval.c, not legacy code here.

BTW, function returns boolean.

And it is logical to either have const int keep or int attr. I normally prefer putting const everywhere I can.

ZyX-I · 2018-02-06T21:29:48Z

src/nvim/message.c

@@ -137,6 +139,81 @@ int msg_attr(const char *s, const int attr) FUNC_ATTR_NONNULL_ARG(1)
  return msg_attr_keep((char_u *)s, attr, false);
 }

+int msg_echo_attr(const char *s, const int attr) FUNC_ATTR_NONNULL_ARG(1)


Do not put FUNC_ATTR on the same line as a function name, this variant both visually hides attributes and produces unneccessary line change if something needs to be added (e.g. new argument or another attribute).

BTW, is this a function intended to be used in ex_echo? It is not good to have two implementations doing essentially the same task; also using function there makes testing a lot easier and allows CI to catch errors before test suite is good enough.

I did not intend to use it in ex_echo, but I guess I could modify ex_echo to use it (if it is the purpose of the question)

ZyX-I · 2018-02-06T21:32:15Z

src/nvim/message.c

+msg_echo_attr_keep(const char *s, const int attr, int keep)
+  FUNC_ATTR_NONNULL_ALL
+{
+  static int entered = 0;


This function contains too many copy-paste from its origin, common code should not be copied if possible.

ZyX-I · 2018-02-06T21:32:35Z

src/nvim/message.c

+{
+  static int entered = 0;
+  int retval;
+  char * buf = NULL;


Here asterisk is formatted as multiplication, but it is not.

ZyX-I · 2018-02-06T21:34:17Z

src/nvim/message.c

+  }
+
+  msg_start();
+  buf = (char *)msg_strtrunc((char_u *)s, false);


And if you do refactor old code, move variable definitions closer to the place where they are actually used. It should be char *const buf here.

ZyX-I · 2018-02-06T21:35:06Z

src/nvim/message.c

+    s = buf;
+  }
+
+  const char *spec_char = "\t\n\r";


This variable still has no reason to exist.

Oh I see, I misunderstood your previous comment

ZyX-I · 2018-02-06T21:36:27Z

src/nvim/message.c

+  FUNC_ATTR_NONNULL_ALL
+{
+  static int entered = 0;
+  int retval;


This one should not exist either, need const bool retval = msg_end() below.

ZyX-I · 2018-02-06T21:37:42Z

src/nvim/message.c

+  retval = msg_end();
+
+  if (keep && retval && vim_strsize((char_u *)s) < (int)(Rows - cmdline_row -1)
+      * Columns + sc_col) {


Alignment is not correct.

AlexandreDubray · 2018-02-08T08:27:41Z

src/nvim/message.c

 {
  return msg_echo_attr_keep(s, attr, false);
 }

-int
-msg_echo_attr_keep(const char *s, const int attr, int keep)
+bool msg_attr_skip(const char *s, const int attr, int *const call)


@ZyX-I I move the common code into this function that return when the message doesn't need to be shown (I will add comments, I forgot them). Is the function ok for you?
Also, i didn't put the add_msg_hist here because (if I understood well the code) it is already done in msg_outtrans_len_attr and msg_puts_attr_len. I guess it doesn't need to be done multiple time?

justinmk · 2018-02-12T02:10:48Z

src/nvim/message.c

+ * When terminal not initialized (yet) mch_errmsg(..) is used.
+ *
+ * return TRUE if wait_return not called
+ */


Please run make lint, it will notify you about lint problems.

justinmk · 2018-02-12T02:14:39Z

typedef int (*FctMsgAttr)(const char *s, const int attr);

What is Fct? If it means "function", that's very unusual. Fn or Fun or Func are far more common. Use git grep to see existing examples:

git grep -E 'typedef.*\(.*\)' -- ':/*.c'

AlexandreDubray · 2018-02-13T22:10:24Z

@justinmk Yep Fct was for function (old habits die hard ^^) corrected now! (plus the lint problem). I think it's ready if there is nothing else :-)

ZyX-I · 2018-02-13T22:30:49Z

I do not see new code used for echo. Also missing tests including outputting following messages:

Regular ones with multiline stack trace.
Single-line regular (i.e. what a sane developer would want to throw with error()) ones.
Consisting of just three newlines, three tabs and three \rs and nothing else. (These are three cases, not one).
Long (longer then screen width) single-line ones with tabs inside.
Multiline with long (…) lines.
Just empty string.

That list repeated for

Throwing lua errors and displaying them on screen.
Throwing lua errors and catching them with redir, execute() or nvim_command_output (since it is easier to write test generator then repeat tests it would make sense to use all variants).
Throwing lua errors and catching them with :try|catch.
Echoing messages with :echo and displaying them on screen.
Echoing messages with :echon and displaying them on screen.
Echoing messages with :echo and catching them with redir/… (…).
Echoing messages with :echon and catching them with … (…).

As I said I would expect you to write a test generator rather then lots of boilerplate tests.

AlexandreDubray · 2018-02-13T22:38:32Z

Oh yeah, I forget this story about the echo, will do that. And also, what do you mean exactly by test generator ?

ZyX-I · 2018-02-13T22:46:19Z

Normally you write tests like describe('text', function() it('text', function() --[[ actual test ]] end) end), placing that directly in the file. By “test generator” I mean some lua function which calls describe() and/or it() with computed text and actual test being a closure acting based on test generator arguments. It is used throught the test code for creating similar tests: e.g. in https://github.com/ZyX-I/neovim/blob/5c84d48b1f0ba2400ffb06f20ceb2da3e0590261/test/functional/eval/null_spec.lua or https://github.com/ZyX-I/neovim/blob/5c84d48b1f0ba2400ffb06f20ceb2da3e0590261/test/functional/ex_cmds/append_spec.lua.

AlexandreDubray · 2018-02-13T22:52:04Z

Ok, thanks for the references, I will look at that.

justinmk · 2019-01-23T20:48:17Z

Merged by #9520. Would be good if someone implemented the tests mentioned in #7969 (comment) .

message: moving emsg to _emsg

b64fd07

_emsg is the same as emsg but has additionnal argument which is a pointer to a function that is used to return the value

marvim added the WIP label Feb 5, 2018

ZyX-I reviewed Feb 5, 2018

View reviewed changes

AlexandreDubray added 4 commits February 5, 2018 23:24

message.c: add msg_echo_attr functions

5fdb2ff

The added function behaves like the non-echo function but display message in a echo-style way (i.e. tab and newline are preserved)

message.c: add typedef for msg_attr function to be passed to _emsg

4ecc0ef

message.c: remove old cast in _emsg

8e87da0

message.c: lint

ca2e206

ZyX-I reviewed Feb 5, 2018

View reviewed changes

src/nvim/message.c Outdated

buf = (char *)msg_strtrunc((char_u *)s, false);

if (buf != NULL) {

s = buf;

}

Copy link

Contributor

ZyX-I Feb 5, 2018

Choose a reason for hiding this comment

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

Too much copy-paste.

ZyX-I reviewed Feb 5, 2018

View reviewed changes

AlexandreDubray added 4 commits February 6, 2018 11:05

message.c: style

1503409

message.c: remove useless call to add_msg_hist in msg_echo_attr_keep

0e08106

message.c: msg_echo style + remove useless variable

57ea723

style

7509cfa

ZyX-I reviewed Feb 6, 2018

View reviewed changes

message.c: style and convention

a788f57

AlexandreDubray commented Feb 8, 2018

View reviewed changes

AlexandreDubray changed the title ~~[WIP] Lua error message better formatting~~ [RFC] Lua error message better formatting Feb 11, 2018

marvim added RFC and removed WIP labels Feb 11, 2018

justinmk reviewed Feb 12, 2018

View reviewed changes

message.c: lint and style

6ffb391

AlexandreDubray changed the title ~~[RFC] Lua error message better formatting~~ [RDY] Lua error message better formatting Feb 13, 2018

AlexandreDubray changed the title ~~[RDY] Lua error message better formatting~~ [RFC] Lua error message better formatting Feb 13, 2018

message.c: style

fadf4d1

ex_echo: reuse code from message.c to show arg to user

e77cd56

ZyX-I mentioned this pull request Apr 12, 2018

Add exc to first line of ErrorResponse message neovim/pynvim#221

Closed

bfredl mentioned this pull request Jan 18, 2019

util: don't add extra newlines in format_exc_skip neovim/pynvim#381

Merged

svermeulen mentioned this pull request Jan 18, 2019

[RFC] Fix nvim_err_write to not truncate error information when triggering exceptions and populating v:errmsg #9519

Closed

bfredl mentioned this pull request Jan 18, 2019

Lua: format multiline messages / emsgf_multiline() #9520

Merged

justinmk closed this Jan 23, 2019

KillTheMule mentioned this pull request Jan 23, 2019

Implement tests for improved lua messages #9541

Closed

Uh oh!

[RFC] Lua error message better formatting #7969

[RFC] Lua error message better formatting #7969

Uh oh!

Conversation

AlexandreDubray commented Feb 5, 2018

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!

ZyX-I Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

AlexandreDubray commented Feb 6, 2018

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!

ZyX-I Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ZyX-I Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

ZyX-I Feb 5, 2018 •

edited

Loading

ZyX-I Feb 6, 2018 •

edited

Loading

ZyX-I Feb 6, 2018 •

edited

Loading