unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#25987: 25.2; support gcc fixit notes
@ 2017-03-05 21:47 Tom Tromey
  2017-03-06 18:35 ` Eli Zaretskii
                   ` (2 more replies)
  0 siblings, 3 replies; 42+ messages in thread
From: Tom Tromey @ 2017-03-05 21:47 UTC (permalink / raw)
  To: 25987


With -fdiagnostics-parseable-fixits, GCC can emit "fixit" notes that
tell how to apply a patch to fix a problem.

For example for this:

struct x {
  int field;
};

struct x y;

int main() {
  return y.feld;
}

I get:

q.c: In function ‘main’:
q.c:8:12: error: ‘struct x’ has no member named ‘feld’; did you mean ‘field’?
   return y.feld;
            ^~~~
            field
fix-it:"q.c":{8:12-8:16}:"field"


The documentation, from the gcc manual:

================================================================
@item -fdiagnostics-parseable-fixits
@opindex fdiagnostics-parseable-fixits
Emit fix-it hints in a machine-parseable format, suitable for consumption
by IDEs.  For each fix-it, a line will be printed after the relevant
diagnostic, starting with the string ``fix-it:''.  For example:

@smallexample
fix-it:"test.c":@{45:3-45:21@}:"gtk_widget_show_all"
@end smallexample

The location is expressed as a half-open range, expressed as a count of
bytes, starting at byte 1 for the initial column.  In the above example,
bytes 3 through 20 of line 45 of ``test.c'' are to be replaced with the
given string:

@smallexample
00000000011111111112222222222
12345678901234567890123456789
  gtk_widget_showall (dlg);
  ^^^^^^^^^^^^^^^^^^
  gtk_widget_show_all
@end smallexample

The filename and replacement string escape backslash as ``\\", tab as ``\t'',
newline as ``\n'', double quotes as ``\"'', non-printable characters as octal
(e.g. vertical tab as ``\013'').

An empty replacement string indicates that the given range is to be removed.
An empty range (e.g. ``45:3-45:3'') indicates that the string is to
be inserted at the given position.
================================================================


I think Emacs should support this in compilation mode buffers.
A few thoughts on the implementation:

* The filename quoting stuff will require a bit of unquoting
* The offsets are byte offsets, not columns, which is a bit of a pain
  (or at least, it is for me since I don't know how to handle that in
  elisp)
* One way for this to work would be to display the buffer and 
  show the proposed change as an overlay; and then use y-or-n-p
  to ask whether it should be applied.  I was thinking something like:

(defun compilation--fixit-make-overlay (start end text)
  (let ((overlay (make-overlay start end)))
    (overlay-put overlay 'face 'diff-removed)
    (overlay-put overlay 'after-string
                 (propertize text 'face 'diff-added))
    overlay))

  With this approach perhaps nothing could be done if the fixit was
  already visited, or if the buffer text already matches the
  replacement.

* However other approaches are possible, like letting the user request
  no confirmation, or request "auto-apply all changes" or something.
  Maybe a fixit should be associated with the previous error somehow.

* Allowing "fixit" to be handled specially in compile mode might mean
  adding a new "type" value, not sure.

Tom



In GNU Emacs 25.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.22.8)
 of 2017-03-02 built on bapiya
Repository revision: 6e788ef0e262fafc014c21f4ad52cc5dc9f1715b
Windowing system distributor 'Fedora Project', version 11.0.11901000
System Description:	Fedora release 25 (Twenty Five)

Configured using:
 'configure --prefix=/home/tromey/Emacs/install/ --with-modules'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LANG: en_US.utf8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  flyspell-mode: t
  which-function-mode: t
  global-auto-revert-mode: t
  erc-services-mode: t
  erc-list-mode: t
  erc-menu-mode: t
  erc-autojoin-mode: t
  erc-ring-mode: t
  erc-networks-mode: t
  erc-pcomplete-mode: t
  erc-track-mode: t
  erc-match-mode: t
  erc-netsplit-mode: t
  erc-hl-nicks-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  savehist-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  auto-fill-function: do-auto-fill
  transient-mark-mode: t

Recent messages:
Discard changes to this group and exit? (y or n) y
Are you sure you want to quit reading news? (y or n) y
Saving Gnus registry (13456 entries) to ~/.gnus.registry.eieio...
Saving Gnus registry (size 13456) to ~/.gnus.registry.eieio...done
Saving file /home/tromey/.newsrc...
Wrote /home/tromey/.newsrc
Saving /home/tromey/.newsrc.eld...
Saving file /home/tromey/.newsrc.eld...
Wrote /home/tromey/.newsrc.eld
Saving /home/tromey/.newsrc.eld...done

Load-path shadows:
/home/tromey/.emacs.d/elpa/bubbles-0.5/bubbles hides /home/tromey/Emacs/install/share/emacs/25.2/lisp/play/bubbles
/home/tromey/.emacs.d/elpa/soap-client-3.1.1/soap-inspect hides /home/tromey/Emacs/install/share/emacs/25.2/lisp/net/soap-inspect
/home/tromey/.emacs.d/elpa/soap-client-3.1.1/soap-client hides /home/tromey/Emacs/install/share/emacs/25.2/lisp/net/soap-client

Features:
(org-bullets org-element org-rmail org-mhe org-irc org-info org-gnus
org-docview doc-view image-mode org-bibtex bibtex org-bbdb org-w3m org
org-macro org-footnote org-pcomplete org-list org-faces org-entities
noutline outline org-version ob-emacs-lisp ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-src ob-keys ob-comint ob-core ob-eval org-compat
org-macs org-loaddefs gnus-html xml url-cache mm-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
gnus-draft xref project mail-hist nnir url-util url-parse url-vars
shr-color shr dom subr-x browse-url flow-fill mm-archive smiley
gnus-cite gnus-async gnus-bcklg qp gnus-ml disp-table gnus-topic nndraft
nnmh nnfolder utf-7 bbdb-gnus bbdb-mua bbdb-com crm gnus-agent gnus-srvr
gnus-score score-mode nnvirtual gnus-msg nntp gnus-cache gnus-registry
registry eieio-compat eieio-base gnus-art mm-uu mml2015 mm-view
mml-smime smime dig gnus-sum gnus-group gnus-undo gnus-start gnus-cloud
nnimap nnmail mail-source utf7 netrc nnoo parse-time gnus-spec gnus-int
gnus-range gnus-win gnus gnus-ems nnheader cus-edit find-dired
bug-reference texinfo vc-mtn vc-hg tabify man term/xterm xterm shadow
emacsbug network-stream nsm starttls tls gnutls mailalias smtpmail sort
mailcap bbdb-message sendmail mail-extr vc-bzr vc-src vc-sccs vc-svn
vc-cvs vc-rcs whitespace log-edit message idna dired rfc822 mml mml-sec
epg mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045
ietf-drums mailabbrev mail-utils gmm-utils mailheader pcvs-util cc-mode
cc-fonts cc-guess cc-menus cc-cmds shell rx dabbrev eieio-opt speedbar
sb-image ezimage dframe find-func copyright debug add-log vc-git
diff-mode easy-mmode misearch multi-isearch jka-compr flyspell ispell
diminish edmacro kmacro projectile grep compile ibuf-ext ibuffer dash
appt diary-lib diary-loaddefs cal-menu calendar cal-loaddefs which-func
imenu minimap autorevert filenotify cus-start cus-load status
erc-services erc-list erc-menu erc-join erc-ring erc-networks
erc-pcomplete pcomplete erc-track erc-match erc-netsplit erc-hl-nicks
color erc-button erc-fill erc-stamp wid-edit erc-goodies erc erc-backend
erc-compat format-spec auth-source eieio gnus-util mm-util help-fns
mail-prsvr password-cache thingatpt pp warnings advice vc-dir ewoc vc
vc-dispatcher cc-styles cc-align cc-engine cc-vars cc-defs bbdb
bbdb-site timezone ange-ftp comint ansi-color ring server savehist
finder-inf dwarf-mode-autoloads gdb-shell-autoloads eieio-core
lisppaste-autoloads pydoc-info-autoloads info-look cl-seq cl-macs cl
weblogger-autoloads info package epg-config seq byte-opt gv bytecomp
byte-compile cl-extra help-mode easymenu cconv cl-loaddefs pcase cl-lib
bbdb-loaddefs time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list newcomment
elisp-mode lisp-mode prog-mode register page menu-bar rfn-eshadow timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
frame cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan
thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian
slovak czech european ethiopic indian cyrillic chinese charscript
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote dbusbind inotify
dynamic-setting system-font-setting font-render-setting move-toolbar gtk
x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 997972 86208)
 (symbols 48 117986 0)
 (miscs 40 21518 46)
 (strings 32 431164 32131)
 (string-bytes 1 10881610)
 (vectors 16 99059)
 (vector-slots 8 2222084 4114)
 (floats 8 851 728)
 (intervals 56 31730 1034)
 (buffers 976 62))





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-05 21:47 bug#25987: 25.2; support gcc fixit notes Tom Tromey
@ 2017-03-06 18:35 ` Eli Zaretskii
  2017-03-07 13:54   ` Tom Tromey
  2017-03-09 16:18 ` Dmitry Gutov
  2018-03-16 16:48 ` David Malcolm
  2 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-06 18:35 UTC (permalink / raw)
  To: Tom Tromey; +Cc: 25987

> From: Tom Tromey <tom@tromey.com>
> Date: Sun, 05 Mar 2017 14:47:57 -0700
> 
> I think Emacs should support this in compilation mode buffers.

I agree, it would be a good feature.

> A few thoughts on the implementation:
> 
> * The filename quoting stuff will require a bit of unquoting

Perhaps shell-unquote-argument will help?

> * The offsets are byte offsets, not columns, which is a bit of a pain
>   (or at least, it is for me since I don't know how to handle that in
>   elisp)

Ouch! why did they do it in bytes?  That's worth a bug report, IMO.

Anyway, filepos-to-bufferpos could help with this problem.

> * One way for this to work would be to display the buffer and 
>   show the proposed change as an overlay;

Or maybe a tooltip on GUI frames?

> * However other approaches are possible, like letting the user request
>   no confirmation, or request "auto-apply all changes" or something.
>   Maybe a fixit should be associated with the previous error somehow.

Is there perhaps some "prior art" for this in other IDEs out there?

Thanks.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-06 18:35 ` Eli Zaretskii
@ 2017-03-07 13:54   ` Tom Tromey
  2017-03-07 15:55     ` Eli Zaretskii
  2017-03-08 18:44     ` Tom Tromey
  0 siblings, 2 replies; 42+ messages in thread
From: Tom Tromey @ 2017-03-07 13:54 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, Tom Tromey

Eli> Ouch! why did they do it in bytes?  That's worth a bug report, IMO.

I'll see if I can find out the rationale.

>> * One way for this to work would be to display the buffer and 
>> show the proposed change as an overlay;

Eli> Or maybe a tooltip on GUI frames?

Yeah.  Or even buttonizing the text so it could be fixed by clicking or
C-RET or something like that.

>> * However other approaches are possible, like letting the user request
>> no confirmation, or request "auto-apply all changes" or something.
>> Maybe a fixit should be associated with the previous error somehow.

Eli> Is there perhaps some "prior art" for this in other IDEs out there?

It's a good question but I don't know the answer.

Tom





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-07 13:54   ` Tom Tromey
@ 2017-03-07 15:55     ` Eli Zaretskii
  2017-03-08 18:34       ` Tom Tromey
  2017-03-08 18:44     ` Tom Tromey
  1 sibling, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-07 15:55 UTC (permalink / raw)
  To: Tom Tromey; +Cc: 25987

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  25987@debbugs.gnu.org
> Date: Tue, 07 Mar 2017 06:54:15 -0700
> 
> Eli> Ouch! why did they do it in bytes?  That's worth a bug report, IMO.
> 
> I'll see if I can find out the rationale.

Thanks.

> >> * However other approaches are possible, like letting the user request
> >> no confirmation, or request "auto-apply all changes" or something.
> >> Maybe a fixit should be associated with the previous error somehow.
> 
> Eli> Is there perhaps some "prior art" for this in other IDEs out there?
> 
> It's a good question but I don't know the answer.

