]>
Commit | Line | Data |
---|---|---|
92646d25 DM |
1 | @c Copyright (C) 2018 Free Software Foundation, Inc. |
2 | @c Free Software Foundation, Inc. | |
3 | @c This is part of the GCC manual. | |
4 | @c For copying conditions, see the file gcc.texi. | |
5 | ||
6 | @node User Experience Guidelines | |
7 | @chapter User Experience Guidelines | |
8 | @cindex user experience guidelines | |
9 | @cindex guidelines, user experience | |
10 | ||
11 | To borrow a slogan from | |
12 | @uref{https://elm-lang.org/blog/compilers-as-assistants, Elm}, | |
13 | ||
14 | @quotation | |
15 | @strong{Compilers should be assistants, not adversaries.} A compiler should | |
16 | not just detect bugs, it should then help you understand why there is a bug. | |
17 | It should not berate you in a robot voice, it should give you specific hints | |
18 | that help you write better code. Ultimately, a compiler should make | |
19 | programming faster and more fun! | |
20 | @author Evan Czaplicki | |
21 | @end quotation | |
22 | ||
23 | This chapter provides guidelines on how to implement diagnostics and | |
24 | command-line options in ways that we hope achieve the above ideal. | |
25 | ||
26 | @menu | |
27 | * Guidelines for Diagnostics:: How to implement diagnostics. | |
28 | * Guidelines for Options:: Guidelines for command-line options. | |
29 | @end menu | |
30 | ||
31 | ||
32 | @node Guidelines for Diagnostics | |
33 | @cindex guidelines for diagnostics | |
34 | @cindex diagnostics, guidelines for | |
35 | ||
36 | @section Guidelines for Diagnostics | |
37 | ||
38 | @subsection Talk in terms of the user's code | |
39 | ||
40 | Diagnostics should be worded in terms of the user's source code, and the | |
41 | source language, rather than GCC's own implementation details. | |
42 | ||
43 | @subsection Diagnostics are actionable | |
44 | @cindex diagnostics, actionable | |
45 | ||
46 | A good diagnostic is @dfn{actionable}: it should assist the user in | |
47 | taking action. | |
48 | ||
49 | Consider what an end user will want to do when encountering a diagnostic. | |
50 | ||
51 | Given an error, an end user will think: ``How do I fix this?'' | |
52 | ||
53 | Given a warning, an end user will think: | |
54 | ||
55 | @itemize @bullet | |
56 | @item | |
57 | ``Is this a real problem?'' | |
58 | @item | |
59 | ``Do I care?'' | |
60 | @item | |
61 | if they decide it's genuine: ``How do I fix this?'' | |
62 | @end itemize | |
63 | ||
64 | A good diagnostic provides pertinent information to allow the user to | |
65 | easily answer the above questions. | |
66 | ||
67 | @subsection The user's attention is important | |
68 | ||
69 | A perfect compiler would issue a warning on every aspect of the user's | |
70 | source code that ought to be fixed, and issue no other warnings. | |
71 | Naturally, this ideal is impossible to achieve. | |
72 | ||
73 | @cindex signal-to-noise ratio (metaphorical usage for diagnostics) | |
74 | @cindex diagnostics, false positive | |
75 | @cindex diagnostics, true positive | |
76 | @cindex false positive | |
77 | @cindex true positive | |
78 | ||
79 | Warnings should have a good @dfn{signal-to-noise ratio}: we should have few | |
80 | @dfn{false positives} (falsely issuing a warning when no warning is | |
81 | warranted) and few @dfn{false negatives} (failing to issue a warning when | |
82 | one @emph{is} justified). | |
83 | ||
84 | Note that a false positive can mean, in practice, a warning that the | |
85 | user doesn't agree with. Ideally a diagnostic should contain enough | |
86 | information to allow the user to make an informed choice about whether | |
87 | they should care (and how to fix it), but a balance must be drawn against | |
88 | overloading the user with irrelevant data. | |
89 | ||
90 | @subsection Precision of Wording | |
91 | ||
92 | Provide the user with details that allow them to identify what the | |
93 | problem is. For example, the vaguely-worded message: | |
94 | ||
95 | @smallexample | |
96 | demo.c:1:1: warning: 'noinline' attribute ignored [-Wattributes] | |
97 | 1 | int foo __attribute__((noinline)); | |
98 | | ^~~ | |
99 | @end smallexample | |
100 | ||
101 | @noindent | |
102 | doesn't tell the user why the attribute was ignored, or what kind of | |
103 | entity the compiler thought the attribute was being applied to (the | |
104 | source location for the diagnostic is also poor; | |
105 | @pxref{input_location_example,,discussion of @code{input_location}}). | |
106 | A better message would be: | |
107 | ||
108 | @smallexample | |
109 | demo.c:1:24: warning: attribute 'noinline' on variable 'foo' was | |
110 | ignored [-Wattributes] | |
111 | 1 | int foo __attribute__((noinline)); | |
112 | | ~~~ ~~~~~~~~~~~~~~~^~~~~~~~~ | |
113 | demo.c:1:24: note: attribute 'noinline' is only applicable to functions | |
114 | @end smallexample | |
115 | ||
116 | @noindent | |
117 | which spells out the missing information (and fixes the location | |
118 | information, as discussed below). | |
119 | ||
120 | The above example uses a note to avoid a combinatorial explosion of possible | |
121 | messages. | |
122 | ||
123 | @subsection Try the diagnostic on real-world code | |
124 | ||
125 | It's worth testing a new warning on many instances of real-world code, | |
126 | written by different people, and seeing what it complains about, and | |
127 | what it doesn't complain about. | |
128 | ||
129 | This may suggest heuristics that silence common false positives. | |
130 | ||
131 | It may also suggest ways to improve the precision of the message. | |
132 | ||
133 | @subsection Make mismatches clear | |
134 | ||
135 | Many diagnostics relate to a mismatch between two different places in the | |
136 | user's source code. Examples include: | |
137 | @itemize @bullet | |
138 | @item | |
139 | a type mismatch, where the type at a usage site does not match the type | |
140 | at a declaration | |
141 | ||
142 | @item | |
143 | the argument count at a call site does not match the parameter count | |
144 | at the declaration | |
145 | ||
146 | @item | |
147 | something is erroneously duplicated (e.g. an error, due to breaking a | |
148 | uniqueness requirement, or a warning, if it's suggestive of a bug) | |
149 | ||
150 | @item | |
151 | an ``opened'' syntactic construct (such as an open-parenthesis) is not | |
152 | closed | |
153 | ||
154 | @c TODO: more examples? | |
155 | @end itemize | |
156 | ||
157 | In each case, the diagnostic should indicate @strong{both} pertinent | |
158 | locations (so that the user can easily see the problem and how to fix it). | |
159 | ||
160 | The standard way to do this is with a note (via @code{inform}). For | |
161 | example: | |
162 | ||
163 | @smallexample | |
164 | auto_diagnostic_group d; | |
165 | if (warning_at (loc, OPT_Wduplicated_cond, | |
166 | "duplicated %<if%> condition")) | |
167 | inform (EXPR_LOCATION (t), "previously used here"); | |
168 | @end smallexample | |
169 | ||
170 | @noindent | |
171 | which leads to: | |
172 | ||
173 | @smallexample | |
174 | demo.c: In function 'test': | |
175 | demo.c:5:17: warning: duplicated 'if' condition [-Wduplicated-cond] | |
176 | 5 | else if (flag > 3) | |
177 | | ~~~~~^~~ | |
178 | demo.c:3:12: note: previously used here | |
179 | 3 | if (flag > 3) | |
180 | | ~~~~~^~~ | |
181 | @end smallexample | |
182 | ||
183 | @noindent | |
184 | The @code{inform} call should be guarded by the return value from the | |
185 | @code{warning_at} call so that the note isn't emitted when the warning | |
186 | is suppressed. | |
187 | ||
188 | For cases involving punctuation where the locations might be near | |
189 | each other, they can be conditionally consolidated via | |
190 | @code{gcc_rich_location::add_location_if_nearby}: | |
191 | ||
192 | @smallexample | |
193 | auto_diagnostic_group d; | |
194 | gcc_rich_location richloc (primary_loc); | |
195 | bool added secondary = richloc.add_location_if_nearby (secondary_loc); | |
196 | error_at (&richloc, "main message"); | |
197 | if (!added secondary) | |
198 | inform (secondary_loc, "message for secondary"); | |
199 | @end smallexample | |
200 | ||
201 | @noindent | |
202 | This will emit either one diagnostic with two locations: | |
203 | @smallexample | |
204 | demo.c:42:10: error: main message | |
205 | (foo) | |
206 | ~ ^ | |
207 | @end smallexample | |
208 | ||
209 | @noindent | |
210 | or two diagnostics: | |
211 | ||
212 | @smallexample | |
213 | demo.c:42:4: error: main message | |
214 | foo) | |
215 | ^ | |
216 | demo.c:40:2: note: message for secondary | |
217 | ( | |
218 | ^ | |
219 | @end smallexample | |
220 | ||
221 | @subsection Location Information | |
222 | @cindex diagnostics, locations | |
223 | @cindex location information | |
224 | @cindex source code, location information | |
225 | @cindex caret | |
226 | ||
227 | GCC's @code{location_t} type can support both ordinary locations, | |
228 | and locations relating to a macro expansion. | |
229 | ||
230 | As of GCC 6, ordinary locations changed from supporting just a | |
231 | point in the user's source code to supporting three points: the | |
232 | @dfn{caret} location, plus a start and a finish: | |
233 | ||
234 | @smallexample | |
235 | a = foo && bar; | |
236 | ~~~~^~~~~~ | |
237 | | | | | |
238 | | | finish | |
239 | | caret | |
240 | start | |
241 | @end smallexample | |
242 | ||
243 | Tokens coming out of libcpp have locations of the form @code{caret == start}, | |
244 | such as for @code{foo} here: | |
245 | ||
246 | @smallexample | |
247 | a = foo && bar; | |
248 | ^~~ | |
249 | | | | |
250 | | finish | |
251 | caret == start | |
252 | @end smallexample | |
253 | ||
254 | Compound expressions should be reported using the location of the | |
255 | expression as a whole, rather than just of one token within it. | |
256 | ||
257 | For example, in @code{-Wformat}, rather than underlining just the first | |
258 | token of a bad argument: | |
259 | ||
260 | @smallexample | |
261 | printf("hello %i %s", (long)0, "world"); | |
262 | ~^ ~ | |
263 | %li | |
264 | @end smallexample | |
265 | ||
266 | @noindent | |
267 | the whole of the expression should be underlined, so that the user can | |
268 | easily identify what is being referred to: | |
269 | ||
270 | @smallexample | |
271 | printf("hello %i %s", (long)0, "world"); | |
272 | ~^ ~~~~~~~ | |
273 | %li | |
274 | @end smallexample | |
275 | ||
276 | @c this was r251239 | |
277 | ||
278 | Avoid using the @code{input_location} global, and the diagnostic functions | |
279 | that implicitly use it---use @code{error_at} and @code{warning_at} rather | |
280 | than @code{error} and @code{warning}, and provide the most appropriate | |
281 | @code{location_t} value available at that phase of the compilation. It's | |
282 | possible to supply secondary @code{location_t} values via | |
283 | @code{rich_location}. | |
284 | ||
285 | @noindent | |
286 | @anchor{input_location_example} | |
287 | For example, in the example of imprecise wording above, generating the | |
288 | diagnostic using @code{warning}: | |
289 | ||
290 | @smallexample | |
291 | // BAD: implicitly uses @code{input_location} | |
292 | warning (OPT_Wattributes, "%qE attribute ignored", name); | |
293 | @end smallexample | |
294 | ||
295 | @noindent | |
296 | leads to: | |
297 | ||
298 | @smallexample | |
299 | // BAD: uses @code{input_location} | |
300 | demo.c:1:1: warning: 'noinline' attribute ignored [-Wattributes] | |
301 | 1 | int foo __attribute__((noinline)); | |
302 | | ^~~ | |
303 | @end smallexample | |
304 | ||
305 | @noindent | |
306 | which thus happened to use the location of the @code{int} token, rather | |
307 | than that of the attribute. Using @code{warning_at} with the location of | |
308 | the attribute, providing the location of the declaration in question | |
309 | as a secondary location, and adding a note: | |
310 | ||
311 | @smallexample | |
312 | auto_diagnostic_group d; | |
313 | gcc_rich_location richloc (attrib_loc); | |
314 | richloc.add_range (decl_loc); | |
315 | if (warning_at (OPT_Wattributes, &richloc, | |
316 | "attribute %qE on variable %qE was ignored", name)) | |
317 | inform (attrib_loc, "attribute %qE is only applicable to functions"); | |
318 | @end smallexample | |
319 | ||
320 | @noindent | |
321 | would lead to: | |
322 | ||
323 | @smallexample | |
324 | // OK: use location of attribute, with a secondary location | |
325 | demo.c:1:24: warning: attribute 'noinline' on variable 'foo' was | |
326 | ignored [-Wattributes] | |
327 | 1 | int foo __attribute__((noinline)); | |
328 | | ~~~ ~~~~~~~~~~~~~~~^~~~~~~~~ | |
329 | demo.c:1:24: note: attribute 'noinline' is only applicable to functions | |
330 | @end smallexample | |
331 | ||
332 | @c TODO labelling of ranges | |
333 | ||
334 | @subsection Coding Conventions | |
335 | ||
336 | See the @uref{https://gcc.gnu.org/codingconventions.html#Diagnostics, | |
337 | diagnostics section} of the GCC coding conventions. | |
338 | ||
339 | In the C++ front end, when comparing two types in a message, use @samp{%H} | |
340 | and @samp{%I} rather than @samp{%T}, as this allows the diagnostics | |
341 | subsystem to highlight differences between template-based types. | |
342 | For example, rather than using @samp{%qT}: | |
343 | ||
344 | @smallexample | |
345 | // BAD: a pair of %qT used in C++ front end for type comparison | |
346 | error_at (loc, "could not convert %qE from %qT to %qT", expr, | |
347 | TREE_TYPE (expr), type); | |
348 | @end smallexample | |
349 | ||
350 | @noindent | |
351 | which could lead to: | |
352 | ||
353 | @smallexample | |
354 | error: could not convert 'map<int, double>()' from 'map<int,double>' | |
355 | to 'map<int,int>' | |
356 | @end smallexample | |
357 | ||
358 | @noindent | |
359 | using @samp{%H} and @samp{%I} (via @samp{%qH} and @samp{%qI}): | |
360 | ||
361 | @smallexample | |
362 | // OK: compare types in C++ front end via %qH and %qI | |
363 | error_at (loc, "could not convert %qE from %qH to %qI", expr, | |
364 | TREE_TYPE (expr), type); | |
365 | @end smallexample | |
366 | ||
367 | @noindent | |
368 | allows the above output to be simplified to: | |
369 | ||
370 | @smallexample | |
371 | error: could not convert 'map<int, double>()' from 'map<[...],double>' | |
372 | to 'map<[...],int>' | |
373 | @end smallexample | |
374 | ||
375 | @noindent | |
376 | where the @code{double} and @code{int} are colorized to highlight them. | |
377 | ||
378 | @c %H and %I were added in r248698. | |
379 | ||
380 | Use @code{auto_diagnostic_group} when issuing multiple related | |
381 | diagnostics (seen in various examples on this page). This informs the | |
382 | diagnostic subsystem that all diagnostics issued within the lifetime | |
383 | of the @code{auto_diagnostic_group} are related. (Currently it doesn't | |
384 | do anything with this information, but we may implement that in the | |
385 | future). | |
386 | ||
387 | @subsection Spelling and Terminology | |
388 | ||
389 | See the @uref{https://gcc.gnu.org/codingconventions.html#Spelling | |
390 | Spelling, terminology and markup} section of the GCC coding conventions. | |
391 | ||
392 | @subsection Fix-it hints | |
393 | @cindex fix-it hints | |
394 | @cindex diagnostics guidelines, fix-it hints | |
395 | ||
396 | GCC's diagnostic subsystem can emit @dfn{fix-it hints}: small suggested | |
397 | edits to the user's source code. | |
398 | ||
399 | They are printed by default underneath the code in question. They | |
400 | can also be viewed via @option{-fdiagnostics-generate-patch} and | |
401 | @option{-fdiagnostics-parseable-fixits}. With the latter, an IDE | |
402 | ought to be able to offer to automatically apply the suggested fix. | |
403 | ||
404 | Fix-it hints can be added to a diagnostic by using a @code{rich_location} | |
405 | rather than a @code{location_t} - the fix-it hints are added to the | |
406 | @code{rich_location} using one of the various @code{add_fixit} member | |
407 | functions of @code{rich_location}. They are documented with | |
408 | @code{rich_location} in @file{libcpp/line-map.h}. | |
409 | It's easiest to use the @code{gcc_rich_location} subclass of | |
410 | @code{rich_location} found in @file{gcc-rich-location.h}, as this | |
411 | implicitly supplies the @code{line_table} variable. | |
412 | ||
413 | For example: | |
414 | ||
415 | @smallexample | |
416 | if (const char *suggestion = hint.suggestion ()) | |
417 | @{ | |
418 | gcc_rich_location richloc (location); | |
419 | richloc.add_fixit_replace (suggestion); | |
420 | error_at (&richloc, | |
421 | "%qE does not name a type; did you mean %qs?", | |
422 | id, suggestion); | |
423 | @} | |
424 | @end smallexample | |
425 | ||
426 | @noindent | |
427 | which can lead to: | |
428 | ||
429 | @smallexample | |
430 | spellcheck-typenames.C:73:1: error: 'singed' does not name a type; did | |
431 | you mean 'signed'? | |
432 | 73 | singed char ch; | |
433 | | ^~~~~~ | |
434 | | signed | |
435 | @end smallexample | |
436 | ||
437 | Non-trivial edits can be built up by adding multiple fix-it hints to one | |
438 | @code{rich_location}. It's best to express the edits in terms of the | |
439 | locations of individual tokens. Various handy functions for adding | |
440 | fix-it hints for idiomatic C and C++ can be seen in | |
441 | @file{gcc-rich-location.h}. | |
442 | ||
443 | @subsubsection Fix-it hints should work | |
444 | ||
445 | When implementing a fix-it hint, please verify that the suggested edit | |
446 | leads to fixed, compilable code. (Unfortunately, this currently must be | |
447 | done by hand using @option{-fdiagnostics-generate-patch}. It would be | |
448 | good to have an automated way of verifying that fix-it hints actually fix | |
449 | the code). | |
450 | ||
451 | For example, a ``gotcha'' here is to forget to add a space when adding a | |
452 | missing reserved word. Consider a C++ fix-it hint that adds | |
453 | @code{typename} in front of a template declaration. A naive way to | |
454 | implement this might be: | |
455 | ||
456 | @smallexample | |
457 | gcc_rich_location richloc (loc); | |
458 | // BAD: insertion is missing a trailing space | |
459 | richloc.add_fixit_insert_before ("typename"); | |
460 | error_at (&richloc, "need %<typename%> before %<%T::%E%> because " | |
461 | "%qT is a dependent scope", | |
462 | parser->scope, id, parser->scope); | |
463 | @end smallexample | |
464 | ||
465 | @noindent | |
466 | When applied to the code, this might lead to: | |
467 | ||
468 | @smallexample | |
469 | T::type x; | |
470 | @end smallexample | |
471 | ||
472 | @noindent | |
473 | being ``corrected'' to: | |
474 | ||
475 | @smallexample | |
476 | typenameT::type x; | |
477 | @end smallexample | |
478 | ||
479 | @noindent | |
480 | In this case, the correct thing to do is to add a trailing space after | |
481 | @code{typename}: | |
482 | ||
483 | @smallexample | |
484 | gcc_rich_location richloc (loc); | |
485 | // OK: note that here we have a trailing space | |
486 | richloc.add_fixit_insert_before ("typename "); | |
487 | error_at (&richloc, "need %<typename%> before %<%T::%E%> because " | |
488 | "%qT is a dependent scope", | |
489 | parser->scope, id, parser->scope); | |
490 | @end smallexample | |
491 | ||
492 | @noindent | |
493 | leading to this corrected code: | |
494 | ||
495 | @smallexample | |
496 | typename T::type x; | |
497 | @end smallexample | |
498 | ||
499 | @subsubsection Express deletion in terms of deletion, not replacement | |
500 | ||
501 | It's best to express deletion suggestions in terms of deletion fix-it | |
502 | hints, rather than replacement fix-it hints. For example, consider this: | |
503 | ||
504 | @smallexample | |
505 | auto_diagnostic_group d; | |
506 | gcc_rich_location richloc (location_of (retval)); | |
507 | tree name = DECL_NAME (arg); | |
508 | richloc.add_fixit_replace (IDENTIFIER_POINTER (name)); | |
509 | warning_at (&richloc, OPT_Wredundant_move, | |
510 | "redundant move in return statement"); | |
511 | @end smallexample | |
512 | ||
513 | @noindent | |
514 | which is intended to e.g. replace a @code{std::move} with the underlying | |
515 | value: | |
516 | ||
517 | @smallexample | |
518 | return std::move (retval); | |
519 | ~~~~~~~~~~^~~~~~~~ | |
520 | retval | |
521 | @end smallexample | |
522 | ||
523 | @noindent | |
524 | where the change has been expressed as replacement, replacing | |
525 | with the name of the declaration. | |
526 | This works for simple cases, but consider this case: | |
527 | ||
528 | @smallexample | |
529 | #ifdef SOME_CONFIG_FLAG | |
530 | # define CONFIGURY_GLOBAL global_a | |
531 | #else | |
532 | # define CONFIGURY_GLOBAL global_b | |
533 | #endif | |
534 | ||
535 | int fn () | |
536 | @{ | |
537 | return std::move (CONFIGURY_GLOBAL /* some comment */); | |
538 | @} | |
539 | @end smallexample | |
540 | ||
541 | @noindent | |
542 | The above implementation erroneously strips out the macro and the | |
543 | comment in the fix-it hint: | |
544 | ||
545 | @smallexample | |
546 | return std::move (CONFIGURY_GLOBAL /* some comment */); | |
547 | ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
548 | global_a | |
549 | @end smallexample | |
550 | ||
551 | @noindent | |
552 | and thus this resulting code: | |
553 | ||
554 | @smallexample | |
555 | return global_a; | |
556 | @end smallexample | |
557 | ||
558 | @noindent | |
559 | It's better to do deletions in terms of deletions; deleting the | |
560 | @code{std::move (} and the trailing close-paren, leading to | |
561 | this: | |
562 | ||
563 | @smallexample | |
564 | return std::move (CONFIGURY_GLOBAL /* some comment */); | |
565 | ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
566 | CONFIGURY_GLOBAL /* some comment */ | |
567 | @end smallexample | |
568 | ||
569 | @noindent | |
570 | and thus this result: | |
571 | ||
572 | @smallexample | |
573 | return CONFIGURY_GLOBAL /* some comment */; | |
574 | @end smallexample | |
575 | ||
576 | @noindent | |
577 | Unfortunately, the pertinent @code{location_t} values are not always | |
578 | available. | |
579 | ||
580 | @c the above was https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01474.html | |
581 | ||
582 | @subsubsection Multiple suggestions | |
583 | ||
584 | In the rare cases where you need to suggest more than one mutually | |
585 | exclusive solution to a problem, this can be done by emitting | |
586 | multiple notes and calling | |
587 | @code{rich_location::fixits_cannot_be_auto_applied} on each note's | |
588 | @code{rich_location}. If this is called, then the fix-it hints in | |
589 | the @code{rich_location} will be printed, but will not be added to | |
590 | generated patches. | |
591 | ||
592 | ||
593 | @node Guidelines for Options | |
594 | @cindex command-line options, guidelines for | |
595 | @cindex options, guidelines for | |
596 | @cindex guidelines for options | |
597 | ||
598 | @section Guidelines for Options | |
599 | ||
600 | @c TODO |