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.

Sign up for GitHub

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

Jump to bottom

flycheck’s tooltips can be displayed sdrawkcab when the source contains bidi directional formatting characters #1919

Open
4 of 7 tasks
db48x opened this issue Nov 8, 2021 · 5 comments · May be fixed by #1920
Open
4 of 7 tasks

flycheck’s tooltips can be displayed sdrawkcab when the source contains bidi directional formatting characters #1919

db48x opened this issue Nov 8, 2021 · 5 comments · May be fixed by #1920
Labels
kind: bug

Comments

@db48x
Copy link

db48x commented Nov 8, 2021

Checklist

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

Bug description

Source code containing bidi directional formatting characters can mess up the direction of the error message itself.

I imagine that localized error messages might cause the same problem as well, though I haven’t any easy way to check.

Steps to reproduce

  1. Start with a source file containing tricky comments:
fn main() {
    let is_admin = false;
    /*‮ } ⁦if is_admin⁩ ⁦ begin admins only */
        println!("You are an admin.");
    /* end admins only ‮ { ⁦*/
}
  1. Rust compiler reports errors
  2. Hover over the first comment, or move point to it, to see the sdrawkcab messages. See the screenshot as well.

Expected behavior

The text direction of the error message is not affected by the content of the source code it arose from.

Screenshots

Screenshot from 2021-11-07 23-50-47

The Fix

I will make a pull request for this, but for the moment here is the change I made to fix this:

(defun flycheck-error-format-message-and-id (err &optional include-snippet)
  "Format the message and id of ERR as human-readable string.

If INCLUDE-SNIPPET is non-nil, prepend the message with a snippet
of the text that the error applies to (such text can only be
determined if the error contains a full span, not just a
beginning position)."
  (let* ((id (flycheck-error-id err))
         (fname (flycheck-error-filename err))
         (other-file-p (and fname (not (equal fname (buffer-file-name))))))
    (concat (and other-file-p (format "In %S:\n" (file-relative-name fname)))
            (and include-snippet
                 (-when-let* ((snippet (flycheck-error-format-snippet err)))
                   (flycheck--format-message "`\N{LEFT-TO-RIGHT MARK}\N{FIRST STRONG ISOLATE}%s\N{POP DIRECTIONAL ISOLATE}\N{LEFT-TO-RIGHT MARK}': " snippet)))
            (or (flycheck-error-message err)
                (format "Unknown %S" (flycheck-error-level err)))
            (and id (format " [%s]" id)))))

Likely the other fields ought to have the same treatment.

System configuration

I’ve got some lsp-mode stuff going on as well, which isn’t reflected here.

Syntax checkers for buffer flycheck.el in emacs-lisp-mode:

First checker to run:

  emacs-lisp
    - may enable:    yes
    - may run:       t
    - executable:    Found at /home/db48x/bin/emacs
    - next checkers: emacs-lisp-checkdoc

Checkers that may run as part of the first checker's chain:

  emacs-lisp-checkdoc
    - may enable: yes
    - executable: Found at /home/db48x/bin/emacs

Flycheck Mode is enabled.  Use C-u C-c ! x to enable disabled
checkers.

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

Flycheck version: 32snapshot (package: 20210825.1804)
Emacs version:    29.0.50
System:           x86_64-pc-linux-gnu
Window system:    x

Emacs configuration:

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

Additional notes

https://en.wikipedia.org/wiki/Bidirectional_text
https://www.w3.org/International/questions/qa-bidi-unicode-controls
@db48x db48x added the kind: bug label Nov 8, 2021
@cpitclaudel
Copy link
Member

That's a funny one, thanks!
Question: won't this fix break messages in right-to-left languages?
(IOW, isn't the issue that rust is giving us a broken error message?)

@db48x
Copy link
Author

db48x commented Nov 8, 2021

No, it doesn’t break RTL messages:

Screenshot from 2021-11-08 01-02-40

The message from Rust is conveyed to Emacs via the LSP protocol, and I don’t think that it is supposed to contain any extra formatting. The client is supposed to do that as needed.

@db48x
Copy link
Author

db48x commented Nov 8, 2021

I want to make a pull request for this, but make check is reporting that the file is misformatted. It doesn’t say why, or what line number is misformatted, but looking at the source I see that it wants all lines to be 80 characters or less.

Do you have any recommendations for that? The format string is 103 characters, and using hex codes instead of character names would be less readable. And using a lot of string concatenation would allocate a lot of memory.

@cpitclaudel
Copy link
Member

Do you have any recommendations for that?

Making a defconst for the format string is probably the simplest.

The message from Rust is conveyed to Emacs via the LSP protocol, and I don’t think that it is supposed to contain any extra formatting.

Does that mean that every IDE needs to wrap LSP messages in those formatting codes?

@db48x
Copy link
Author

db48x commented Nov 8, 2021

The message from Rust is conveyed to Emacs via the LSP protocol, and I don’t think that it is supposed to contain any extra formatting.

Does that mean that every IDE needs to wrap LSP messages in those formatting codes?

It depends on how the client displays the message. Flycheck is putting four pieces of information into a single string and then displaying it, so it needs to be careful.

However, if we put the information into four columns of an HTML table, then each cell would be shaped and printed independently; the bidi state from one could not affect the others and thus no special care would need to be taken. Even without a table, Emacs could arrange to shape the separately, calling into libharfbuzz (or the equivalent API on other OSes) several times instead of once. But then the tooltip would have to be defined by a list of lists of strings, where the strings in the inner list are shaped independently but printed on a single line.

Do you have any recommendations for that?

Making a defconst for the format string is probably the simplest.

I suppose concatenating once when the constant is defined wouldn’t hurt.

db48x added a commit to db48x/flycheck that referenced this issue Nov 8, 2021
@db48x
isolate the snippet of source code used in formatted error messages
8a2bb60 
lest bidi formatting characters cause the error message to be rendered
sdrawkcab. In principle we should probably do this to the filename and
even the error message, since those could be suspect as well. As
ordinary RTL text doesn’t cause any problem, it is in practice not so
important. Note also that if flycheck is to be localized, this format
string must be localized as well (or the localization system must
support such substitutions directly).

Fixes flycheck#1919.
@db48x db48x linked a pull request Nov 8, 2021 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

isolate the snippet of source code used in formatted error messages db48x/flycheck
2 participants
@db48x @cpitclaudel