Maybe it's worth asking on emacs-devel.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-07 15:55     ` Eli Zaretskii
@ 2017-03-08 18:34       ` Tom Tromey
  2017-03-08 19:22         ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: Tom Tromey @ 2017-03-08 18:34 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, Tom Tromey

Tom> I'll see if I can find out the rationale.

Eli> Thanks.

The reason is compatibility with clang, which had this option first.
http://releases.llvm.org/3.8.0/tools/clang/docs/UsersManual.html#cmdoption-fdiagnostics-parseable-fixits

Tom





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-07 13:54   ` Tom Tromey
  2017-03-07 15:55     ` Eli Zaretskii
@ 2017-03-08 18:44     ` Tom Tromey
  2017-03-08 19:28       ` Eli Zaretskii
  1 sibling, 1 reply; 42+ messages in thread
From: Tom Tromey @ 2017-03-08 18:44 UTC (permalink / raw)
  To: Tom Tromey; +Cc: 25987

Eli> Is there perhaps some "prior art" for this in other IDEs out there?

Tom> It's a good question but I don't know the answer.

David Malcolm (who wrote the gcc feature) pointed me at the Eclipse bug
for this:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=497670

It sounds like they're going to integrate it into their "quick fix"
feature, which is an Eclipse thing.

They also discuss the need to attach the fixit to an earlier error.

Tom





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-08 18:34       ` Tom Tromey
@ 2017-03-08 19:22         ` Eli Zaretskii
  2017-03-09  4:20           ` Richard Stallman
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-08 19:22 UTC (permalink / raw)
  To: Tom Tromey; +Cc: 25987

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  25987@debbugs.gnu.org
> Date: Wed, 08 Mar 2017 11:34:38 -0700
> 
> Tom> I'll see if I can find out the rationale.
> 
> Eli> Thanks.
> 
> The reason is compatibility with clang, which had this option first.
> http://releases.llvm.org/3.8.0/tools/clang/docs/UsersManual.html#cmdoption-fdiagnostics-parseable-fixits

Thanks.  So maybe GCC should have a similar option, named differently,
which would report in characters.

Anyway, "we have the technology" in Emacs to deal with this.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-08 18:44     ` Tom Tromey
@ 2017-03-08 19:28       ` Eli Zaretskii
  2017-03-09 16:37         ` Dmitry Gutov
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-08 19:28 UTC (permalink / raw)
  To: Tom Tromey; +Cc: 25987

> From: Tom Tromey <tom@tromey.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  25987@debbugs.gnu.org
> Date: Wed, 08 Mar 2017 11:44:59 -0700
> 
> David Malcolm (who wrote the gcc feature) pointed me at the Eclipse bug
> for this:
> 
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=497670
> 
> It sounds like they're going to integrate it into their "quick fix"
> feature, which is an Eclipse thing.

Thanks.

Quick fixes work by presenting a kind of tooltip.  We should probably
have something similar, but it cannot be the only UI, to support TTY
and GUI users who don't like the mouse.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-08 19:22         ` Eli Zaretskii
@ 2017-03-09  4:20           ` Richard Stallman
  2017-03-09 15:36             ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: Richard Stallman @ 2017-03-09  4:20 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, tom

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > The reason is compatibility with clang, which had this option first.
  > > http://releases.llvm.org/3.8.0/tools/clang/docs/UsersManual.html#cmdoption-fdiagnostics-parseable-fixits

  > Thanks.  So maybe GCC should have a similar option, named differently,
  > which would report in characters.

I am not sure whether that is serious or in jest.
Do you have a serious suggestion for a change in GCC?
If so, what would it be?

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09  4:20           ` Richard Stallman
@ 2017-03-09 15:36             ` Eli Zaretskii
  0 siblings, 0 replies; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-09 15:36 UTC (permalink / raw)
  To: rms; +Cc: 25987, tom

> From: Richard Stallman <rms@gnu.org>
> CC: tom@tromey.com, 25987@debbugs.gnu.org
> Date: Wed, 08 Mar 2017 23:20:20 -0500
> 
>   > > The reason is compatibility with clang, which had this option first.
>   > > http://releases.llvm.org/3.8.0/tools/clang/docs/UsersManual.html#cmdoption-fdiagnostics-parseable-fixits
> 
>   > Thanks.  So maybe GCC should have a similar option, named differently,
>   > which would report in characters.
> 
> I am not sure whether that is serious or in jest.

It's serious.

> Do you have a serious suggestion for a change in GCC?
> If so, what would it be?

Like I wrote: I think GCC should introduce a (differently-named)
option which would report positions inside a line in terms of
characters, not bytes.  Reports in bytes require applications that use
these hints to somehow preserve or reconstruct the original encoding
of the file as it was presented to the compiler, something that could
be a pain.

I understand why GCC developers wanted to be bug-compatible with
clang, to allow the various front-ends use GCC without any changes,
but it sounds wrong for GCC to be forever stuck with this bad design
decision made by clang.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-05 21:47 bug#25987: 25.2; support gcc fixit notes Tom Tromey
  2017-03-06 18:35 ` Eli Zaretskii
@ 2017-03-09 16:18 ` Dmitry Gutov
  2017-03-09 16:53   ` Eli Zaretskii
  2017-08-06  3:31   ` Tom Tromey
  2018-03-16 16:48 ` David Malcolm
  2 siblings, 2 replies; 42+ messages in thread
From: Dmitry Gutov @ 2017-03-09 16:18 UTC (permalink / raw)
  To: Tom Tromey, 25987

On 05.03.2017 23:47, Tom Tromey wrote:

> * One way for this to work would be to display the buffer and
>    show the proposed change as an overlay; and then use y-or-n-p
>    to ask whether it should be applied.  I was thinking something like:
> 
> (defun compilation--fixit-make-overlay (start end text)
>    (let ((overlay (make-overlay start end)))
>      (overlay-put overlay 'face 'diff-removed)
>      (overlay-put overlay 'after-string
>                   (propertize text 'face 'diff-added))
>      overlay))
> 
>    With this approach perhaps nothing could be done if the fixit was
>    already visited, or if the buffer text already matches the
>    replacement.

I'm not sure we want to tie this feature to compilation-mode. Many modes 
that derive from it don't support fix-its, e.g. those of them that run 
the test suites. And even for those that do, Compilation provides a very 
basic UI.

Even to find and apply all fix-its, we'd need to add some new buffer, to 
show the list.

