Do not add a colon chunk to the code completion of class inheritance access modifiers

[yvvan]

Summary

With enabled CINDEXTEST_CODE_COMPLETE_PATTERNS env option (which enables IncludeCodePatterns in completion options) code completion after colon currently suggests access modifiers with 2 completion chunks which is incorrect.

Example:
class A : <Cursor>B
{
}

Currently we get 'NotImplemented:{TypedText public}{Colon :} (40)'
but the correct line is just 'NotImplemented:{TypedText public} (40)'

The fix introduces more specific scope that occurs between ':' and '{'
It allows us to determine when we don't need to add ':' as a second chunk to the public/protected/private access modifiers.

[erikjv]

lgtm, but someone else will probably have to review it too.

lib/Parse/ParseDeclCXX.cpp

3196

InheritanceScope instead of ClassScope? I mean, it's not really the scope of the class, that would be inside the class decl.
Also, clang-format this line.

[yvvan]

Fix according to the review comment

[arphaman]

include/clang/Sema/Scope.h

129

Could you please rebase this patch? There's another scope flag that uses this value already.

[yvvan]

Rebased to update ScopeFlags.
Rereview please

[yvvan]

ping

[arphaman]

Overall LGTM, just a couple of comments:

include/clang/Sema/Scope.h

128

Nit: Add '.' to the end of the comment + clang-format.

lib/Parse/ParseDeclCXX.cpp

3196

Might be better to use getCurScope()->getFlags() | Scope::ClassInheritanceScope to avoid dealing with any future scoping rule changes if such changes will occur.

lib/Sema/SemaCodeComplete.cpp

1650

80 columns violation, please clang-format

include/clang/Sema/Scope.h

enum ScopeFlags {
     /// This scope corresponds to an SEH try.
     SEHTryScope = 0x80000,
 
     /// This scope corresponds to an SEH except.
     SEHExceptScope = 0x100000,
 
     /// We are currently in the filter expression of an SEH except block.
     SEHFilterScope = 0x200000,
+
+    /// We are between inheritance colon and the real class/struct definition scope
+    ClassInheritanceScope = 0x400000,
   };
 private:
   /// The parent scope for this scope.  This is null for the translation-unit
   /// scope.
   Scope *AnyParent;
 
   /// Flags - This contains a set of ScopeFlags, which indicates how the scope
   /// interrelates with other control flow statements.

lib/Parse/ParseDeclCXX.cpp

if (getLangOpts().CPlusPlus && Tok.is(tok::identifier)) {
     if (!Tok.is(tok::colon) && !Tok.is(tok::l_brace)) {
       if (TagDecl)
         Actions.ActOnTagDefinitionError(getCurScope(), TagDecl);
       return;
     }
   }
 
   if (Tok.is(tok::colon)) {
+    ParseScope ClassScope(this, Scope::ClassScope|Scope::DeclScope | Scope::ClassInheritanceScope);
+
     ParseBaseClause(TagDecl);
     if (!Tok.is(tok::l_brace)) {
       bool SuggestFixIt = false;
       SourceLocation BraceLoc = PP.getLocForEndOfToken(PrevTokLocation);
       if (Tok.isAtStartOfLine()) {
         switch (Tok.getKind()) {
         case tok::kw_private:
         case tok::kw_protected:

lib/Sema/SemaCodeComplete.cpp

if (SemaRef.getLangOpts().CPlusPlus) {
         Results.AddResult(Result(Builder.TakeString()));
       }
 
       AddStaticAssertResult(Builder, Results, SemaRef.getLangOpts());
 
       if (CCC == Sema::PCC_Class) {
         AddTypedefResult(Results);
 
+        bool IsNotInheritanceScope = !(S->getFlags() & Scope::ClassInheritanceScope);
         // public:
         Builder.AddTypedTextChunk("public");
-        if (Results.includeCodePatterns())
+        if (IsNotInheritanceScope && Results.includeCodePatterns())
           Builder.AddChunk(CodeCompletionString::CK_Colon);
         Results.AddResult(Result(Builder.TakeString()));
 
         // protected:
         Builder.AddTypedTextChunk("protected");
-        if (Results.includeCodePatterns())
+        if (IsNotInheritanceScope && Results.includeCodePatterns())
           Builder.AddChunk(CodeCompletionString::CK_Colon);
         Results.AddResult(Result(Builder.TakeString()));
 
         // private:
         Builder.AddTypedTextChunk("private");
-        if (Results.includeCodePatterns())
+        if (IsNotInheritanceScope && Results.includeCodePatterns())
           Builder.AddChunk(CodeCompletionString::CK_Colon);
         Results.AddResult(Result(Builder.TakeString()));
       }
     }
     // Fall through
 
   case Sema::PCC_Template:
   case Sema::PCC_MemberTemplate:

test/Index/complete-super.cpp

 // CHECK-ACCESS: NotImplemented:{TypedText private} (40)
 // CHECK-ACCESS: NotImplemented:{TypedText protected} (40)
 // CHECK-ACCESS: NotImplemented:{TypedText public} (40)
 
 // RUN: env CINDEXTEST_CODE_COMPLETE_PATTERNS=1 c-index-test -code-completion-at=%s:5:1 %s | FileCheck -check-prefix=CHECK-ACCESS-PATTERN %s
 // CHECK-ACCESS-PATTERN: NotImplemented:{TypedText private}{Colon :} (40)
 // CHECK-ACCESS-PATTERN: NotImplemented:{TypedText protected}{Colon :} (40)
 // CHECK-ACCESS-PATTERN: NotImplemented:{TypedText public}{Colon :} (40)
+
+// RUN: env CINDEXTEST_CODE_COMPLETE_PATTERNS=1 c-index-test -code-completion-at=%s:10:12 %s | FileCheck -check-prefix=CHECK-INHERITANCE-PATTERN %s
+// CHECK-INHERITANCE-PATTERN: NotImplemented:{TypedText private} (40)
+// CHECK-INHERITANCE-PATTERN: NotImplemented:{TypedText protected} (40)
+// CHECK-INHERITANCE-PATTERN: NotImplemented:{TypedText public} (40)