RFC: Standardization of reports' sentence spacing

Clang Frontend Static Analyzer
1

Background

Currently there are different spacing conventions in the Clang Static Analyzer (CSA). Some of the user facing error messages and warnings use single space after the end of a sentence, but some of them use double spaces.

There are two type of spacing conventions:

  • English spacing: removed spaces around most punctuation marks, but double-spaced after sentences, colons, and semicolons.
  • French spacing: inserted spaces around most punctuation marks, but single-spaced after sentences, colons, and semicolons.

(I have learnt about this on Wikipedia: History of sentence spacing - Wikipedia)

Problem

Currently the two spacing conventions are mixed in teh CSA:

Double space in BugReporter.cpp:

C.getActivePath().push_front(
    std::make_shared<PathDiagnosticControlFlowPiece>(
        Start, End, "Loop condition is false.  Exiting loop"));
...
...
...

C.getActivePath().push_front(
    std::make_shared<PathDiagnosticControlFlowPiece>(
        Start, End, "Loop condition is true.  Entering loop body"));

Single space in BugReporter.cpp:

switch (S->getStmtClass()) {
default:
  os << "No cases match in the switch statement. "
    "Control jumps to line "

Occurence of single vs. double spaces

I have written a simple Python script to measure the occurences of single vs. double spaces in the CSA, Clang frontend, and clang-tidy. These are the numbers:

Clang Static Analyzer:

Overall numbers: single space = 107, double space = 15

From these numbers 81 in single spaces category are in help text rather than emitted
diagnostics: 64 in AnalyzerOptions.def, 15 in Checkers.td, and 2 in Analyses.def.

Emitted bug reports and notes: single space = 26, double space = 15.

Clang frontend:

Overall numbers: single space = 48, double space = 1

Form these 35 are compiler diagnostics and 13 are help text and and tool messages. 
The one double space is in a user-facing file: DiagnosticSemaKinds.td:407.

Clang-tidy:

Overall numbers: single space = 33, double space = 0

Out of the 33 there are 3 cases are check diagnostics.

How do other projects work in this space?

GCC has a rule for it in its Coding Conventions, but only for its Texinfo
documentation, not for diagnostics:

Full stops that end sentences should be followed by two spaces or by end of line.

Its diagnostic guidelines do not mention sentence spacing. The only rule for messages I could find is in the contrib/check-internal-format-escaping.py lint script, which says “After a sentence there should be two spaces.” This script has to be run manually on gcc.pot.

However, it does not seem that this rule is applied consistently. I went through gcc.pot and there are mixed cases of using double and single spaces. I have not run my script on GCC, so I do not have exact numbers, but I will extend it and come back with numbers if needed.

Cppcheck does not seem to have any rules on this. The only rule around this manner is:

It's very important that warning messages are well written and with enough details.

This is in theirphilosophy.md file while their CONTRIBUTING.md file does not contain any information.

Proposed change

Since changing all double spaces to single space or the other way around does affect users I think the change should be addressed based on the discussion in this RFC. However, I would suggest that we should add more details on spacing after the discussion in this RFC to “Clang” CFE Internals Manual - Clang

This issue came up in [analyzer] Fix double space in generateMinimalDiagForBlockEdge emitted notes by benedekaibas · Pull Request #228777 · llvm/llvm-project · GitHub. If we can settle on a common agreement I will update the PR based on that. After that I would also open a PR with the agreed changes in this RFC if any.

1 Like
2

That’s not a bad idea, but I think this bullet covers the usual expectations:

Diagnostics in Clang do not start with a capital letter and do not end with punctuation.

The static analyzer using punctuation is inconsistent with the rest of the compiler; if we were considering sweeping changes, one change to at least consider is to stop using complete sentences in static analyzer diagnostics. (This inconsistency was a bit more palatable when the static analyzer was basically only invoked via a separate tool like scan-build, but now that it’s integrated into clang-tidy, this incosistency is becoming more obvious.)

If we’re insisting on sticking with full sentences using punctuation for the static analyzer, I prefer single space over double space to try to keep the diagnostic length down. But it’s not a strong preference.

3 Likes
3

As an ESL who’s native language is Russian, I always felt that double space after a period is a typo, so my preference is single space.

I’m not sure what are the authoritative source on the matter for English, but I found the following in the Chicago Manual of Style (18th edition):

6.7 Punctuation and space—one space or two? In typeset matter, one
space, not two, should be used between two sentences—whether the
first ends in a period, a question mark, an exclamation point, or a closing
quotation mark or parenthesis.

1 Like
4

I support this idea, and – especially based on the statictics – I support changing to single spaces.

I also completely agree with this, and I support eliminating the complete sentences from the static analyzer diagnostics.

In fact, even the static analyzer consistently omits the full stop at the end of the message, so the static analyzer only prints full sentences in the case when a message consists of multiple sentences:

First sentence is complete. Second sentence lacks the full stop at the end

I personally think that this is really ugly, so I strongly support replacing these full stops in the middle the message with semicolons. (According to @bkaibas01’s numbers, this would affect 41 diagnostics, which sounds manageable – most of the messages would remain unchanged.)

1 Like
5

I think we have a common agreement so far on using single spaces. For me it also makes sense to make the CSA consistent with the compiler and remove punctuation. The only thing I want to highlight is that the CSA’s warnings, notes can get extremely long compared to the compiler’s diagnostics.

For example in CheckSecuritySyntaxOnly.cpp:

  PathDiagnosticLocation CELoc =
    PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
  BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy,
                     "Potential insecure memory buffer bounds restriction in "
                     "call 'strcat'",
                     "Security",
                     "Call to function 'strcat' is insecure as it does not "
                     "provide bounding of the memory buffer. Replace "
                     "unbounded copy functions with analogous functions that "
                     "support length arguments such as 'strlcat'. CWE-119.",
                     CELoc, CE->getCallee()->getSourceRange());