On the other hand, this can be a quick-and-dirty way to try out the 
feature and how handy it is. Showing the "quick fix" buttons on top of 
the fix-it text can be it (not sure it that's the UI you had in mind).





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-08 19:28       ` Eli Zaretskii
@ 2017-03-09 16:37         ` Dmitry Gutov
  2017-03-09 16:56           ` Eli Zaretskii
  2017-08-06  3:34           ` Tom Tromey
  0 siblings, 2 replies; 42+ messages in thread
From: Dmitry Gutov @ 2017-03-09 16:37 UTC (permalink / raw)
  To: Eli Zaretskii, Tom Tromey; +Cc: 25987

On 08.03.2017 21:28, Eli Zaretskii wrote:

> Quick fixes work by presenting a kind of tooltip.  We should probably
> have something similar, but it cannot be the only UI, to support TTY
> and GUI users who don't like the mouse.

Tooltip or not, that's a visualization concern.

I think a better first question would be where this feature would fit 
among the existing Emacs tools.

If we look at Eclipse and friends, we can observe that (almost?) every 
warning or error that the IDE shows to the user comes with a set of 
actions, suggesting possible fixes, improvements, etc.

Thinking along these lines, the closest things we have are Flymake and 
Flyspell. The third-party Flycheck is even better is several respects 
(e.g. it can show a listing of all problems in the file buffer), but 
sadly we won't be able to use it, since the primary author is sternly 
against copyright assignment.

But it'd be good to teach Flymake about fixits, and when we can't use 
tooltips, use a text-based prompts a la Flyspell.

Bonus points for teaching Flymake to use multiple checkers in one file, 
and making Flyspell one of them.

I guess the first difficulty is how to reconcile project-global 
compilation logs with buffer-local warnings and errors.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 16:18 ` Dmitry Gutov
@ 2017-03-09 16:53   ` Eli Zaretskii
  2017-03-09 17:49     ` Dmitry Gutov
  2017-08-06  3:31   ` Tom Tromey
  1 sibling, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-09 16:53 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 25987, tom

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 9 Mar 2017 18:18:04 +0200
> 
> I'm not sure we want to tie this feature to compilation-mode. Many modes 
> that derive from it don't support fix-its, e.g. those of them that run 
> the test suites. And even for those that do, Compilation provides a very 
> basic UI.

So you suggest a separate minor mode, is that right?





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 16:37         ` Dmitry Gutov
@ 2017-03-09 16:56           ` Eli Zaretskii
  2017-03-09 17:37             ` Dmitry Gutov
  2017-08-06  3:34           ` Tom Tromey
  1 sibling, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-09 16:56 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 25987, tom

> Cc: 25987@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 9 Mar 2017 18:37:30 +0200
> 
> If we look at Eclipse and friends, we can observe that (almost?) every 
> warning or error that the IDE shows to the user comes with a set of 
> actions, suggesting possible fixes, improvements, etc.
> 
> Thinking along these lines, the closest things we have are Flymake and 
> Flyspell. The third-party Flycheck is even better is several respects 
> (e.g. it can show a listing of all problems in the file buffer), but 
> sadly we won't be able to use it, since the primary author is sternly 
> against copyright assignment.

It sounds gross to ask people to use Flymake just to be able to use
something like this.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 16:56           ` Eli Zaretskii
@ 2017-03-09 17:37             ` Dmitry Gutov
  2017-03-09 18:32               ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: Dmitry Gutov @ 2017-03-09 17:37 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, tom

On 09.03.2017 18:56, Eli Zaretskii wrote:

> It sounds gross to ask people to use Flymake just to be able to use
> something like this.

What's gross about that? Don't we like Flymake?





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 16:53   ` Eli Zaretskii
@ 2017-03-09 17:49     ` Dmitry Gutov
  2017-03-09 18:35       ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: Dmitry Gutov @ 2017-03-09 17:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, tom

On 09.03.2017 18:53, Eli Zaretskii wrote:

> So you suggest a separate minor mode, is that right?

A minor mode would be a good way toward the "quick-and-dirty" solution.

But we'd still want in-buffer indicators and a way to promptly 
invalidate them as soon as the user makes some changes.

Ideally, also a way to refresh them automatically (e.g. as soon as the 
user saves the changes). These requirements spell "Flymake" to me.






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 17:37             ` Dmitry Gutov
@ 2017-03-09 18:32               ` Eli Zaretskii
  2017-03-09 21:26                 ` Dmitry Gutov
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-09 18:32 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 25987, tom

> Cc: 25987@debbugs.gnu.org, tom@tromey.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 9 Mar 2017 19:37:45 +0200
> 
> On 09.03.2017 18:56, Eli Zaretskii wrote:
> 
> > It sounds gross to ask people to use Flymake just to be able to use
> > something like this.
> 
> What's gross about that?

It's a large package, and its purpose is not just fix compilation
errors.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 17:49     ` Dmitry Gutov
@ 2017-03-09 18:35       ` Eli Zaretskii
  0 siblings, 0 replies; 42+ messages in thread
From: Eli Zaretskii @ 2017-03-09 18:35 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 25987, tom

> Cc: tom@tromey.com, 25987@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 9 Mar 2017 19:49:19 +0200
> 
> On 09.03.2017 18:53, Eli Zaretskii wrote:
> 
> > So you suggest a separate minor mode, is that right?
> 
> A minor mode would be a good way toward the "quick-and-dirty" solution.
> 
> But we'd still want in-buffer indicators and a way to promptly 
> invalidate them as soon as the user makes some changes.
> 
> Ideally, also a way to refresh them automatically (e.g. as soon as the 
> user saves the changes). These requirements spell "Flymake" to me.

We could do both, of course.  Or Flymake could turn on that minor
mode.






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 18:32               ` Eli Zaretskii
@ 2017-03-09 21:26                 ` Dmitry Gutov
  0 siblings, 0 replies; 42+ messages in thread
From: Dmitry Gutov @ 2017-03-09 21:26 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, tom

On 09.03.2017 20:32, Eli Zaretskii wrote:

>>> It sounds gross to ask people to use Flymake just to be able to use
>>> something like this.
>>
>> What's gross about that?
> 
> It's a large package, and its purpose is not just fix compilation
> errors.

Its purpose is just showing them, and we, ideally, need both.

Flymake is moderately large because it includes some checker 
definitions. We could extract the core to a separate file.

Anyway, yes, a minor mode that would act on the fixits inside the 
Compilation buffer might be helpful.

But that's basically ignoring the prior art in IDEs that you asked 
about, where "quick fixes" go together with in-buffer visualizations.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 16:18 ` Dmitry Gutov
  2017-03-09 16:53   ` Eli Zaretskii
@ 2017-08-06  3:31   ` Tom Tromey
  1 sibling, 0 replies; 42+ messages in thread
From: Tom Tromey @ 2017-08-06  3:31 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 25987, Tom Tromey

>>>>> "Dmitry" == Dmitry Gutov <dgutov@yandex.ru> writes:

>> * One way for this to work would be to display the buffer and
>> show the proposed change as an overlay; and then use y-or-n-p
>> to ask whether it should be applied.  I was thinking something like:
[...]

Dmitry> I'm not sure we want to tie this feature to compilation-mode. Many
Dmitry> modes that derive from it don't support fix-its, e.g. those of them
Dmitry> that run the test suites. And even for those that do, Compilation
Dmitry> provides a very basic UI.

To me it seems very natural.  I compile, then as I next-error through
the results, Emacs offers to apply a change.

I think it's fine that some sub-modes don't support fix-its.  Actually I
don't understand how that would be a problem.  Presumably `grep' or
whatever isn't going to emit a fixit note anyhow.

Dmitry> Even to find and apply all fix-its, we'd need to add some new buffer,
Dmitry> to show the list.

That doesn't seem necessary to me either.

Tom





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-09 16:37         ` Dmitry Gutov
  2017-03-09 16:56           ` Eli Zaretskii
@ 2017-08-06  3:34           ` Tom Tromey
  1 sibling, 0 replies; 42+ messages in thread
From: Tom Tromey @ 2017-08-06  3:34 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 25987, Tom Tromey

>>>>> "Dmitry" == Dmitry Gutov <dgutov@yandex.ru> writes:

Dmitry> Thinking along these lines, the closest things we have are Flymake and
Dmitry> Flyspell. The third-party Flycheck is even better is several respects
Dmitry> (e.g. it can show a listing of all problems in the file buffer), but
Dmitry> sadly we won't be able to use it, since the primary author is sternly
Dmitry> against copyright assignment.

It'd be great to evolve Flymake into something more like Flycheck.

Dmitry> But it'd be good to teach Flymake about fixits, and when we can't use
Dmitry> tooltips, use a text-based prompts a la Flyspell.

I think this would be a good addition, kind of like LSP mode; but also
not really necessary -- at least for me, I usually use M-x compile for C
and C++ projects, and some integration with next-error would be more
than sufficient.

Tom





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2017-03-05 21:47 bug#25987: 25.2; support gcc fixit notes Tom Tromey
  2017-03-06 18:35 ` Eli Zaretskii
  2017-03-09 16:18 ` Dmitry Gutov
@ 2018-03-16 16:48 ` David Malcolm
  2018-03-16 20:19   ` Eli Zaretskii
  2 siblings, 1 reply; 42+ messages in thread
From: David Malcolm @ 2018-03-16 16:48 UTC (permalink / raw)
  To: 25987

[I'm not familiar with this bug-tracking system; let's see if this
works]

I'm the author of the -fdiagnostics-parseable-fixits feature in GCC; I
maintain the GCC diagnostics subsystem.

As Tom(?) noted, I made -fdiagnostics-parseable-fixits report in bytes,
partly because that's the status quo for all of GCC's "column"
reporting, and partly because that's what clang implemented for this. 
It appears that Emacs can already decode from byte-offsets in a line
back to columns, so this seems like a compatibility detail in case we
fix everything to use real column numbers.

I'm also an Emacs user (though not a Emacs developer, sorry; my Lisp is
practically non-existent) - I just wondered what the status of this is.
I'm keen to be able to use it (GCC will get better faster!), even if
it's just a new command in compilation mode to go ahead and apply a
fix-it hint for the current diagnostic, as seen by next-error, if it
has one, maybe prettifying the printing of such lines, with the user
having to manually supply -fdiagnostics-parseable-fixits somehow.

FWIW GCC 8 is gaining lots more fix-it hints (including ones that add
newlines, for missing "#include" suggestions); see:

https://developers.redhat.com/blog/2018/03/15/gcc-8-usability-improvements/

for a blog post I wrote about it.

Thanks; hope this is constructive.
Dave






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2018-03-16 16:48 ` David Malcolm
@ 2018-03-16 20:19   ` Eli Zaretskii
  2020-10-06 18:17     ` David Malcolm
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2018-03-16 20:19 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987

> From: David Malcolm <dmalcolm@redhat.com>
> Date: Fri, 16 Mar 2018 12:48:07 -0400
> 
> It appears that Emacs can already decode from byte-offsets in a line
> back to columns

Actually, doing this translation is a pain, because it requires to
know the original encoding used by GCC for its messages, and that info
is gone by the time we have the text decoded in an Emacs buffer.
Emacs can guess the encoding, but that guesswork is bound to fail.  So
reporting the offsets in characters (a.k.a. columns) is really a
necessity.

Thanks.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2018-03-16 20:19   ` Eli Zaretskii
@ 2020-10-06 18:17     ` David Malcolm
  2020-10-06 18:37       ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: David Malcolm @ 2020-10-06 18:17 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987

On Fri, 2018-03-16 at 22:19 +0200, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Date: Fri, 16 Mar 2018 12:48:07 -0400
> > 
> > It appears that Emacs can already decode from byte-offsets in a
> > line
> > back to columns
> 
> Actually, doing this translation is a pain, because it requires to
> know the original encoding used by GCC for its messages, and that
> info
> is gone by the time we have the text decoded in an Emacs buffer.
> Emacs can guess the encoding, but that guesswork is bound to
> fail.  So
> reporting the offsets in characters (a.k.a. columns) is really a
> necessity.
> 
> Thanks.

Sorry for the long delay in responding.

In GCC 11 we've revamped the column number handling in how we emit
diagnostics; see:
  https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555632.html

GCC 11 diagnostics now (by default) should use actual column numbers,
rather than byte counts.

We haven't changed -fdiagnostics-parseable-fixits; it still emits its
range information in terms of byte offsets (and e.g. Eclipse already
consumes that option); this is bug-for-bug compatible with clang, I
believe (which had that option first).

Note that characters != columns, or, at least, we have to be careful
about what we mean.  Consider the case of 🙂 aka SLIGHTLY SMILING FACE
(U+1F642) which is a single unicode code point, but occupies two
columns, with its UTF-8 encoding requiring four bytes.

When I type it into an Emacs buffer, and look at the column number I
see that Emacs appears to treat the character as occupying two columns.
 Is that the kind of column numbering you would want for machine-
readable fix-it hints?

Similarly, the column numbering emitted by GCC 11 diagnostics is
affected by tab characters as tab stops, which seems to reflect Emacs
behavior as well.

I can add an additional option for fix-it hints that's similar to
-fdiagnostics-parseable-fixits, but with a different output format (or
to add an argument to that option, perhaps).

Before I do that, I wanted to check that it would be consumable by
Emacs.  What works for you?  Would it help to send the proposed GCC
patch to this bug address (or to the emacs-devel list?).

The existing option is documented here:
https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Message-Formatting-Options.html#index-fdiagnostics-parseable-fixits

Alternatively, we already have a JSON output option (-fdiagnostics-
format=json); perhaps something like that could be used?


Feature freeze for GCC 11 is about a month away; I'd love for Emacs to
be able to consume GCC fix-it hints (and have GCC and Emacs fix my
typos for me)

Dave






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-06 18:17     ` David Malcolm
@ 2020-10-06 18:37       ` Eli Zaretskii
  2020-10-12 22:27         ` David Malcolm
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2020-10-06 18:37 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987

> From: David Malcolm <dmalcolm@redhat.com>
> Cc: 25987@debbugs.gnu.org
> Date: Tue, 06 Oct 2020 14:17:55 -0400
> 
> In GCC 11 we've revamped the column number handling in how we emit
> diagnostics; see:
>   https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555632.html
> 
> GCC 11 diagnostics now (by default) should use actual column numbers,
> rather than byte counts.

That's good news, thanks.

> We haven't changed -fdiagnostics-parseable-fixits; it still emits its
> range information in terms of byte offsets (and e.g. Eclipse already
> consumes that option); this is bug-for-bug compatible with clang, I
> believe (which had that option first).

So fixit hints will still count bytes?  Or will GCC 11 emit such
hints even without -fdiagnostics-parseable-fixits on the command line?

> Note that characters != columns, or, at least, we have to be careful
> about what we mean.  Consider the case of 🙂 aka SLIGHTLY SMILING FACE
> (U+1F642) which is a single unicode code point, but occupies two
> columns, with its UTF-8 encoding requiring four bytes.
> 
> When I type it into an Emacs buffer, and look at the column number I
> see that Emacs appears to treat the character as occupying two columns.
>  Is that the kind of column numbering you would want for machine-
> readable fix-it hints?

Yes.  Emacs computes the width of each character by using UCD, the
Unicode Character Database (specifically, the EastAsianWidth.txt file
that is part of the UCD).  If GCC gets its column counts from a
similar DB, then it will match what Emacs does.

> Similarly, the column numbering emitted by GCC 11 diagnostics is
> affected by tab characters as tab stops, which seems to reflect Emacs
> behavior as well.

Yes.

> I can add an additional option for fix-it hints that's similar to
> -fdiagnostics-parseable-fixits, but with a different output format (or
> to add an argument to that option, perhaps).

If an option is needed for getting the hints, then a special option
which reports columns in hints will be appreciated, as it will make
the Emacs support for processing those hints 100% accurate and devoid
of encoding guesswork.

> Before I do that, I wanted to check that it would be consumable by
> Emacs.  What works for you?  Would it help to send the proposed GCC
> patch to this bug address (or to the emacs-devel list?).

I don't know how many people here build their own GCC, and thus could
try the patch, but if sending the patch is not too much trouble,
perhaps posting it on emacs-devel would be a good idea.  If you do
that, please cite this bug report, so that people who try that could
respond here with their experience.

> Alternatively, we already have a JSON output option (-fdiagnostics-
> format=json); perhaps something like that could be used?

Emacs can parse JSON.  What are the pros and cons of the JSON
alternative wrt to the text alternative?

> Feature freeze for GCC 11 is about a month away; I'd love for Emacs to
> be able to consume GCC fix-it hints (and have GCC and Emacs fix my
> typos for me)

Agreed; let's try to make that happen.

Thanks.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-06 18:37       ` Eli Zaretskii
@ 2020-10-12 22:27         ` David Malcolm
  2020-10-13  7:34           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-13 14:37           ` Eli Zaretskii
  0 siblings, 2 replies; 42+ messages in thread
From: David Malcolm @ 2020-10-12 22:27 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987

[-- Attachment #1: Type: text/plain, Size: 5228 bytes --]

On Tue, 2020-10-06 at 21:37 +0300, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Cc: 25987@debbugs.gnu.org
> > Date: Tue, 06 Oct 2020 14:17:55 -0400
> > 
> > In GCC 11 we've revamped the column number handling in how we emit
> > diagnostics; see:
> >   
> > https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555632.html
> > 
> > GCC 11 diagnostics now (by default) should use actual column
> > numbers,
> > rather than byte counts.
> 
> That's good news, thanks.
> 
> > We haven't changed -fdiagnostics-parseable-fixits; it still emits
> > its
> > range information in terms of byte offsets (and e.g. Eclipse
> > already
> > consumes that option); this is bug-for-bug compatible with clang, I
> > believe (which had that option first).
> 
> So fixit hints will still count bytes?  Or will GCC 11 emit such
> hints even without -fdiagnostics-parseable-fixits on the command
> line?
> 
> > Note that characters != columns, or, at least, we have to be
> > careful
> > about what we mean.  Consider the case of 🙂 aka SLIGHTLY SMILING
> > FACE
> > (U+1F642) which is a single unicode code point, but occupies two
> > columns, with its UTF-8 encoding requiring four bytes.
> > 
> > When I type it into an Emacs buffer, and look at the column number
> > I
> > see that Emacs appears to treat the character as occupying two
> > columns.
> >  Is that the kind of column numbering you would want for machine-
> > readable fix-it hints?
> 
> Yes.  Emacs computes the width of each character by using UCD, the
> Unicode Character Database (specifically, the EastAsianWidth.txt file
> that is part of the UCD).  If GCC gets its column counts from a
> similar DB, then it will match what Emacs does.
> 
> > Similarly, the column numbering emitted by GCC 11 diagnostics is
> > affected by tab characters as tab stops, which seems to reflect
> > Emacs
> > behavior as well.
> 
> Yes.
> 
> > I can add an additional option for fix-it hints that's similar to
> > -fdiagnostics-parseable-fixits, but with a different output format
> > (or
> > to add an argument to that option, perhaps).
> 
> If an option is needed for getting the hints, then a special option
> which reports columns in hints will be appreciated, as it will make
> the Emacs support for processing those hints 100% accurate and devoid
> of encoding guesswork.
> 
> > Before I do that, I wanted to check that it would be consumable by
> > Emacs.  What works for you?  Would it help to send the proposed GCC
> > patch to this bug address (or to the emacs-devel list?).
> 
> I don't know how many people here build their own GCC, and thus could
> try the patch, but if sending the patch is not too much trouble,
> perhaps posting it on emacs-devel would be a good idea.  If you do
> that, please cite this bug report, so that people who try that could
> respond here with their experience.
> 
> > Alternatively, we already have a JSON output option (-fdiagnostics-
> > format=json); perhaps something like that could be used?
> 
> Emacs can parse JSON.  What are the pros and cons of the JSON
> alternative wrt to the text alternative?

The existing "-fdiagnostics-format=json" GCC option replaces the
existing diagnostic output with a big blob of JSON to stderr, all on
one line.  Although I implemented it, I now feel it to be rather half-
baked.

I like how -fdiagnostics-parseable-fixits adds extra lines of output,
with a prefix that's ought to be easy to detect.

BTW, does Emacs set anything in the environment that GCC could detect?

> > Feature freeze for GCC 11 is about a month away; I'd love for Emacs
> > to
> > be able to consume GCC fix-it hints (and have GCC and Emacs fix my
> > typos for me)
> 
> Agreed; let's try to make that happen.

I put together a test file showing various features to try to ensure
that GCC and Emacs interoperate on this.  I'm attaching it.

This can also be seen on Compiler Explorer at:
  https://godbolt.org/z/zazejq
which adds the existing
  -fdiagnostics-parseable-fixits -fdiagnostics-generate-patch
options.

Ideas for other test cases are most welcome.

Does Emacs have an automated test suite that could test this feature?
A long-postponed goal for me for GCC's testsuite is to ensure that fix-
it hints apply and actually fix the problem (clang's testsuite has
tests that verify this for clang's fix-it hints).

Another complication to consider: the locations in a fix-it hint refer
to the original source file, before any changes are made.  If the user
interface supports the user e.g. clicking on fix-it hints and
selectively apply them one by one, then after each fix-it hint is
applied, all locations after that point potentially need to be "fixed
up" somehow, to reflect the changes to the buffer.  GCC's own code
implements this in gcc/edit-context.{c|h}, for implementing -
fdiagnostics-generate-patch, which applies all fix-its "atomically" -
but the use-case I thinking of involves clicking on fix-it hints one-
by-one in the compilation buffer, perhaps replacing the fix-it output
with an "Apply fix" button.

(to cover this case I made sure the demo file contained examples of a
fix-it hint that adds a line, and one that has multiple fix-it hints on
the same line).


Thoughts?

Dave

[-- Attachment #2: demo.c --]
[-- Type: text/x-csrc, Size: 1048 bytes --]

/* A dummy include, to offset the fix-it include in test_1 below.  */
#include <stdio.h>

struct vert
{
  int color;
};

void *test_1 (void)
{
  /* -Wimplicit-function-declaration should generate a fix-it hint that
     adds a whole new line suggesting a #include, added after the
     existing #include.  */
  return malloc (4096);
}

void test_2a (struct vert *v)
{
  /* Some non-ASCII text preceding a fix-it hint.  */
  /* 🙂 🙂 🙂 */ v->colour = 0;
}

void test_2b (struct vert *v)
{
  /* Some other non-ASCII text preceding a fix-it hint.  */
  /* π π π */ v->colour = 0;
}

void test_2c (struct vert *v)
{
  /* Yet more non-ASCII text preceding a fix-it hint.  */
  /* 文字化け */ v->colour = 0;
}

void test_3 (struct vert *v)
{
	  /* Tabs and spaces preceding a fix-it hint.  */
	  v->colour = 0;
}

void test_4 (struct vert *v)
{
  /* More than one fix-it on the same line.  */
  if (v->colour) v->colour += 2;
}

double test_5 (void)
{
  /* Non-ASCII replacement.  */
  const double two_π = (3.141 * 2);
  return two_pi;
}

^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-12 22:27         ` David Malcolm
@ 2020-10-13  7:34           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-13 14:37           ` Eli Zaretskii
  1 sibling, 0 replies; 42+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-13  7:34 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987, Eli Zaretskii

David Malcolm <dmalcolm@redhat.com> writes:

> On Tue, 2020-10-06 at 21:37 +0300, Eli Zaretskii wrote:
>> > From: David Malcolm <dmalcolm@redhat.com>
>> > Cc: 25987@debbugs.gnu.org
>> > Date: Tue, 06 Oct 2020 14:17:55 -0400
>> > 
>> > In GCC 11 we've revamped the column number handling in how we emit
>> > diagnostics; see:
>> >   
>> > https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555632.html
>> > 
>> > GCC 11 diagnostics now (by default) should use actual column
>> > numbers,
>> > rather than byte counts.
>> 
>> That's good news, thanks.
>> 
>> > We haven't changed -fdiagnostics-parseable-fixits; it still emits
>> > its
>> > range information in terms of byte offsets (and e.g. Eclipse
>> > already
>> > consumes that option); this is bug-for-bug compatible with clang, I
>> > believe (which had that option first).
>> 
>> So fixit hints will still count bytes?  Or will GCC 11 emit such
>> hints even without -fdiagnostics-parseable-fixits on the command
>> line?
>> 
>> > Note that characters != columns, or, at least, we have to be
>> > careful
>> > about what we mean.  Consider the case of 🙂 aka SLIGHTLY SMILING
>> > FACE
>> > (U+1F642) which is a single unicode code point, but occupies two
>> > columns, with its UTF-8 encoding requiring four bytes.
>> > 
>> > When I type it into an Emacs buffer, and look at the column number
>> > I
>> > see that Emacs appears to treat the character as occupying two
>> > columns.
>> >  Is that the kind of column numbering you would want for machine-
>> > readable fix-it hints?
>> 
>> Yes.  Emacs computes the width of each character by using UCD, the
>> Unicode Character Database (specifically, the EastAsianWidth.txt file
>> that is part of the UCD).  If GCC gets its column counts from a
>> similar DB, then it will match what Emacs does.
>> 
>> > Similarly, the column numbering emitted by GCC 11 diagnostics is
>> > affected by tab characters as tab stops, which seems to reflect
>> > Emacs
>> > behavior as well.
>> 
>> Yes.
>> 
>> > I can add an additional option for fix-it hints that's similar to
>> > -fdiagnostics-parseable-fixits, but with a different output format
>> > (or
>> > to add an argument to that option, perhaps).
>> 
>> If an option is needed for getting the hints, then a special option
>> which reports columns in hints will be appreciated, as it will make
>> the Emacs support for processing those hints 100% accurate and devoid
>> of encoding guesswork.
>> 
>> > Before I do that, I wanted to check that it would be consumable by
>> > Emacs.  What works for you?  Would it help to send the proposed GCC
>> > patch to this bug address (or to the emacs-devel list?).
>> 
>> I don't know how many people here build their own GCC, and thus could
>> try the patch, but if sending the patch is not too much trouble,
>> perhaps posting it on emacs-devel would be a good idea.  If you do
>> that, please cite this bug report, so that people who try that could
>> respond here with their experience.
>> 
>> > Alternatively, we already have a JSON output option (-fdiagnostics-
>> > format=json); perhaps something like that could be used?
>> 
>> Emacs can parse JSON.  What are the pros and cons of the JSON
>> alternative wrt to the text alternative?
>
> The existing "-fdiagnostics-format=json" GCC option replaces the
> existing diagnostic output with a big blob of JSON to stderr, all on
> one line.  Although I implemented it, I now feel it to be rather half-
> baked.
>
> I like how -fdiagnostics-parseable-fixits adds extra lines of output,
> with a prefix that's ought to be easy to detect.
>
> BTW, does Emacs set anything in the environment that GCC could detect?

Hi Dave,

If we are searching for a way to ask GCC to dump a file instead of
logging to sterr I think Emacs could easily set the env variable before
invoking the compilation.  Which one I think is to be agreed as would be
probably used by a flymake back-end or same other special case as the
one discussed.

>> > Feature freeze for GCC 11 is about a month away; I'd love for Emacs
>> > to
>> > be able to consume GCC fix-it hints (and have GCC and Emacs fix my
>> > typos for me)
>> 
>> Agreed; let's try to make that happen.
>
> I put together a test file showing various features to try to ensure
> that GCC and Emacs interoperate on this.  I'm attaching it.
>
> This can also be seen on Compiler Explorer at:
>   https://godbolt.org/z/zazejq
> which adds the existing
>   -fdiagnostics-parseable-fixits -fdiagnostics-generate-patch
> options.
>
> Ideas for other test cases are most welcome.
>
> Does Emacs have an automated test suite that could test this feature?
> A long-postponed goal for me for GCC's testsuite is to ensure that fix-
> it hints apply and actually fix the problem (clang's testsuite has
> tests that verify this for clang's fix-it hints).

Yes we have a testsuite, we could have a test that runs only with like
GCC versions >= 11 or keep in the testsuite some pre cooked GCC output.
Some tests should be easy to write.

  Andrea





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-12 22:27         ` David Malcolm
  2020-10-13  7:34           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-13 14:37           ` Eli Zaretskii
  2020-10-14 22:43             ` David Malcolm
  2020-10-20 14:52             ` David Malcolm
  1 sibling, 2 replies; 42+ messages in thread
From: Eli Zaretskii @ 2020-10-13 14:37 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987

> From: David Malcolm <dmalcolm@redhat.com>
> Cc: 25987@debbugs.gnu.org
> Date: Mon, 12 Oct 2020 18:27:29 -0400
> 
> I like how -fdiagnostics-parseable-fixits adds extra lines of output,
> with a prefix that's ought to be easy to detect.

Yes, I think that's preferable, indeed.

> BTW, does Emacs set anything in the environment that GCC could detect?

Yes, Emacs sets INSIDE_EMACS when it runs a sub-process.

> Does Emacs have an automated test suite that could test this feature?

Yes, see the tests in the test/ subdirectory of the Emacs tree.

> Another complication to consider: the locations in a fix-it hint refer
> to the original source file, before any changes are made.  If the user
> interface supports the user e.g. clicking on fix-it hints and
> selectively apply them one by one, then after each fix-it hint is
> applied, all locations after that point potentially need to be "fixed
> up" somehow, to reflect the changes to the buffer.

That could be handled automatically by defining a marker at each
hint's location, then they will move as text is edited.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-13 14:37           ` Eli Zaretskii
@ 2020-10-14 22:43             ` David Malcolm
  2020-10-15  7:47               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-15 13:53               ` Eli Zaretskii
  2020-10-20 14:52             ` David Malcolm
  1 sibling, 2 replies; 42+ messages in thread
From: David Malcolm @ 2020-10-14 22:43 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987

On Tue, 2020-10-13 at 17:37 +0300, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Cc: 25987@debbugs.gnu.org
> > Date: Mon, 12 Oct 2020 18:27:29 -0400
> > 
> > I like how -fdiagnostics-parseable-fixits adds extra lines of
> > output,
> > with a prefix that's ought to be easy to detect.
> 
> Yes, I think that's preferable, indeed.

(nods)

> > BTW, does Emacs set anything in the environment that GCC could
> > detect?
> 
> Yes, Emacs sets INSIDE_EMACS when it runs a sub-process.

I'm increasingly warming to the idea of having it be enabled by an
environment variable, rather than a command-line option.

My thinking is that it's a pain to have to set up the build system to
inject a particular command-line option that's intended purely for
consumption by an IDE.  As an environment variable, the IDE can inject
it into the environment, regardless of whatever build system is in use,
and then consume the extra output.

(For reference GCC's existing environment variables are listed here:
https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html )

Something like GCC_EXTRA_DIAGNOSTIC_SOMETHING=foo, for some SOMETHING
and some foo?

I thought about maybe emitting JSON, which would be extendable.  But it
would have to be all on one line, which is less optimal, and would be
much more verbose than the existing format.

So maybe just use the existing format, but fixing it so that columns
are columns, as discussed.

So perhaps a fix-it aware Emacs mode could set this when compiling:

  GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v2

(v2 since v1 is the existing format from the cmdline option)

In his email, Andrea suggests outputting to a file.  How would 
that work?  It strikes me as making it difficult to associate the
output from stderr with that to the file, or would the output to the
file need to replace that from stderr (in which case what about lines
of output from "make"? or other build system messages, etc).

Arguably these various approaches are a poor-man's version of a
language server (I tried implementing that for GCC a few years ago, but
my proof of concept was not successful:
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01448.html )

> > Does Emacs have an automated test suite that could test this
> > feature?
> 
> Yes, see the tests in the test/ subdirectory of the Emacs tree.

(nods)

> > Another complication to consider: the locations in a fix-it hint
> > refer
> > to the original source file, before any changes are made.  If the
> > user
> > interface supports the user e.g. clicking on fix-it hints and
> > selectively apply them one by one, then after each fix-it hint is
> > applied, all locations after that point potentially need to be
> > "fixed
> > up" somehow, to reflect the changes to the buffer.
> 
> That could be handled automatically by defining a marker at each
> hint's location, then they will move as text is edited.

With the caveat that I'm a mere user of Emacs, that sounds reasonable.
(I mentioned it more as a complication I thought of that whoever
implements this on the Emacs side needs to be aware of).


Thoughts?

Dave






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-14 22:43             ` David Malcolm
@ 2020-10-15  7:47               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-01-14 21:37                 ` David Malcolm
  2020-10-15 13:53               ` Eli Zaretskii
  1 sibling, 1 reply; 42+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-15  7:47 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987, Eli Zaretskii

David Malcolm <dmalcolm@redhat.com> writes:

[...]

>> 
>> Yes, Emacs sets INSIDE_EMACS when it runs a sub-process.
>
> I'm increasingly warming to the idea of having it be enabled by an
> environment variable, rather than a command-line option.
>
> My thinking is that it's a pain to have to set up the build system to
> inject a particular command-line option that's intended purely for
> consumption by an IDE.  As an environment variable, the IDE can inject
> it into the environment, regardless of whatever build system is in use,
> and then consume the extra output.
>
> (For reference GCC's existing environment variables are listed here:
> https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html )
>
> Something like GCC_EXTRA_DIAGNOSTIC_SOMETHING=foo, for some SOMETHING
> and some foo?

Yes, INSIDE_EMACS is not an option if we want to have GCC to depose
special files, would work if is just a matter of changing format but I
don't think this is the case (more on this below).

> I thought about maybe emitting JSON, which would be extendable.  But it
> would have to be all on one line, which is less optimal, and would be
> much more verbose than the existing format.
>
> So maybe just use the existing format, but fixing it so that columns
> are columns, as discussed.
>
> So perhaps a fix-it aware Emacs mode could set this when compiling:
>
>   GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v2
>
> (v2 since v1 is the existing format from the cmdline option)
>
> In his email, Andrea suggests outputting to a file.  How would 
> that work?  It strikes me as making it difficult to associate the
> output from stderr with that to the file, or would the output to the
> file need to replace that from stderr (in which case what about lines
> of output from "make"? or other build system messages, etc).

Here come the trouble.  Ideally would be great to just emit to stderr
and integrate with compilation-mode.  My understanding is that this
would just not work for parallel builds as the output on stderr can be
intermixed, so the idea to dump to a special file.

I think (may be wrong) that this guided patching (but also the
navigation through the hierarchical hints that your static analyzer
produces) should be all handled by an ipotethical dedicated gcc-mode
that consume these json files.  I toyed already with this approach for
the static analyzer hints here [1].

You are correct suggesting that the connection from the stderr output in
compilation-mode may not be trivial but assuming this is a requirement
we have quite some info in the diagnostic message to search for the
corresponding diagnostic in the json.

"bar.c:350:3: error: invalid conversion to type 'foo_t'"

Maybe we could even add a diagnostic ID to make the connection simpler?

Or the other option is that we make it working saftely only for non
parallel builds just using stderr, but I don't think this is very future
proof, especially given you are constantly adding cool new features to
GCC we could make use of :)

  Andrea

[1] <https://gitlab.com/koral/gcc-analyzer-mode/-/blob/master/gcc-analyzer-mode.el>





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-14 22:43             ` David Malcolm
  2020-10-15  7:47               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-15 13:53               ` Eli Zaretskii
  2020-10-15 14:23                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2020-10-15 13:53 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987

> From: David Malcolm <dmalcolm@redhat.com>
> Cc: 25987@debbugs.gnu.org
> Date: Wed, 14 Oct 2020 18:43:33 -0400
> 
> In his email, Andrea suggests outputting to a file.  How would 
> that work?  It strikes me as making it difficult to associate the
> output from stderr with that to the file, or would the output to the
> file need to replace that from stderr (in which case what about lines
> of output from "make"? or other build system messages, etc).

I'm not sure how a separate file comes into this.  Aren't we talking
about the "normal" GCC diagnostic output, just augmented by hints?
That has the advantage that it is also human-readable, and could help
the user make changes other than accepting the hints.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-15 13:53               ` Eli Zaretskii
@ 2020-10-15 14:23                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-15 14:29                   ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-15 14:23 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, David Malcolm

Eli Zaretskii <eliz@gnu.org> writes:

>> From: David Malcolm <dmalcolm@redhat.com>
>> Cc: 25987@debbugs.gnu.org
>> Date: Wed, 14 Oct 2020 18:43:33 -0400
>> 
>> In his email, Andrea suggests outputting to a file.  How would 
>> that work?  It strikes me as making it difficult to associate the
>> output from stderr with that to the file, or would the output to the
>> file need to replace that from stderr (in which case what about lines
>> of output from "make"? or other build system messages, etc).
>
> I'm not sure how a separate file comes into this.  Aren't we talking
> about the "normal" GCC diagnostic output, just augmented by hints?
> That has the advantage that it is also human-readable, and could help
> the user make changes other than accepting the hints.

I explained this in my last mail on this thread, to make it short using
stderr can't work reliably for parallel builds.  Maybe we decide that's
good enought but has to be considered now as IMO it's a strong
limitation.

  Andrea





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-15 14:23                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-15 14:29                   ` Eli Zaretskii
  2020-10-15 14:44                     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2020-10-15 14:29 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: 25987, dmalcolm

> From: Andrea Corallo <akrl@sdf.org>
> Cc: David Malcolm <dmalcolm@redhat.com>,  25987@debbugs.gnu.org
> Date: Thu, 15 Oct 2020 14:23:05 +0000
> 
> > I'm not sure how a separate file comes into this.  Aren't we talking
> > about the "normal" GCC diagnostic output, just augmented by hints?
> > That has the advantage that it is also human-readable, and could help
> > the user make changes other than accepting the hints.
> 
> I explained this in my last mail on this thread, to make it short using
> stderr can't work reliably for parallel builds.

That's not a problem the feature discussed here can fix: it happens
with any parallel build run by "M-x compile".  The solution to that is
elsewhere (e.g., in using the GNU Make's command-line switches which
control parallelsim).





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-15 14:29                   ` Eli Zaretskii
@ 2020-10-15 14:44                     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 42+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-15 14:44 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987, dmalcolm

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: David Malcolm <dmalcolm@redhat.com>,  25987@debbugs.gnu.org
>> Date: Thu, 15 Oct 2020 14:23:05 +0000
>> 
>> > I'm not sure how a separate file comes into this.  Aren't we talking
>> > about the "normal" GCC diagnostic output, just augmented by hints?
>> > That has the advantage that it is also human-readable, and could help
>> > the user make changes other than accepting the hints.
>> 
>> I explained this in my last mail on this thread, to make it short using
>> stderr can't work reliably for parallel builds.
>
> That's not a problem the feature discussed here can fix: it happens
> with any parallel build run by "M-x compile".

Yes, but this is less severe as the line is tipically preserved and the
regexps we use for the goto-error are not affected, so in practice it
works.  The case of the patch is more sensitive.

> The solution to that is
> elsewhere (e.g., in using the GNU Make's command-line switches which
> control parallelsim).

That is saying is good enough.  I'm fine with that in case, but this was
to explain why the separate file came in.

  Andrea





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-13 14:37           ` Eli Zaretskii
  2020-10-14 22:43             ` David Malcolm
@ 2020-10-20 14:52             ` David Malcolm
  2020-10-20 15:54               ` Eli Zaretskii
  1 sibling, 1 reply; 42+ messages in thread
From: David Malcolm @ 2020-10-20 14:52 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

On Tue, 2020-10-13 at 17:37 +0300, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Cc: 25987@debbugs.gnu.org
> > Date: Mon, 12 Oct 2020 18:27:29 -0400
> > 
> > I like how -fdiagnostics-parseable-fixits adds extra lines of
> > output,
> > with a prefix that's ought to be easy to detect.
> 
> Yes, I think that's preferable, indeed.
> 
> > BTW, does Emacs set anything in the environment that GCC could
> > detect?
> 
> Yes, Emacs sets INSIDE_EMACS when it runs a sub-process.
> 
> > Does Emacs have an automated test suite that could test this
> > feature?
> 
> Yes, see the tests in the test/ subdirectory of the Emacs tree.
> 
> > Another complication to consider: the locations in a fix-it hint
> > refer
> > to the original source file, before any changes are made.  If the
> > user
> > interface supports the user e.g. clicking on fix-it hints and
> > selectively apply them one by one, then after each fix-it hint is
> > applied, all locations after that point potentially need to be
> > "fixed
> > up" somehow, to reflect the changes to the buffer.
> 
> That could be handled automatically by defining a marker at each
> hint's location, then they will move as text is edited.

Thanks Eli and Andrea.

I've implemented a proof-of-concept of this.

I'm attaching the patch for discussion/review purposes (it probably
doesn't quite apply cleanly to gcc master as I had some other
diagnostic stuff in that working copy; it also hasn't yet been
subjected to the usual testing for a gcc patch).

I'm also attaching the stderr with
  GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v2
in the env when using the patched gcc to compile the demo.c file from
message #82.

Hopefully the output looks consumable by Emacs.

One possible issue: in the final diagnostic, there's a fix-it hint with
non-ASCII replacement text, replacing "two_pi" with "two_π" (where the
final char in the latter is GREEK SMALL LETTER PI, U+03C0)

This replacement currently expressed as encoded bytes i.e:

fix-it:"demo.c":{51:10-51:16}:"two_\317\200"

where \317\200 is the octal-escaped representation of the two bytes of
the UTF-8 encoding of the character.

Is this going to work for Emacs?

Dave

[-- Attachment #2: demo.out --]
[-- Type: text/plain, Size: 2624 bytes --]

demo.c: In function ‘test_1’:
demo.c:14:10: warning: implicit declaration of function ‘malloc’ [-Wimplicit-function-declaration]
   14 |   return malloc (4096);
      |          ^~~~~~
demo.c:3:1: note: include ‘<stdlib.h>’ or provide a declaration of ‘malloc’
    2 | #include <stdio.h>
  +++ |+#include <stdlib.h>
    3 | 
fix-it:"demo.c":{3:1-3:1}:"#include <stdlib.h>\n"
demo.c:14:10: warning: incompatible implicit declaration of built-in function ‘malloc’ [-Wbuiltin-declaration-mismatch]
   14 |   return malloc (4096);
      |          ^~~~~~
demo.c:14:10: note: include ‘<stdlib.h>’ or provide a declaration of ‘malloc’
demo.c: In function ‘test_2a’:
demo.c:20:21: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   20 |   /* 🙂 🙂 🙂 */ v->colour = 0;
      |                     ^~~~~~
      |                     color
fix-it:"demo.c":{20:21-20:27}:"color"
demo.c: In function ‘test_2b’:
demo.c:26:18: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   26 |   /* π π π */ v->colour = 0;
      |                  ^~~~~~
      |                  color
fix-it:"demo.c":{26:18-26:24}:"color"
demo.c: In function ‘test_2c’:
demo.c:32:21: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   32 |   /* 文字化け */ v->colour = 0;
      |                     ^~~~~~
      |                     color
fix-it:"demo.c":{32:21-32:27}:"color"
demo.c: In function ‘test_3’:
demo.c:38:14: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   38 |           v->colour = 0;
      |              ^~~~~~
      |              color
fix-it:"demo.c":{38:14-38:20}:"color"
demo.c: In function ‘test_4’:
demo.c:44:10: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   44 |   if (v->colour) v->colour += 2;
      |          ^~~~~~
      |          color
fix-it:"demo.c":{44:10-44:16}:"color"
demo.c:44:21: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   44 |   if (v->colour) v->colour += 2;
      |                     ^~~~~~
      |                     color
fix-it:"demo.c":{44:21-44:27}:"color"
demo.c: In function ‘test_5’:
demo.c:51:10: error: ‘two_pi’ undeclared (first use in this function); did you mean ‘two_π’?
   51 |   return two_pi;
      |          ^~~~~~
      |          two_π
fix-it:"demo.c":{51:10-51:16}:"two_\317\200"
demo.c:51:10: note: each undeclared identifier is reported only once for each function it appears in

[-- Attachment #3: 0001-Add-GCC_EXTRA_DIAGNOSTIC_OUTPUT-environment-variable.patch --]
[-- Type: text/x-patch, Size: 15789 bytes --]

From 2e1ed6ea7ea1e524f2f5b5035327ed280fff475d Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 19 Oct 2020 20:19:48 -0400
Subject: [PATCH] Add GCC_EXTRA_DIAGNOSTIC_OUTPUT environment variable for
 fix-it hints

GCC has had the ability to emit fix-it hints in machine-readable form
since GCC 7 via -fdiagnostics-parseable-fixits and
-fdiagnostics-generate-patch.

The former emits additional specially-formatted lines to stderr; the
option and its format were directly taken from a pre-existing option
in clang.

Ideally this could be used by IDEs so that the user can select specific
fix-it hints and have the IDE apply them to the user's source code
(perhaps turning them into clickable elements, perhaps with an
"Apply All" option, etc).  Eclipse CDT has supported this option in
this way for a few years:
  https://bugs.eclipse.org/bugs/show_bug.cgi?id=497670

As a user of Emacs I would like Emacs to support such a feature.
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25987 tracks supporting
GCC fix-it output in Emacs.  The discussion there identifies two issues
with the existing option:

(a) columns in the output are specified as byte-offsets within the
line (for exact compatibility with the option in clang), whereas emacs
would prefer to consume them as what GCC 11 calls "display columns".
https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Message-Formatting-Options.html#index-fdiagnostics-column-unit

(b) injecting a command-line option into the build is a fiddly manual
step, varying between build systems.  It's far easier for the
user if Emacs simply sets an environment variable when compiling,
GCC uses this to enable the option if it recognizes the value, and
the emacs compilation buffer decodes the additional lines of output
and adds appropriate widgets.  In some ways it is a workaround for
not having a language server.  Doing it this way means that for the
various combinations of older and newer GCC and older and newer Emacs
that a sufficiently modern combination of both can automatically
support the rich fix-it UI, whereas other combinations will either
not provide the envvar, or silently ignore it, gracefully doing
nothing extra.

Hence this patch adds a new GCC_EXTRA_DIAGNOSTIC_OUTPUT environment
variable to GCC which enables output of machine-parseable fix-it hints.

GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v1 is equivalent to the existing
-fdiagnostics-parseable-fixits option.

GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v2 is the same, but changes the
column output mode to "display columns" rather than bytes, as
required by Emacs.

gcc/ChangeLog:
	* diagnostic.c (diagnostic_initialize): Eliminate
	parseable_fixits_p in favor of initializing extra_output_kind from
	GCC_EXTRA_DIAGNOSTIC_OUTPUT.
	(convert_column_unit): New function, split out from...
	(diagnostic_converted_column): ...this.
	(print_parseable_fixits): Add "column_unit" and "tabstop" params.
	Use them to call convert_column_unit on the column values.
	(diagnostic_report_diagnostic): Eliminate conditional on
	parseable_fixits_p in favor of a switch statement on
	extra_output_kind, passing the appropriate values to the new
	params of print_parseable_fixits.
	(selftest::test_print_parseable_fixits_none): Update for new
	params of print_parseable_fixits.
	(selftest::test_print_parseable_fixits_insert): Likewise.
	(selftest::test_print_parseable_fixits_remove): Likewise.
	(selftest::test_print_parseable_fixits_replace): Likewise.
	(selftest::test_print_parseable_fixits_bytes_vs_display_columns):
	New.
	(selftest::diagnostic_c_tests): Call it.
	* diagnostic.h (enum diagnostics_extra_output_kind): New.
	(diagnostic_context::parseable_fixits_p): Delete field in favor
	of...
	(diagnostic_context::extra_output_kind): ...this new field.
	* doc/invoke.texi (Environment Variables): Add
	GCC_EXTRA_DIAGNOSTIC_OUTPUT.
	* opts.c (common_handle_option): Update handling of
	OPT_fdiagnostics_parseable_fixits for change to diagnostic_context
	fields.
---
 gcc/diagnostic.c    | 138 ++++++++++++++++++++++++++++++++++++--------
 gcc/diagnostic.h    |  23 +++++++-
 gcc/doc/invoke.texi |  19 ++++++
 gcc/opts.c          |   4 +-
 4 files changed, 155 insertions(+), 29 deletions(-)

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 1b6c9845892..7724f0e050b 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -219,7 +219,14 @@ diagnostic_initialize (diagnostic_context *context, int n_opts)
   context->show_line_numbers_p = false;
   context->min_margin_width = 0;
   context->show_ruler_p = false;
-  context->parseable_fixits_p = false;
+  if (const char *var = getenv ("GCC_EXTRA_DIAGNOSTIC_OUTPUT"))
+    {
+      if (!strcmp (var, "fixits-v1"))
+	context->extra_output_kind = EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1;
+      else if (!strcmp (var, "fixits-v2"))
+	context->extra_output_kind = EXTRA_DIAGNOSTIC_OUTPUT_fixits_v2;
+      /* Silently ignore unrecognized values.  */
+    }
   context->column_unit = DIAGNOSTICS_COLUMN_UNIT_DISPLAY;
   context->column_origin = 1;
   context->tabstop = 8;
@@ -358,29 +365,40 @@ diagnostic_get_color_for_kind (diagnostic_t kind)
 }
 
 /* Given an expanded_location, convert the column (which is in 1-based bytes)
-   to the requested units and origin.  Return -1 if the column is
-   invalid (<= 0).  */
-int
-diagnostic_converted_column (diagnostic_context *context, expanded_location s)
+   to the requested units, without converting the origin.
+   Return -1 if the column is invalid (<= 0).  */
+
+static int
+convert_column_unit (enum diagnostics_column_unit column_unit,
+		     int tabstop,
+		     expanded_location s)
 {
   if (s.column <= 0)
     return -1;
 
-  int one_based_col;
-  switch (context->column_unit)
+  switch (column_unit)
     {
+    default:
+      gcc_unreachable ();
+
     case DIAGNOSTICS_COLUMN_UNIT_DISPLAY:
-      one_based_col = location_compute_display_column (s, context->tabstop);
-      break;
+      return location_compute_display_column (s, tabstop);
 
     case DIAGNOSTICS_COLUMN_UNIT_BYTE:
-      one_based_col = s.column;
-      break;
-
-    default:
-      gcc_unreachable ();
+      return s.column;
     }
+}
 
+/* Given an expanded_location, convert the column (which is in 1-based bytes)
+   to the requested units and origin.  Return -1 if the column is
+   invalid (<= 0).  */
+int
+diagnostic_converted_column (diagnostic_context *context, expanded_location s)
+{
+  int one_based_col
+    = convert_column_unit (context->column_unit, context->tabstop, s);
+  if (one_based_col <= 0)
+    return -1;
   return one_based_col + (context->column_origin - 1);
 }
 
@@ -920,11 +938,16 @@ print_escaped_string (pretty_printer *pp, const char *text)
   pp_character (pp, '"');
 }
 
-/* Implementation of -fdiagnostics-parseable-fixits.  Print a
-   machine-parseable version of all fixits in RICHLOC to PP.  */
+/* Implementation of -fdiagnostics-parseable-fixits and
+   GCC_EXTRA_DIAGNOSTIC_OUTPUT.
+   Print a machine-parseable version of all fixits in RICHLOC to PP,
+   using COLUMN_UNIT to express columns.
+   Use TABSTOP when handling DIAGNOSTICS_COLUMN_UNIT_DISPLAY.  */
 
 static void
-print_parseable_fixits (pretty_printer *pp, rich_location *richloc)
+print_parseable_fixits (pretty_printer *pp, rich_location *richloc,
+			enum diagnostics_column_unit column_unit,
+			int tabstop)
 {
   gcc_assert (pp);
   gcc_assert (richloc);
@@ -942,9 +965,13 @@ print_parseable_fixits (pretty_printer *pp, rich_location *richloc)
       /* For compatibility with clang, print as a half-open range.  */
       location_t next_loc = hint->get_next_loc ();
       expanded_location next_exploc = expand_location (next_loc);
+      int start_col
+	= convert_column_unit (column_unit, tabstop, start_exploc);
+      int next_col
+	= convert_column_unit (column_unit, tabstop, next_exploc);
       pp_printf (pp, ":{%i:%i-%i:%i}:",
-		 start_exploc.line, start_exploc.column,
-		 next_exploc.line, next_exploc.column);
+		 start_exploc.line, start_col,
+		 next_exploc.line, next_col);
       print_escaped_string (pp, hint->get_string ());
       pp_newline (pp);
     }
@@ -1210,10 +1237,22 @@ diagnostic_report_diagnostic (diagnostic_context *context,
   if (context->show_option_requested)
     print_option_information (context, diagnostic, orig_diag_kind);
   (*diagnostic_finalizer (context)) (context, diagnostic, orig_diag_kind);
-  if (context->parseable_fixits_p)
+  switch (context->extra_output_kind)
     {
-      print_parseable_fixits (context->printer, diagnostic->richloc);
+    default:
+      break;
+    case EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1:
+      print_parseable_fixits (context->printer, diagnostic->richloc,
+			      DIAGNOSTICS_COLUMN_UNIT_BYTE,
+			      context->tabstop);
+      pp_flush (context->printer);
+      break;
+    case EXTRA_DIAGNOSTIC_OUTPUT_fixits_v2:
+      print_parseable_fixits (context->printer, diagnostic->richloc,
+			      DIAGNOSTICS_COLUMN_UNIT_DISPLAY,
+			      context->tabstop);
       pp_flush (context->printer);
+      break;
     }
   diagnostic_action_after_output (context, diagnostic->kind);
   diagnostic->x_data = NULL;
@@ -2012,7 +2051,7 @@ test_print_parseable_fixits_none ()
   pretty_printer pp;
   rich_location richloc (line_table, UNKNOWN_LOCATION);
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("", pp_formatted_text (&pp));
 }
 
@@ -2031,7 +2070,7 @@ test_print_parseable_fixits_insert ()
   location_t where = linemap_position_for_column (line_table, 10);
   richloc.add_fixit_insert_before (where, "added content");
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("fix-it:\"test.c\":{5:10-5:10}:\"added content\"\n",
 		pp_formatted_text (&pp));
 }
@@ -2053,7 +2092,7 @@ test_print_parseable_fixits_remove ()
   where.m_finish = linemap_position_for_column (line_table, 20);
   richloc.add_fixit_remove (where);
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("fix-it:\"test.c\":{5:10-5:21}:\"\"\n",
 		pp_formatted_text (&pp));
 }
@@ -2075,11 +2114,59 @@ test_print_parseable_fixits_replace ()
   where.m_finish = linemap_position_for_column (line_table, 20);
   richloc.add_fixit_replace (where, "replacement");
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("fix-it:\"test.c\":{5:10-5:21}:\"replacement\"\n",
 		pp_formatted_text (&pp));
 }
 
+/* Verify that print_parseable_fixits correctly handles
+   DIAGNOSTICS_COLUMN_UNIT_BYTE vs DIAGNOSTICS_COLUMN_UNIT_COLUMN.  */
+
+static void
+test_print_parseable_fixits_bytes_vs_display_columns ()
+{
+  line_table_test ltt;
+  rich_location richloc (line_table, UNKNOWN_LOCATION);
+
+  /* 1-based byte offsets:     12345677778888999900001234567.  */
+  const char *const content = "smile \xf0\x9f\x98\x82 colour\n";
+  /* 1-based display cols:     123456[......7-8.....]9012345.  */
+  const int tabstop = 8;
+
+  temp_source_file tmp (SELFTEST_LOCATION, ".c", content);
+  const char *const fname = tmp.get_filename ();
+
+  linemap_add (line_table, LC_ENTER, false, fname, 0);
+  linemap_line_start (line_table, 1, 100);
+  linemap_add (line_table, LC_LEAVE, false, NULL, 0);
+  source_range where;
+  where.m_start = linemap_position_for_column (line_table, 12);
+  where.m_finish = linemap_position_for_column (line_table, 17);
+  richloc.add_fixit_replace (where, "color");
+
+  const int buf_len = strlen (fname) + 100;
+  char *const expected = XNEWVEC (char, buf_len);
+
+  {
+    pretty_printer pp;
+    print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE,
+			    tabstop);
+    snprintf (expected, buf_len,
+	      "fix-it:\"%s\":{1:12-1:18}:\"color\"\n", fname);
+    ASSERT_STREQ (expected, pp_formatted_text (&pp));
+  }
+  {
+    pretty_printer pp;
+    print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_DISPLAY,
+			    tabstop);
+    snprintf (expected, buf_len,
+	      "fix-it:\"%s\":{1:10-1:16}:\"color\"\n", fname);
+    ASSERT_STREQ (expected, pp_formatted_text (&pp));
+  }
+
+  XDELETEVEC (expected);
+}
+
 /* Verify that
      diagnostic_get_location_text (..., SHOW_COLUMN)
    generates EXPECTED_LOC_TEXT, given FILENAME, LINE, COLUMN, with
@@ -2202,6 +2289,7 @@ diagnostic_c_tests ()
   test_print_parseable_fixits_insert ();
   test_print_parseable_fixits_remove ();
   test_print_parseable_fixits_replace ();
+  test_print_parseable_fixits_bytes_vs_display_columns ();
   test_diagnostic_get_location_text ();
   test_num_digits ();
 
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 1d2fa119643..e88d63c2128 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -69,6 +69,22 @@ enum diagnostic_path_format
   DPF_HTML
 };
 
+/* An enum for capturing values of GCC_EXTRA_DIAGNOSTIC_OUTPUT,
+   and for -fdiagnostics-parseable-fixits.  */
+
+enum diagnostics_extra_output_kind
+{
+  /* No extra output, or an unrecognized value.  */
+  EXTRA_DIAGNOSTIC_OUTPUT_none,
+
+  /* Emit fix-it hints using the "fixits-v1" format, equivalent to
+     -fdiagnostics-parseable-fixits.  */
+  EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1,
+
+  /* Emit fix-it hints using the "fixits-v2" format.  */
+  EXTRA_DIAGNOSTIC_OUTPUT_fixits_v2
+};
+
 /* A diagnostic is described by the MESSAGE to send, the FILE and LINE of
    its context and its KIND (ice, error, warning, note, ...)  See complete
    list in diagnostic.def.  */
@@ -296,9 +312,10 @@ struct diagnostic_context
      source output.  */
   bool show_ruler_p;
 
-  /* If true, print fixits in machine-parseable form after the
-     rest of the diagnostic.  */
-  bool parseable_fixits_p;
+  /* Used to specify additional diagnostic output to be emitted after the
+     rest of the diagnostic.  This is for implementing
+     -fdiagnostics-parseable-fixits and GCC_EXTRA_DIAGNOSTIC_OUTPUT.  */
+  enum diagnostics_extra_output_kind extra_output_kind;
 
   /* What units to use when outputting the column number.  */
   enum diagnostics_column_unit column_unit;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 55f7a9854aa..f83fe8f735c 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -31998,6 +31998,25 @@ Recognize EUCJP characters.
 If @env{LANG} is not defined, or if it has some other value, then the
 compiler uses @code{mblen} and @code{mbtowc} as defined by the default locale to
 recognize and translate multibyte characters.
+
+@item GCC_EXTRA_DIAGNOSTIC_OUTPUT
+If @env{GCC_EXTRA_DIAGNOSTIC_OUTPUT} is set to one of the following values,
+then additional text will be emitted to stderr when fix-it hints are
+emitted.  @option{-fdiagnostics-parseable-fixits} and
+@option{-fno-diagnostics-parseable-fixits} take precedence over this
+environment variable.
+
+@table @samp
+@item fixits-v1
+Emit parseable fix-it hints, equivalent to
+@option{-fdiagnostics-parseable-fixits}.  In particular, columns are
+expressed as a count of bytes, starting at byte 1 for the initial column.
+
+@item fixits-v2
+As @code{fixits-v1}, but columns are expressed as display columns,
+as per @option{-fdiagnostics-column-unit=display}.
+@end table
+
 @end table
 
 @noindent
diff --git a/gcc/opts.c b/gcc/opts.c
index da503c32dd0..b776f3b31e1 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2422,7 +2422,9 @@ common_handle_option (struct gcc_options *opts,
       break;
 
     case OPT_fdiagnostics_parseable_fixits:
-      dc->parseable_fixits_p = value;
+      dc->extra_output_kind = (value
+			       ? EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1
+			       : EXTRA_DIAGNOSTIC_OUTPUT_none);
       break;
 
     case OPT_fdiagnostics_column_unit_:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-20 14:52             ` David Malcolm
@ 2020-10-20 15:54               ` Eli Zaretskii
  2020-11-11 19:36                 ` David Malcolm
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2020-10-20 15:54 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987

> From: David Malcolm <dmalcolm@redhat.com>
> Cc: 25987@debbugs.gnu.org
> Date: Tue, 20 Oct 2020 10:52:05 -0400
> 
> One possible issue: in the final diagnostic, there's a fix-it hint with
> non-ASCII replacement text, replacing "two_pi" with "two_π" (where the
> final char in the latter is GREEK SMALL LETTER PI, U+03C0)
> 
> This replacement currently expressed as encoded bytes i.e:
> 
> fix-it:"demo.c":{51:10-51:16}:"two_\317\200"
> 
> where \317\200 is the octal-escaped representation of the two bytes of
> the UTF-8 encoding of the character.
> 
> Is this going to work for Emacs?

You mean, GCC doesn't actually emit the UTF-8 encoding of π, it emits
its ASCII-fied representation?  We'd need to decode that, but is that
really justified?  Why not emit UTF-8?





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-20 15:54               ` Eli Zaretskii
@ 2020-11-11 19:36                 ` David Malcolm
  2020-11-12 13:54                   ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: David Malcolm @ 2020-11-11 19:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987

On Tue, 2020-10-20 at 18:54 +0300, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Cc: 25987@debbugs.gnu.org
> > Date: Tue, 20 Oct 2020 10:52:05 -0400
> > 
> > One possible issue: in the final diagnostic, there's a fix-it hint
> > with
> > non-ASCII replacement text, replacing "two_pi" with "two_π" (where
> > the
> > final char in the latter is GREEK SMALL LETTER PI, U+03C0)
> > 
> > This replacement currently expressed as encoded bytes i.e:
> > 
> > fix-it:"demo.c":{51:10-51:16}:"two_\317\200"
> > 
> > where \317\200 is the octal-escaped representation of the two bytes
> > of
> > the UTF-8 encoding of the character.
> > 
> > Is this going to work for Emacs?
> 
> You mean, GCC doesn't actually emit the UTF-8 encoding of π, it emits
> its ASCII-fied representation?  We'd need to decode that, but is that
> really justified?  Why not emit UTF-8?

I have an implementation that simply emits UTF-8 in quotes, escaping
backslash, tab, newline, and doublequotes as before.  (we have to
escape at least newline, given that fix-it hint replacement text can
contain them, and we're using newline to terminate the parseable hint).

However, the filename also needs to be escaped.  Currently I'm applying
the same escaping rules to both filename and replacement text.
What is the encoding of the filename?  What if the bytes in a filename
aren't UTF-8 encoded?  How does emacs handle this case?  I tried
creating file with the name "byte 0xff" .txt, and with valid UTF-8 non-
ascii names and emacs reported them as \377.txt and with the UTF-8
names respectively, so perhaps I should simply emit the bytes and
pretend they are UTF-8?

Dave

[1] https://dwheeler.com/essays/fixing-unix-linux-filenames.html






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-11-11 19:36                 ` David Malcolm
@ 2020-11-12 13:54                   ` Eli Zaretskii
  2020-11-13 16:47                     ` David Malcolm
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2020-11-12 13:54 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987

> From: David Malcolm <dmalcolm@redhat.com>
> Cc: 25987@debbugs.gnu.org
> Date: Wed, 11 Nov 2020 14:36:49 -0500
> 
> On Tue, 2020-10-20 at 18:54 +0300, Eli Zaretskii wrote:
> > > From: David Malcolm <dmalcolm@redhat.com>
> > > Cc: 25987@debbugs.gnu.org
> > > Date: Tue, 20 Oct 2020 10:52:05 -0400
> > > 
> > > One possible issue: in the final diagnostic, there's a fix-it hint
> > > with
> > > non-ASCII replacement text, replacing "two_pi" with "two_π" (where
> > > the
> > > final char in the latter is GREEK SMALL LETTER PI, U+03C0)
> > > 
> > > This replacement currently expressed as encoded bytes i.e:
> > > 
> > > fix-it:"demo.c":{51:10-51:16}:"two_\317\200"
> > > 
> > > where \317\200 is the octal-escaped representation of the two bytes
> > > of
> > > the UTF-8 encoding of the character.
> > > 
> > > Is this going to work for Emacs?
> > 
> > You mean, GCC doesn't actually emit the UTF-8 encoding of π, it emits
> > its ASCII-fied representation?  We'd need to decode that, but is that
> > really justified?  Why not emit UTF-8?
> 
> I have an implementation that simply emits UTF-8 in quotes, escaping
> backslash, tab, newline, and doublequotes as before.  (we have to
> escape at least newline, given that fix-it hint replacement text can
> contain them, and we're using newline to terminate the parseable hint).

Sorry, I've lost the context: where did those non-ASCII names come
from? are they names of variables in the user's program?  If so, in
what encoding does GCC quote portions of the source code in its
warning/error messages?  Does it use the exact byte stream it found in
the source, or does it perform any conversions of the encoding?

> However, the filename also needs to be escaped.  Currently I'm applying
> the same escaping rules to both filename and replacement text.
> What is the encoding of the filename?  What if the bytes in a filename
> aren't UTF-8 encoded?  How does emacs handle this case?

Emacs has a separate variable for the encoding of file names, which
gets set from the locale settings.  But this is not necessarily
relevant to the issue at hand, because we are talking about processing
output from a sub-process (GCC) which includes both file names and
other stuff, such as fragments of the source code.  When Emacs
processes sub-process output, it generally assumes all of it is
encoded in the same encoding.  So if, for example, you encode
non-ASCII variables in UTF-8 while the file names are emitted in some
other encoding (perhaps because the locale's codeset is not UTF-8),
then there will be complications: we will have to read the output from
GCC in its raw form, and then decode "by hand" (in Lisp) each part of
it as appropriate (which means we will need to be able to identifye
each such part).

So it's important to understand the situation and its limitations for
proposing the best solution.

> I tried creating file with the name "byte 0xff" .txt, and with valid
> UTF-8 non- ascii names and emacs reported them as \377.txt and with
> the UTF-8 names respectively, so perhaps I should simply emit the
> bytes and pretend they are UTF-8?

What do you mean by "pretend" in this context?





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-11-12 13:54                   ` Eli Zaretskii
@ 2020-11-13 16:47                     ` David Malcolm
  2020-11-14 14:21                       ` Eli Zaretskii
  0 siblings, 1 reply; 42+ messages in thread
From: David Malcolm @ 2020-11-13 16:47 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987

On Thu, 2020-11-12 at 15:54 +0200, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Cc: 25987@debbugs.gnu.org
> > Date: Wed, 11 Nov 2020 14:36:49 -0500
> > 
> > On Tue, 2020-10-20 at 18:54 +0300, Eli Zaretskii wrote:
> > > > From: David Malcolm <dmalcolm@redhat.com>
> > > > Cc: 25987@debbugs.gnu.org
> > > > Date: Tue, 20 Oct 2020 10:52:05 -0400
> > > > 
> > > > One possible issue: in the final diagnostic, there's a fix-it
> > > > hint
> > > > with
> > > > non-ASCII replacement text, replacing "two_pi" with "two_π"
> > > > (where
> > > > the
> > > > final char in the latter is GREEK SMALL LETTER PI, U+03C0)
> > > > 
> > > > This replacement currently expressed as encoded bytes i.e:
> > > > 
> > > > fix-it:"demo.c":{51:10-51:16}:"two_\317\200"
> > > > 
> > > > where \317\200 is the octal-escaped representation of the two
> > > > bytes
> > > > of
> > > > the UTF-8 encoding of the character.
> > > > 
> > > > Is this going to work for Emacs?
> > > 
> > > You mean, GCC doesn't actually emit the UTF-8 encoding of π, it
> > > emits
> > > its ASCII-fied representation?  We'd need to decode that, but is
> > > that
> > > really justified?  Why not emit UTF-8?
> > 
> > I have an implementation that simply emits UTF-8 in quotes,
> > escaping
> > backslash, tab, newline, and doublequotes as before.  (we have to
> > escape at least newline, given that fix-it hint replacement text
> > can
> > contain them, and we're using newline to terminate the parseable
> > hint).
> 
> Sorry, I've lost the context: where did those non-ASCII names come
> from? are they names of variables in the user's program?  

The names are identifiers from the user's program (names of variables,
types, macros, etc), where an error has been issued, typically due to a
misspelling of an identifier.  For example, somewhere there's a
declaration of a constant named "two_π", and later the code erroneously
references it as "two_pi"; we want to emit a diagnostic saying:
  did you mean "two_π"?
and provide a machine-readable fix-it hint suggesting the replacement
of the pertinent source range with "two_π".

GCC converts the source code from any encoding specified by -finput-
charset= to use UTF-8 internally...

https://gcc.gnu.org/onlinedocs/cpp/Character-sets.html

> If so, in
> what encoding does GCC quote portions of the source code in its
> warning/error messages?
>   Does it use the exact byte stream it found in
> the source, or does it perform any conversions of the encoding?

...however there's a bug in GCC in how we print the source code itself,
where we blithely emit the undecoded bytes directly to stderr when
quoting the lines of source.  This GCC bug is 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93067 (aka PR
other/93067).  We ought to encode the source code into UTF-8 when
printing it (which may be a no-op for the common case).  The annotation
lines we print under the source lines for fix-it hints and labels are
already printed in UTF-8, however.

That said, the above bug is orthogonal to the fix-it hint issue, which
prints the names in a different way (using UTF-8 encoded strings in
GCC's symbol table, rather than scraping them from the filesystem,
which is how the buggy source-quoting routines work).

> > However, the filename also needs to be escaped.  Currently I'm
> > applying
> > the same escaping rules to both filename and replacement text.
> > What is the encoding of the filename?  What if the bytes in a
> > filename
> > aren't UTF-8 encoded?  How does emacs handle this case?
> 
> Emacs has a separate variable for the encoding of file names, which
> gets set from the locale settings.  But this is not necessarily
> relevant to the issue at hand, because we are talking about
> processing
> output from a sub-process (GCC) which includes both file names and
> other stuff, such as fragments of the source code.  When Emacs
> processes sub-process output, it generally assumes all of it is
> encoded in the same encoding.  So if, for example, you encode
> non-ASCII variables in UTF-8 while the file names are emitted in some
> other encoding (perhaps because the locale's codeset is not UTF-8),
> then there will be complications: we will have to read the output
> from
> GCC in its raw form, and then decode "by hand" (in Lisp) each part of
> it as appropriate (which means we will need to be able to identifye
> each such part).
> 
> So it's important to understand the situation and its limitations for
> proposing the best solution.

As far as I can tell GCC handles filenames as raw bytes, and doesn't
make any attempt to decode them, and emits them as bytes again in
diagnostic messages.

> > I tried creating file with the name "byte 0xff" .txt, and with
> > valid
> > UTF-8 non- ascii names and emacs reported them as \377.txt and with
> > the UTF-8 names respectively, so perhaps I should simply emit the
> > bytes and pretend they are UTF-8?
> 
> What do you mean by "pretend" in this context?

By "pretend" I mean simply re-emitting the bytes of the filename to
stderr and ignoring encoding issues in them, despite the fact that the
rest of the stream is supposed to be UTF-8-encoded.

Currently the parseable-fixits option uses IS_PRINT on each "char"
(i.e. byte) so that any non-printable bytes get octal-escaped.  Is that
acceptable for filenames?  The other approach, to "pretend they're UTF-
8", would mean to not escape such bytes, so that if they are UTF-8 they
are faithfully re-emitted.

I think I like the approach where the filename part of the fixit line
is octal-escaped, and the replacement text is UTF-8, but I don't know
what's going to be best for you.

Hope the above clarifies things.

Dave






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-11-13 16:47                     ` David Malcolm
@ 2020-11-14 14:21                       ` Eli Zaretskii
  2020-11-14 19:46                         ` David Malcolm
  0 siblings, 1 reply; 42+ messages in thread
From: Eli Zaretskii @ 2020-11-14 14:21 UTC (permalink / raw)
  To: David Malcolm; +Cc: 25987

> From: David Malcolm <dmalcolm@redhat.com>
> Cc: 25987@debbugs.gnu.org
> Date: Fri, 13 Nov 2020 11:47:18 -0500
> 
> The names are identifiers from the user's program (names of variables,
> types, macros, etc), where an error has been issued, typically due to a
> misspelling of an identifier.  For example, somewhere there's a
> declaration of a constant named "two_π", and later the code erroneously
> references it as "two_pi"; we want to emit a diagnostic saying:
>   did you mean "two_π"?
> and provide a machine-readable fix-it hint suggesting the replacement
> of the pertinent source range with "two_π".
> 
> GCC converts the source code from any encoding specified by -finput-
> charset= to use UTF-8 internally...
> 
> https://gcc.gnu.org/onlinedocs/cpp/Character-sets.html

And then GCC outputs these identifiers in UTF-8?  Or does it convert
back to the original input-charset?

> ...however there's a bug in GCC in how we print the source code itself,
> where we blithely emit the undecoded bytes directly to stderr when
> quoting the lines of source.  This GCC bug is 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93067 (aka PR
> other/93067).  We ought to encode the source code into UTF-8 when
> printing it (which may be a no-op for the common case).

I'm not sure you are right here: I think it is better for GCC to use
the original bytestream, because the user's locale might not support
UTF-8 well; it is better to show the source to the user in the
encoding in which it was written.

However, I'm not familiar with GCC internals, so it is not clear to me
whether the bug report will indeed affect the way source fragments
will be output: the bug report only talks about converting the input,
and I don't know enough to understand how will that affect output.

> The annotation lines we print under the source lines for fix-it
> hints and labels are already printed in UTF-8, however.

The annotations are in US English, though, right?  If not, when will
they include non-ASCII characters?

> That said, the above bug is orthogonal to the fix-it hint issue, which
> prints the names in a different way (using UTF-8 encoded strings in
> GCC's symbol table, rather than scraping them from the filesystem,
> which is how the buggy source-quoting routines work).
> [...]
> As far as I can tell GCC handles filenames as raw bytes, and doesn't
> make any attempt to decode them, and emits them as bytes again in
> diagnostic messages.

This is okay, but since the other parts are in UTF-8, this will
complicate things, as I mentioned in my previous message.

> > > I tried creating file with the name "byte 0xff" .txt, and with
> > > valid
> > > UTF-8 non- ascii names and emacs reported them as \377.txt and with
> > > the UTF-8 names respectively, so perhaps I should simply emit the
> > > bytes and pretend they are UTF-8?
> > 
> > What do you mean by "pretend" in this context?
> 
> By "pretend" I mean simply re-emitting the bytes of the filename to
> stderr and ignoring encoding issues in them, despite the fact that the
> rest of the stream is supposed to be UTF-8-encoded.

As explained, it will be easier for Emacs to process GCC output if its
encoding is consistent.

> Currently the parseable-fixits option uses IS_PRINT on each "char"
> (i.e. byte) so that any non-printable bytes get octal-escaped.  Is that
> acceptable for filenames?  The other approach, to "pretend they're UTF-
> 8", would mean to not escape such bytes, so that if they are UTF-8 they
> are faithfully re-emitted.
> 
> I think I like the approach where the filename part of the fixit line
> is octal-escaped, and the replacement text is UTF-8, but I don't know
> what's going to be best for you.

Given your description, it sounds like it will not be simple whatever
you do.

I guess we should first try getting the plain-ASCII case to work, as
that is the most frequent use case anyway.





^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-11-14 14:21                       ` Eli Zaretskii
@ 2020-11-14 19:46                         ` David Malcolm
  0 siblings, 0 replies; 42+ messages in thread
From: David Malcolm @ 2020-11-14 19:46 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25987

On Sat, 2020-11-14 at 16:21 +0200, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Cc: 25987@debbugs.gnu.org
> > Date: Fri, 13 Nov 2020 11:47:18 -0500
> > 
> > The names are identifiers from the user's program (names of
> > variables,
> > types, macros, etc), where an error has been issued, typically due
> > to a
> > misspelling of an identifier.  For example, somewhere there's a
> > declaration of a constant named "two_π", and later the code
> > erroneously
> > references it as "two_pi"; we want to emit a diagnostic saying:
> >   did you mean "two_π"?
> > and provide a machine-readable fix-it hint suggesting the
> > replacement
> > of the pertinent source range with "two_π".
> > 
> > GCC converts the source code from any encoding specified by
> > -finput-
> > charset= to use UTF-8 internally...
> > 
> > https://gcc.gnu.org/onlinedocs/cpp/Character-sets.html
> 
> And then GCC outputs these identifiers in UTF-8?  Or does it convert
> back to the original input-charset?

It emits them as UTF-8 when emitting diagnostics.

> > ...however there's a bug in GCC in how we print the source code
> > itself,
> > where we blithely emit the undecoded bytes directly to stderr when
> > quoting the lines of source.  This GCC bug is 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93067 (aka PR
> > other/93067).  We ought to encode the source code into UTF-8 when
> > printing it (which may be a no-op for the common case).
> 
> I'm not sure you are right here: I think it is better for GCC to use
> the original bytestream, because the user's locale might not support
> UTF-8 well; it is better to show the source to the user in the
> encoding in which it was written.

This seems to me to lead to a bigger question: what should the encoding
of GCC's stderr be?  Right now I believe we emit a mix of UTF-8 and
other encodings, as noted in my earlier post.

> However, I'm not familiar with GCC internals, so it is not clear to
> me
> whether the bug report will indeed affect the way source fragments
> will be output: the bug report only talks about converting the input,
> and I don't know enough to understand how will that affect output.
> 
> > The annotation lines we print under the source lines for fix-it
> > hints and labels are already printed in UTF-8, however.
> 
> The annotations are in US English, though, right?  If not, when will
> they include non-ASCII characters?

Annotation lines can contain labels as of GCC 9, and these can contain
identifiers; for example in this C++ type mismatch error, where the
types of the pertinent expressions are labeled:
$ g++ t.cc
t.cc: In function 'int test(const shape&, const shape&)':
t.cc:15:4: error: no match for 'operator+' (operand types are
'boxed_value<double>' and 'boxed_value<double>')
   14 |   return (width(s1) * height(s1)
      |           ~~~~~~~~~~~~~~~~~~~~~~
      |                     |
      |                     boxed_value<[...]>
   15 |    + width(s2) * height(s2));
      |    ^ ~~~~~~~~~~~~~~~~~~~~~~
      |                |
      |                boxed_value<[...]>

where "boxed_value" is an identifier and in theory could have non-ASCII 
characters in it.

> > That said, the above bug is orthogonal to the fix-it hint issue,
> > which
> > prints the names in a different way (using UTF-8 encoded strings in
> > GCC's symbol table, rather than scraping them from the filesystem,
> > which is how the buggy source-quoting routines work).
> > [...]
> > As far as I can tell GCC handles filenames as raw bytes, and
> > doesn't
> > make any attempt to decode them, and emits them as bytes again in
> > diagnostic messages.
> 
> This is okay, but since the other parts are in UTF-8, this will
> complicate things, as I mentioned in my previous message.
> 
> > > > I tried creating file with the name "byte 0xff" .txt, and with
> > > > valid
> > > > UTF-8 non- ascii names and emacs reported them as \377.txt and
> > > > with
> > > > the UTF-8 names respectively, so perhaps I should simply emit
> > > > the
> > > > bytes and pretend they are UTF-8?
> > > 
> > > What do you mean by "pretend" in this context?
> > 
> > By "pretend" I mean simply re-emitting the bytes of the filename to
> > stderr and ignoring encoding issues in them, despite the fact that
> > the
> > rest of the stream is supposed to be UTF-8-encoded.
> 
> As explained, it will be easier for Emacs to process GCC output if
> its
> encoding is consistent.

Indeed.  I'll raise this issue on the GCC mailing list.

> > Currently the parseable-fixits option uses IS_PRINT on each "char"
> > (i.e. byte) so that any non-printable bytes get octal-escaped.  Is
> > that
> > acceptable for filenames?  The other approach, to "pretend they're
> > UTF-
> > 8", would mean to not escape such bytes, so that if they are UTF-8
> > they
> > are faithfully re-emitted.
> > 
> > I think I like the approach where the filename part of the fixit
> > line
> > is octal-escaped, and the replacement text is UTF-8, but I don't
> > know
> > what's going to be best for you.
> 
> Given your description, it sounds like it will not be simple whatever
> you do.
> 
> I guess we should first try getting the plain-ASCII case to work, as
> that is the most frequent use case anyway.

I added some test cases and posted the patch to the gcc-patches mailing
list here:
  "[PATCH/RFC] Add GCC_EXTRA_DIAGNOSTIC_OUTPUT environment variable for
fix-it hints"
  https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559105.html

Thanks
Dave






^ permalink raw reply	[flat|nested] 42+ messages in thread

* bug#25987: 25.2; support gcc fixit notes
  2020-10-15  7:47               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-01-14 21:37                 ` David Malcolm
  0 siblings, 0 replies; 42+ messages in thread
From: David Malcolm @ 2021-01-14 21:37 UTC (permalink / raw)
  To: 25987; +Cc: Andrea Corallo

I've now pushed the patch:
   "[PATCH/RFC] Add GCC_EXTRA_DIAGNOSTIC_OUTPUT environment variable
for fix-it hints"
  https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559105.html

to the gcc master branch targeting gcc-11:

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=f10960558540636800cf5d3d6355969621fbc17e

Dave






^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2021-01-14 21:37 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-05 21:47 bug#25987: 25.2; support gcc fixit notes Tom Tromey
2017-03-06 18:35 ` Eli Zaretskii
2017-03-07 13:54   ` Tom Tromey
2017-03-07 15:55     ` Eli Zaretskii
2017-03-08 18:34       ` Tom Tromey
2017-03-08 19:22         ` Eli Zaretskii
2017-03-09  4:20           ` Richard Stallman
2017-03-09 15:36             ` Eli Zaretskii
2017-03-08 18:44     ` Tom Tromey
2017-03-08 19:28       ` Eli Zaretskii
2017-03-09 16:37         ` Dmitry Gutov
2017-03-09 16:56           ` Eli Zaretskii
2017-03-09 17:37             ` Dmitry Gutov
2017-03-09 18:32               ` Eli Zaretskii
2017-03-09 21:26                 ` Dmitry Gutov
2017-08-06  3:34           ` Tom Tromey
2017-03-09 16:18 ` Dmitry Gutov
2017-03-09 16:53   ` Eli Zaretskii
2017-03-09 17:49     ` Dmitry Gutov
2017-03-09 18:35       ` Eli Zaretskii
2017-08-06  3:31   ` Tom Tromey
2018-03-16 16:48 ` David Malcolm
2018-03-16 20:19   ` Eli Zaretskii
2020-10-06 18:17     ` David Malcolm
2020-10-06 18:37       ` Eli Zaretskii
2020-10-12 22:27         ` David Malcolm
2020-10-13  7:34           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-13 14:37           ` Eli Zaretskii
2020-10-14 22:43             ` David Malcolm
2020-10-15  7:47               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-01-14 21:37                 ` David Malcolm
2020-10-15 13:53               ` Eli Zaretskii
2020-10-15 14:23                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-15 14:29                   ` Eli Zaretskii
2020-10-15 14:44                     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-20 14:52             ` David Malcolm
2020-10-20 15:54               ` Eli Zaretskii
2020-11-11 19:36                 ` David Malcolm
2020-11-12 13:54                   ` Eli Zaretskii
2020-11-13 16:47                     ` David Malcolm
2020-11-14 14:21                       ` Eli Zaretskii
2020-11-14 19:46                         ` David Malcolm

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).