[clangd] Add a flag for spelled references in the Index

[kbobyrev]

Summary

This patch allows the index does to provide a way to distinguish implicit references (e.g. coming from macro expansions) from the spelled ones. The corresponding flag was added to RefKind and symbols that are referenced without spelling their name explicitly are now marked implicit. This allows fixing incorrect behavior when renaming a symbol that was referenced in macro expansions would try to rename macro invocations.

[kbobyrev]

The patch could be shorter and slightly less confusing if I preserved 1:1 RefKind::Implicit <-> index::SymbolKind::Implicit relation, but I thought we might want to keep RefKind being 1 byte so that we don't waste unnecessary memory. Also, since we might want to start using findExplicitReferences (or something else we come up with) for indexing, it shouldn't be critical. Let me know if you think it's unnecessary.

+CC @hokein

[merge_guards_bot]

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

clang-tools-extra/clangd/index/Ref.h

33

instead of doing that, could we rather de-couple two enums completely and have a symbolRoleToRefKind(and vice-versa) method instead(we already have some customization in toRefKind now) ?

as current change increases the risk of overlapping in future(e.g. someone might change symbolrole::declaration and cause failures/regressions in clangd)

clang-tools-extra/clangd/index/SymbolCollector.cpp

297

this might be expensive to trigger while indexing for each reference.

could we rather do it as post-processing inside SymbolCollector::finish while making use of TokenBuffer::spelledTokens(you can create a tokenbuffer through TokenCollector with the PP inside SymbolCollector.)

300

It seems like we are only checking for syntactic occurence of a symbol. Can we make use of Spelled Rather than Implicit (which has some semantic meaning) to make that clear?

[kadircet]

clang-tools-extra/clangd/index/Ref.h

33

note that this would also require a bump to version of on-disk index in clangd/index/Serialization.cpp, as old information regarding RefKind won't be usable anymore.

[hokein]

In D72746#1821097, @kbobyrev wrote:

The patch could be shorter and slightly less confusing if I preserved 1:1 RefKind::Implicit <-> index::SymbolKind::Implicit relation, but I thought we might want to keep RefKind being 1 byte so that we don't waste unnecessary memory.

I think it is time to create our own Declaration, Definition in our RefKind (rather than reusing the enums in libindex).

Also, since we might want to start using findExplicitReferences (or something else we come up with) for indexing, it shouldn't be critical. Let me know if you think it's unnecessary.

Yeah, we may have a plan in the future, but I don't think switching to findExplicitReferences is trivial, there are a few things we need to figure out carefully, e.g. how to support macros, implicit references.

clang-tools-extra/clangd/index/Ref.h

30–66

I'm skeptical about definition of index::SymbolRole::Implicit.

I think the implicit we want here is references that are spelled/written as the same as the named decls, so it is more likely to align with index::SymbolRole::NameReferences.

clang-tools-extra/clangd/index/SymbolCollector.cpp

300

+1, Implicit is ambiguous, maybe Named, Explicit (this reflects to the findExplicitReferences)?

[kbobyrev]

Address alsmost all comments.

clang-tools-extra/clangd/index/Ref.h

33

instead of doing that, could we rather de-couple two enums completely and have a symbolRoleToRefKind(and vice-versa) method instead(we already have some customization in toRefKind now) ?
as current change increases the risk of overlapping in future(e.g. someone might change symbolrole::declaration and cause failures/regressions in clangd)

Makes sense, will do!

note that this would also require a bump to version of on-disk index in clangd/index/Serialization.cpp, as old information regarding RefKind won't be usable anymore.

I checked the Serialization code and the serialization code should be OK as long as RefKind stays one byte. Can you please elaborate on this?

Do you mean that the index should be generated again after this change because it would no longer be valid? (this one I understand)
Or do you mean there should be some other change in the code for me to do to land this patch?

[kbobyrev]

Need to make use of the TokenBuffer, ran into some difficulties when lexing some files and triggering assertions in the process. Will figure it out and update the patch.

[merge_guards_bot]

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kbobyrev]

