[READY] Fix empty location extent after missing semicolon

[micbou]

We currently use the following (convoluted) method to compute the location extent of a diagnostic i.e. the range of the identifier where the diagnostic occurred:

• obtain a range from the clang_getCursorExtent function, which is supposed to contain our identifier;
• extract all the tokens from that range with clang_tokenize;
• find the token with the same starting position as the diagnostic;
• use the length of that token to get the ending position;
• compute the range.

Unfortunately, this doesn't always work as shown in issue #859, In that case, clang_getCursorExtent returns an empty range for all diagnostics after the missing semicolon.

This PR fixes that issue by extracting the tokens from the diagnostic position to the immediate next character then using clang_getTokenExtent to get the range of the identifier (multiline identifiers are supported).

Fixes #859.

---

This change is 

[micbou]

In fact, we don't need to compute the location of the next character for the end of the range. We can use the same position for the start and the end.

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #860 into master will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
- Coverage   94.78%   94.76%   -0.03%     
==========================================
  Files          79       79              
  Lines        5431     5427       -4     
  Branches      170      171       +1     
==========================================
- Hits         5148     5143       -5     
  Misses        235      235              
- Partials       48       49       +1

[joelhock]

Thank you for the quick response, I can verify that this does indeed fix the issue I reported in #859

I hesitate to complicate this further with a much more minor nit, but perhaps there's a quick fix since you're already thinking about this: both with and without this PR, ycmd will take this code:

class A {
    A()// extent of missing semicolon is this whole comment if no space before it
};

and, if there is no space before the comment, as shown, ycmd will report the entire comment length as the extent for the missing semicolon diagnostic.

[
    {
        "kind": "ERROR",
        "text": "expected ';' at end of declaration list",
        "ranges": [],
        "location": {
            "filepath": "mysource.cpp",
            "column_num": 8,
            "line_num": 2
        },
        "location_extent": {
            "start": {
                "filepath": "mysource.cpp",
                "column_num": 8,
                "line_num": 2
            },
            "end": {
                "filepath": "mysource.cpp",
                "column_num": 82,
                "line_num": 2
            }
        },
        "fixit_available": true
    }
]

[micbou]

Comments are now ignored when computing the location extent. I think this makes sense as comments are not a functional part of the code.

---

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

cpp/ycm/ClangCompleter/ClangHelpers.cpp, line 174 at r2 (raw file):

  for ( size_t i = 0; i < num_tokens; ++i ) {
    CXToken token = tokens[ i ];
    if ( clang_getTokenKind( token ) == CXToken_Comment )

IIRC there are parser errors that can happen within comments. For example, I feel like clang can diagnose /* within comment (in fact, i'm certain it can):

Also with -pedantic you can get errors about multi-line // comments (yep, seriously):

Does this therefore make that not work, or work less well?

---

ycmd/tests/clang/diagnostics_test.py, line 275 at r2 (raw file):

        'filepath': '/foo'
      } ),
      # NOTE: we always return a location extent even if we can't find one since

In this scenario, i feel like the right thing to do is to put the squiggle under the character that is the "location" of the diagnostic. Is that what happens ? If not, would it be possible to return a range which was just that one character? In the case of missing semi-colon, the squiggle should be the place where the semi-colon would be written (which i think is the position returned in the location entry?

---

Comments from Reviewable

[micbou]

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

cpp/ycm/ClangCompleter/ClangHelpers.cpp, line 174 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

IIRC there are parser errors that can happen within comments. For example, I feel like clang can diagnose /* within comment (in fact, i'm certain it can):

Also with -pedantic you can get errors about multi-line // comments (yep, seriously):

Does this therefore make that not work, or work less well?

No, the behavior is identical because, with or without these changes, there is no location extent and so the location itself is used (at least in YCM). However, if we don't ignore the comments, we get

which doesn't feel right.

---

ycmd/tests/clang/diagnostics_test.py, line 275 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

In this scenario, i feel like the right thing to do is to put the squiggle under the character that is the "location" of the diagnostic. Is that what happens ? If not, would it be possible to return a range which was just that one character? In the case of missing semi-colon, the squiggle should be the place where the semi-colon would be written (which i think is the position returned in the location entry?

If there is no location extent, YCM falls back to the location itself but that may not be the case for other clients so I think your suggestion is sound. Done.

---

Comments from Reviewable

[puremourning]

nice work!

---

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

cpp/ycm/ClangCompleter/ClangHelpers.cpp, line 174 at r2 (raw file):

Previously, micbou wrote…

No, the behavior is identical because, with or without these changes, there is no location extent and so the location itself is used (at least in YCM). However, if we don't ignore the comments, we get

which doesn't feel right.

OK makes sense, thanks!

---

ycmd/tests/clang/diagnostics_test.py, line 275 at r2 (raw file):

Previously, micbou wrote…

If there is no location extent, YCM falls back to the location itself but that may not be the case for other clients so I think your suggestion is sound. Done.

Great!

---

Comments from Reviewable

[puremourning]

Reviewed 1 of 2 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[micbou]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

cpp/ycm/ClangCompleter/ClangHelpers.cpp, line 174 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

OK makes sense, thanks!

Wait, the second example in my screenshot still happens if we ignore comments. Looks like libclang returns multiple as an identifier token. Well, I guess we can't do much about it.

---

Comments from Reviewable

[Valloric]

Thanks for the PR!

@zzbot r=puremourning

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[zzbot]

📌 Commit dff5163 has been approved by puremourning

[zzbot]

⌛ Testing commit dff5163 with merge 2657b65...

[zzbot]

☀️ Test successful - status-appveyor, status-travis
Approved by: puremourning
Pushing 2657b65 to master...