From 92646d25778952a1fd24fa9a6f407cd421606195 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Fri, 19 Oct 2018 19:50:02 +0000 Subject: [PATCH] gccint.texi: add user experience guidelines gcc/ChangeLog: * Makefile.in (TEXI_GCCINT_FILES): Add ux.texi. * doc/gccint.texi: Include ux.texi and use it in top-level menu. * doc/ux.texi: New file. From-SVN: r265322 --- gcc/ChangeLog | 6 + gcc/Makefile.in | 2 +- gcc/doc/gccint.texi | 2 + gcc/doc/ux.texi | 600 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 609 insertions(+), 1 deletion(-) create mode 100644 gcc/doc/ux.texi diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 970f72a5676..3875841b5fb 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2018-10-19 David Malcolm + + * Makefile.in (TEXI_GCCINT_FILES): Add ux.texi. + * doc/gccint.texi: Include ux.texi and use it in top-level menu. + * doc/ux.texi: New file. + 2018-10-19 Segher Boessenkool * config/rs6000/rs6000.h (REG_ALLOC_ORDER): Move 68 (that is, CR0) to diff --git a/gcc/Makefile.in b/gcc/Makefile.in index 52240d17f3d..d2da9ff5f07 100644 --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -3134,7 +3134,7 @@ TEXI_GCCINT_FILES = gccint.texi gcc-common.texi gcc-vers.texi \ gnu.texi gpl_v3.texi fdl.texi contrib.texi languages.texi \ sourcebuild.texi gty.texi libgcc.texi cfg.texi tree-ssa.texi \ loop.texi generic.texi gimple.texi plugins.texi optinfo.texi \ - match-and-simplify.texi poly-int.texi + match-and-simplify.texi ux.texi poly-int.texi TEXI_GCCINSTALL_FILES = install.texi install-old.texi fdl.texi \ gcc-common.texi gcc-vers.texi diff --git a/gcc/doc/gccint.texi b/gcc/doc/gccint.texi index 1a1af418eb6..2554b31a952 100644 --- a/gcc/doc/gccint.texi +++ b/gcc/doc/gccint.texi @@ -125,6 +125,7 @@ Additional tutorial information is linked to from * LTO:: Using Link-Time Optimization. * Match and Simplify:: How to write expression simplification patterns for GIMPLE and GENERIC +* User Experience Guidelines:: Guidelines for implementing diagnostics and options. * Funding:: How to help assure funding for free software. * GNU Project:: The GNU Project and GNU/Linux. @@ -162,6 +163,7 @@ Additional tutorial information is linked to from @include plugins.texi @include lto.texi @include match-and-simplify.texi +@include ux.texi @include funding.texi @include gnu.texi diff --git a/gcc/doc/ux.texi b/gcc/doc/ux.texi new file mode 100644 index 00000000000..9185f68c58c --- /dev/null +++ b/gcc/doc/ux.texi @@ -0,0 +1,600 @@ +@c Copyright (C) 2018 Free Software Foundation, Inc. +@c Free Software Foundation, Inc. +@c This is part of the GCC manual. +@c For copying conditions, see the file gcc.texi. + +@node User Experience Guidelines +@chapter User Experience Guidelines +@cindex user experience guidelines +@cindex guidelines, user experience + +To borrow a slogan from + @uref{https://elm-lang.org/blog/compilers-as-assistants, Elm}, + +@quotation +@strong{Compilers should be assistants, not adversaries.} A compiler should +not just detect bugs, it should then help you understand why there is a bug. +It should not berate you in a robot voice, it should give you specific hints +that help you write better code. Ultimately, a compiler should make +programming faster and more fun! +@author Evan Czaplicki +@end quotation + +This chapter provides guidelines on how to implement diagnostics and +command-line options in ways that we hope achieve the above ideal. + +@menu +* Guidelines for Diagnostics:: How to implement diagnostics. +* Guidelines for Options:: Guidelines for command-line options. +@end menu + + +@node Guidelines for Diagnostics +@cindex guidelines for diagnostics +@cindex diagnostics, guidelines for + +@section Guidelines for Diagnostics + +@subsection Talk in terms of the user's code + +Diagnostics should be worded in terms of the user's source code, and the +source language, rather than GCC's own implementation details. + +@subsection Diagnostics are actionable +@cindex diagnostics, actionable + +A good diagnostic is @dfn{actionable}: it should assist the user in +taking action. + +Consider what an end user will want to do when encountering a diagnostic. + +Given an error, an end user will think: ``How do I fix this?'' + +Given a warning, an end user will think: + +@itemize @bullet +@item +``Is this a real problem?'' +@item +``Do I care?'' +@item +if they decide it's genuine: ``How do I fix this?'' +@end itemize + +A good diagnostic provides pertinent information to allow the user to +easily answer the above questions. + +@subsection The user's attention is important + +A perfect compiler would issue a warning on every aspect of the user's +source code that ought to be fixed, and issue no other warnings. +Naturally, this ideal is impossible to achieve. + +@cindex signal-to-noise ratio (metaphorical usage for diagnostics) +@cindex diagnostics, false positive +@cindex diagnostics, true positive +@cindex false positive +@cindex true positive + +Warnings should have a good @dfn{signal-to-noise ratio}: we should have few +@dfn{false positives} (falsely issuing a warning when no warning is +warranted) and few @dfn{false negatives} (failing to issue a warning when +one @emph{is} justified). + +Note that a false positive can mean, in practice, a warning that the +user doesn't agree with. Ideally a diagnostic should contain enough +information to allow the user to make an informed choice about whether +they should care (and how to fix it), but a balance must be drawn against +overloading the user with irrelevant data. + +@subsection Precision of Wording + +Provide the user with details that allow them to identify what the +problem is. For example, the vaguely-worded message: + +@smallexample +demo.c:1:1: warning: 'noinline' attribute ignored [-Wattributes] + 1 | int foo __attribute__((noinline)); + | ^~~ +@end smallexample + +@noindent +doesn't tell the user why the attribute was ignored, or what kind of +entity the compiler thought the attribute was being applied to (the +source location for the diagnostic is also poor; +@pxref{input_location_example,,discussion of @code{input_location}}). +A better message would be: + +@smallexample +demo.c:1:24: warning: attribute 'noinline' on variable 'foo' was + ignored [-Wattributes] + 1 | int foo __attribute__((noinline)); + | ~~~ ~~~~~~~~~~~~~~~^~~~~~~~~ +demo.c:1:24: note: attribute 'noinline' is only applicable to functions +@end smallexample + +@noindent +which spells out the missing information (and fixes the location +information, as discussed below). + +The above example uses a note to avoid a combinatorial explosion of possible +messages. + +@subsection Try the diagnostic on real-world code + +It's worth testing a new warning on many instances of real-world code, +written by different people, and seeing what it complains about, and +what it doesn't complain about. + +This may suggest heuristics that silence common false positives. + +It may also suggest ways to improve the precision of the message. + +@subsection Make mismatches clear + +Many diagnostics relate to a mismatch between two different places in the +user's source code. Examples include: +@itemize @bullet + @item + a type mismatch, where the type at a usage site does not match the type + at a declaration + + @item + the argument count at a call site does not match the parameter count + at the declaration + + @item + something is erroneously duplicated (e.g. an error, due to breaking a + uniqueness requirement, or a warning, if it's suggestive of a bug) + + @item + an ``opened'' syntactic construct (such as an open-parenthesis) is not + closed + + @c TODO: more examples? +@end itemize + +In each case, the diagnostic should indicate @strong{both} pertinent +locations (so that the user can easily see the problem and how to fix it). + +The standard way to do this is with a note (via @code{inform}). For +example: + +@smallexample + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wduplicated_cond, + "duplicated % condition")) + inform (EXPR_LOCATION (t), "previously used here"); +@end smallexample + +@noindent +which leads to: + +@smallexample +demo.c: In function 'test': +demo.c:5:17: warning: duplicated 'if' condition [-Wduplicated-cond] + 5 | else if (flag > 3) + | ~~~~~^~~ +demo.c:3:12: note: previously used here + 3 | if (flag > 3) + | ~~~~~^~~ +@end smallexample + +@noindent +The @code{inform} call should be guarded by the return value from the +@code{warning_at} call so that the note isn't emitted when the warning +is suppressed. + +For cases involving punctuation where the locations might be near +each other, they can be conditionally consolidated via +@code{gcc_rich_location::add_location_if_nearby}: + +@smallexample + auto_diagnostic_group d; + gcc_rich_location richloc (primary_loc); + bool added secondary = richloc.add_location_if_nearby (secondary_loc); + error_at (&richloc, "main message"); + if (!added secondary) + inform (secondary_loc, "message for secondary"); +@end smallexample + +@noindent +This will emit either one diagnostic with two locations: +@smallexample + demo.c:42:10: error: main message + (foo) + ~ ^ +@end smallexample + +@noindent +or two diagnostics: + +@smallexample + demo.c:42:4: error: main message + foo) + ^ + demo.c:40:2: note: message for secondary + ( + ^ +@end smallexample + +@subsection Location Information +@cindex diagnostics, locations +@cindex location information +@cindex source code, location information +@cindex caret + +GCC's @code{location_t} type can support both ordinary locations, +and locations relating to a macro expansion. + +As of GCC 6, ordinary locations changed from supporting just a +point in the user's source code to supporting three points: the +@dfn{caret} location, plus a start and a finish: + +@smallexample + a = foo && bar; + ~~~~^~~~~~ + | | | + | | finish + | caret + start +@end smallexample + +Tokens coming out of libcpp have locations of the form @code{caret == start}, +such as for @code{foo} here: + +@smallexample + a = foo && bar; + ^~~ + | | + | finish + caret == start +@end smallexample + +Compound expressions should be reported using the location of the +expression as a whole, rather than just of one token within it. + +For example, in @code{-Wformat}, rather than underlining just the first +token of a bad argument: + +@smallexample + printf("hello %i %s", (long)0, "world"); + ~^ ~ + %li +@end smallexample + +@noindent +the whole of the expression should be underlined, so that the user can +easily identify what is being referred to: + +@smallexample + printf("hello %i %s", (long)0, "world"); + ~^ ~~~~~~~ + %li +@end smallexample + +@c this was r251239 + +Avoid using the @code{input_location} global, and the diagnostic functions +that implicitly use it---use @code{error_at} and @code{warning_at} rather +than @code{error} and @code{warning}, and provide the most appropriate +@code{location_t} value available at that phase of the compilation. It's +possible to supply secondary @code{location_t} values via +@code{rich_location}. + +@noindent +@anchor{input_location_example} +For example, in the example of imprecise wording above, generating the +diagnostic using @code{warning}: + +@smallexample + // BAD: implicitly uses @code{input_location} + warning (OPT_Wattributes, "%qE attribute ignored", name); +@end smallexample + +@noindent +leads to: + +@smallexample +// BAD: uses @code{input_location} +demo.c:1:1: warning: 'noinline' attribute ignored [-Wattributes] + 1 | int foo __attribute__((noinline)); + | ^~~ +@end smallexample + +@noindent +which thus happened to use the location of the @code{int} token, rather +than that of the attribute. Using @code{warning_at} with the location of +the attribute, providing the location of the declaration in question +as a secondary location, and adding a note: + +@smallexample + auto_diagnostic_group d; + gcc_rich_location richloc (attrib_loc); + richloc.add_range (decl_loc); + if (warning_at (OPT_Wattributes, &richloc, + "attribute %qE on variable %qE was ignored", name)) + inform (attrib_loc, "attribute %qE is only applicable to functions"); +@end smallexample + +@noindent +would lead to: + +@smallexample +// OK: use location of attribute, with a secondary location +demo.c:1:24: warning: attribute 'noinline' on variable 'foo' was + ignored [-Wattributes] + 1 | int foo __attribute__((noinline)); + | ~~~ ~~~~~~~~~~~~~~~^~~~~~~~~ +demo.c:1:24: note: attribute 'noinline' is only applicable to functions +@end smallexample + +@c TODO labelling of ranges + +@subsection Coding Conventions + +See the @uref{https://gcc.gnu.org/codingconventions.html#Diagnostics, +diagnostics section} of the GCC coding conventions. + +In the C++ front end, when comparing two types in a message, use @samp{%H} +and @samp{%I} rather than @samp{%T}, as this allows the diagnostics +subsystem to highlight differences between template-based types. +For example, rather than using @samp{%qT}: + +@smallexample + // BAD: a pair of %qT used in C++ front end for type comparison + error_at (loc, "could not convert %qE from %qT to %qT", expr, + TREE_TYPE (expr), type); +@end smallexample + +@noindent +which could lead to: + +@smallexample +error: could not convert 'map()' from 'map' + to 'map' +@end smallexample + +@noindent +using @samp{%H} and @samp{%I} (via @samp{%qH} and @samp{%qI}): + +@smallexample + // OK: compare types in C++ front end via %qH and %qI + error_at (loc, "could not convert %qE from %qH to %qI", expr, + TREE_TYPE (expr), type); +@end smallexample + +@noindent +allows the above output to be simplified to: + +@smallexample +error: could not convert 'map()' from 'map<[...],double>' + to 'map<[...],int>' +@end smallexample + +@noindent +where the @code{double} and @code{int} are colorized to highlight them. + +@c %H and %I were added in r248698. + +Use @code{auto_diagnostic_group} when issuing multiple related +diagnostics (seen in various examples on this page). This informs the +diagnostic subsystem that all diagnostics issued within the lifetime +of the @code{auto_diagnostic_group} are related. (Currently it doesn't +do anything with this information, but we may implement that in the +future). + +@subsection Spelling and Terminology + +See the @uref{https://gcc.gnu.org/codingconventions.html#Spelling +Spelling, terminology and markup} section of the GCC coding conventions. + +@subsection Fix-it hints +@cindex fix-it hints +@cindex diagnostics guidelines, fix-it hints + +GCC's diagnostic subsystem can emit @dfn{fix-it hints}: small suggested +edits to the user's source code. + +They are printed by default underneath the code in question. They +can also be viewed via @option{-fdiagnostics-generate-patch} and +@option{-fdiagnostics-parseable-fixits}. With the latter, an IDE +ought to be able to offer to automatically apply the suggested fix. + +Fix-it hints can be added to a diagnostic by using a @code{rich_location} +rather than a @code{location_t} - the fix-it hints are added to the +@code{rich_location} using one of the various @code{add_fixit} member +functions of @code{rich_location}. They are documented with +@code{rich_location} in @file{libcpp/line-map.h}. +It's easiest to use the @code{gcc_rich_location} subclass of +@code{rich_location} found in @file{gcc-rich-location.h}, as this +implicitly supplies the @code{line_table} variable. + +For example: + +@smallexample + if (const char *suggestion = hint.suggestion ()) + @{ + gcc_rich_location richloc (location); + richloc.add_fixit_replace (suggestion); + error_at (&richloc, + "%qE does not name a type; did you mean %qs?", + id, suggestion); + @} +@end smallexample + +@noindent +which can lead to: + +@smallexample +spellcheck-typenames.C:73:1: error: 'singed' does not name a type; did + you mean 'signed'? + 73 | singed char ch; + | ^~~~~~ + | signed +@end smallexample + +Non-trivial edits can be built up by adding multiple fix-it hints to one +@code{rich_location}. It's best to express the edits in terms of the +locations of individual tokens. Various handy functions for adding +fix-it hints for idiomatic C and C++ can be seen in +@file{gcc-rich-location.h}. + +@subsubsection Fix-it hints should work + +When implementing a fix-it hint, please verify that the suggested edit +leads to fixed, compilable code. (Unfortunately, this currently must be +done by hand using @option{-fdiagnostics-generate-patch}. It would be +good to have an automated way of verifying that fix-it hints actually fix +the code). + +For example, a ``gotcha'' here is to forget to add a space when adding a +missing reserved word. Consider a C++ fix-it hint that adds +@code{typename} in front of a template declaration. A naive way to +implement this might be: + +@smallexample +gcc_rich_location richloc (loc); +// BAD: insertion is missing a trailing space +richloc.add_fixit_insert_before ("typename"); +error_at (&richloc, "need % before %<%T::%E%> because " + "%qT is a dependent scope", + parser->scope, id, parser->scope); +@end smallexample + +@noindent +When applied to the code, this might lead to: + +@smallexample +T::type x; +@end smallexample + +@noindent +being ``corrected'' to: + +@smallexample +typenameT::type x; +@end smallexample + +@noindent +In this case, the correct thing to do is to add a trailing space after +@code{typename}: + +@smallexample +gcc_rich_location richloc (loc); +// OK: note that here we have a trailing space +richloc.add_fixit_insert_before ("typename "); +error_at (&richloc, "need % before %<%T::%E%> because " + "%qT is a dependent scope", + parser->scope, id, parser->scope); +@end smallexample + +@noindent +leading to this corrected code: + +@smallexample +typename T::type x; +@end smallexample + +@subsubsection Express deletion in terms of deletion, not replacement + +It's best to express deletion suggestions in terms of deletion fix-it +hints, rather than replacement fix-it hints. For example, consider this: + +@smallexample + auto_diagnostic_group d; + gcc_rich_location richloc (location_of (retval)); + tree name = DECL_NAME (arg); + richloc.add_fixit_replace (IDENTIFIER_POINTER (name)); + warning_at (&richloc, OPT_Wredundant_move, + "redundant move in return statement"); +@end smallexample + +@noindent +which is intended to e.g. replace a @code{std::move} with the underlying +value: + +@smallexample + return std::move (retval); + ~~~~~~~~~~^~~~~~~~ + retval +@end smallexample + +@noindent +where the change has been expressed as replacement, replacing +with the name of the declaration. +This works for simple cases, but consider this case: + +@smallexample +#ifdef SOME_CONFIG_FLAG +# define CONFIGURY_GLOBAL global_a +#else +# define CONFIGURY_GLOBAL global_b +#endif + +int fn () +@{ + return std::move (CONFIGURY_GLOBAL /* some comment */); +@} +@end smallexample + +@noindent +The above implementation erroneously strips out the macro and the +comment in the fix-it hint: + +@smallexample + return std::move (CONFIGURY_GLOBAL /* some comment */); + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + global_a +@end smallexample + +@noindent +and thus this resulting code: + +@smallexample + return global_a; +@end smallexample + +@noindent +It's better to do deletions in terms of deletions; deleting the +@code{std::move (} and the trailing close-paren, leading to +this: + +@smallexample + return std::move (CONFIGURY_GLOBAL /* some comment */); + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CONFIGURY_GLOBAL /* some comment */ +@end smallexample + +@noindent +and thus this result: + +@smallexample + return CONFIGURY_GLOBAL /* some comment */; +@end smallexample + +@noindent +Unfortunately, the pertinent @code{location_t} values are not always +available. + +@c the above was https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01474.html + +@subsubsection Multiple suggestions + +In the rare cases where you need to suggest more than one mutually +exclusive solution to a problem, this can be done by emitting +multiple notes and calling +@code{rich_location::fixits_cannot_be_auto_applied} on each note's +@code{rich_location}. If this is called, then the fix-it hints in +the @code{rich_location} will be printed, but will not be added to +generated patches. + + +@node Guidelines for Options +@cindex command-line options, guidelines for +@cindex options, guidelines for +@cindex guidelines for options + +@section Guidelines for Options + +@c TODO -- 2.30.2