I started using TokenBuffer, but I ran into the following issue: when I'm
creating TokenBuffer and TokenCollector, they do not contain any tokens.
Preprocessor does not seem to have a non-null Lexer instance, TokenWatcher
(set in TokenCollector) is not triggered anywhere and neither is
Lexer::Lex. I don't have much familiarity with the code and I looked at the
other usages of TokenBuffer but didn't figure out what's wrong with the code
in this patch. I suspect the lexer in Preprocessor should be re-initialized
somehow? I'm certainly missing something here.

[merge_guards_bot]

Unit tests: fail. 61674 tests passed, 181 failed and 781 were skipped.

failed: Clangd.Clangd/background-index.test
failed: Clangd.Clangd/code-action-request.test
failed: Clangd.Clangd/compile-commands-path-in-initialize.test
failed: Clangd.Clangd/completion-auto-trigger.test
failed: Clangd.Clangd/completion-snippets.test
failed: Clangd.Clangd/completion.test
failed: Clangd.Clangd/diagnostic-category.test
failed: Clangd.Clangd/diagnostics-no-tidy.test
failed: Clangd.Clangd/diagnostics-notes.test
failed: Clangd.Clangd/diagnostics.test
failed: Clangd.Clangd/did-change-configuration-params.test
failed: Clangd.Clangd/document-link.test
failed: Clangd.Clangd/execute-command.test
failed: Clangd.Clangd/filestatus.test
failed: Clangd.Clangd/fixits-codeaction.test
failed: Clangd.Clangd/fixits-command.test
failed: Clangd.Clangd/fixits-embed-in-diagnostic.test
failed: Clangd.Clangd/formatting.test
failed: Clangd.Clangd/hover.test
failed: Clangd.Clangd/index-tools.test
failed: Clangd.Clangd/path-mappings.test
failed: Clangd.Clangd/protocol.test
failed: Clangd.Clangd/references.test
failed: Clangd.Clangd/rename.test
failed: Clangd.Clangd/request-reply.test
failed: Clangd.Clangd/selection-range.test
failed: Clangd.Clangd/semantic-highlighting.test
failed: Clangd.Clangd/signature-help-with-offsets.test
failed: Clangd.Clangd/signature-help.test
failed: Clangd.Clangd/symbol-info.test
failed: Clangd.Clangd/symbols.test
failed: Clangd.Clangd/system-include-extractor.test
failed: Clangd.Clangd/test-uri-posix.test
failed: Clangd.Clangd/textdocument-didchange-fail.test
failed: Clangd.Clangd/trace.test
failed: Clangd.Clangd/tweaks-format.test
failed: Clangd.Clangd/type-hierarchy.test
failed: Clangd.Clangd/unsupported-method.test
failed: Clangd.Clangd/utf8.test
failed: Clangd.Clangd/xrefs.test
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.CmdLineHash
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.DirectIncludesTest
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.IndexTwoFiles
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.NoCrashOnErrorFile
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.NoDotsInAbsPath
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageEmptyFile
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageLoad
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageTest
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.UncompilableFiles
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.CommentsFromSystemHeaders
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.DynamicIndexIncludeInsertion
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.DynamicIndexMultiFile
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.UsingDecl
failed: Clangd Unit Tests._/ClangdTests/CrossFileRenameTests.DirtyBuffer
failed: Clangd Unit Tests._/ClangdTests/CrossFileRenameTests.WithUpToDateIndex
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.BasicSymbols
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.DeclarationDefinition
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Enums
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.ExternSymbol
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.FromMacro
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.FuncTemplates
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.InHeaderFile
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Namespaces
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.NoLocals
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Qualifiers
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.QualifiersWithTemplateArgs
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.TempSpecs
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Template
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Unnamed
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.UsingDirectives
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.ClassMembers
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.CollectMacros
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.CustomizedURIScheme
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.HasSystemHeaderMappingsInPreamble
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IncludeCollected
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IndexAST
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IndexMultiASTAndDeduplicate
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.MacroRefs
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.MergeMainFileSymbols
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.NoLocal
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.RebuildWithPreamble
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.ReferencesInMainFileWithPreamble
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.Refs
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.Relations
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.TemplateParamsInLabel
failed: Clangd Unit Tests._/ClangdTests/FindReferences.NeedsIndexForMacro
failed: Clangd Unit Tests._/ClangdTests/FindReferences.NeedsIndexForSymbols
failed: Clangd Unit Tests._/ClangdTests/HeaderSourceSwitchTest.ClangdServerIntegration
failed: Clangd Unit Tests._/ClangdTests/HeaderSourceSwitchTest.FromHeaderToSource
failed: Clangd Unit Tests._/ClangdTests/HeaderSourceSwitchTest.FromSourceToHeader
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.CollectIncludeGraph
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.IncludeGraphDynamicInclude
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.IncludeGraphSelfInclude
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.IncludeGraphSkippedFile
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.NoWarnings
failed: Clangd Unit Tests._/ClangdTests/LocateSymbol.WithIndex
failed: Clangd Unit Tests._/ClangdTests/LocateSymbol.WithIndexPreferredLocation
failed: Clangd Unit Tests._/ClangdTests/MergeIndexTest.Refs
failed: Clangd Unit Tests._/ClangdTests/QualityTests.ConstructorDestructor
failed: Clangd Unit Tests._/ClangdTests/QualityTests.IsInstanceMember
failed: Clangd Unit Tests._/ClangdTests/QualityTests.NoBoostForClassConstructor
failed: Clangd Unit Tests._/ClangdTests/QualityTests.SymbolQualitySignalExtraction
failed: Clangd Unit Tests._/ClangdTests/QualityTests.SymbolRelevanceSignalExtraction
failed: Clangd Unit Tests._/ClangdTests/RenameTest.Renameable
failed: Clangd Unit Tests._/ClangdTests/SignatureHelpTest.DynamicIndexDocumentation
failed: Clangd Unit Tests._/ClangdTests/Subtypes.ClassTemplate
failed: Clangd Unit Tests._/ClangdTests/Subtypes.DependentBase
failed: Clangd Unit Tests._/ClangdTests/Subtypes.LazyResolution
failed: Clangd Unit Tests._/ClangdTests/Subtypes.MultipleInheritance
failed: Clangd Unit Tests._/ClangdTests/Subtypes.SimpleInheritance
failed: Clangd Unit Tests._/ClangdTests/Subtypes.TemplateSpec1
failed: Clangd Unit Tests._/ClangdTests/Subtypes.TemplateSpec2
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.AvoidUsingFwdDeclsAsCanonicalDecls
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CBuiltins
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CanonicalSTLHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ClassForwardDeclarationIsCanonical
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ClassMembers
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CollectMacros
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CollectSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CountReferences
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.DeprecatedSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.DoNotIndexSymbolsInFriendDecl
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Documentation
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ExternC
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.FileLocal
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.HeaderAsMainFile
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.HeaderGuardDetected
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IWYUPragma
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IWYUPragmaWithDoubleQuotes
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ImplementationDetail
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IncFileInNonHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IncludeEnums
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IncludeHeaderSameAsFileURI
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.InvalidSourceLoc
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Locations
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MacroRefInHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MacroRefWithoutCollectingSymbol
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MacrosWithRefFilter
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MainFileIsHeaderWhenSkipIncFile
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.NameReferences
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.NamelessSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.NonModularHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ObjCPropertyImpl
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ObjCSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Origin
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ReferencesInFriendDecl
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Refs
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.RefsInHeaders
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.RefsOnMacros
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Relations
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Scopes
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SkipIncFileWhenCanonicalizeHeaders
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SkipInlineNamespace
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Snippet
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolFormedByCLI
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolFormedFromRegisteredSchemeFromMacro
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolRelativeNoFallback
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolRelativeWithFallback
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolWithDocumentation
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolsInMainFile
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Template
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.TemplateArgs
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.UTF16Character
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.UnittestURIScheme
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.UsingDecl
failed: Clangd Unit Tests._/ClangdTests/TypeHierarchy.DeriveFromImplicitSpec
failed: Clangd Unit Tests._/ClangdTests/TypeHierarchy.DeriveFromPartialSpec
failed: Clangd Unit Tests._/ClangdTests/TypeHierarchy.DeriveFromTemplate
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.AnonymousNamespace
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Enums
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.GlobalNamespaceQueries
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Globals
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.InMainFile
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Macros
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.MultiFile
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Namespaces
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.NoLocals
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Ranking
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.TempSpecs
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Unnamed
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.WithLimit

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

