Correct invalid end location in diagnostics for some identifiers.

[erikjv]

Summary

Declaration names in DeclSpec had only their start set to a valid
location, so when the type specifier was missing, only the carret would
be shown at the first character of the name of a member declaration. Now
the whole identifier is underlined, which is useful for IDEs using
libclang.

Also, when the lookup for an identifier-expression failed, the end
location for that identifier was invalid.

Fixes PR25374.

[klimek]

lib/Sema/SemaType.cpp

1339 ↗

(On Diff #59886)

Do you know in which cases we get source ranges that are half valid?

[erikjv]

I'll add tests in the next diff, but I'd like to know if these changes are ok.

lib/Sema/SemaType.cpp

1339 ↗

(On Diff #59886)

In Parser::ParseTypeofSpecifier the 'cast range' for a typeof(...) is parsed, but is found to be invalid. Then it explicitly sets the DeclSpec's end location to an invalid location.

[klimek]

lib/Sema/SemaType.cpp

1339 ↗

(On Diff #59886)

Ok, in this case, we'll need Richard to verify whether he intended half-valid source locations something that users need to deal with.

[rsmith]

lib/Sema/SemaExpr.cpp

2046–2047

I'm indifferent on this change: I don't see much point in adding a single-token range when we already point the caret at that token, but I don't see any harm either.

lib/Sema/SemaType.cpp

1339 ↗

(On Diff #59886)

The code in ParseTypeofSpecifier does this:

if (CastRange.getEnd().isInvalid())
  // FIXME: Not accurate, the range gets one token more than it should.
  DS.SetRangeEnd(Tok.getLocation());
else
  DS.SetRangeEnd(CastRange.getEnd());

This should always set the end of the range to a valid location. Generally, I think we should avoid half-valid ranges rather than trying to cope with them downstream.

[erikjv]

lib/Sema/SemaExpr.cpp

2046–2047

It's mostly about how much is "underlined". If there is only a caret, that quite often translates into a single character being pointed out, instead of an identifier (i.e. the first character). Hene the extension of the range.

[klimek]

It's mostly about how much is "underlined". If there is only a caret, that quite often translates into a single character being pointed out, instead of an identifier (i.e. the first character). Hene the extension of the range.

Aren't we mostly doing token positions? It's basically always wrong to point at a single character, right?

[rsmith]

lib/Parse/ParseDecl.cpp

2702

This doesn't look right: this is a source range containing exactly one token. If we don't have *any* decl-specifiers (such as for a constructor, destructor, or conversion function), this is the wrong range.

How about instead setting the start location to be invalid in DeclSpec::Finish if the end location is invalid?

lib/Sema/SemaExpr.cpp

2046–2047

A ^ with no ~s should be interpreted as indicating a location, not highlighting a single character. We use that pattern in a huge number of diagnostics; it doesn't make sense to do something different here.

I also don't think this is the correct range to underline. If the name comprises multiple tokens (such as operator int), this will incorrectly highlight only the first one. A wrong underline seems worse than no underline.

lib/Parse/ParseDecl.cpp

 /// [OpenCL] '__kernel'
 ///       'friend': [C++ dcl.friend]
 ///       'constexpr': [C++0x dcl.constexpr]
 void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
                                         const ParsedTemplateInfo &TemplateInfo,
                                         AccessSpecifier AS,
                                         DeclSpecContext DSContext,
                                         LateParsedAttrList *LateAttrs) {
-  if (DS.getSourceRange().isInvalid()) {
-    // Start the range at the current token but make the end of the range
-    // invalid.  This will make the entire range invalid unless we successfully
-    // consume a token.
+  if (DS.getSourceRange().isInvalid() && !Tok.is(tok::eof)) {
     DS.SetRangeStart(Tok.getLocation());
-    DS.SetRangeEnd(SourceLocation());
+    DS.SetRangeEnd(Tok.getLocation());
   }
 
   bool EnteringContext = (DSContext == DSC_class || DSContext == DSC_top_level);
   bool AttrsLastTime = false;
   ParsedAttributesWithRange attrs(AttrFactory);
   // We use Sema's policy to get bool macros right.
   PrintingPolicy Policy = Actions.getPrintingPolicy();
   while (1) {

lib/Sema/SemaExpr.cpp

Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
   if (!SS.isEmpty()) {
     Diag(R.getNameLoc(), diag::err_no_member)
       << Name << computeDeclContext(SS, false)
       << SS.getRange();
     return true;
   }
 
   // Give up, we can't recover.
-  Diag(R.getNameLoc(), diagnostic) << Name;
+  auto Builder = Diag(R.getNameLoc(), diagnostic) << Name;
+  if (Name.isIdentifier())
+    Builder << SourceRange(R.getNameLoc());
   return true;
 }
 
 /// In Microsoft mode, if we are inside a template class whose parent class has
 /// dependent base classes, and we can't resolve an unqualified identifier, then
 /// assume the identifier is a member of a dependent base class.  We can only
 /// recover successfully in static methods, instance methods, and other contexts
 /// where 'this' is available.  This doesn't precisely match MSVC's

test/Index/diagnostic-ranges.cpp

+// RUN: c-index-test -test-load-source all %s > /dev/null 2> %t.err
+// RUN: FileCheck < %t.err -check-prefix=CHECK-RANGE %s
+
+struct Foo {
+  someIdentifierLeadingToAnInvalidRange;
+};
+
+template <typename T>
+struct Bar {
+  Bar(const T&) {}
+};
+  
+void f()
+{
+  for (Bar<__typeof__(v.x)> container(v.x); true; )
+    return;
+}
+
+// CHECK-RANGE: diagnostic-ranges.cpp:5:3: error: C++ requires a type specifier for all declarations
+// CHECK-RANGE: diagnostic-ranges.cpp:15:39: error: use of undeclared identifier 'v'
+// CHECK-RANGE: diagnostic-ranges.cpp:5:3:{5:3-5:40}: error: C++ requires a type specifier for all declarations
+// CHECK-RANGE: diagnostic-ranges.cpp:15:23:{15:23-15:24}: error: use of undeclared identifier 'v'
+