From b1e47b01aa0d348034b6940eff18f4560423e11a Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Wed, 2 Nov 2022 12:51:44 +0800 Subject: [PATCH 1/3] feat: Add support for manually specifiying gems for cross-repo. --- core/Error.cc | 2 +- core/GlobalState.cc | 20 ++- core/errors/errors.h | 1 + core/errors/scip_ruby.h | 10 ++ docs/scip-ruby/CLI.md | 51 +++++- scip_indexer/BUILD | 1 + scip_indexer/Debug.cc | 5 +- scip_indexer/Debug.h | 2 - scip_indexer/SCIPGemMetadata.cc | 100 ++++++++++++ scip_indexer/SCIPGemMetadata.h | 21 +++ scip_indexer/SCIPIndexer.cc | 148 ++++++++++++------ scip_indexer/SCIPIndexer.h | 14 ++ scip_indexer/SCIPSymbolRef.cc | 27 +++- scip_indexer/SCIPSymbolRef.h | 5 +- scip_indexer/SCIPUtils.cc | 4 +- scip_indexer/SCIPUtils.h | 32 +++- .../testdata/multifile/gem-map/current.rb | 11 ++ .../multifile/gem-map/current.snapshot.rb | 16 ++ .../gem-map}/downstream.rb | 0 .../multifile/gem-map/downstream.snapshot.rb | 10 ++ .../testdata/multifile/gem-map/gem-map.json | 2 + .../multifile/gem-map/scip-ruby-args.rb | 2 + .../gem-map}/upstream.rb | 0 .../multifile/gem-map/upstream.snapshot.rb | 7 + .../testdata/multigem/gem-map/gem-map.json | 2 - test/scip_test_runner.cc | 90 +++++++++-- 26 files changed, 495 insertions(+), 88 deletions(-) create mode 100644 core/errors/scip_ruby.h create mode 100644 test/scip/testdata/multifile/gem-map/current.rb create mode 100644 test/scip/testdata/multifile/gem-map/current.snapshot.rb rename test/scip/testdata/{multigem => multifile/gem-map}/downstream.rb (100%) create mode 100644 test/scip/testdata/multifile/gem-map/downstream.snapshot.rb create mode 100644 test/scip/testdata/multifile/gem-map/gem-map.json create mode 100644 test/scip/testdata/multifile/gem-map/scip-ruby-args.rb rename test/scip/testdata/{multigem => multifile/gem-map}/upstream.rb (100%) create mode 100644 test/scip/testdata/multifile/gem-map/upstream.snapshot.rb delete mode 100644 test/scip/testdata/multigem/gem-map/gem-map.json diff --git a/core/Error.cc b/core/Error.cc index 4847172c45..644a3b822f 100644 --- a/core/Error.cc +++ b/core/Error.cc @@ -124,7 +124,7 @@ string Error::toString(const GlobalState &gs) const { stringstream buf; buf << RESET_STYLE << FILE_POS_STYLE << loc.filePosToString(gs) << RESET_STYLE << ": " << ERROR_COLOR << restoreColors(header, ERROR_COLOR) << RESET_COLOR; - if (what.code != 25900) { // SCIPRubyDebug + if (what != scip_indexer::errors::SCIPRubyDebug && what != scip_indexer::errors::SCIPRuby) { buf << LOW_NOISE_COLOR << " " << gs.errorUrlBase << what.code << RESET_COLOR; } if (loc.exists()) { diff --git a/core/GlobalState.cc b/core/GlobalState.cc index d45bf29c80..6083ce97df 100644 --- a/core/GlobalState.cc +++ b/core/GlobalState.cc @@ -13,6 +13,7 @@ #include "core/hashing/hashing.h" #include "core/lsp/Task.h" #include "core/lsp/TypecheckEpochManager.h" +#include #include #include "absl/strings/str_cat.h" @@ -1663,14 +1664,21 @@ FileRef GlobalState::enterFile(const shared_ptr &file) { } files.emplace_back(file); + // GlobalState initialization guarantees the 0 slot will be taken, so this is OK. auto ret = FileRef(filesUsed() - 1); fileRefByPath[string(file->path())] = ret; return ret; } FileRef GlobalState::enterFile(string_view path, string_view source) { + std::string pathBuf; + if (this->isSCIPRuby) { // See [NOTE: scip-ruby-path-normalization] + pathBuf = string(std::filesystem::path(path).lexically_normal()); + } else { + pathBuf = string(path); + } return GlobalState::enterFile( - make_shared(string(path.begin(), path.end()), string(source.begin(), source.end()), File::Type::Normal)); + make_shared(move(pathBuf), string(source.begin(), source.end()), File::Type::Normal)); } FileRef GlobalState::enterNewFileAt(const shared_ptr &file, FileRef id) { @@ -1686,7 +1694,13 @@ FileRef GlobalState::enterNewFileAt(const shared_ptr &file, FileRef id) { } FileRef GlobalState::reserveFileRef(string path) { - return GlobalState::enterFile(make_shared(move(path), "", File::Type::NotYetRead)); + std::string pathBuf; + if (this->isSCIPRuby) { // See [NOTE: scip-ruby-path-normalization] + pathBuf = string(std::filesystem::path(path).lexically_normal()); + } else { + pathBuf = move(path); + } + return GlobalState::enterFile(make_shared(move(pathBuf), "", File::Type::NotYetRead)); } NameRef GlobalState::nextMangledName(ClassOrModuleRef owner, NameRef origName) { @@ -2155,7 +2169,7 @@ bool GlobalState::shouldReportErrorOn(Loc loc, ErrorClass what) const { return false; } if (this->isSCIPRuby && !this->unsilenceErrors) { - if (what.code != 25900) { // SCIPRubyDebug + if (what != scip_indexer::errors::SCIPRubyDebug && what != scip_indexer::errors::SCIPRuby) { return false; } } diff --git a/core/errors/errors.h b/core/errors/errors.h index 2dc9eb422a..fbc9f2a972 100644 --- a/core/errors/errors.h +++ b/core/errors/errors.h @@ -15,3 +15,4 @@ #include "core/errors/packager.h" #include "core/errors/parser.h" #include "core/errors/resolver.h" +#include "core/errors/scip_ruby.h" diff --git a/core/errors/scip_ruby.h b/core/errors/scip_ruby.h new file mode 100644 index 0000000000..77d05e51d3 --- /dev/null +++ b/core/errors/scip_ruby.h @@ -0,0 +1,10 @@ +#ifndef SORBET_CORE_ERRORS_SCIP_RUBY_H +#define SORBET_CORE_ERRORS_SCIP_RUBY_H +#include "core/Error.h" + +namespace sorbet::scip_indexer::errors { +constexpr sorbet::core::ErrorClass SCIPRubyDebug{25900, sorbet::core::StrictLevel::False}; +constexpr sorbet::core::ErrorClass SCIPRuby{25901, sorbet::core::StrictLevel::False}; +} // namespace sorbet::scip_indexer::errors + +#endif // SORBET_CORE_ERRORS_SCIP_RUBY_H diff --git a/docs/scip-ruby/CLI.md b/docs/scip-ruby/CLI.md index bb9a051398..c49fcf99ab 100644 --- a/docs/scip-ruby/CLI.md +++ b/docs/scip-ruby/CLI.md @@ -20,6 +20,55 @@ For example, with Git, you can use the last tag string. For repos which index every commit, you could also use the SHA instead (`git rev-parse HEAD`). +## `--gem-map-path ` + +At the moment, scip-ruby requires an extra step for cross-repo +code navigation; you need to supply information about which. +file belongs to which gem explicitly, using a newline-delimited +JSON file in the following format: + + +```json +{"path": "a/b/c.rb", "gem": "my_gem@1.2.3"} +{"path": "a/b/d.rb", "gem": "my_gem@1.2.3"} +{"path": "a/x/y.rb", "gem": "other_gem@3.4.9"} +... +``` + +Then pass the path to the JSON file: + +```bash +scip-ruby --gem-map-path path/to/cross-repo-metadata.json +``` + +Paths are interpreted relative to the working directory +for the `scip-ruby` invocation. + +If information about the gem being indexed +cannot be inferred from the filesystem, then you can supply +the `--gem-metadata` argument as described earlier. + +If you run into an error message where a path in the JSON file +is not recognized by `scip-ruby`, you can re-run the indexing command +with extra arguments `--log-recorded-filepaths --debug-log-file out.log` +to identify differences between the JSON file +and paths created by traversing directories. + +## `--index-file ` + +The path for emitting the SCIP index. Defaults to `index.scip`. + ## `--unquiet-errors` scip-ruby defaults to running in Sorbet's quiet mode, as scip-ruby supports @@ -27,4 +76,4 @@ indexing `# typed: false` files on a best-effort basis, but Sorbet may rightfully flag many errors in those files. The number of errors can be overwhelming if there is a large amount of untyped code. -This flag restores Sorbet's default behavior. +This flag restores Sorbet's default behavior. \ No newline at end of file diff --git a/scip_indexer/BUILD b/scip_indexer/BUILD index 5e5330deef..4cd6786786 100644 --- a/scip_indexer/BUILD +++ b/scip_indexer/BUILD @@ -61,6 +61,7 @@ cc_library( "@com_google_absl//absl/strings", "@com_google_absl//absl/synchronization", "@cxxopts", + "@rapidjson", "@spdlog", ], ) diff --git a/scip_indexer/Debug.cc b/scip_indexer/Debug.cc index 985f511373..20b4ba3fc8 100644 --- a/scip_indexer/Debug.cc +++ b/scip_indexer/Debug.cc @@ -3,15 +3,14 @@ #include "core/GlobalState.h" #include "core/Loc.h" +#include "core/errors/scip_ruby.h" #include "scip_indexer/Debug.h" namespace sorbet::scip_indexer { -constexpr sorbet::core::ErrorClass SCIPRubyDebug{25900, sorbet::core::StrictLevel::False}; - void _log_debug(const sorbet::core::GlobalState &gs, sorbet::core::Loc loc, std::string s) { - if (auto e = gs.beginError(loc, SCIPRubyDebug)) { + if (auto e = gs.beginError(loc, errors::SCIPRubyDebug)) { auto lines = absl::StrSplit(s, '\n'); for (auto line = lines.begin(); line != lines.end(); line++) { auto text = std::string(line->begin(), line->length()); diff --git a/scip_indexer/Debug.h b/scip_indexer/Debug.h index 20c431ba13..d7661c6710 100644 --- a/scip_indexer/Debug.h +++ b/scip_indexer/Debug.h @@ -55,8 +55,6 @@ template std::string showVec(const V &v, Fn f) { return out.str(); } -extern const sorbet::core::ErrorClass SCIPRubyDebug; - void _log_debug(const sorbet::core::GlobalState &gs, sorbet::core::Loc loc, std::string s); } // namespace sorbet::scip_indexer diff --git a/scip_indexer/SCIPGemMetadata.cc b/scip_indexer/SCIPGemMetadata.cc index 27a20894e9..d19240fc9e 100644 --- a/scip_indexer/SCIPGemMetadata.cc +++ b/scip_indexer/SCIPGemMetadata.cc @@ -8,6 +8,15 @@ #include #include "absl/strings/str_replace.h" + +#include "common/FileSystem.h" +#include "common/common.h" +#include "core/FileRef.h" +#include "core/errors/scip_ruby.h" + +// Uses ENFORCE as defined by common/common.h, so put this later +#include "rapidjson/document.h" + #include "scip_indexer/SCIPGemMetadata.h" using namespace std; @@ -169,4 +178,95 @@ pair> GemMetadata::readFromConfig(const Fi return {GemMetadata(name.value_or(currentDirName()), version.value_or("latest")), errors}; } +GemMapping::GemMapping() { + // The 'ruby' namespace is reserved by RubyGems.org, so we won't run into any + // actual gem called 'ruby', so we use that here. 'stdlib' would not be appropriate + // as Sorbet contains RBIs for both core and stdlib modules. + // For the version, we're not really doing any versioning for core & stdlib RBIs, + // (e.g. handling both 2.7 and 3) so let's stick to latest for now. + this->stdlibGem = make_shared(GemMetadata::tryParse("ruby@latest").value()); +} + +optional> GemMapping::lookupGemForFile(const core::GlobalState &gs, core::FileRef file) const { + ENFORCE(file.exists()); + auto it = this->map.find(file); + if (it != this->map.end()) { + return it->second; + } + auto filepath = file.data(gs).path(); + if (absl::StartsWith(filepath, core::File::URL_PREFIX)) { + return this->stdlibGem; + } + if (this->currentGem.has_value()) { + // TODO Should we enforce here in debug builds? + // Fallback to this if set, to avoid collisions with other gems. + return this->currentGem.value(); + } + return nullopt; +} + +void GemMapping::populateFromNDJSON(const core::GlobalState &gs, const FileSystem &fs, const std::string &ndjsonPath) { + istringstream input(fs.readFile(ndjsonPath)); + auto currentDir = filesystem::path(fs.getCurrentDir()); + unsigned errorCount = 0; + for (string line; getline(input, line);) { + auto jsonLine = std::string(absl::StripAsciiWhitespace(line)); + if (jsonLine.empty()) { + continue; + } + rapidjson::Document document; + document.Parse(jsonLine); + if (document.HasParseError() || !document.IsObject()) { + continue; + } + if (document.HasMember("path") && document["path"].IsString() && document.HasMember("gem") && + document["gem"].IsString()) { + auto jsonPath = document["path"].GetString(); + auto fileRef = gs.findFileByPath(jsonPath); + if (!fileRef.exists()) { + vector alternatePaths; + auto path = filesystem::path(jsonPath); + // [NOTE: scip-ruby-path-normalization] + // We normalize paths upon entry into GlobalState, so it is sufficient to only check + // different normalized combinations here. One common situation where normalization is + // necessary is if you pass '.' as the directory argument, without normalization, + // the entered paths would be './'-prefixed, but you could reasonably forget to add + // the same in the JSON file. + auto normalizedPath = path.lexically_normal(); + if (normalizedPath != path) { + alternatePaths.push_back(normalizedPath); + } + if (normalizedPath.is_absolute()) { + alternatePaths.push_back(string(normalizedPath.lexically_relative(currentDir).lexically_normal())); + } else { + alternatePaths.push_back(string((currentDir / normalizedPath).lexically_normal())); + } + bool foundFile = absl::c_any_of(alternatePaths, [&](auto &altPath) -> bool { + fileRef = gs.findFileByPath(altPath); + return fileRef.exists(); + }); + if (!foundFile) { + errorCount++; + if (errorCount < 5) { + if (auto e = gs.beginError(core::Loc(), errors::SCIPRuby)) { + e.setHeader("File path in JSON map doesn't match known paths: {}", jsonPath); + e.addErrorNote("Use --debug-log-filepaths to check the paths being recorded by scip-ruby"); + } + } + continue; + } + } + auto gemMetadata = GemMetadata::tryParse(document["gem"].GetString()); + if (!gemMetadata.has_value()) { + continue; + } + this->map[fileRef] = make_shared(move(gemMetadata.value())); + } + } +} + +void GemMapping::markCurrentGem(GemMetadata gem) { + this->currentGem = make_shared(gem); +} + } // namespace sorbet::scip_indexer diff --git a/scip_indexer/SCIPGemMetadata.h b/scip_indexer/SCIPGemMetadata.h index 4407969bac..bcf01c11b0 100644 --- a/scip_indexer/SCIPGemMetadata.h +++ b/scip_indexer/SCIPGemMetadata.h @@ -1,6 +1,7 @@ #ifndef SORBET_SCIP_GEM_METADATA #define SORBET_SCIP_GEM_METADATA +#include #include #include #include @@ -8,6 +9,9 @@ #include "absl/strings/str_split.h" #include "common/FileSystem.h" +#include "common/common.h" +#include "core/FileRef.h" +#include "core/GlobalState.h" namespace sorbet::scip_indexer { @@ -71,6 +75,23 @@ class GemMetadata final { static std::pair> readFromGemspec(const std::string &); }; +// Type carrying gem information for each file, which is used during +// symbol emission to ensure correct symbol names for cross-repo. +class GemMapping final { + std::shared_ptr stdlibGem; + std::optional> currentGem; + UnorderedMap> map; + +public: + GemMapping(); + + std::optional> lookupGemForFile(const core::GlobalState &gs, core::FileRef file) const; + + void populateFromNDJSON(const core::GlobalState &, const FileSystem &fs, const std::string &ndjsonPath); + + void markCurrentGem(GemMetadata gem); +}; + } // namespace sorbet::scip_indexer #endif // SORBET_SCIP_GEM_METADATA diff --git a/scip_indexer/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc index 29657662cd..8298ef548a 100644 --- a/scip_indexer/SCIPIndexer.cc +++ b/scip_indexer/SCIPIndexer.cc @@ -30,12 +30,13 @@ #include "core/Loc.h" #include "core/SymbolRef.h" #include "core/Symbols.h" -#include "main/pipeline/semantic_extension/SemanticExtension.h" +#include "core/errors/scip_ruby.h" #include "sorbet_version/sorbet_version.h" #include "scip_indexer/Debug.h" #include "scip_indexer/SCIPFieldResolve.h" #include "scip_indexer/SCIPGemMetadata.h" +#include "scip_indexer/SCIPIndexer.h" #include "scip_indexer/SCIPProtoExt.h" #include "scip_indexer/SCIPSymbolRef.h" #include "scip_indexer/SCIPUtils.h" @@ -181,7 +182,7 @@ class SCIPState { // // FIXME(varun): This seems redundant, get rid of it. - GemMetadata gemMetadata; + GemMapping gemMap; public: UnorderedMap> occurrenceMap; @@ -207,9 +208,9 @@ class SCIPState { vector externalSymbols; public: - SCIPState(GemMetadata metadata) + SCIPState(GemMapping gemMap) : symbolStringCache(), localOccurrenceCache(), symbolOccurrenceCache(), potentialRefOnlySymbols(), - gemMetadata(metadata), occurrenceMap(), emittedSymbols(), symbolMap(), documents(), externalSymbols() {} + gemMap(gemMap), occurrenceMap(), emittedSymbols(), symbolMap(), documents(), externalSymbols() {} ~SCIPState() = default; SCIPState(SCIPState &&) = default; SCIPState &operator=(SCIPState &&other) = default; @@ -224,8 +225,8 @@ class SCIPState { /// If the returned value is as success, the pointer is non-null. /// /// The argument symbol is used instead of recomputing from scratch if it is non-null. - absl::Status saveSymbolString(const core::GlobalState &gs, UntypedGenericSymbolRef symRef, - const scip::Symbol *symbol, std::string &output) { + utils::Result saveSymbolString(const core::GlobalState &gs, UntypedGenericSymbolRef symRef, + const scip::Symbol *symbol, std::string &output) { auto pair = this->symbolStringCache.find(symRef); if (pair != this->symbolStringCache.end()) { // Yes, there is a "redundant" string copy here when we could "just" @@ -233,26 +234,26 @@ class SCIPState { // that creates a footgun where this method cannot be safely called // across invocations to non-const method calls on SCIPState. output = pair->second; - return absl::OkStatus(); + return utils::Result::okValue(); } absl::Status status; if (symbol) { - status = scip::utils::emitSymbolString(*symbol, output); + status = utils::emitSymbolString(*symbol, output); } else { scip::Symbol symbol; - status = symRef.symbolForExpr(gs, this->gemMetadata, {}, symbol); - if (!status.ok()) { - return status; + auto result = symRef.symbolForExpr(gs, this->gemMap, {}, symbol); + if (!result.ok()) { + return result; } - status = scip::utils::emitSymbolString(symbol, output); + status = utils::emitSymbolString(symbol, output); } if (!status.ok()) { - return status; + return utils::Result::statusValue(status); } symbolStringCache.insert({symRef, output}); - return absl::OkStatus(); + return utils::Result::okValue(); } private: @@ -372,7 +373,7 @@ class SCIPState { untypedSymRef.saveRelationships(gs, this->relationshipsMap[file], rels, [this, &gs](UntypedGenericSymbolRef sym, std::string &out) { auto status = this->saveSymbolString(gs, sym, nullptr, out); - ENFORCE(status.ok()); + ENFORCE(status.skip() || status.ok()); }); } @@ -402,14 +403,18 @@ class SCIPState { auto occLoc = loc.has_value() ? core::Loc(file, loc.value()) : symRef.symbolLoc(gs); scip::Symbol symbol; auto untypedSymRef = symRef.withoutType(); - auto status = untypedSymRef.symbolForExpr(gs, this->gemMetadata, occLoc, symbol); - if (!status.ok()) { - return status; + auto result = untypedSymRef.symbolForExpr(gs, this->gemMap, occLoc, symbol); + if (result.skip()) { + return absl::OkStatus(); + } + if (!result.ok()) { + return result.status(); } std::string symbolString; - status = this->saveSymbolString(gs, untypedSymRef, &symbol, symbolString); - if (!status.ok()) { - return status; + result = this->saveSymbolString(gs, untypedSymRef, &symbol, symbolString); + ENFORCE(!result.skip(), "Should've skipped earlier"); + if (!result.ok()) { + return result.status(); } SmallVec docs; @@ -454,9 +459,12 @@ class SCIPState { auto &gs = ctx.state; auto file = ctx.file; std::string symbolString; - auto status = this->saveSymbolString(gs, symRef.withoutType(), nullptr, symbolString); - if (!status.ok()) { - return status; + auto result = this->saveSymbolString(gs, symRef.withoutType(), nullptr, symbolString); + if (result.skip()) { + return absl::OkStatus(); + } + if (!result.ok()) { + return result.status(); } SmallVec overrideDocs{}; @@ -507,7 +515,14 @@ class SCIPState { scip::Document document; // TODO(varun): Double-check the path code and maybe document it, // to make sure its guarantees match what SCIP expects. - document.set_relative_path(string(file.data(gs).path())); + ENFORCE(file.exists()); + auto path = file.data(gs).path(); + ENFORCE(!path.empty()); + if (path.front() == '/') { + document.set_relative_path(filesystem::path(path).lexically_relative(filesystem::current_path())); + } else { + document.set_relative_path(string(path)); + } auto occurrences = this->occurrenceMap.find(file); if (occurrences != this->occurrenceMap.end()) { @@ -1114,18 +1129,37 @@ namespace sorbet::pipeline::semantic_extension { using LocalSymbolTable = UnorderedMap; class SCIPSemanticExtension : public SemanticExtension { -public: string indexFilePath; - string gemMetadataString; - scip_indexer::GemMetadata gemMetadata; + scip_indexer::Config config; + scip_indexer::GemMapping gemMap; using SCIPState = sorbet::scip_indexer::SCIPState; +public: + using StateMap = UnorderedMap>; + +private: mutable struct { - UnorderedMap> states; + StateMap states; absl::Mutex mtx; } mutableState; +public: + SCIPSemanticExtension(string indexFilePath, scip_indexer::Config config, scip_indexer::GemMapping gemMap, + StateMap states) + : indexFilePath(indexFilePath), config(config), gemMap(gemMap), mutableState{states, {}} {} + + ~SCIPSemanticExtension() {} + + virtual unique_ptr deepCopy(const core::GlobalState &from, core::GlobalState &to) override { + // FIXME: Technically, this would copy the state too, but we haven't implemented + // deep-copying for it because it doesn't matter for scip-ruby. + StateMap map; + return make_unique(this->indexFilePath, this->config, this->gemMap, map); + }; + virtual void merge(const core::GlobalState &from, core::GlobalState &to, core::NameSubstitution &subst) override{}; + +private: // Return a shared_ptr here as we need a stable address to avoid an invalid reference // on table resizing, and we need to maintain at least two pointers, // an "owning ref" from the table and a mutable borrow from the caller. @@ -1141,7 +1175,7 @@ class SCIPSemanticExtension : public SemanticExtension { // We will move the state out later, so use a no-op deleter. return mutableState.states[this_thread::get_id()] = - shared_ptr(new SCIPState(gemMetadata), [](SCIPState *) {}); + shared_ptr(new SCIPState(gemMap), [](SCIPState *) {}); } } @@ -1149,24 +1183,30 @@ class SCIPSemanticExtension : public SemanticExtension { return this->indexFilePath.empty(); } +public: void run(core::MutableContext &ctx, ast::ClassDef *cd) const override {} virtual void prepareForTypechecking(const core::GlobalState &gs) override { - auto maybeMetadata = scip_indexer::GemMetadata::tryParse(this->gemMetadataString); + auto maybeMetadata = scip_indexer::GemMetadata::tryParse(this->config.gemMetadata); + scip_indexer::GemMetadata currentGem; if (maybeMetadata.has_value()) { - this->gemMetadata = maybeMetadata.value(); + currentGem = maybeMetadata.value(); } // TODO: Issue error for incorrect format in string... - if (this->gemMetadata.name().empty() || this->gemMetadata.version().empty()) { - auto [metadata, errors] = scip_indexer::GemMetadata::readFromConfig(OSFileSystem()); - this->gemMetadata = metadata; + if (currentGem.name().empty() || currentGem.version().empty()) { + auto [gem, errors] = scip_indexer::GemMetadata::readFromConfig(OSFileSystem()); + currentGem = gem; for (auto &error : errors) { - if (auto e = gs.beginError(core::Loc(), scip_indexer::SCIPRubyDebug)) { + if (auto e = gs.beginError(core::Loc(), scip_indexer::errors::SCIPRubyDebug)) { e.setHeader("{}: {}", error.kind == scip_indexer::GemMetadataError::Kind::Error ? "error" : "warning", error.message); } } } + this->gemMap.markCurrentGem(currentGem); + if (!this->config.gemMapPath.empty()) { + this->gemMap.populateFromNDJSON(gs, OSFileSystem(), this->config.gemMapPath); + } }; virtual void finishTypecheckFile(const core::GlobalState &gs, const core::FileRef &file) const override { @@ -1269,27 +1309,23 @@ class SCIPSemanticExtension : public SemanticExtension { traversal.traverse(cfg); scipStateRef.clearFunctionLocalCaches(); } - - virtual unique_ptr deepCopy(const core::GlobalState &from, core::GlobalState &to) override { - return make_unique(this->indexFilePath, this->gemMetadataString, this->gemMetadata); - }; - virtual void merge(const core::GlobalState &from, core::GlobalState &to, core::NameSubstitution &subst) override{}; - - SCIPSemanticExtension(string indexFilePath, string gemMetadataString, scip_indexer::GemMetadata gemMetadata) - : indexFilePath(indexFilePath), gemMetadataString(gemMetadataString), gemMetadata(gemMetadata), mutableState() { - } - ~SCIPSemanticExtension() {} }; class SCIPSemanticExtensionProvider : public SemanticExtensionProvider { public: void injectOptions(cxxopts::Options &optsBuilder) const override { optsBuilder.add_options("indexer")("index-file", "Output SCIP index to a directory, which must already exist", - cxxopts::value())( + cxxopts::value()); + optsBuilder.add_options("indexer")( "gem-metadata", "Metadata in 'name@version' format to be used for cross-repository code navigation. For repositories which " "index every commit, the SHA should be used for the version instead of a git tag (or equivalent).", cxxopts::value()); + optsBuilder.add_options("indexer")( + "gem-map-path", + "Path to newline-delimited JSON file which describes how to map paths to gem name and versions. See the" + " scip-ruby docs on GitHub for information about the JSON schema.", + cxxopts::value()); }; unique_ptr readOptions(cxxopts::ParseResult &providedOptions) const override { if (providedOptions.count("version") > 0) { @@ -1308,12 +1344,22 @@ class SCIPSemanticExtensionProvider : public SemanticExtensionProvider { } else { indexFilePath = "index.scip"; } - auto gemMetadataString = - providedOptions.count("gem-metadata") > 0 ? providedOptions["gem-metadata"].as() : ""; - return make_unique(indexFilePath, gemMetadataString, scip_indexer::GemMetadata()); + scip_indexer::Config config{}; + if (providedOptions.count("gem-map-path") > 0) { + config.gemMapPath = providedOptions["gem-map-path"].as(); + } + if (providedOptions.count("gem-metadata") > 0) { + config.gemMetadata = providedOptions["gem-metadata"].as(); + } + scip_indexer::GemMapping gemMap{}; + SCIPSemanticExtension::StateMap stateMap; + return make_unique(indexFilePath, config, gemMap, stateMap); }; virtual unique_ptr defaultInstance() const override { - return make_unique("index.scip", "", scip_indexer::GemMetadata()); + scip_indexer::GemMapping gemMap{}; + scip_indexer::Config config{}; + SCIPSemanticExtension::StateMap stateMap; + return make_unique("index.scip", config, gemMap, stateMap); }; static vector getProviders(); virtual ~SCIPSemanticExtensionProvider() = default; diff --git a/scip_indexer/SCIPIndexer.h b/scip_indexer/SCIPIndexer.h index 15827eafa6..78a4a41d50 100644 --- a/scip_indexer/SCIPIndexer.h +++ b/scip_indexer/SCIPIndexer.h @@ -1,6 +1,20 @@ #ifndef SORBET_SCIP_INDEXER #define SORBET_SCIP_INDEXER +#include + #include "main/pipeline/semantic_extension/SemanticExtension.h" +namespace sorbet::scip_indexer { + +// Indexer configuration options unrelated to I/O +struct Config { + // Argument to --gem-metadata, missing state folded to "" + std::string gemMetadata; + // Argument to --gem-map-path, missing state folded to "" + std::string gemMapPath; +}; + +} // namespace sorbet::scip_indexer + #endif \ No newline at end of file diff --git a/scip_indexer/SCIPSymbolRef.cc b/scip_indexer/SCIPSymbolRef.cc index 5f3b3ec398..17efc65bf0 100644 --- a/scip_indexer/SCIPSymbolRef.cc +++ b/scip_indexer/SCIPSymbolRef.cc @@ -19,6 +19,7 @@ #include "scip_indexer/SCIPGemMetadata.h" #include "scip_indexer/SCIPProtoExt.h" #include "scip_indexer/SCIPSymbolRef.h" +#include "scip_indexer/SCIPUtils.h" using namespace std; @@ -33,13 +34,26 @@ string showRawRelationshipsMap(const core::GlobalState &gs, const RelationshipsM } // Try to compute a scip::Symbol for this value. -absl::Status UntypedGenericSymbolRef::symbolForExpr(const core::GlobalState &gs, const GemMetadata &metadata, - optional loc, scip::Symbol &symbol) const { +utils::Result UntypedGenericSymbolRef::symbolForExpr(const core::GlobalState &gs, const GemMapping &gemMap, + optional loc, scip::Symbol &symbol) const { // Don't set symbol.scheme and package.manager here because // those are hard-coded to 'scip-ruby' and 'gem' anyways. scip::Package package; - package.set_name(metadata.name()); - package.set_version(metadata.version()); + auto owningFile = this->selfOrOwner.loc(gs).file(); + // Synthetic symbols and built-in like constructs do not have a source location. + if (!owningFile.exists()) { + return utils::Result::skipValue(); + } + auto metadata = gemMap.lookupGemForFile(gs, owningFile); + ENFORCE(metadata.has_value(), "missing gem information for file {} which contains symbol {}", + owningFile.data(gs).path(), this->showRaw(gs)); + if (metadata.has_value()) { + package.set_name(metadata.value()->name()); + package.set_version(metadata.value()->version()); + } else { + package.set_name("__scip-ruby-bug__"); + package.set_version("bug"); + } *symbol.mutable_package() = move(package); InlinedVector descriptors; @@ -77,7 +91,8 @@ absl::Status UntypedGenericSymbolRef::symbolForExpr(const core::GlobalState &gs, descriptor.set_suffix(scip::Descriptor::Type); break; default: - return absl::InvalidArgumentError("unexpected expr type for symbol computation"); + return utils::Result::statusValue( + absl::InvalidArgumentError("unexpected expr type for symbol computation")); } descriptors.push_back(move(descriptor)); cur = cur.owner(gs); @@ -93,7 +108,7 @@ absl::Status UntypedGenericSymbolRef::symbolForExpr(const core::GlobalState &gs, ENFORCE(!descriptor.name().empty()); *symbol.add_descriptors() = move(descriptor); } - return absl::OkStatus(); + return utils::Result::okValue(); } string UntypedGenericSymbolRef::showRaw(const core::GlobalState &gs) const { diff --git a/scip_indexer/SCIPSymbolRef.h b/scip_indexer/SCIPSymbolRef.h index 21086c0094..1ac38fe9bf 100644 --- a/scip_indexer/SCIPSymbolRef.h +++ b/scip_indexer/SCIPSymbolRef.h @@ -18,6 +18,7 @@ #include "scip_indexer/SCIPFieldResolve.h" #include "scip_indexer/SCIPGemMetadata.h" +#include "scip_indexer/SCIPUtils.h" namespace scip { // Avoid needlessly including protobuf header here. class Symbol; @@ -66,8 +67,8 @@ class UntypedGenericSymbolRef final { } // Try to compute a scip::Symbol for this value. - absl::Status symbolForExpr(const core::GlobalState &gs, const GemMetadata &metadata, std::optional loc, - scip::Symbol &symbol) const; + utils::Result symbolForExpr(const core::GlobalState &gs, const GemMapping &gemMap, std::optional loc, + scip::Symbol &symbol) const; void saveRelationships(const core::GlobalState &gs, const RelationshipsMap &relationshipMap, diff --git a/scip_indexer/SCIPUtils.cc b/scip_indexer/SCIPUtils.cc index 6704dce4d4..a8ec5a5599 100644 --- a/scip_indexer/SCIPUtils.cc +++ b/scip_indexer/SCIPUtils.cc @@ -6,7 +6,7 @@ #include "proto/SCIP.pb.h" -namespace scip::utils { +namespace sorbet::scip_indexer::utils { using namespace std; @@ -111,4 +111,4 @@ absl::Status emitSymbolString(const scip::Symbol &symbol, string &out) { return absl::OkStatus(); } -}; // end namespace scip::utils \ No newline at end of file +}; // namespace sorbet::scip_indexer::utils \ No newline at end of file diff --git a/scip_indexer/SCIPUtils.h b/scip_indexer/SCIPUtils.h index d8de48fed3..f25f8aec46 100644 --- a/scip_indexer/SCIPUtils.h +++ b/scip_indexer/SCIPUtils.h @@ -4,15 +4,41 @@ #include #include "absl/status/status.h" - #include "proto/SCIP.pb.h" -namespace scip::utils { +namespace sorbet::scip_indexer::utils { + +class Result { + bool skip_; + absl::Status status_; + + Result(bool sk, absl::Status st) : skip_(sk), status_(st) {} + +public: + static Result skipValue() { + return Result(true, absl::OkStatus()); + } + static Result okValue() { + return Result(false, absl::OkStatus()); + } + static Result statusValue(absl::Status status) { + return Result(false, status); + } + bool skip() const { + return this->skip_; + } + bool ok() const { + return !this->skip() && this->status().ok(); + } + absl::Status status() const { + return this->status_; + } +}; absl::Status emitDescriptorString(const scip::Descriptor &descriptor, std::string &out); absl::Status emitSymbolString(const scip::Symbol &symbol, std::string &out); -} // end namespace scip::utils +} // namespace sorbet::scip_indexer::utils #endif // SORBET_SCIP_UTILS \ No newline at end of file diff --git a/test/scip/testdata/multifile/gem-map/current.rb b/test/scip/testdata/multifile/gem-map/current.rb new file mode 100644 index 0000000000..e4ed69d526 --- /dev/null +++ b/test/scip/testdata/multifile/gem-map/current.rb @@ -0,0 +1,11 @@ +# typed: true + +require 'upstream' +require 'downstream' + +# File missing from gem map; check fallback behavior + +def h() + f() + g() +end diff --git a/test/scip/testdata/multifile/gem-map/current.snapshot.rb b/test/scip/testdata/multifile/gem-map/current.snapshot.rb new file mode 100644 index 0000000000..364a9650bc --- /dev/null +++ b/test/scip/testdata/multifile/gem-map/current.snapshot.rb @@ -0,0 +1,16 @@ + # typed: true + + require 'upstream' +#^^^^^^^ reference [..] Kernel#require(). + require 'downstream' +#^^^^^^^ reference [..] Kernel#require(). + + # File missing from gem map; check fallback behavior + + def h() +# ^ definition my_current_gem 2 Object#h(). + f() +# ^ reference my_downstream_gem 1 Object#f(). + g() +# ^ reference my_upstream_gem 1 Object#g(). + end diff --git a/test/scip/testdata/multigem/downstream.rb b/test/scip/testdata/multifile/gem-map/downstream.rb similarity index 100% rename from test/scip/testdata/multigem/downstream.rb rename to test/scip/testdata/multifile/gem-map/downstream.rb diff --git a/test/scip/testdata/multifile/gem-map/downstream.snapshot.rb b/test/scip/testdata/multifile/gem-map/downstream.snapshot.rb new file mode 100644 index 0000000000..303ffee6c0 --- /dev/null +++ b/test/scip/testdata/multifile/gem-map/downstream.snapshot.rb @@ -0,0 +1,10 @@ + # typed: true + + require 'upstream' +#^^^^^^^ reference [..] Kernel#require(). + + def f() +# ^ definition my_downstream_gem 1 Object#f(). + g() +# ^ reference my_upstream_gem 1 Object#g(). + end diff --git a/test/scip/testdata/multifile/gem-map/gem-map.json b/test/scip/testdata/multifile/gem-map/gem-map.json new file mode 100644 index 0000000000..fde090dd2f --- /dev/null +++ b/test/scip/testdata/multifile/gem-map/gem-map.json @@ -0,0 +1,2 @@ +{"path": "test/scip/testdata/multifile/gem-map/downstream.rb", "gem": "my_downstream_gem@1"} +{"gem": "my_upstream_gem@1", "path": "test/scip/testdata/multifile/gem-map/upstream.rb"} diff --git a/test/scip/testdata/multifile/gem-map/scip-ruby-args.rb b/test/scip/testdata/multifile/gem-map/scip-ruby-args.rb new file mode 100644 index 0000000000..cddf8428c0 --- /dev/null +++ b/test/scip/testdata/multifile/gem-map/scip-ruby-args.rb @@ -0,0 +1,2 @@ +# gem-map: gem-map.json +# gem-metadata: my_current_gem@2 diff --git a/test/scip/testdata/multigem/upstream.rb b/test/scip/testdata/multifile/gem-map/upstream.rb similarity index 100% rename from test/scip/testdata/multigem/upstream.rb rename to test/scip/testdata/multifile/gem-map/upstream.rb diff --git a/test/scip/testdata/multifile/gem-map/upstream.snapshot.rb b/test/scip/testdata/multifile/gem-map/upstream.snapshot.rb new file mode 100644 index 0000000000..bbd3ebc729 --- /dev/null +++ b/test/scip/testdata/multifile/gem-map/upstream.snapshot.rb @@ -0,0 +1,7 @@ + # typed: true + + def g() +# ^ definition my_upstream_gem 1 Object#g(). + puts 'Hello' +# ^^^^ reference [..] Kernel#puts(). + end diff --git a/test/scip/testdata/multigem/gem-map/gem-map.json b/test/scip/testdata/multigem/gem-map/gem-map.json deleted file mode 100644 index 72732ebf6e..0000000000 --- a/test/scip/testdata/multigem/gem-map/gem-map.json +++ /dev/null @@ -1,2 +0,0 @@ -{"path": "downstream.rb", "gem": "my_downstream_gem@1"} -{"gem": "my_upstream_gem@1", "path": "upstream.rb"} diff --git a/test/scip_test_runner.cc b/test/scip_test_runner.cc index 06d753180c..1fd7af340d 100644 --- a/test/scip_test_runner.cc +++ b/test/scip_test_runner.cc @@ -119,6 +119,64 @@ PATH } } +TEST_CASE("GemMapParsing") { + if (!onlyRunUnitTests) { + return; + } + vector> errors; + + auto logger = spdlog::stderr_color_mt("file-to-gem-map-test"); + auto errorCollector = make_shared(); + auto errorQueue = make_shared(*logger, *logger, errorCollector); + core::GlobalState gs(errorQueue); + gs.initEmpty(); // needed for proper file table access + + core::FileRef nested, flippedOrder, missing, absolute1, absolute2, absolute3, unnormalized1, unnormalized2; + { + core::UnfreezeFileTable fileTableAccess(gs); + nested = gs.enterFile("nes/ted", ""); + flippedOrder = gs.enterFile("flippedOrder", ""); + missing = gs.enterFile("missing", ""); + absolute1 = gs.enterFile("/gem-map/absolute1", ""); + absolute2 = gs.enterFile("/gem-map/absolute2", ""); + absolute3 = gs.enterFile("absolute3", ""); + unnormalized1 = gs.enterFile("./unnormalized1", ""); + unnormalized2 = gs.enterFile("unnormalized2", ""); + } + + std::string testJSON = R"( +{"path": "nes/ted", "gem": "start@0"} +{"gem": "blah@1.1", "path": "flippedOrder"} +{"path": "/gem-map/absolute1", "gem": "abs@0"} +{"path": "absolute2", "gem": "abs@0"} +{"path": "/gem-map/absolute3", "gem": "abs@0"} +{"path": "unnormalized1", "gem": "un@0"} +{"path": "./unnormalized2", "gem": "un@0"} +)"; + + scip_indexer::GemMapping gemMap{}; + + MockFileSystem fs("/gem-map"); + fs.writeFile("gem-map.json", testJSON); + gemMap.populateFromNDJSON(gs, fs, "gem-map.json"); + + auto nestedGem = gemMap.lookupGemForFile(gs, nested); + ENFORCE(nestedGem.has_value()); + ENFORCE(*nestedGem->get() == scip_indexer::GemMetadata::forTest("start", "0")); + + auto flippedOrderGem = gemMap.lookupGemForFile(gs, flippedOrder); + ENFORCE(flippedOrderGem.has_value()); + ENFORCE(*flippedOrderGem->get() == scip_indexer::GemMetadata::forTest("blah", "1.1")); + + ENFORCE(!gemMap.lookupGemForFile(gs, missing).has_value()); + + ENFORCE(gemMap.lookupGemForFile(gs, absolute1).has_value()); + ENFORCE(gemMap.lookupGemForFile(gs, absolute2).has_value()); + ENFORCE(gemMap.lookupGemForFile(gs, absolute3).has_value()); + ENFORCE(gemMap.lookupGemForFile(gs, unnormalized1).has_value()); + ENFORCE(gemMap.lookupGemForFile(gs, unnormalized2).has_value()); +} + // Copied from pipeline_test_runner.cc class CFGCollectorAndTyper { // TODO(varun): Copy this over to scip_test_runner.cc public: @@ -238,6 +296,7 @@ void formatSnapshot(const scip::Document &document, FormatOptions options, std:: return absl::StrReplaceAll(symbol, { {"scip-ruby gem ", ""}, // indexer prefix {"placeholder_name placeholder_version", "[..]"}, // test placeholder + {"ruby latest", "[..]"} // core and stdlib }); }; size_t occ_i = 0; @@ -339,7 +398,7 @@ string snapshot_path(string source_file_path) { } struct TestSettings { - optional gemMetadata; + scip_indexer::Config config; UnorderedMap formatOptions; }; @@ -380,17 +439,20 @@ void compareSnapshots(const scip::Index &index, const TestSettings &settings, } } -pair, FormatOptions> readMagicComments(string_view path) { - optional gemMetadata = nullopt; +pair readMagicComments(string_view path) { + scip_indexer::Config config; FormatOptions options{.showDocs = false}; ifstream input(path); for (string line; getline(input, line);) { if (absl::StrContains(line, "# gem-metadata: ")) { auto s = absl::StripPrefix(line, "# gem-metadata: "); ENFORCE(!s.empty()); - gemMetadata = s; - } - if (absl::StrContains(line, "# options: ")) { + config.gemMetadata = s; + } else if (absl::StrContains(line, "# gem-map: ")) { + auto s = absl::StripPrefix(line, "# gem-map: "); + ENFORCE(!s.empty()); + config.gemMapPath = std::string(std::filesystem::path(path).parent_path().append(s)); + } else if (absl::StrContains(line, "# options: ")) { auto s = absl::StripPrefix(line, "# options: "); ENFORCE(!s.empty()); if (absl::StrContains(s, "showDocs")) { @@ -398,7 +460,7 @@ pair, FormatOptions> readMagicComments(string_view path) { } } } - return {gemMetadata, options}; + return {config, options}; } void test_one_gem(Expectations &test, const TestSettings &settings) { @@ -435,9 +497,14 @@ void test_one_gem(Expectations &test, const TestSettings &settings) { cxxopts::Options options{"scip-ruby-snapshot-test"}; scipProvider->injectOptions(options); std::vector argv = {"scip-ruby-snapshot-test", "--index-file", indexFilePath.c_str()}; - auto metadata = settings.gemMetadata.value_or("placeholder_name@placeholder_version"); + auto &cfg = settings.config; + auto gemMetadata = cfg.gemMetadata.empty() ? "placeholder_name@placeholder_version" : cfg.gemMetadata; argv.push_back("--gem-metadata"); - argv.push_back(metadata.c_str()); + argv.push_back(gemMetadata.c_str()); + if (!cfg.gemMapPath.empty()) { + argv.push_back("--gem-map-path"); + argv.push_back(cfg.gemMapPath.c_str()); + } argv.push_back(nullptr); auto parseResult = options.parse(argv.size() - 1, argv.data()); @@ -532,11 +599,10 @@ TEST_CASE("SCIPTest") { Expectations test = Expectations::getExpectations(inputFileOrDir); TestSettings settings; - settings.gemMetadata = nullopt; if (test.isFolderTest) { auto argsFilePath = test.folder + "scip-ruby-args.rb"; if (FileOps::exists(argsFilePath)) { - settings.gemMetadata = readMagicComments(argsFilePath).first; + settings.config = readMagicComments(argsFilePath).first; } ENFORCE(test.sourceFiles.size() > 0); for (auto &sourceFile : test.sourceFiles) { @@ -548,7 +614,7 @@ TEST_CASE("SCIPTest") { ENFORCE(test.sourceFiles.size() == 1); auto path = test.folder + test.sourceFiles[0]; auto &options = settings.formatOptions[path]; - std::tie(settings.gemMetadata, options) = readMagicComments(path); + std::tie(settings.config, options) = readMagicComments(path); } test_one_gem(test, settings); From c6e28330cdb7a0add8924f8c4ebeac715d5d4255 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Fri, 4 Nov 2022 17:32:18 +0800 Subject: [PATCH 2/3] Update docs/scip-ruby/CLI.md --- docs/scip-ruby/CLI.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/scip-ruby/CLI.md b/docs/scip-ruby/CLI.md index c49fcf99ab..4d3132de51 100644 --- a/docs/scip-ruby/CLI.md +++ b/docs/scip-ruby/CLI.md @@ -23,7 +23,7 @@ instead (`git rev-parse HEAD`). ## `--gem-map-path ` At the moment, scip-ruby requires an extra step for cross-repo -code navigation; you need to supply information about which. +code navigation; you need to supply information about which file belongs to which gem explicitly, using a newline-delimited JSON file in the following format: From a2469a3396a7d86e3538931adc5dd8723b4c16cd Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Fri, 4 Nov 2022 20:17:36 +0800 Subject: [PATCH 3/3] fix: Fix bugs in handling for globals, stdlib URLs etc. --- core/GlobalState.cc | 6 +-- scip_indexer/SCIPGemMetadata.cc | 16 ++++---- scip_indexer/SCIPGemMetadata.h | 4 +- scip_indexer/SCIPIndexer.cc | 1 - scip_indexer/SCIPSymbolRef.cc | 40 ++++++++++++------- .../testdata/fields_and_attrs.snapshot.rb | 4 -- test/scip/testdata/globals.snapshot.rb | 24 +++++------ test/scip/testdata/hoverdocs.snapshot.rb | 4 -- .../testdata/implicit_super_arg.snapshot.rb | 1 - test/scip/testdata/prop.snapshot.rb | 6 +-- test/scip/testdata/struct.snapshot.rb | 4 +- test/scip/testdata/test_case.snapshot.rb | 5 --- test/scip_test_runner.cc | 9 ++--- 13 files changed, 60 insertions(+), 64 deletions(-) diff --git a/core/GlobalState.cc b/core/GlobalState.cc index 6083ce97df..ae20d5af3c 100644 --- a/core/GlobalState.cc +++ b/core/GlobalState.cc @@ -1671,8 +1671,8 @@ FileRef GlobalState::enterFile(const shared_ptr &file) { } FileRef GlobalState::enterFile(string_view path, string_view source) { - std::string pathBuf; - if (this->isSCIPRuby) { // See [NOTE: scip-ruby-path-normalization] + string pathBuf; + if (this->isSCIPRuby && !absl::StartsWith(path, "https://")) { // See [NOTE: scip-ruby-path-normalization] pathBuf = string(std::filesystem::path(path).lexically_normal()); } else { pathBuf = string(path); @@ -1695,7 +1695,7 @@ FileRef GlobalState::enterNewFileAt(const shared_ptr &file, FileRef id) { FileRef GlobalState::reserveFileRef(string path) { std::string pathBuf; - if (this->isSCIPRuby) { // See [NOTE: scip-ruby-path-normalization] + if (this->isSCIPRuby && !absl::StartsWith(path, "https://")) { // See [NOTE: scip-ruby-path-normalization] pathBuf = string(std::filesystem::path(path).lexically_normal()); } else { pathBuf = move(path); diff --git a/scip_indexer/SCIPGemMetadata.cc b/scip_indexer/SCIPGemMetadata.cc index d19240fc9e..8a49834244 100644 --- a/scip_indexer/SCIPGemMetadata.cc +++ b/scip_indexer/SCIPGemMetadata.cc @@ -178,14 +178,14 @@ pair> GemMetadata::readFromConfig(const Fi return {GemMetadata(name.value_or(currentDirName()), version.value_or("latest")), errors}; } -GemMapping::GemMapping() { - // The 'ruby' namespace is reserved by RubyGems.org, so we won't run into any - // actual gem called 'ruby', so we use that here. 'stdlib' would not be appropriate - // as Sorbet contains RBIs for both core and stdlib modules. - // For the version, we're not really doing any versioning for core & stdlib RBIs, - // (e.g. handling both 2.7 and 3) so let's stick to latest for now. - this->stdlibGem = make_shared(GemMetadata::tryParse("ruby@latest").value()); -} +// The 'ruby' namespace is reserved by RubyGems.org, so we won't run into any +// actual gem called 'ruby', so we use that here. 'stdlib' would not be appropriate +// as Sorbet contains RBIs for both core and stdlib modules. +// For the version, we're not really doing any versioning for core & stdlib RBIs, +// (e.g. handling both 2.7 and 3) so let's stick to latest for now. +GemMapping::GemMapping() + : currentGem(), map(), stdlibGem(make_shared(GemMetadata::tryParse("ruby@latest").value())), + globalPlaceholderGem(make_shared(GemMetadata::tryParse("_global_@latest").value())) {} optional> GemMapping::lookupGemForFile(const core::GlobalState &gs, core::FileRef file) const { ENFORCE(file.exists()); diff --git a/scip_indexer/SCIPGemMetadata.h b/scip_indexer/SCIPGemMetadata.h index bcf01c11b0..b9ef8bd06b 100644 --- a/scip_indexer/SCIPGemMetadata.h +++ b/scip_indexer/SCIPGemMetadata.h @@ -78,11 +78,13 @@ class GemMetadata final { // Type carrying gem information for each file, which is used during // symbol emission to ensure correct symbol names for cross-repo. class GemMapping final { - std::shared_ptr stdlibGem; std::optional> currentGem; UnorderedMap> map; + std::shared_ptr stdlibGem; public: + std::shared_ptr globalPlaceholderGem; + GemMapping(); std::optional> lookupGemForFile(const core::GlobalState &gs, core::FileRef file) const; diff --git a/scip_indexer/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc index 8298ef548a..ddb3cf37bd 100644 --- a/scip_indexer/SCIPIndexer.cc +++ b/scip_indexer/SCIPIndexer.cc @@ -399,7 +399,6 @@ class SCIPState { optional loc = nullopt) { // In practice, there doesn't seem to be any situation which triggers // a duplicate definition being emitted, so skip calling cacheOccurrence here. - auto occLoc = loc.has_value() ? core::Loc(file, loc.value()) : symRef.symbolLoc(gs); scip::Symbol symbol; auto untypedSymRef = symRef.withoutType(); diff --git a/scip_indexer/SCIPSymbolRef.cc b/scip_indexer/SCIPSymbolRef.cc index 17efc65bf0..853f8aed54 100644 --- a/scip_indexer/SCIPSymbolRef.cc +++ b/scip_indexer/SCIPSymbolRef.cc @@ -33,27 +33,37 @@ string showRawRelationshipsMap(const core::GlobalState &gs, const RelationshipsM }); } +bool isGlobal(const core::GlobalState &gs, core::NameRef name) { + if (!name.exists()) + return false; + auto shortName = name.shortName(gs); + return !shortName.empty() && shortName.front() == '$'; +} + // Try to compute a scip::Symbol for this value. utils::Result UntypedGenericSymbolRef::symbolForExpr(const core::GlobalState &gs, const GemMapping &gemMap, optional loc, scip::Symbol &symbol) const { + optional> metadata; + if (isGlobal(gs, this->name)) { + metadata = gemMap.globalPlaceholderGem; + } else { + auto owningFile = loc.has_value() ? loc->file() : this->selfOrOwner.loc(gs).file(); + if (!owningFile.exists()) { + // Synthetic symbols and built-in like constructs do not have a source location. + return utils::Result::skipValue(); + } + metadata = gemMap.lookupGemForFile(gs, owningFile); + ENFORCE(metadata.has_value(), "missing gem information for file {} which contains symbol {}", + owningFile.data(gs).path(), this->showRaw(gs)); + if (!metadata.has_value()) { + return utils::Result::skipValue(); + } + } // Don't set symbol.scheme and package.manager here because // those are hard-coded to 'scip-ruby' and 'gem' anyways. scip::Package package; - auto owningFile = this->selfOrOwner.loc(gs).file(); - // Synthetic symbols and built-in like constructs do not have a source location. - if (!owningFile.exists()) { - return utils::Result::skipValue(); - } - auto metadata = gemMap.lookupGemForFile(gs, owningFile); - ENFORCE(metadata.has_value(), "missing gem information for file {} which contains symbol {}", - owningFile.data(gs).path(), this->showRaw(gs)); - if (metadata.has_value()) { - package.set_name(metadata.value()->name()); - package.set_version(metadata.value()->version()); - } else { - package.set_name("__scip-ruby-bug__"); - package.set_version("bug"); - } + package.set_name(metadata.value()->name()); + package.set_version(metadata.value()->version()); *symbol.mutable_package() = move(package); InlinedVector descriptors; diff --git a/test/scip/testdata/fields_and_attrs.snapshot.rb b/test/scip/testdata/fields_and_attrs.snapshot.rb index ac62842526..96b4667729 100644 --- a/test/scip/testdata/fields_and_attrs.snapshot.rb +++ b/test/scip/testdata/fields_and_attrs.snapshot.rb @@ -54,8 +54,6 @@ def m2 # ^^ definition [..] L#m2(). # FIXME: Missing references self.class.y = self.class.x -# ^^^^^ reference [..] Object#class(). -# ^^^^^ reference [..] Object#class(). return end end @@ -87,8 +85,6 @@ def m3 # ^^ definition [..] N#m3(). # FIXME: Missing references self.class.b = self.class.a -# ^^^^^ reference [..] Object#class(). -# ^^^^^ reference [..] Object#class(). end end diff --git a/test/scip/testdata/globals.snapshot.rb b/test/scip/testdata/globals.snapshot.rb index 38560c5e7a..2311c5f980 100644 --- a/test/scip/testdata/globals.snapshot.rb +++ b/test/scip/testdata/globals.snapshot.rb @@ -1,18 +1,18 @@ # typed: true $aa = 0 -#^^^ definition [..] `>`#$aa. +#^^^ definition [global] `>`#$aa. def f # ^ definition [..] Object#f(). $aa = 10 -# ^^^ definition [..] `>`#$aa. +# ^^^ definition [global] `>`#$aa. $bb = $aa -# ^^^ definition [..] `>`#$bb. -# ^^^ reference [..] `>`#$aa. +# ^^^ definition [global] `>`#$bb. +# ^^^ reference [global] `>`#$aa. $aa = $bb -# ^^^ reference (write) [..] `>`#$aa. -# ^^^ reference [..] `>`#$bb. +# ^^^ reference (write) [global] `>`#$aa. +# ^^^ reference [global] `>`#$bb. return end @@ -21,23 +21,23 @@ class C def g # ^ definition [..] C#g(). $c = $bb -# ^^ definition [..] `>`#$c. -# ^^^^^^^^ reference [..] `>`#$c. -# ^^^ reference [..] `>`#$bb. +# ^^ definition [global] `>`#$c. +# ^^^^^^^^ reference [global] `>`#$c. +# ^^^ reference [global] `>`#$bb. end end puts $c #^^^^ reference [..] Kernel#puts(). -# ^^ reference [..] `>`#$c. +# ^^ reference [global] `>`#$c. $d = T.let(0, Integer) -#^^ definition [..] `>`#$d. +#^^ definition [global] `>`#$d. # ^^^^^^^ definition local 3~#119448696 # ^^^^^^^ reference [..] Integer# def g # ^ definition [..] Object#g(). $d -# ^^ reference [..] `>`#$d. +# ^^ reference [global] `>`#$d. end diff --git a/test/scip/testdata/hoverdocs.snapshot.rb b/test/scip/testdata/hoverdocs.snapshot.rb index f0b01bcef8..d4a37815fc 100644 --- a/test/scip/testdata/hoverdocs.snapshot.rb +++ b/test/scip/testdata/hoverdocs.snapshot.rb @@ -35,7 +35,6 @@ def m2 end sig { params(C, T::Boolean).returns(T::Boolean) } -# ^ reference [..] `T.untyped`# # ^^^^^^^ reference [..] T#Boolean. # ^^^^^^^ reference [..] T#Boolean. def m3(c, b) @@ -101,7 +100,6 @@ def m5 # And... # ...one more doc comment sig { params(C, T::Boolean).returns(T::Boolean) } -# ^ reference [..] `T.untyped`# # ^^^^^^^ reference [..] T#Boolean. # ^^^^^^^ reference [..] T#Boolean. def m6(c, b) @@ -226,7 +224,6 @@ def f1 #^^^ reference [..] Sorbet#Private#``#sig(). # ^^^^^^^ reference [..] T#Private#Methods#DeclBuilder#returns(). # ^ reference [..] T# -# ^^^^^^^ reference [..] `T.untyped`# # ^^^^^^^^^^ reference [..] Sorbet#Private#Static# def f2 # ^^ definition [..] Object#f2(). @@ -258,7 +255,6 @@ def f3 # undocumented global function #^^^ reference [..] Sorbet#Private#``#sig(). # ^^^^^^^ reference [..] T#Private#Methods#DeclBuilder#returns(). # ^ reference [..] T# -# ^^^^^^^ reference [..] `T.untyped`# # ^^^^^^^^^^ reference [..] Sorbet#Private#Static# def f4 # another undocumented global function # ^^ definition [..] Object#f4(). diff --git a/test/scip/testdata/implicit_super_arg.snapshot.rb b/test/scip/testdata/implicit_super_arg.snapshot.rb index 003672d3b8..78128e167e 100644 --- a/test/scip/testdata/implicit_super_arg.snapshot.rb +++ b/test/scip/testdata/implicit_super_arg.snapshot.rb @@ -25,6 +25,5 @@ def f(a, b) # ^ definition local 1~#3809224601 # ^ definition local 2~#3809224601 super -# ^^^^^ reference [..] `>`#``(). end end diff --git a/test/scip/testdata/prop.snapshot.rb b/test/scip/testdata/prop.snapshot.rb index 230a9782e7..4b4d711448 100644 --- a/test/scip/testdata/prop.snapshot.rb +++ b/test/scip/testdata/prop.snapshot.rb @@ -132,13 +132,13 @@ def self.created_prop(opts={}); end token_prop # ^^^^^ definition [..] PropHelpers#`token=`(). # ^^^^^ definition [..] PropHelpers#token(). -# ^^^^^^^^^^ reference [..] String# # ^^^^^^^^^^ reference [..] ``#token_prop(). +# ^^^^^^^^^^ reference [..] String# created_prop # ^^^^^^^ definition [..] PropHelpers#`created=`(). # ^^^^^^^ definition [..] PropHelpers#created(). -# ^^^^^^^^^^^^ reference [..] Float# # ^^^^^^^^^^^^ reference [..] ``#created_prop(). +# ^^^^^^^^^^^^ reference [..] Float# end class PropHelpers2 @@ -150,8 +150,8 @@ def self.timestamped_token_prop(opts={}); end def self.created_prop(opts={}); end # ^^^^^^^^^^^^ definition [..] ``#created_prop(). timestamped_token_prop -# ^^^^^^^^^^^^^^^^^^^^^^ reference [..] String# # ^^^^^^^^^^^^^^^^^^^^^^ reference [..] ``#timestamped_token_prop(). +# ^^^^^^^^^^^^^^^^^^^^^^ reference [..] String# # ^^^^^ definition [..] PropHelpers2#`token=`(). # ^^^^^ definition [..] PropHelpers2#token(). created_prop(immutable: true) diff --git a/test/scip/testdata/struct.snapshot.rb b/test/scip/testdata/struct.snapshot.rb index a27a45559a..2eeac29b92 100644 --- a/test/scip/testdata/struct.snapshot.rb +++ b/test/scip/testdata/struct.snapshot.rb @@ -51,12 +51,12 @@ def f #^^^^^^^^^^^^^^^^^^^^ definition [..] Struct# #^^^^^^^^^^^^^^^^^^^^ definition local 5~#119448696 #^^^^^^^^^^^^^^^^^^^^ definition [..] POINT#initialize(). -# ^ reference [..] BasicObject# # ^ definition [..] POINT#`x=`(). # ^ definition [..] POINT#x(). -# ^ reference [..] BasicObject# +# ^ reference [..] BasicObject# # ^ definition [..] POINT#`y=`(). # ^ definition [..] POINT#y(). +# ^ reference [..] BasicObject# def array # ^^^^^ definition [..] POINT#array(). [x, y] diff --git a/test/scip/testdata/test_case.snapshot.rb b/test/scip/testdata/test_case.snapshot.rb index 96fd11e0de..8548e2850c 100644 --- a/test/scip/testdata/test_case.snapshot.rb +++ b/test/scip/testdata/test_case.snapshot.rb @@ -22,7 +22,6 @@ def assert(test) # Helper method to direct calls to `test` instead of Kernel#test sig { params(args: T.untyped, block: T.nilable(T.proc.void)).void } -# ^^^^ reference [..] `>`#void(). def self.test(*args, &block) # ^^^^ definition [..] ``#test(). end @@ -50,12 +49,10 @@ class NoMatchTest < ActiveSupport::TestCase # ^^^^^^ reference [..] Kernel#extend(). sig { params(block: T.proc.void).void } -# ^^^^ reference [..] `>`#void(). def self.setup(&block); end # ^^^^^ definition [..] ``#setup(). sig { params(block: T.proc.void).void } -# ^^^^ reference [..] `>`#void(). def self.teardown(&block); end # ^^^^^^^^ definition [..] ``#teardown(). end @@ -66,12 +63,10 @@ class NoParentClass # ^^^^^^ reference [..] Kernel#extend(). sig { params(block: T.proc.void).void } -# ^^^^ reference [..] `>`#void(). def self.setup(&block); end # ^^^^^ definition [..] ``#setup(). sig { params(block: T.proc.void).void } -# ^^^^ reference [..] `>`#void(). def self.teardown(&block); end # ^^^^^^^^ definition [..] ``#teardown(). diff --git a/test/scip_test_runner.cc b/test/scip_test_runner.cc index 1fd7af340d..0a674f86e1 100644 --- a/test/scip_test_runner.cc +++ b/test/scip_test_runner.cc @@ -293,11 +293,10 @@ void formatSnapshot(const scip::Document &document, FormatOptions options, std:: }); auto formatSymbol = [](const std::string &symbol) -> string { // Strip out repeating information for cleaner snapshots. - return absl::StrReplaceAll(symbol, { - {"scip-ruby gem ", ""}, // indexer prefix - {"placeholder_name placeholder_version", "[..]"}, // test placeholder - {"ruby latest", "[..]"} // core and stdlib - }); + return absl::StrReplaceAll(symbol, {{"scip-ruby gem ", ""}, // indexer prefix + {"placeholder_name placeholder_version", "[..]"}, // test placeholder + {"ruby latest", "[..]"}, // core and stdlib + {"_global_ latest", "[global]"}}); }; size_t occ_i = 0; std::ifstream input(document.relative_path());