diff --git a/scip_indexer/SCIPGemMetadata.cc b/scip_indexer/SCIPGemMetadata.cc index 2e6d2a010..756140a93 100644 --- a/scip_indexer/SCIPGemMetadata.cc +++ b/scip_indexer/SCIPGemMetadata.cc @@ -39,28 +39,38 @@ GemMetadataError failedToParseVersionFromGemspecWarning = GemMetadataError failedToParseGemfileLockWarning{GMEKind::Warning, "Failed to extract name and version from Gemfile.lock"}; -pair> GemMetadata::readFromGemfileLock(const string &contents) { - istringstream lines(contents); - bool sawPATH = false; - bool sawSpecs = false; +void GemDependencies::modifyCurrentGem(optional name, optional version) { + if (this->currentGem.name().empty() && name.has_value()) { + this->currentGem._name = name.value(); + } + if (this->currentGem.version().empty() && version.has_value()) { + this->currentGem._version = version.value(); + } +} + +vector GemDependencies::populateFromGemfileLock(const string &contents) { optional name; optional version; vector errors; - // PATH - // remote: . - // specs: - // my_gem_name (M.N.P) - for (string line; getline(lines, line);) { - if (absl::StartsWith(line, "PATH")) { - sawPATH = true; - continue; - } - if (sawPATH && absl::StrContains(line, "specs:")) { - sawSpecs = true; - continue; - } - if (sawSpecs) { - std::regex specLineRegex(R"END(\s+([A-Za-z0-9_\-]+)\s*\((.+)\)\s*)END"); + std::regex specLineRegex(R"END( ([A-Za-z0-9_\-]+)\s*\(([.0-9a-zA-Z_\-]+)\)\s*))END"); + // ^ Fixed 4 spaces to avoid picking indirect dep constraints in dep list. + { + istringstream lines(contents); + bool sawPATH = false; + bool sawSpecs = false; + // PATH + // remote: . + // specs: + // my_gem_name (M.N.P) + for (string line; getline(lines, line);) { + if (!sawPATH) { + sawPATH = absl::StartsWith(line, "PATH"); + continue; + } + if (!sawSpecs) { + sawSpecs = absl::StrContains(line, "specs:"); + continue; + } std::smatch matches; if (std::regex_match(line, matches, specLineRegex)) { name = matches[1].str(); @@ -68,14 +78,44 @@ pair> GemMetadata::readFromGemfileLock(con } break; } + if (!name.has_value()) { + errors.push_back(failedToParseGemfileLockWarning); + } } - if (!name.has_value()) { - errors.push_back(failedToParseGemfileLockWarning); + { + istringstream lines(contents); + bool sawGEM = false; + bool sawSpecs = false; + // GEM + // remote: https://rubygems.org/ + // specs: + // dep1 (M.N.P) + // indirect-dep1 (~> X.Y.Z) + // indirect-dep2 (>= X.Y.Z) + // dep2 (M.N.P) + for (string line; getline(lines, line);) { + if (!sawGEM) { + sawGEM = absl::StartsWith(line, "GEM"); + continue; + } + if (!absl::StartsWith(line, " ")) { // section end + break; + } + if (!sawSpecs) { + sawSpecs = absl::StrContains(line, "specs:"); + continue; + } + std::smatch matches; + if (std::regex_match(line, matches, specLineRegex)) { + this->addDependency(matches[1].str(), matches[2].str()); + } + } } - return {GemMetadata{name.value_or(""), version.value_or("")}, errors}; + this->modifyCurrentGem(name, version); + return errors; } -pair> GemMetadata::readFromGemspec(const string &contents) { +vector GemDependencies::populateFromGemspec(const string &contents) { optional name; optional version; vector errors; @@ -117,10 +157,11 @@ pair> GemMetadata::readFromGemspec(const s if (!name.has_value() || !version.has_value()) { errors.push_back(failedToParseGemspecWarning); } - return {GemMetadata{name.value_or(""), version.value_or("")}, errors}; + this->modifyCurrentGem(name, version); + return errors; } -pair> GemMetadata::readFromConfig(const FileSystem &fs) { +vector GemDependencies::populateFromConfig(const FileSystem &fs) { UnorderedSet extensions{".lock", ".gemspec"}; auto paths = fs.listFilesInDir(".", extensions, /*recursive*/ false, {}, {}); vector errors; @@ -135,24 +176,18 @@ pair> GemMetadata::readFromConfig(const Fi }; if (paths.empty()) { errors.push_back(configNotFoundError); - return {GemMetadata(currentDirName(), "latest"), errors}; + this->currentGem = GemMetadata(currentDirName(), "latest"); + return errors; } - optional name{}; - optional version{}; - auto copyState = [&](auto &m, auto &errs) { - name = m.name().empty() ? name : m.name(); - version = m.version().empty() ? version : m.version(); - absl::c_copy(errs, std::back_inserter(errors)); - }; for (auto &path : paths) { if (!absl::EndsWith(path, "Gemfile.lock")) { continue; } - auto [gemMetadata, parseErrors] = GemMetadata::readFromGemfileLock(fs.readFile(path)); - if (!gemMetadata.name().empty() && !gemMetadata.version().empty()) { - return {gemMetadata, {}}; + auto parseErrors = this->populateFromGemfileLock(fs.readFile(path)); + if (!this->currentGem.name().empty() && !this->currentGem.version().empty()) { + return {}; } - copyState(gemMetadata, parseErrors); + absl::c_copy(parseErrors, std::back_inserter(errors)); break; } string gemspecPath{}; @@ -161,21 +196,23 @@ pair> GemMetadata::readFromConfig(const Fi continue; } gemspecPath = filename; - auto [gemMetadata, parseErrors] = GemMetadata::readFromGemspec(fs.readFile(filename)); - if (!gemMetadata.name().empty() && !gemMetadata.version().empty()) { - return {gemMetadata, {}}; + auto parseErrors = this->populateFromGemspec(fs.readFile(filename)); + if (!this->currentGem.name().empty() && !this->currentGem.version().empty()) { + return {}; } - copyState(gemMetadata, parseErrors); + absl::c_copy(parseErrors, std::back_inserter(errors)); break; } - if (name.has_value() && version.has_value()) { + if (!this->currentGem.name().empty() && !this->currentGem.version().empty()) { errors.clear(); } - if (!name.has_value() && !gemspecPath.empty()) { + optional name{}; + if (this->currentGem.name().empty() && !gemspecPath.empty()) { vector components = absl::StrSplit(gemspecPath, '/'); name = string(absl::StripSuffix(components.back(), ".gemspec")); } - return {GemMetadata(name.value_or(currentDirName()), version.value_or("latest")), errors}; + this->modifyCurrentGem(name.value_or(currentDirName()), "latest"); + return errors; } // The 'ruby' namespace is reserved by RubyGems.org, so we won't run into any @@ -218,28 +255,6 @@ optional> GemMapping::lookupGemForFile(const core::Globa 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; - } - // See https://sorbet.org/docs/rbi#quickref for description of the standard layout. - // Based on some Sourcegraph searches, it looks like RBI files can be named either - // gem_name.rbi or gem_name@version.rbi. - if (absl::StrContains(filepath, "sorbet/rbi/")) { - if (absl::StrContains(filepath, "sorbet/rbi/gems/") || absl::StrContains(filepath, "sorbet/rbi/annotations/") || - absl::StrContains(filepath, "sorbet/rbi/dsl/")) { - auto metadata = tryParseFilepath(filepath); - if (metadata.has_value()) { - return metadata; - } - } - // hidden-definitions and todo.rbi get treated as part of the current gem - } - 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; } @@ -303,8 +318,39 @@ void GemMapping::populateFromNDJSON(const core::GlobalState &gs, const FileSyste } } -void GemMapping::markCurrentGem(GemMetadata gem) { - this->currentGem = make_shared(gem); +void GemMapping::populateCache(core::FileRef fileRef, shared_ptr file) { + auto it = this->map.find(fileRef); + if (it != this->map.end()) { + return; + } + auto metadata = this->identifyGem(file->path()); + if (metadata.has_value()) { + this->map.insert({fileRef, metadata.value()}); + } +} + +optional> GemMapping::identifyGem(string_view filepath) const { + if (absl::StartsWith(filepath, core::File::URL_PREFIX)) { + return this->stdlibGem; + } + // See https://sorbet.org/docs/rbi#quickref for description of the standard layout. + // Based on some Sourcegraph searches, it looks like RBI files can be named either + // gem_name.rbi or gem_name@version.rbi. + if (absl::StrContains(filepath, "sorbet/rbi/")) { + if (absl::StrContains(filepath, "sorbet/rbi/gems/") || absl::StrContains(filepath, "sorbet/rbi/annotations/") || + absl::StrContains(filepath, "sorbet/rbi/dsl/")) { + auto metadata = tryParseFilepath(filepath); + if (metadata.has_value()) { + return metadata; + } + } + // hidden-definitions and todo.rbi get treated as part of the current gem + } + if (this->currentGem.has_value()) { + // Fallback to this if set, to avoid collisions with other gems. + return this->currentGem.value(); + } + return nullopt; } } // namespace sorbet::scip_indexer diff --git a/scip_indexer/SCIPGemMetadata.h b/scip_indexer/SCIPGemMetadata.h index 86ba39066..d8373bce1 100644 --- a/scip_indexer/SCIPGemMetadata.h +++ b/scip_indexer/SCIPGemMetadata.h @@ -33,14 +33,23 @@ extern GemMetadataError configNotFoundError, multipleGemspecWarning, failedToPar failedToParseGemspecWarning, failedToParseNameFromGemspecWarning, failedToParseVersionFromGemspecWarning, failedToParseGemfileLockWarning; +class GemDependencies; +class GemMapping; + class GemMetadata final { std::string _name; std::string _version; GemMetadata(std::string name, std::string version) : _name(name), _version(version) {} + friend GemDependencies; + friend GemMapping; + public: GemMetadata() = default; + GemMetadata(GemMetadata &&) = default; + GemMetadata &operator=(GemMetadata &&) = default; + GemMetadata(const GemMetadata &) = default; GemMetadata &operator=(const GemMetadata &) = default; // Don't call this method outside test code! @@ -67,18 +76,35 @@ class GemMetadata final { bool operator==(const GemMetadata &other) const { return this->name() == other.name() && this->version() == other.version(); } +}; + +class GemDependencies { + // Mapping of gem name -> version for dependencies + UnorderedMap versionMap; - // HACK: Do a best-effort parse of any config files to extract the name and version. - static std::pair> readFromConfig(const FileSystem &fs); +public: + GemMetadata currentGem; + + GemDependencies() = default; + + // Parse Gemfile.lock and .gemspec on a best-effort basis to extract dependency names and versions. + std::vector populateFromConfig(const FileSystem &); private: - static std::pair> readFromGemfileLock(const std::string &); - static std::pair> readFromGemspec(const std::string &); + std::vector populateFromGemfileLock(const std::string &fileContents); + std::vector populateFromGemspec(const std::string &fileContents); + + void addDependency(std::string &&name, std::string &&version) { + this->versionMap.insert({std::string(name), std::string(version)}); + } + + void modifyCurrentGem(std::optional name, std::optional version); }; // Type carrying gem information for each file, which is used during // symbol emission to ensure correct symbol names for cross-repo. class GemMapping final { + GemDependencies gemDeps; std::optional> currentGem; UnorderedMap> map; std::shared_ptr stdlibGem; @@ -90,9 +116,22 @@ class GemMapping final { std::optional> lookupGemForFile(const core::GlobalState &gs, core::FileRef file) const; + // Record gem metadata for the file based on the externally specified file. void populateFromNDJSON(const core::GlobalState &, const FileSystem &fs, const std::string &ndjsonPath); - void markCurrentGem(GemMetadata gem); + // Record gem metadata for the file based on the filepath + void populateCache(core::FileRef, std::shared_ptr file); + + void addGemDependencies(GemDependencies &&deps) { + if (!deps.currentGem.name().empty()) { + auto metadata = GemMetadata(deps.currentGem.name(), deps.currentGem.version()); + this->currentGem = std::make_shared(std::move(metadata)); + } + this->gemDeps = deps; + } + +private: + std::optional> identifyGem(std::string_view filepath) const; }; } // namespace sorbet::scip_indexer diff --git a/scip_indexer/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc index 2c9489c83..d5353d327 100644 --- a/scip_indexer/SCIPIndexer.cc +++ b/scip_indexer/SCIPIndexer.cc @@ -1187,25 +1187,33 @@ class SCIPSemanticExtension : public SemanticExtension { virtual void prepareForTypechecking(const core::GlobalState &gs) override { auto maybeMetadata = scip_indexer::GemMetadata::tryParse(this->config.gemMetadata); + scip_indexer::GemDependencies deps; scip_indexer::GemMetadata currentGem; if (maybeMetadata.has_value()) { - currentGem = maybeMetadata.value(); - } // TODO: Issue error for incorrect format in string... - 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::errors::SCIPRubyDebug)) { - e.setHeader("{}: {}", - error.kind == scip_indexer::GemMetadataError::Kind::Error ? "error" : "warning", - error.message); - } + deps.currentGem = move(maybeMetadata.value()); + } + auto errors = deps.populateFromConfig(OSFileSystem()); + for (auto &error : errors) { + 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); + this->gemMap.addGemDependencies(move(deps)); if (!this->config.gemMapPath.empty()) { this->gemMap.populateFromNDJSON(gs, OSFileSystem(), this->config.gemMapPath); } + // Eagerly compute gem information from paths if necessary here, + // to avoid synchronization when trying to access gem information + // from multiple threads. + int i = -1; + for (auto file : gs.getFiles()) { + i++; + if (i == 0) { + continue; + } + this->gemMap.populateCache(core::FileRef(i), file); + } }; virtual void finishTypecheckFile(const core::GlobalState &gs, const core::FileRef &file) const override { diff --git a/scip_indexer/SCIPSymbolRef.cc b/scip_indexer/SCIPSymbolRef.cc index 853f8aed5..72b2b04ef 100644 --- a/scip_indexer/SCIPSymbolRef.cc +++ b/scip_indexer/SCIPSymbolRef.cc @@ -11,6 +11,7 @@ #include "spdlog/fmt/fmt.h" #include "common/FileSystem.h" +#include "common/common.h" #include "common/sort.h" #include "core/Loc.h" #include "main/lsp/LSPLoop.h" @@ -48,6 +49,10 @@ utils::Result UntypedGenericSymbolRef::symbolForExpr(const core::GlobalState &gs metadata = gemMap.globalPlaceholderGem; } else { auto owningFile = loc.has_value() ? loc->file() : this->selfOrOwner.loc(gs).file(); + if (this->name.exists() && absl::StrContains(this->name.shortName(gs), "Loader")) { + fmt::print(stderr, "Loader path: {}, loc: {}", owningFile.data(gs).path(), + loc.has_value() ? loc->showRawLineColumn(gs) : "<>"); + } if (!owningFile.exists()) { // Synthetic symbols and built-in like constructs do not have a source location. return utils::Result::skipValue(); diff --git a/test/BUILD b/test/BUILD index 478bdd882..5b89aa104 100644 --- a/test/BUILD +++ b/test/BUILD @@ -77,6 +77,8 @@ cc_binary( "//test/scip:__pkg__", "//tools:__pkg__", ], + # sprintf has been deprecated in newer macOS SDKs + copts = ["-Wno-deprecated-declarations"], deps = [ # TODO(varun): Revisit dependencies after rewriting test runner. "//ast/desugar", diff --git a/test/scip_test_runner.cc b/test/scip_test_runner.cc index baf9db49b..4629a6ddb 100644 --- a/test/scip_test_runner.cc +++ b/test/scip_test_runner.cc @@ -106,7 +106,8 @@ PATH for (auto &testCase : testCases) { MockFileSystem fs("/lolz"); fs.writeFile(testCase.fileName, testCase.content); - auto [metadata, actualErrors] = GemMetadata::readFromConfig(fs); + GemDependencies deps; + auto actualErrors = deps.populateFromConfig(fs); UnorderedSet actualErrorSet(actualErrors.begin(), actualErrors.end()); UnorderedSet expectedErrorSet(testCase.expectedErrors.begin(), testCase.expectedErrors.end()); auto showError = [](const auto &err) -> string { return err.message + "\n"; }; @@ -202,8 +203,10 @@ TEST_CASE("GemInference") { notSorbetRBI = gs.enterFile("myproject/x.rbi", ""); } + scip_indexer::GemDependencies deps; + deps.addCurrentGem(scip_indexer::GemMetadata::forTest("mygem", "33")); scip_indexer::GemMapping gemMap{}; - gemMap.markCurrentGem(scip_indexer::GemMetadata::forTest("mygem", "33")); + gemMap.addGemDependencies(std::move(deps)); auto checkGem = [&](core::FileRef file, string name, string version) { auto gem = gemMap.lookupGemForFile(gs, file);