The warning currently looks like this for a simple example:

example.m:7:3: warning: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer.
      Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'.
      CWE-119 [security.insecureAPI.strcpy]
    7 |   strcat(x, y);
      |   ^~~~~~
example.m:7:3: note: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer.
      Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'.
      CWE-119
    7 |   strcat(x, y);
      |   ^~~~~~
2 warnings generated.

With the changes in the emitted report it would become:

example.m:7:3: warning: call to function 'strcat' is insecure as it does not provide bounding of the memory
      buffer; replace unbounded copy functions with analogous functions that support length arguments such as
      'strlcat' (CWE-119) [security.insecureAPI.strcpy]
    7 |   strcat(x, y);
      |   ^~~~~~
example.m:7:3: note: call to function 'strcat' is insecure as it does not provide bounding of the memory
      buffer; replace unbounded copy functions with analogous functions that support length arguments such as
      'strlcat' (CWE-119)
    7 |   strcat(x, y);
      |   ^~~~~~
2 warnings generated.

For some of these longs reports I think we should also consider rewriting the warning to a shorter form, if possible.

The roadmap I have so far is:
1st PR: Replace double spaces to single space
2nd PR: Make CSA diagnostics consistent with the compiler (If other folks agree as well)
3rd PR: Rewrite some of the long diagnostics to a shorter form (This is only possible if 2nd RP has a common agreement and so does this PR)

1 Like
6

I’m surprised we are not using table-gened diagnostics for CSA.
If we did, we would benefit from the sanitation we run on the clang diags, which i think would have caught that.

1 Like
7

The “English” double-space convention was common in the era of typewriters because typewriters didn’t have an em space and had mono-space character widths. This convention has waned in the years since variable-width fonts became a thing. There’s a dedicated wikipedia page about sentence spacing (Sentence spacing in language and style guides - Wikipedia).

All of the major American style guides (MLA, APA and Chicago) have all adopted single-space after punctuation.

I’d make a strong argument that we should follow that convention because the two-space convention has been fading in popularity for decades to the point where it is (IMO) archaic.

2 Likes
8

Yes, I have read about this as well. I do not think double spacing adds anything to the generated warnings, notes anymore.

9

<sidenote>

The static analyzer often finds complex issue which cannot be adequately explained by marking its location with a “one-size-fits-all” string literal diagnostic message (that could be loaded from table-gen).

For compiler warnings every relevant information is spelled out in the immediate vicinity of the error location, so the user will understand them if we state the error kind (in a “frozen” message) and point at the error location (and perhaps place note diagnostics on a few other relevant locations).

A static analyzer bug report is found at the end of a symbolic execution path, and although we often place dozens of note tags to describe the path (entering this or that branch, calling a function etc.), it is still very important to state the final conclusions of the analyzer in a rich message.

For example security.ArrayBound has dozens of source lines to generate messages like “Access of ‘TwoElements’ at a negative or overflowing index, while it holds only 2 ‘char’ elements” where every part (the name/description of the accessed buffer, the kind of the overflow – or the concrete index when it’s known –, the optionally present note about the size of the buffer etc.) is dynamically generated. Some of this information may have been deduced from analyzing distant code, so the user wouldn’t be able to mentally recreate this if we just printed a literal string like “Out of bounds access”.
</sidenote>