In D72746#1827608, @kbobyrev wrote:

I started using TokenBuffer, but I ran into the following issue: when I'm
creating TokenBuffer and TokenCollector, they do not contain any tokens.
Preprocessor does not seem to have a non-null Lexer instance, TokenWatcher
(set in TokenCollector) is not triggered anywhere and neither is
Lexer::Lex. I don't have much familiarity with the code and I looked at the
other usages of TokenBuffer but didn't figure out what's wrong with the code
in this patch. I suspect the lexer in Preprocessor should be re-initialized
somehow? I'm certainly missing something here.

right, I forgot that TokenCollector must be constructed before starting to process the file, and unfortunately in some
code paths we create a SymbolCollector long after parsing the file. You can make use of syntax::Tokenize for now,
as we only care about SpelledTokens in the file. Please leave a TODO though saying that, we should rather pass one from
the caller of SymbolCollector.

Sorry for the inconvenience.

clang-tools-extra/clangd/index/Ref.h

33

Do you mean that the index should be generated again after this change because it would no longer be valid? (this one I understand)

yes i meant this

[kbobyrev]

Address review comments

[kbobyrev]

Will put a TODO and revert helpers back to use plain binary search over the tokens.

[kbobyrev]

Actually, this approach might be more generic (i.e. not relying on
implementation too much). Added the FIXME.

