Skip to content

Implements support for the enclosing_range field in SCIP occurrences, #504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
simonhollis wants to merge 1 commit into
base: main
Choose a base branch
from
+75 −33
Open

Implements support for the enclosing_range field in SCIP occurrences, #504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions fetch_deps.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ _RAPIDJSON_COMMIT = "a98e99992bd633a2736cc41f96ec85ef0c50e44d"
_WYHASH_COMMIT = "ea3b25e1aef55d90f707c3a292eeb9162e2615d8"
_SPDLOG_COMMIT = "edc51df1bdad8667b628999394a1e7c4dc6f3658"
_PROTOBUF_VERSION = "3.21.12"
_SCIP_COMMIT = "aa0e511dcfefbacc3b96dcc2fe2abd9894416b1e"
_SCIP_COMMIT = "9d5796159c97c3b107355c424f6c06fb1a0ea01c"
_UTFCPP_VERSION = "4.0.5"
# ^ When bumping this version, check if any new fields are introduced
# in the types for which we implement hashing and comparison in
Expand Down Expand Up @@ -117,7 +117,7 @@ def fetch_direct_dependencies():

http_archive(
name = "scip",
sha256 = "b1d2fc009345857aa32cdddec11b75ce1e5c20430f668044231ed309d48b7355",
sha256 = "de37d2118ce87a817d627a7c8056630b7815f4477bbf1838dc46507e89275bb4",
build_file = "@scip_clang//third_party:scip.BUILD",
strip_prefix = "scip-%s" % _SCIP_COMMIT,
urls = ["https://github.com/sourcegraph/scip/archive/%s.zip" % _SCIP_COMMIT],
Expand Down
89 changes: 63 additions & 26 deletions indexer/Indexer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ void TuIndexer::saveBindingDecl(const clang::BindingDecl &bindingDecl) {
return;
}
this->saveDefinition(optSymbol.value(), bindingDecl.getLocation(),
std::nullopt);
std::nullopt, 0, bindingDecl.getSourceRange());
}

void TuIndexer::saveClassTemplateDecl(const clang::ClassTemplateDecl &) {
Expand All @@ -476,7 +476,8 @@ void TuIndexer::saveEnumConstantDecl(

ENFORCE(enumConstantDecl.getBeginLoc() == enumConstantDecl.getLocation());
this->saveDefinition(symbol, enumConstantDecl.getLocation(),
std::move(symbolInfo));
std::move(symbolInfo), 0,
enumConstantDecl.getSourceRange());
}

void TuIndexer::saveEnumDecl(const clang::EnumDecl &enumDecl) {
Expand Down Expand Up @@ -513,7 +514,8 @@ void TuIndexer::saveFieldDecl(const clang::FieldDecl &fieldDecl) {
}
scip::SymbolInformation symbolInfo{};
this->getDocComment(fieldDecl).addTo(symbolInfo);
this->saveDefinition(optSymbol.value(), fieldDecl.getLocation(), symbolInfo);
this->saveDefinition(optSymbol.value(), fieldDecl.getLocation(), symbolInfo,
0, fieldDecl.getSourceRange());
}

void TuIndexer::saveFieldReference(const clang::FieldDecl &fieldDecl,
Expand Down Expand Up @@ -557,7 +559,8 @@ void TuIndexer::saveFunctionDecl(const clang::FunctionDecl &functionDecl) {
// and for 'operator<<', it would exclude the range of '<<'.
// So just rely on the single token implementation for now.
this->saveDefinition(symbol, functionDecl.getLocation(),
std::move(symbolInfo));
std::move(symbolInfo), 0,
functionDecl.getSourceRange());
} else {
this->saveForwardDeclaration(symbol, functionDecl.getLocation(),
this->getDocComment(functionDecl));
Expand Down Expand Up @@ -611,7 +614,8 @@ void TuIndexer::saveNamespaceDecl(const clang::NamespaceDecl &namespaceDecl) {
namespaceDecl.isInlineNamespace() ? "inline " : "",
namespaceDecl.getName());

this->saveDefinition(symbol, startLoc, std::move(symbolInfo));
this->saveDefinition(symbol, startLoc, std::move(symbolInfo), 0,
namespaceDecl.getSourceRange());
}

