Multi-line error highlights make it hard to work

[DamienCassou]

Checklist

• I have checked existing issues for potential duplicates before creating this one.
• I have read the [Troubleshooting guide][].

Bug description

It's not a bug, rather an annoying feature.

Flycheck correctly marks the code above as being problematic. Eslint complains similarly:

$ eslint country-tests.js 

country-tests.js
  3:1  error  Unexpected fdescribe  jasmine/no-focused-tests

✖ 1 problem (1 error, 0 warnings)

The jasmine/no-focused-tests rule discourages the use of fdescribe() unless during development. Because everything is underlined in flycheck now, using fdescribe() becomes nearly impossible even during development.

Expected behavior

I think the previous behavior of only marking the first letter of fdescribe was more usable.

System configuration

  javascript-eslint
    - may enable:  yes
    - executable:  Found at /usr/local/bin/eslint_d
    - config file: found


--------------------

Flycheck version: 32snapshot
Emacs version:    27.0.90
System:           x86_64-pc-linux-gnu
Window system:    x

Emacs configuration:

• Plain Emacs / Custom configuration
• Spacemacs
• Doom Emacs
• Other shared configuration

[cpitclaudel]

Sorry for the trouble. I think this is a bug, but on the jasmine/no-focused-tests side. We're just respecting the range that it returns, which is way to big (as you suggest, highlighting just one word would be perfect). We can work around this in Flycheck, but could you also report it to them?

[cpitclaudel]

I wonder if we should put a cap on the number of highlighted lines

[cpitclaudel]

Maybe we ought to have a variable at least that says whether to use spans.

[DamienCassou]

could you also report it to them?

done: tlvince/eslint-plugin-jasmine#230

I wonder if we should put a cap on the number of highlighted lines

do you know of any case where highlighting more than 1 line is meaningful?

[cpitclaudel]

done: tlvince/eslint-plugin-jasmine#230

Brilliant 👏

do you know of any case where highlighting more than 1 line is meaningful?

Yes, unfortunately; here's an OCaml example where we currently do terrible, and supporting (multi-line) ranges will help (I haven't pushed multi-line support to the flycheck-ocaml checker yet):

let f (x: int) =
  match x with
  | 0 -> "a"
  | _ -> (+)
           1 3

Right now we highlight the first character of (+) and say This expression has type int but an expression was expected of type string, which is quite confusing.

Here's a Coq example showing a similar issue:

Definition a :=
  True \/
  False
    :: [].

This parses as Definition a := True \/ (False :: []).

Which is ill-typed; the error spans the whole False :: [] term; if we highlighted just False (the first line), we'd be suggesting the wrong thing.

But indeed, giant error spans are a problem. In fact, I turned multi-line support off entirely in phpmd because it only highlighted huge spans…

Maybe we should find less intrusive ways to report errors, so that it wouldn't be such a problem to develop even with large error spans highlighted. But you seem to have customized flycheck to highlight the text in red in addition to underlining it, which goes against that idea. WDYT?

[ghost]

Same issue with eslint in a normal JS file with js2-mode and web-mode

[ghost]

After some tests looks like the issue was introduces in 936dbf5#diff-374cdd9563b826c712b1dc6a373a0bc1, before that commit all works good to me

[cpitclaudel]

After some tests looks like the issue was introduces in 936dbf5#diff-374cdd9563b826c712b1dc6a373a0bc1, before that commit all works good to me

Can you double-check? I don't think that commit is related.
Can you show the source file?

[ghost]

Yes I confirm the issue with https://github.com/flycheck/flycheck/blob/936dbf52a223244eee8fe2b26efed4a3b6379331/flycheck.el,

Works fine with the previous commit code: https://github.com/flycheck/flycheck/blob/7be341c24a235281ef4e5b87787bff35a23007c6/flycheck.el

You can test it with a small code:

// date-test.js
/**
 * Helper for creating a formatted date string
 * @param {date} d Date object
 * @param {string} lang Language (only supports English 'en' and Spanish 'es')
 * @param {string} timeZone Time zone to use, defaults to America/New_York. Will be ignored in
 *                          unsupported browsers.
 * @returns {string} Spanish formatted date (ie: '18 Jul | 06:29 EDT')
 */
export function formatDate(d, lang = 'es', timeZone = 'America/New_York') {
  const time = timeZone;
  const order = lang;

  return `${order} – ${time}`;
}

and this eslint plugin: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/prefer-default-export.md

extends:
  - eslint:recommended
plugins:
  - import
rules:
  import/prefer-default-export: 2

[DamienCassou]