[kbobyrev]

Move added rename unit test to the end of the list.

[merge_guards_bot]

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[hokein]

the scope of this patch is not very clear, the changes touch two different code parts SymbolCollector, and Rename, and we are lacking tests for SymbolCollector. I'd suggest spliting this patch into smaller patches:

• a patch that adds a new kind to the ref, and updates the SymbolCollector
• a patch that updates the rename accordingly

clang-tools-extra/clangd/index/Ref.h

33

could you please add some brief documentation on these fields?

34

The All now indicates all spelled refs. I think All should include both non-spelled and spell refs, which should be declaration | Definition | Reference.

clang-tools-extra/clangd/index/SymbolCollector.cpp

579

looks like we don't use the MainFileURI variable below, I think we can remove this if statement.

586

since we only use FilesToTokensCache in this function, make it as a local variable rather than class member.

587

note that the DeclRefs may contains references from non-main files, e.g. included headers if RefsInHeaders is true. I think we need to tokenize other files if the reference is not from main file. CollectRef lambda is a better place to place the FilesToTokensCache logic.

[kadircet]

clang-tools-extra/clangd/index/Ref.h

34

I don't follow the argument here ,All is rather a bitmask that should be ORd with a RefKindSet.
It is not like we were saying only the references that are both declarations and definitions were acceptable before, we were saying All implies either a declaration, definition or a reference in here. So the logic seems to be correct.

[hokein]

clang-tools-extra/clangd/index/Ref.h

34

ah, you are right, I misthought about that, thanks for the clarification.

[kbobyrev]

Address review comments, add implicit references filter for SymbolCollector,
test changes.

clang-tools-extra/clangd/index/SymbolCollector.cpp

587

note that the DeclRefs may contains references from non-main files, e.g. included headers if RefsInHeaders is true. I think we need to tokenize other files if the reference is not from main file.

Fair enough, fixed that!

CollectRef lambda is a better place to place the FilesToTokensCache logic.

I don't really agree with that. In my opinion CollectRef should remain a simple helper that simply puts pre-processed reference into the storage. Complicating it (and also making it work with both MacroRefs and DeclRefs while the token spelling check is only required for declarations looks suboptimal to me. If you really want me to do that, please let me know, but I personally think it might be better this way.

[kbobyrev]

Attempt to drop collected reference before doing more computation to improve
performance.

[merge_guards_bot]

Unit tests: pass. 61857 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61857 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[hokein]

clang-tools-extra/clangd/index/Ref.h

32

could you confirm with it? this looks like a Foo class definition.

108

nit: I don't understand why we need this change? it seems irrelevant to the patch.

clang-tools-extra/clangd/index/SymbolCollector.cpp

178

I was confused at the first time reading the code -- it turns out we reset the flag in the caller.

Maybe adjust the function like RefKind toRefKind(index::SymbolRoleSet Roles, bool isSpelled)?

587