void TuIndexer::trySaveTypeReference(const clang::Type *type,
Expand Down Expand Up @@ -799,7 +803,8 @@ void TuIndexer::saveTagDecl(const clang::TagDecl &tagDecl) {
}
}
}
this->saveDefinition(symbol, tagDecl.getLocation(), std::move(symbolInfo));
this->saveDefinition(symbol, tagDecl.getLocation(), std::move(symbolInfo), 0,
tagDecl.getSourceRange());
}

void TuIndexer::saveTagTypeLoc(const clang::TagTypeLoc &tagTypeLoc) {
Expand All @@ -813,11 +818,12 @@ void TuIndexer::saveTagTypeLoc(const clang::TagTypeLoc &tagTypeLoc) {
}
}

#define SAVE_TEMPLATE_PARM(name_) \
void TuIndexer::save##name_##Decl(const clang::name_##Decl &decl) { \
if (auto optSymbol = this->symbolFormatter.get##name_##Symbol(decl)) { \
this->saveDefinition(*optSymbol, decl.getLocation(), std::nullopt); \
} \
#define SAVE_TEMPLATE_PARM(name_) \
void TuIndexer::save##name_##Decl(const clang::name_##Decl &decl) { \
if (auto optSymbol = this->symbolFormatter.get##name_##Symbol(decl)) { \
this->saveDefinition(*optSymbol, decl.getLocation(), std::nullopt, 0, \
decl.getSourceRange()); \
} \
}
FOR_EACH_TEMPLATE_PARM_TO_BE_INDEXED(SAVE_TEMPLATE_PARM)
#undef SAVE_TEMPLATE_PARM
Expand Down Expand Up @@ -892,7 +898,8 @@ void TuIndexer::saveTypedefNameDecl(
scip::SymbolInformation symbolInfo{};
this->getDocComment(typedefNameDecl).addTo(symbolInfo);
this->saveDefinition(*optSymbol, typedefNameDecl.getLocation(),
std::move(symbolInfo));
std::move(symbolInfo), 0,
typedefNameDecl.getSourceRange());
}