Maybe we should find less intrusive ways to report errors, so that it wouldn't be such a problem to develop even with large error spans highlighted. But you seem to have customized flycheck to highlight the text in red in addition to underlining it, which goes against that idea. WDYT?

I'm using modus-operandi theme from @protesilaos. I guess the themes could be improved to highlight that there is a problem without making it hard to fix it.

[protesilaos]

Thanks for noting this! I will review them right now and report back.

EDIT: @DamienCassou I just pushed a commit to master that only applies an underline without changing the foreground. Please let me know if that improves things. Maybe contact me directly if this is not the right place for further feedback on the matter. Thanks!

[cpitclaudel]

@vxcamiloxv thanks. As in Damien's case, this is partly an upstream bug: eslint reports a giant error range, so we obediently highlight a giant portion of the buffer. Can you report this issue to the maintainer of the eslint plugin?

…but I agree that, in the meantime, we should find a fix, since this isn't very convenient.

I can think of a few options:

• Switching to a different highlighting style, such as inserting markers instead of highlighting, something →like this← or ⚠️[like this], or even using the indicators we currently put in the margin, »like this«

• Putting a bound on the number of highlighted characters, so that we highlight at most, say, 30 characters; but then we'd need to chose how to clarify that the error goes on.

• Adding a setting to disable region highlighting fully.

etc. I'd love to hear ideas and opinions.

[DamienCassou]

or a combination: underline if the error is on a single line and have markers in case the error spans several lines. Markers have the advantage of allowing multiple errors in the same region such as a small error inside an error spanning several lines.

[ghost]

@cpitclaudel I'll report the bug also to the upstream eslint plugin

[cpitclaudel]

Hi all,

I've made a bit of progress on this. Here are some mock-ups:

• Delimiters around the error span:
  

• Fringe indicators
  

The idea would be to show regular underlines + fringe indicators up to a configurable number of lines, and then switch to paired delimiters + fringe indicators for longer errors.

Now I need to pick a default delimiter and fringe style. Do you have preferences among the above?

[NicolasPetton]

@cpitclaudel In terms of UX, don't you think it could be distracting/annoying to have delimiters shown in the code, as they will move text horizontally on the line?

[cpitclaudel]

@NicolasPetton Definitely, yes. Especially with transient errors that appear and disappear as you type. From my experience programming with it for a few hours, the most annoying part is the start marker, because that moves the place where the point is as well (whereas the end of the error tends to be after the point). On the other hand, it's quite nice to see nested errors.

Can you think of something else that would be more discrete? (I'm fairly happy with the continuation lines in the fringe, but I don't think these are enough on their own).

[cpitclaudel]

Also, thanks for having a look!

[DamienCassou]

Thank you very much for your work. Isn't it the responsibility of the theme to decide how to show something?

[NicolasPetton]

Can you think of something else that would be more discrete? (I'm fairly happy with the continuation lines in the fringe, but I don't think these are enough on their own).

What about simply underlining the first line? With the indications in the fringe, wouldn't it be enough?

[cpitclaudel]

Thank you very much for your work. Isn't it the responsibility of the theme to decide how to show something?

I don't think Emacs themes have this level of control, and in any case we need to come up with good defaults. But in any case, this stuff would be configurable.

[cpitclaudel]

What about simply underlining the first line? With the indications in the fringe, wouldn't it be enough?

Interesting. That's a heavier change because we'd need two overlays instead of one, but we could do that. That being said, there's still the problem of indicating the end of the error.

[protesilaos]

Thank you very much for your work. Isn't it the responsibility of the theme to decide how to show something?

A theme is supposed to control the "faces" themselves (defface), but not how they are applied to interface/code constructs. So in this case the theme is responsible for the colour and style of the underline (straight line or wave), the colour of the affected text, and the styles of the fringe indicators.

[fmdkdd]

What about showing the full error span only for the currently highlighted error (the one that triggered -display-error)? The full span may be useful to understand the error, but it doesn't need to be displayed the rest of the time (when navigating or editing).

[cpitclaudel]

When I'm typing text with a fast-enough checker parts of the current line are often highlighted, so I worry that this too will cause issues.

[DamienCassou]

That being said, there's still the problem of indicating the end of the error.

Do we really have to?

[cpitclaudel]

Do we really have to?

Good question. I don't have a very strong opinion. It can help to see where the error ends, but if it messes up the layout it might not be worth it.

[cpitclaudel]

OK, I've cleaned up my branch and added documentation. Please see this PR: #1743. It would be great if you could have a look and ideally give it a try (comments on the design are also very welcome). I've defaulted to hiding underlines when the error spans more than 4 lines. The threshold is customizable and delimiters can be added if desired.