I don't really agree with that. In my opinion CollectRef should remain a simple helper that simply puts pre-processed reference into the storage. Complicating it (and also making it work with both MacroRefs and DeclRefs while the token spelling check is only required for declarations looks suboptimal to me. If you really want me to do that, please let me know, but I personally think it might be better this way.

SG.

clang-tools-extra/clangd/index/SymbolCollector.h

91 ↗

(On Diff #239545)

what's the use case for this flag? I think we don't need it as we always want all references.

clang-tools-extra/clangd/refactor/Rename.cpp

299 ↗

(On Diff #239545)

again, I would suggest doing this change in a follow-up patch.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

710

could you add a test case for class constructor/destructor to make sure the references are marked spelled?

class [[Foo]] {
   [[Foo]]();
   ~[[Foo]]();
}

718

I think we need to verify the RefKind in the test, just add a new gtest matcher IsRefKind.

nit: we can simplify the code by aggregating the above 3 EXPECT_THATs, like

EXPECT_THAT(Refs,
             UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
                                       HaveRanges(Header.ranges("Foo")))...);

clang/include/clang/Tooling/Syntax/Tokens.h

324 ↗

(On Diff #239545)

nit: I would make this function come immediately after the above one (without a blank line) to avoid the duplicated documentations, e.g.

/// The spelled tokens that overlap or touch a spelling location Loc.
/// This always returns 0-2 tokens.
llvm::ArrayRef<syntax::Token>
spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef<syntax::Token> Tokens);
llvm::ArrayRef<syntax::Token>
spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer &Tokens);

the same below.

[kbobyrev]

Address a number of comments.

clang-tools-extra/clangd/index/Ref.h

32

Good point, I thought it should be both.

108

This one is required for SizeIs matcher, but I believe I could use UnorderedAre instead (to make sure there are no extra references).

clang-tools-extra/clangd/index/SymbolCollector.cpp

178

If my understanding is correct, the suggestion is to take isSpelled argument and apply the RefKind flag based on the value of the argument instead of using SymbolRole::Implicit. Is that right? In this case I would be concerned about doing because that increase the amount of code in case there is any other index provider that sets the flags itself.

What do you think?

clang/include/clang/Tooling/Syntax/Tokens.h

324 ↗

(On Diff #239545)

Makes sense. I was concerned about Doxygen not generating comments for both of these functions, but I think it should be OK.

[kbobyrev]

Remove unnecessary include.

[merge_guards_bot]

Unit tests: fail. 61851 tests passed, 6 failed and 780 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: lld.ELF/linkerscript/filename-spec.s

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 61851 tests passed, 6 failed and 780 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: lld.ELF/linkerscript/filename-spec.s

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[hokein]

thanks, mostly good. the only concern from my side is about the SymbolRole::Implicit, I think we should get rid of it.

clang-tools-extra/clangd/index/Ref.h

53

maybe make sense to include an AST example?

class Foo { public: Foo(); };

Foo [[f]]; // there is a reference of `Foo` constructor around `f`, this is a non-spelled reference obviously.

clang-tools-extra/clangd/index/SymbolCollector.cpp

178

it is not about the amount of code, it is about layering. I think we should not use the Implicit. With the current change, the SymbolRole::Implicit comes from two sources:

1. the flag can be set in libindex;
2. the flag can be set in SymbolCollector (via the post-processing in finish)

for 1), the implementation of libindex is not completed, only object-c related decls get set. I think our aim is to set the Spelled flag if 2) is true.

Another way doing that would be keep the toRefKind implementation unchanged, and set the Spelled flag afterwards.

clang-tools-extra/clangd/index/SymbolLocation.h

12 ↗

(On Diff #241994)

I would not do this change, it introduces a new dependency to this header.

clang-tools-extra/clangd/unittests/RenameTests.cpp

36

do we need this change in this patch?

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

726

I found the RefKindIs is hard to follow without reading its implementation, I think we can do it in another way, retrieve all spelled refs from Refs, and verify them:

EXPECT_THAT(SpelledRefs,
             UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
                                       AllOf(HaveRanges(Main.ranges("spelled"))), SpelledKind()));

[kbobyrev]

Address the comments.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kbobyrev]

Remove an artifact.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[hokein]

thanks, looks good, a few nits.