10

I suspect there might be a misunderstanding, because I think Clang’s diagnostic system can handle what you describe. An example: https://github.com/llvm/llvm-project/blob/1b4c442b121b96d4fa143a845d4fb761899c146f/clang/include/clang/Basic/DiagnosticSemaKinds.td#L153

I can imagine static analyzer to be even more dynamic with its messages than what you described, but it should be able to handle this much. Documentation can be found here.

4 Likes
11

Thanks for showing this, this is really nice! I wasn’t aware of this functionality (I mostly contribute to the analyzer with a few tidy commits), and when I heard tablegen I guessed rigid format strings, instead of these rich tools with %select et al.

Based on this example and the documentation, I guess that it would be technically possible to express the security.ArrayBound warnings in this system – but probably not comfortably. The current implementation has lots of if statements, which are sometimes nested and use << into a string stream to compose the message. To express this in a fixed template, it would need to contain lots of %select parts and lots of parameters.

However, this is probably the most complex warning message composed by the analyzer, so the majority of the warning messages could be nicely expressed in this system.

12

I think there is a common agreement for using single spaces.

This is the plan I have in mind to make the discussed changes.

NOTE: As of now by the 2nd PR I do not mean switching the CSA reports to table-gened diagnostics!

  • For the table-gened diagnostics in the CSA: I plan on addressing this (if the implementation is sound) in a separate work. This is something that I think would need to be tested locally and then having a separate RFC as well where I can show the results of the implementation I have locally (this is my plan to do!). We might hit corner cases where it would turn out that using the current way of emitting diagnostics is better than switching to table-gen.
1 Like
13

Yeah, as someone who’s done quite a bit of typesetting, I feel like double-spacing is mostly obsolete these days—that said, I personally also never liked how it looks.

Note: some people disagree which one is which because of course it would be too convenient if ‘French spacing’ always meant the same thing ;Þ

2 Likes
14

This is what I have seen in journals around this topic, thank you for confirming it!

15

The 1st PR is up: [analyzer] Remove double spaces after sentence endings in emitted diagnostics by benedekaibas · Pull Request #228777 · llvm/llvm-project · GitHub

I agree with you on this, but I have to admit that I do not have the in depth knowledge to decide on this alone. In this discussion @DonatNagyE has agreed with this change which is another +1 for this change. However, I think we should wait for other Clang Static Analyzer code owners, maintainers on their thoughts. They have a wider and more in depth understanding of this than I do and they might share something important that can impact the 2nd PR:

I mention the code owners, maintainers of the CSA I know, but feel free to mention anyone else in case I have missed someone: @Xazax-hun, @steakhal, @NoQ, @necto

1 Like
16

Now as I’m revisiting this topic, I suddenly realize that if we’re touching some messages to remove the doubled spaces, then in those messages we should also join the sentences with a semicolon. (I.e. the current dot-space-space-uppercase character pattern should be changed to semicolon-space-lowercase and not dot-space-uppercase.)

This is an obviously better choice:

  • The scope of the current commit is not increased (we change exactly the same messages).
  • The scope of a potential follow-up commit is significantly reduced (when we want to get rid of all the complete sentences, it will cause less disruption).
  • Even if we don’t implement the follow-up, the diagnostics will be

Note that the First part; second part format already coexists with the First part. Second part format (and its double-spaced variant), so we can transition toward it gradually.

@bkaibas You proposed a roadmap that looks like

and my suggestion would replace this with:

  • 1st PR: change First part. Second part (with double space) to First part; second part (eliminate doubled spaces)
  • 2nd PR: change First part. Second part (with single space) to First part; second part (eliminate full sentences in messages)
  • 3rd PR: change the first letter of every analyzer message[1] to lowercase (full consistency with the rest of the compiler)

On this roadmap the first two PRs are manageable, IIRC they impact dozens of message kinds, which is still a small fraction of all the message kinds. The third PR (if we want to implement it) would be very disruptive (it changes every message) but it is almost completely mechanical, so we can offer a “follow the old behavior” flag etc. to make the transition more manageable.

  1. except messages that start with a proper noun ↩︎

1 Like
17

@DonatNagyE I agree with the proposed roadmap. I think we are on the same page for the 2nd and 3rd PR.

Yes, combining the removal of the double spaces and applying semicolons (in the messages that previously used two spaces) makes sense. These are two separate changes that would deserve both their own PRs, but they touch the same messages, so downstream users only have to update their test once. So I think we should go forward with your proposed roadmap.