void TuIndexer::saveUsingShadowDecl(
Expand All @@ -903,7 +910,8 @@ void TuIndexer::saveUsingShadowDecl(
scip::SymbolInformation symbolInfo{};
this->getDocComment(usingShadowDecl).addTo(symbolInfo);
this->saveDefinition(*optSymbol, usingShadowDecl.getLocation(),
std::move(symbolInfo));
std::move(symbolInfo), 0,
usingShadowDecl.getSourceRange());
}
if (auto *namedDecl = usingShadowDecl.getTargetDecl()) {
if (auto optSymbol =
Expand Down Expand Up @@ -938,14 +946,16 @@ void TuIndexer::saveVarDecl(const clang::VarDecl &varDecl) {
}
if (varDecl.isLocalVarDeclOrParm()) {
GET_SYMBOL;
this->saveDefinition(*optSymbol, loc, std::nullopt);
this->saveDefinition(*optSymbol, loc, std::nullopt, 0,
varDecl.getSourceRange());
}
if (varDecl.isStaticDataMember() || varDecl.isFileVarDecl()) {
GET_SYMBOL;
// Non-static data members are handled by saveFieldDecl
scip::SymbolInformation symbolInfo{};
this->getDocComment(varDecl).addTo(symbolInfo);
this->saveDefinition(*optSymbol, loc, symbolInfo);
this->saveDefinition(*optSymbol, loc, symbolInfo, 0,
varDecl.getSourceRange());
}
#undef GET_SYMBOL
}
Expand Down Expand Up @@ -1153,7 +1163,8 @@ void TuIndexer::saveForwardDeclaration(SymbolNameRef symbol,

void TuIndexer::saveReference(SymbolNameRef symbol, clang::SourceLocation loc,
const clang::Decl *maybeFwdDecl,
int32_t extraRoles) {
int32_t extraRoles,
clang::SourceRange enclosingRange) {
auto expansionLoc = this->sourceManager.getExpansionLoc(loc);
auto fileId = this->sourceManager.getFileID(expansionLoc);
if (!this->fileIdsToBeIndexed.contains({fileId})) {
Expand All @@ -1171,13 +1182,13 @@ void TuIndexer::saveReference(SymbolNameRef symbol, clang::SourceLocation loc,
optStableFileId->path, range);
return;
}
(void)this->saveOccurrence(symbol, expansionLoc, extraRoles);
(void)this->saveOccurrence(symbol, expansionLoc, extraRoles, enclosingRange);
}

void TuIndexer::saveDefinition(
SymbolNameRef symbol, clang::SourceLocation loc,
std::optional<scip::SymbolInformation> &&optSymbolInfo,
int32_t extraRoles) {
std::optional<scip::SymbolInformation> &&optSymbolInfo, int32_t extraRoles,
clang::SourceRange enclosingRange) {
auto expansionLoc = this->sourceManager.getExpansionLoc(loc);
auto fileId = this->sourceManager.getFileID(expansionLoc);
if (!this->fileIdsToBeIndexed.contains({fileId})) {
Expand All @@ -1192,7 +1203,8 @@ void TuIndexer::saveDefinition(
}
if (optStableFileId->isInProject) {
auto &doc = this->saveOccurrence(symbol, expansionLoc,
extraRoles | scip::SymbolRole::Definition);
extraRoles | scip::SymbolRole::Definition,
enclosingRange);
if (optSymbolInfo.has_value()) {
doc.symbolInfos.emplace(symbol, std::move(optSymbolInfo.value()));
}
Expand All @@ -1214,19 +1226,44 @@ void TuIndexer::saveExternalSymbol(SymbolNameRef symbol,

PartialDocument &TuIndexer::saveOccurrence(SymbolNameRef symbol,
clang::SourceLocation expansionLoc,
int32_t allRoles) {
int32_t allRoles,
clang::SourceRange enclosingRange) {
auto [range, fileId] = this->getTokenExpansionRange(expansionLoc);
return this->saveOccurrenceImpl(symbol, range, fileId, allRoles);
FileLocalSourceRange enclosingLocalRange{};
// Check if range is valid, non-empty, and not inverted before calling
// fromNonEmpty fromNonEmpty enforces: start.isValid() && end.isValid() &&
// start <= end
auto begin = enclosingRange.getBegin();
auto end = enclosingRange.getEnd();
if (enclosingRange.isValid() && begin.isValid() && end.isValid()
&& begin != end && begin < end) {
Comment on lines +1238 to +1239

@rakiz rakiz Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this PR, workers crash on many TUs with:

Indexer.cc: enforced condition sourceManager.getFileID(end) == fileId ... range should not be split across files

saveOccurrence passes enclosingRange to FileLocalSourceRange::fromNonEmpty, which ENFORCEs both endpoints share a FileID, but an enclosing range can span a macro expansion or #include boundary, so the ENFORCE trips and kills the worker.

The crashed workers don't recover: the driver logs timeout: no workers have responded yet and terminating worker … due to worker timeout, respawns them, they crash again. On one full index this ran ~2.5h with 205 crashes and 197 worker-timeout terminations, and no .scip was ever produced (the run doesn't error out, it just never finishes).

Fix: add a same-file guard to the condition and skip enclosing_range otherwise (it's optional):

if (enclosingRange.isValid() && begin.isValid() && end.isValid()
    && begin != end && begin < end
    && sourceManager.getFileID(begin) == sourceManager.getFileID(end)) {

With this guard: 0 crashes over a full index.

auto [localRange, enclosingFileId] =
FileLocalSourceRange::fromNonEmpty(this->sourceManager, enclosingRange);
if (enclosingFileId == fileId) {
enclosingLocalRange = localRange;
}
}
return this->saveOccurrenceImpl(symbol, range, fileId, allRoles,
enclosingLocalRange);
}

PartialDocument &TuIndexer::saveOccurrenceImpl(SymbolNameRef symbol,
FileLocalSourceRange range,
clang::FileID fileId,
int32_t allRoles) {
PartialDocument &
TuIndexer::saveOccurrenceImpl(SymbolNameRef symbol, FileLocalSourceRange range,
clang::FileID fileId, int32_t allRoles,
FileLocalSourceRange enclosingRange) {
scip::Occurrence occ;
range.addTo(occ);
occ.set_symbol(symbol.value.data(), symbol.value.size());
occ.set_symbol_roles(allRoles);
// Add enclosing range if provided (i.e., if it has non-zero coordinates)
if (enclosingRange.startLine > 0 && enclosingRange.endLine > 0) {
occ.add_enclosing_range(enclosingRange.startLine - 1);
occ.add_enclosing_range(enclosingRange.startColumn - 1);
if (enclosingRange.startLine != enclosingRange.endLine) {
occ.add_enclosing_range(enclosingRange.endLine - 1);
}
occ.add_enclosing_range(enclosingRange.endColumn - 1);
}
auto &doc = this->documentMap[{fileId}];
doc.occurrences.emplace_back(scip::OccurrenceExt{std::move(occ)});
return doc;
Expand Down
15 changes: 10 additions & 5 deletions indexer/Indexer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include "clang/AST/RawCommentList.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"

#include "indexer/ApproximateNameResolver.h"
#include "indexer/ClangAstMacros.h"
Expand All @@ -37,13 +38,13 @@ FOR_EACH_TYPE_TO_BE_INDEXED(FORWARD_DECLARE)
class ASTContext;
class Decl;
class DeclarationNameInfo;
class FileID;
class LangOptions;
class MacroDefinition;
class MacroInfo;
class NamedDecl;
class NestedNameSpecifierLoc;
class QualType;
class SourceManager;
class TagDecl;
class TagTypeLoc;
class Token;
Expand Down Expand Up @@ -370,7 +371,8 @@ class TuIndexer final {

void saveReference(SymbolNameRef symbol, clang::SourceLocation loc,
const clang::Decl *maybeFwdDecl = nullptr,
int32_t extraRoles = 0);
int32_t extraRoles = 0,
clang::SourceRange enclosingRange = {});

/// Helper method for recording a \c scip::Occurrence and a
/// \c scip::SymbolInformation for a definition.
Expand All @@ -380,7 +382,8 @@ class TuIndexer final {
/// For local variables, \param symbolInfo should be \c std::nullopt.
void saveDefinition(SymbolNameRef symbol, clang::SourceLocation loc,
std::optional<scip::SymbolInformation> &&symbolInfo,
int32_t extraRoles = 0);
int32_t extraRoles = 0,
clang::SourceRange enclosingRange = {});

/// Only for use inside \c saveDefinition.
void saveExternalSymbol(SymbolNameRef symbol, scip::SymbolInformation &&);
Expand All @@ -395,12 +398,14 @@ class TuIndexer final {
/// since SCIP only tracks SymbolInformation values in external code.
PartialDocument &saveOccurrence(SymbolNameRef symbol,
clang::SourceLocation loc,
int32_t allRoles = 0);
int32_t allRoles = 0,
clang::SourceRange enclosingRange = {});

PartialDocument &saveOccurrenceImpl(SymbolNameRef symbol,
FileLocalSourceRange range,
clang::FileID fileId,
int32_t allRoles = 0);
int32_t allRoles = 0,
FileLocalSourceRange enclosingRange = {});

DocComment getDocComment(const clang::Decl &) const;
};
Expand Down