before landing the patch, could you please run the clangd-indexer on the llvm project and measure the before/after indexing time? to make sure we don't introduce a big latency.
you can run the command like time ./bin/clangd-indexer -format=binary -executor=all-TUs . > static-index.idx

clang-tools-extra/clangd/index/SymbolCollector.cpp

577–578

macro is tricky, macro references are not marked spelled, it is OK for now as we are not interested in them, but I think most of time they are spelled in the source code. Maybe add a comment?

592

nit: the comment seems stale now.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

723

nit: instead of checking all refs, I'd check implicit references explicitly, similar to the SpelledRefs below (just add a UnspelledSlab).

[kbobyrev]

Address another round of comments.

[merge_guards_bot]

Unit tests: fail. 61852 tests passed, 5 failed and 780 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

clang-tools-extra/clangd/index/Ref.h

 namespace clang {
 namespace clangd {
 
 /// Describes the kind of a cross-reference.
 ///
 /// This is a bitfield which can be combined from different kinds.
 enum class RefKind : uint8_t {
   Unknown = 0,
-  Declaration = static_cast<uint8_t>(index::SymbolRole::Declaration),
-  Definition = static_cast<uint8_t>(index::SymbolRole::Definition),
-  Reference = static_cast<uint8_t>(index::SymbolRole::Reference),
-  All = Declaration | Definition | Reference,
+  // Points to symbol declaration. Example:
+  //
+  // class Foo;
+  //       ^ Foo declaration
+  // Foo foo;
+  // ^ this does not reference Foo declaration
+  Declaration = 1 << 0,
+  // Points to symbol definition. Example:
+  //
+  // int foo();
+  //     ^ references foo declaration, but not foo definition
+  // int foo() { return 42; }
+  //     ^ references foo definition, but not declaration
+  // bool bar() { return true; }
+  //      ^ references both definition and declaration
+  Definition = 1 << 1,
+  // Points to symbol reference. Example:
+  //
+  // int Foo = 42;
+  // int Bar = Foo + 1;
+  //           ^ this is a reference to Foo
+  Reference = 1 << 2,
+  // The reference explicitly spells out declaration's name. Such references can
+  // not come from macro expansions or implicit AST nodes.
+  //
+  // class Foo { public: Foo() {} };
+  //       ^ references declaration, definition and explicitly spells out name
+  // #define MACRO Foo
+  //     v there is an implicit constructor call here which is not a spelled ref
+  // Foo foo;
+  // ^ this reference explicitly spells out Foo's name
+  // struct Bar {
+  //   MACRO Internal;
+  //   ^ this references Foo, but does not explicitly spell out its name
+  // };
+  Spelled = 1 << 3,
+  All = Declaration | Definition | Reference | Spelled,
 };
 
 inline RefKind operator|(RefKind L, RefKind R) {
   return static_cast<RefKind>(static_cast<uint8_t>(L) |
                               static_cast<uint8_t>(R));
 }
 inline RefKind &operator|=(RefKind &L, RefKind R) { return L = L | R; }
 inline RefKind operator&(RefKind A, RefKind B) {

 /// An efficient structure of storing large set of symbol references in memory.
 /// Filenames are deduplicated.
 class RefSlab {
 public:
   // Refs are stored in order.
   using value_type = std::pair<SymbolID, llvm::ArrayRef<Ref>>;
   using const_iterator = std::vector<value_type>::const_iterator;
   using iterator = const_iterator;
 
   RefSlab() = default;
   RefSlab(RefSlab &&Slab) = default;
   RefSlab &operator=(RefSlab &&RHS) = default;
 
   const_iterator begin() const { return Refs.begin(); }
   const_iterator end() const { return Refs.end(); }
   /// Gets the number of symbols.
   size_t size() const { return Refs.size(); }

clang-tools-extra/clangd/index/SymbolCollector.cpp

 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 namespace {

 // the first seen declaration as canonical declaration is not a good enough
 // heuristic.
 bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) {
   const auto &SM = ND.getASTContext().getSourceManager();
   return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
          isa<TagDecl>(&ND) && !isInsideMainFile(ND.getLocation(), SM);
 }
 
-RefKind toRefKind(index::SymbolRoleSet Roles) {
-  return static_cast<RefKind>(static_cast<unsigned>(RefKind::All) & Roles);
+RefKind toRefKind(index::SymbolRoleSet Roles, bool Spelled = false) {
+  RefKind Result = RefKind::Unknown;
+  if (Roles & static_cast<unsigned>(index::SymbolRole::Declaration))
+    Result |= RefKind::Declaration;
+  if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
+    Result |= RefKind::Definition;
+  if (Roles & static_cast<unsigned>(index::SymbolRole::Reference))
+    Result |= RefKind::Reference;
+  if (Spelled)
+    Result |= RefKind::Spelled;
+  return Result;
 }
 
 bool shouldIndexRelation(const index::SymbolRelation &R) {
   // We currently only index BaseOf relations, for type hierarchy subtypes.
   return R.Roles & static_cast<unsigned>(index::SymbolRole::RelationBaseOf);
 }
 
 } // namespace
if (!ID)
     return true;
 
   // Note: we need to process relations for all decl occurrences, including
   // refs, because the indexing code only populates relations for specific
   // occurrences. For example, RelationBaseOf is only populated for the
   // occurrence inside the base-specifier.
   processRelations(*ND, *ID, Relations);
 
-  bool CollectRef = static_cast<unsigned>(Opts.RefFilter) & Roles;
+  bool CollectRef = static_cast<bool>(Opts.RefFilter & toRefKind(Roles));
   bool IsOnlyRef =
       !(Roles & (static_cast<unsigned>(index::SymbolRole::Declaration) |
                  static_cast<unsigned>(index::SymbolRole::Definition)));
 
   if (IsOnlyRef && !CollectRef)
     return true;
 
   // ND is the canonical (i.e. first) declaration. If it's in the main file
   // (which is not a header), then no public declaration was visible, so assume
   // it's main-file only.
   bool IsMainFileOnly =
       SM.isWrittenInMainFile(SM.getExpansionLoc(ND->getBeginLoc())) &&
       !isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
                     ASTCtx->getLangOpts());
   // In C, printf is a redecl of an implicit builtin! So check OrigD instead.
   if (ASTNode.OrigD->isImplicit() ||
if (Found == URICache.end()) {
         // given location, e.g. symbols formed via macro concatenation.
         return None;
       }
     }
     return Found->second;
   };
   auto CollectRef =
       [&](SymbolID ID,
-          const std::pair<SourceLocation, index::SymbolRoleSet> &LocAndRole) {
+          const std::pair<SourceLocation, index::SymbolRoleSet> &LocAndRole,
+          bool Spelled = false) {
         auto FileID = SM.getFileID(LocAndRole.first);
         // FIXME: use the result to filter out references.
         shouldIndexFile(FileID);
         if (auto FileURI = GetURI(FileID)) {
           auto Range =
               getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
           Ref R;
           R.Location.Start = Range.first;
           R.Location.End = Range.second;
           R.Location.FileURI = FileURI->c_str();
-          R.Kind = toRefKind(LocAndRole.second);
+          R.Kind = toRefKind(LocAndRole.second, Spelled);
           Refs.insert(ID, R);
         }
       };
   // Populate Refs slab from MacroRefs.
-  for (const auto &IDAndRefs : MacroRefs) {
+  // FIXME: All MacroRefs are marked as Spelled now, but this should be checked.
+  for (const auto &IDAndRefs : MacroRefs)
     for (const auto &LocAndRole : IDAndRefs.second)
       CollectRef(IDAndRefs.first, LocAndRole);
-  }
   // Populate Refs slab from DeclRefs.
-  if (auto MainFileURI = GetURI(SM.getMainFileID())) {
-    for (const auto &It : DeclRefs) {
-      if (auto ID = getSymbolID(It.first)) {
-        for (const auto &LocAndRole : It.second)
-          CollectRef(*ID, LocAndRole);
+  llvm::DenseMap<FileID, std::vector<syntax::Token>> FilesToTokensCache;
+  for (auto &DeclAndRef : DeclRefs) {
+    if (auto ID = getSymbolID(DeclAndRef.first)) {
+      for (auto &LocAndRole : DeclAndRef.second) {
+        const auto FileID = SM.getFileID(LocAndRole.first);
+        // FIXME: It's better to use TokenBuffer by passing spelled tokens from
+        // the caller of SymbolCollector.
+        if (!FilesToTokensCache.count(FileID))
+          FilesToTokensCache[FileID] =
+              syntax::tokenize(FileID, SM, ASTCtx->getLangOpts());
+        llvm::ArrayRef<syntax::Token> Tokens = FilesToTokensCache[FileID];
+        // Check if the referenced symbol is spelled exactly the same way the
+        // corresponding NamedDecl is. If it is, mark this reference as spelled.
+        const auto *IdentifierToken =
+            spelledIdentifierTouching(LocAndRole.first, Tokens);
+        DeclarationName Name = DeclAndRef.first->getDeclName();
+        bool Spelled = IdentifierToken && Name.isIdentifier() &&
+                       Name.getAsString() == IdentifierToken->text(SM);
+        CollectRef(*ID, LocAndRole, Spelled);
       }
     }
   }
 
   ReferencedDecls.clear();
   ReferencedMacros.clear();
   DeclRefs.clear();
   FilesToIndexCache.clear();

clang-tools-extra/clangd/unittests/RenameTests.cpp

 using testing::Pair;
 using testing::IsEmpty;
 using testing::UnorderedElementsAre;
 using testing::UnorderedElementsAreArray;
 
 // Convert a Range to a Ref.
 Ref refWithRange(const clangd::Range &Range, const std::string &URI) {
   Ref Result;
   Result.Kind = RefKind::Reference;
   Result.Location.Start.setLine(Range.start.line);
   Result.Location.Start.setColumn(Range.start.character);
   Result.Location.End.setLine(Range.end.line);
   Result.Location.End.setColumn(Range.end.character);
   Result.Location.FileURI = URI.c_str();
   return Result;
 }
 
llvm::StringRef FooCC;
           R"cpp(
         #include "foo.h"
         [[INT]] foo() {}
       )cpp",
       },
       {
           // variables.
           R"cpp(
         static const int [[VA^R]] = 123;
       )cpp",
           R"cpp(
         #include "foo.h"
         int s = [[VAR]];
       )cpp",
       },
       {
           // scope enums.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

 TEST_F(SymbolCollectorTest, MacrosWithRefFilter) {
   Annotations Header("#define $macro[[MACRO]](X) (X + 1)");
   Annotations Main("void foo() { int x = $macro[[MACRO]](1); }");
   CollectorOpts.RefFilter = RefKind::Unknown;
   runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  struct Foo;
+  #define MACRO Foo
+  )cpp");
+  Annotations Main(R"cpp(
+  struct $spelled[[Foo]] {
+    $spelled[[Foo]]();
+    ~$spelled[[Foo]]();
+  };
+  $spelled[[Foo]] Variable1;
+  $implicit[[MACRO]] Variable2;
+  )cpp");
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = false;
+  runSymbolCollector(Header.code(), Main.code());
+  const auto SpelledRanges = Main.ranges("spelled");
+  const auto ImplicitRanges = Main.ranges("implicit");
+  RefSlab::Builder SpelledSlabBuilder, ImplicitSlabBuilder;
+  for (const auto &SymbolAndRefs : Refs) {
+    const auto Symbol = SymbolAndRefs.first;
+    for (const auto &Ref : SymbolAndRefs.second)
+      if ((Ref.Kind & RefKind::Spelled) != RefKind::Unknown)
+        SpelledSlabBuilder.insert(Symbol, Ref);
+      else
+        ImplicitSlabBuilder.insert(Symbol, Ref);
+  }
+  const auto SpelledRefs = std::move(SpelledSlabBuilder).build(),
+             ImplicitRefs = std::move(ImplicitSlabBuilder).build();
+  EXPECT_THAT(SpelledRefs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+                                         HaveRanges(SpelledRanges))));
+  EXPECT_THAT(ImplicitRefs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+                                          HaveRanges(ImplicitRanges))));
+}
+
 TEST_F(SymbolCollectorTest, NameReferences) {
   CollectorOpts.RefFilter = RefKind::All;
   CollectorOpts.RefsInHeaders = true;
   Annotations Header(R"(
     class [[Foo]] {
     public:
       [[Foo]]() {}
       ~[[Foo]]() {}