From 69fbb5dbdd677c3bf91ad84d31931db337967a38 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Fri, 14 Oct 2022 18:48:05 +0800 Subject: [PATCH 1/5] fix: Try to infer gem name and version. --- common/FileOps.h | 2 + common/FileSystem.cc | 4 + common/FileSystem.h | 4 + common/common.cc | 7 + docs/scip-ruby/CONTRIBUTING.md | 19 ++- main/pipeline/pipeline.cc | 3 + .../semantic_extension/SemanticExtension.h | 1 + plugin_injector/plugin_injector.cc | 2 + scip_indexer/Debug.h | 2 + scip_indexer/SCIPIndexer.cc | 35 +++- scip_indexer/SCIPSymbolRef.cc | 159 ++++++++++++++++++ scip_indexer/SCIPSymbolRef.h | 38 ++++- test/helpers/MockFileSystem.cc | 26 ++- test/helpers/MockFileSystem.h | 1 + test/scip/scip_test.bzl | 13 ++ test/scip_test_runner.cc | 95 ++++++++++- 16 files changed, 391 insertions(+), 20 deletions(-) diff --git a/common/FileOps.h b/common/FileOps.h index 3d0e9bd99c..ca64663134 100644 --- a/common/FileOps.h +++ b/common/FileOps.h @@ -33,6 +33,8 @@ class FileOps final { // NOTE: This does not create parent directories if they exist. static bool ensureDir(std::string_view path); + static std::string getCurrentDir(); + // NOTE: this is a minimal wrapper around rmdir, and as such will raise an exception if the directory is not empty // when it's removed. static void removeDir(std::string_view path); diff --git a/common/FileSystem.cc b/common/FileSystem.cc index 60a418b971..0c1de2702d 100644 --- a/common/FileSystem.cc +++ b/common/FileSystem.cc @@ -12,6 +12,10 @@ void OSFileSystem::writeFile(string_view filename, string_view text) { return FileOps::write(filename, text); } +std::string OSFileSystem::getCurrentDir() const { + return FileOps::getCurrentDir(); +} + vector OSFileSystem::listFilesInDir(string_view path, const UnorderedSet &extensions, bool recursive, const std::vector &absoluteIgnorePatterns, const std::vector &relativeIgnorePatterns) const { diff --git a/common/FileSystem.h b/common/FileSystem.h index 8c6c7ba020..16450c579f 100644 --- a/common/FileSystem.h +++ b/common/FileSystem.h @@ -21,6 +21,9 @@ class FileSystem { /** Writes the specified data to the given file. */ virtual void writeFile(std::string_view filename, std::string_view text) = 0; + /** Gets the path for the current directory. */ + virtual std::string getCurrentDir() const = 0; + /** * Returns a list of all files in the given directory. Returns paths that include the path to directory. * @@ -45,6 +48,7 @@ class OSFileSystem final : public FileSystem { std::string readFile(std::string_view path) const override; void writeFile(std::string_view filename, std::string_view text) override; + std::string getCurrentDir() const override; std::vector listFilesInDir(std::string_view path, const UnorderedSet &extensions, bool recursive, const std::vector &absoluteIgnorePatterns, const std::vector &relativeIgnorePatterns) const override; diff --git a/common/common.cc b/common/common.cc index 91a92cd461..9f34dda8d2 100644 --- a/common/common.cc +++ b/common/common.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -87,6 +88,12 @@ bool sorbet::FileOps::ensureDir(string_view path) { return true; } +std::string sorbet::FileOps::getCurrentDir() { + char buf[MAXPATHLEN + 1]; + getcwd(buf, sizeof(buf)); + return std::string(buf); +} + void sorbet::FileOps::removeDir(string_view path) { auto err = rmdir(string(path).c_str()); if (err) { diff --git a/docs/scip-ruby/CONTRIBUTING.md b/docs/scip-ruby/CONTRIBUTING.md index 417f54a2b0..a951d41912 100644 --- a/docs/scip-ruby/CONTRIBUTING.md +++ b/docs/scip-ruby/CONTRIBUTING.md @@ -147,7 +147,13 @@ scip-ruby myfile.rb --index-file index.scip ### Running tests -Run snapshot tests, which are self-contained: +There are currently 3 kinds of tests: +- Snapshot tests: These cover indexer output. +- Unit tests: These cover some internals which are not possible + to test via snapshots. +- Repo/Integration tests: These try to index an OSS repo using scip-ruby. + +To run snapshot tests, which are self-contained: ``` ./bazel test //test/scip --config=dbg @@ -159,6 +165,12 @@ Updating snapshots: ./bazel test //test/scip:update --config=dbg ``` +You can run unit tests using: + +``` +./bazel test //test/scip:unit_tests --config=dbg +``` + WARNING: Repo tests are kinda' broken right now; they're disabled in CI (see ci.yml), and may or may not work on your machine. @@ -182,6 +194,11 @@ you add a new test, you should create matching `.snapshot.rb` files since those are used as inputs to Bazel. If you know of a way to get rid of that annoyance, submit a PR. +### Writing a new unit test + +See the existing unit tests in `scip_test_runner.cc` +and follow the same structure. + ### Writing a new repo test First, clone the repo using Sorbet locally diff --git a/main/pipeline/pipeline.cc b/main/pipeline/pipeline.cc index 2cb13e6514..9a7ff237a9 100644 --- a/main/pipeline/pipeline.cc +++ b/main/pipeline/pipeline.cc @@ -1125,6 +1125,9 @@ void typecheck(const core::GlobalState &gs, vector what, const { ProgressIndicator cfgInferProgress(opts.showProgress, "CFG+Inference", what.size()); + for (auto &extension : gs.semanticExtensions) { + extension->prepareForTypechecking(gs); + } workers.multiplexJob("typecheck", [&gs, &opts, epoch, &epochManager, &preemptionManager, fileq, outputq, cancelable, intentionallyLeakASTs]() { vector processedFiles; diff --git a/main/pipeline/semantic_extension/SemanticExtension.h b/main/pipeline/semantic_extension/SemanticExtension.h index e08df20998..900234adf0 100644 --- a/main/pipeline/semantic_extension/SemanticExtension.h +++ b/main/pipeline/semantic_extension/SemanticExtension.h @@ -27,6 +27,7 @@ class CFG; namespace pipeline::semantic_extension { class SemanticExtension { public: + virtual void prepareForTypechecking(const core::GlobalState &) = 0; virtual void finishTypecheckFile(const core::GlobalState &, const core::FileRef &) const = 0; virtual void finishTypecheck(const core::GlobalState &) const = 0; virtual void typecheck(const core::GlobalState &, core::FileRef file, cfg::CFG &, ast::MethodDef &) const = 0; diff --git a/plugin_injector/plugin_injector.cc b/plugin_injector/plugin_injector.cc index 023042cafc..957b09f79e 100644 --- a/plugin_injector/plugin_injector.cc +++ b/plugin_injector/plugin_injector.cc @@ -186,6 +186,8 @@ class LLVMSemanticExtension : public SemanticExtension { virtual void run(core::MutableContext &ctx, ast::ClassDef *klass) const override{}; + virtual void prepareForTypechecking(const core::GlobalState &gs) override{}; + virtual void typecheck(const core::GlobalState &gs, core::FileRef file, cfg::CFG &cfg, ast::MethodDef &md) const override { if (!shouldCompile(gs, file)) { diff --git a/scip_indexer/Debug.h b/scip_indexer/Debug.h index d7661c6710..20c431ba13 100644 --- a/scip_indexer/Debug.h +++ b/scip_indexer/Debug.h @@ -55,6 +55,8 @@ 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/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc index f6907b4561..65a80aa47b 100644 --- a/scip_indexer/SCIPIndexer.cc +++ b/scip_indexer/SCIPIndexer.cc @@ -1115,6 +1115,7 @@ using LocalSymbolTable = UnorderedMap; class SCIPSemanticExtension : public SemanticExtension { public: string indexFilePath; + string gemMetadataString; scip_indexer::GemMetadata gemMetadata; using SCIPState = sorbet::scip_indexer::SCIPState; @@ -1149,6 +1150,24 @@ class SCIPSemanticExtension : public SemanticExtension { 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); + if (maybeMetadata.has_value()) { + this->gemMetadata = 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; + for (auto &error : errors) { + if (auto e = gs.beginError(core::Loc(), scip_indexer::SCIPRubyDebug)) { + e.setHeader("{}: {}", + error.kind == scip_indexer::GemMetadataError::Kind::Error ? "error" : "warning", + error.message); + } + } + } + }; + virtual void finishTypecheckFile(const core::GlobalState &gs, const core::FileRef &file) const override { if (this->doNothing()) { return; @@ -1251,12 +1270,13 @@ class SCIPSemanticExtension : public SemanticExtension { } virtual unique_ptr deepCopy(const core::GlobalState &from, core::GlobalState &to) override { - return make_unique(this->indexFilePath, this->gemMetadata); + 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, scip_indexer::GemMetadata metadata) - : indexFilePath(indexFilePath), gemMetadata(metadata), mutableState() {} + SCIPSemanticExtension(string indexFilePath, string gemMetadataString, scip_indexer::GemMetadata gemMetadata) + : indexFilePath(indexFilePath), gemMetadataString(gemMetadataString), gemMetadata(gemMetadata), mutableState() { + } ~SCIPSemanticExtension() {} }; @@ -1285,13 +1305,12 @@ class SCIPSemanticExtensionProvider : public SemanticExtensionProvider { } else { indexFilePath = "index.scip"; } - return make_unique( - indexFilePath, - scip_indexer::GemMetadata::tryParseOrDefault( - providedOptions.count("gem-metadata") > 0 ? providedOptions["gem-metadata"].as() : "")); + auto gemMetadataString = + providedOptions.count("gem-metadata") > 0 ? providedOptions["gem-metadata"].as() : ""; + return make_unique(indexFilePath, gemMetadataString, scip_indexer::GemMetadata()); }; virtual unique_ptr defaultInstance() const override { - return make_unique("index.scip", scip_indexer::GemMetadata::tryParseOrDefault("")); + return make_unique("index.scip", "", scip_indexer::GemMetadata()); }; static vector getProviders(); virtual ~SCIPSemanticExtensionProvider() = default; diff --git a/scip_indexer/SCIPSymbolRef.cc b/scip_indexer/SCIPSymbolRef.cc index 17f1fd32e7..2f6c98cf66 100644 --- a/scip_indexer/SCIPSymbolRef.cc +++ b/scip_indexer/SCIPSymbolRef.cc @@ -1,8 +1,11 @@ // NOTE: Protobuf headers should go first since they use poisoned functions. #include "proto/SCIP.pb.h" +#include #include +#include #include +#include #include #include "absl/status/status.h" @@ -10,6 +13,7 @@ #include "absl/strings/str_replace.h" #include "spdlog/fmt/fmt.h" +#include "common/FileSystem.h" #include "common/sort.h" #include "core/Loc.h" #include "main/lsp/LSPLoop.h" @@ -30,6 +34,161 @@ string showRawRelationshipsMap(const core::GlobalState &gs, const RelationshipsM }); } +using GMEKind = GemMetadataError::Kind; +GemMetadataError configNotFoundError = + GemMetadataError{GMEKind::Error, "Failed to find .gemspec file for identifying gem version"}; +GemMetadataError multipleGemspecWarning = GemMetadataError{ + GMEKind::Warning, "Found multiple .gemspec files when trying to infer Gem name and version; picking the first one " + "lexicographically. Consider passing --gem-metadata name@version explicitly instead"}; +GemMetadataError failedToParseGemspecWarning = GemMetadataError{ + GMEKind::Warning, "Failed to parse .gemspec file for inferring gem name and version. Consider passing " + "--gem-metadata name@version explicitly instead"}; +GemMetadataError failedToParseNameFromGemspecWarning = GemMetadataError{ + GMEKind::Warning, "Failed to parse gem name from .gemspec file; using the .gemspec's base name as a proxy"}; +GemMetadataError failedToParseVersionFromGemspecWarning = + GemMetadataError{GMEKind::Warning, "Failed to parse gem version from .gemspec file"}; +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; + 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::smatch matches; + if (std::regex_match(line, matches, specLineRegex)) { + name = matches[1].str(); + version = matches[2].str(); + } + break; + } + } + if (!name.has_value()) { + errors.push_back(failedToParseGemfileLockWarning); + } + return {GemMetadata{name.value_or(""), version.value_or("")}, errors}; +} + +pair> GemMetadata::readFromGemspec(const string &contents) { + optional name; + optional version; + vector errors; + + std::regex stringKeyRegex(R"END(\s*["'](.+)["'](.freeze)?)END"); + const auto readValue = [&](const string_view line) -> string { + vector entries = absl::StrSplit(line, '='); + if (entries.size() != 2) { + return ""; + } + std::smatch matches; + if (std::regex_match(entries[1], matches, stringKeyRegex)) { + return matches[1].str(); + } + return ""; + }; + istringstream lines(contents); + for (string line; std::getline(lines, line);) { + if (name.has_value() && version.has_value()) { + break; + } + if (!name.has_value() && absl::StrContains(line, ".name =")) { + name = readValue(line); + if (name->empty()) { + errors.push_back(failedToParseNameFromGemspecWarning); + continue; + } + } + // NOTE: In some cases, the version may be stored symbolically, + // in which case this parsing will fail. + if (!version.has_value() && absl::StrContains(line, ".version =")) { + version = readValue(line); + if (version->empty()) { + errors.push_back(failedToParseVersionFromGemspecWarning); + continue; + } + } + } + if (!name.has_value() || !version.has_value()) { + errors.push_back(failedToParseGemspecWarning); + } + return {GemMetadata{name.value_or(""), version.value_or("")}, errors}; +} + +pair> GemMetadata::readFromConfig(const FileSystem &fs) { + UnorderedSet extensions{".lock", ".gemspec"}; + auto paths = fs.listFilesInDir(".", extensions, /*recursive*/ false, {}, {}); + vector errors; + auto currentDirName = [&fs]() -> std::string { + auto currentDirPath = fs.getCurrentDir(); + ENFORCE(!currentDirPath.empty()); + while (currentDirPath.back() == '/') { + currentDirPath.pop_back(); + ENFORCE(!currentDirPath.empty()); + } + return std::filesystem::path(move(currentDirPath)).filename(); + }; + if (paths.empty()) { + errors.push_back(configNotFoundError); + return {GemMetadata(currentDirName(), "latest"), 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, {}}; + } + copyState(gemMetadata, parseErrors); + break; + } + string gemspecPath{}; + for (auto &filename : paths) { + if (!absl::EndsWith(filename, ".gemspec")) { + continue; + } + gemspecPath = filename; + auto [gemMetadata, parseErrors] = GemMetadata::readFromGemspec(fs.readFile(filename)); + if (!gemMetadata.name().empty() && !gemMetadata.version().empty()) { + return {gemMetadata, {}}; + } + copyState(gemMetadata, parseErrors); + break; + } + if (name.has_value() && version.has_value()) { + errors.clear(); + } + if (!name.has_value() && !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}; +} + // 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 { diff --git a/scip_indexer/SCIPSymbolRef.h b/scip_indexer/SCIPSymbolRef.h index 5763f5b2f7..c19e46f9fd 100644 --- a/scip_indexer/SCIPSymbolRef.h +++ b/scip_indexer/SCIPSymbolRef.h @@ -3,6 +3,7 @@ #include #include +#include #include #include "absl/status/status.h" @@ -26,19 +27,41 @@ namespace sorbet::scip_indexer { template using SmallVec = InlinedVector; +struct GemMetadataError { + enum class Kind { Error, Warning } kind; + std::string message; + + template friend H AbslHashValue(H h, const GemMetadataError &x) { + return H::combine(std::move(h), x.kind, x.message); + } + + bool operator==(const GemMetadataError &other) const { + return this->kind == other.kind && this->message == other.message; + } +}; + +extern GemMetadataError configNotFoundError, multipleGemspecWarning, failedToParseGemspecWarning, + failedToParseGemspecWarning, failedToParseNameFromGemspecWarning, failedToParseVersionFromGemspecWarning, + failedToParseGemfileLockWarning; + +struct GemMetadataInferenceTestCase; + class GemMetadata final { std::string _name; std::string _version; GemMetadata(std::string name, std::string version) : _name(name), _version(version) {} + friend GemMetadataInferenceTestCase; + public: + GemMetadata() = default; GemMetadata &operator=(const GemMetadata &) = default; - static GemMetadata tryParseOrDefault(std::string metadata) { - std::vector v = absl::StrSplit(metadata, '@'); + static std::optional tryParse(const std::string &nameAndVersion) { + std::vector v = absl::StrSplit(nameAndVersion, '@'); if (v.size() != 2 || v[0].empty() || v[1].empty()) { - return GemMetadata{"TODO", "TODO"}; + return std::nullopt; } return GemMetadata{v[0], v[1]}; } @@ -50,6 +73,15 @@ class GemMetadata final { const std::string &version() const { return this->_version; } + + bool operator==(const GemMetadata &other) const { + return this->name() == other.name() && this->version() == other.version(); + } + + // HACK: Do a best-effort parse of any config files to extract the name and version. + static std::pair> readFromConfig(const FileSystem &fs); + static std::pair> readFromGemfileLock(const std::string &); + static std::pair> readFromGemspec(const std::string &); }; class UntypedGenericSymbolRef; diff --git a/test/helpers/MockFileSystem.cc b/test/helpers/MockFileSystem.cc index 59b2190c20..5eb44a67c6 100644 --- a/test/helpers/MockFileSystem.cc +++ b/test/helpers/MockFileSystem.cc @@ -1,3 +1,8 @@ +#include + +#include "absl/strings/match.h" +#include "absl/strings/strip.h" + #include "test/helpers/MockFileSystem.h" using namespace std; @@ -41,9 +46,28 @@ void MockFileSystem::deleteFile(string_view filename) { } } +std::string MockFileSystem::getCurrentDir() const { + return rootPath; +} + vector MockFileSystem::listFilesInDir(string_view path, const UnorderedSet &extensions, bool recursive, const vector &absoluteIgnorePatterns, const vector &relativeIgnorePatterns) const { - Exception::raise("Not implemented."); + if (path != "." || !absoluteIgnorePatterns.empty() || !relativeIgnorePatterns.empty() || recursive) { + Exception::raise("Not implemented full support for all parameters in MockFileSystem::listFilesInDir"); + } + vector out{}; + for (auto &[filePath, _] : contents) { + auto relativePath = absl::StripPrefix(absl::StripPrefix(filePath, rootPath), "/"); + if (absl::StrContains(relativePath, '/')) { + continue; + } + for (auto &ext : extensions) { + if (absl::EndsWith(relativePath, ext)) { + out.push_back(std::string(relativePath)); + } + } + } + return out; } } // namespace sorbet::test diff --git a/test/helpers/MockFileSystem.h b/test/helpers/MockFileSystem.h index eb2b7c863f..004e5c5cc4 100644 --- a/test/helpers/MockFileSystem.h +++ b/test/helpers/MockFileSystem.h @@ -17,6 +17,7 @@ class MockFileSystem final : public FileSystem { std::string readFile(std::string_view path) const override; void writeFile(std::string_view filename, std::string_view text) override; void deleteFile(std::string_view filename); + std::string getCurrentDir() const override; std::vector listFilesInDir(std::string_view path, const UnorderedSet &extensions, bool recursive, const std::vector &absoluteIgnorePatterns, const std::vector &relativeIgnorePatterns) const override; diff --git a/test/scip/scip_test.bzl b/test/scip/scip_test.bzl index 4a98f9d7ea..89f5bdfb38 100644 --- a/test/scip/scip_test.bzl +++ b/test/scip/scip_test.bzl @@ -27,6 +27,7 @@ def scip_test_suite(paths, multifile_paths): else: file_groups[d] = [p] + tests.append(scip_unit_tests()) for dir, files in file_groups.items(): names = scip_multifile_test(dir, files) tests.append(names[0]) @@ -41,6 +42,18 @@ def scip_test_suite(paths, multifile_paths): tests = updates, ) +def scip_unit_tests(): + test_name = "unit_tests" + args = ["unit_tests"] + native.sh_test( + name = "unit_tests", + srcs = ["scip_test_runner.sh"], + args = ["only_unit_tests"], + data = ["//test:scip_test_runner"], + size = "small", + ) + return "unit_tests" + def scip_test(path): if not path.endswith(".rb") or path.endswith(".snapshot.rb"): return None diff --git a/test/scip_test_runner.cc b/test/scip_test_runner.cc index bb46ad2cdb..f4f0dacd2c 100644 --- a/test/scip_test_runner.cc +++ b/test/scip_test_runner.cc @@ -43,18 +43,88 @@ #include "payload/binary/binary.h" #include "resolver/resolver.h" #include "rewriter/rewriter.h" +#include "scip_indexer/Debug.h" #include "scip_indexer/SCIPIndexer.h" +#include "scip_indexer/SCIPSymbolRef.h" +#include "test/helpers/MockFileSystem.h" #include "test/helpers/expectations.h" #include "test/helpers/position_assertions.h" -// NOTE: This code in this file is largely copied from parser_test_runner.cc. +namespace sorbet::scip_indexer { +using namespace std; + +struct GemMetadataInferenceTestCase { + string fileName; + string content; + GemMetadata expectedMetadata; + std::vector expectedErrors; + + static GemMetadata makeMetadata(string name, string version) { + return GemMetadata(name, version); + } + GemMetadataInferenceTestCase(string fileName, GemMetadata metadata, std::vector expectedErrors, + string content) + : fileName(fileName), content(content), expectedMetadata(metadata), expectedErrors(expectedErrors) {} +}; +} // namespace sorbet::scip_indexer + +// NOTE: This code in this file is largely copied from parser_test_runner.cc. namespace sorbet::test { using namespace std; bool update; string inputFileOrDir; string outputFileOrDir; +bool onlyRunUnitTests; + +TEST_CASE("GemMetadataInference") { + if (!onlyRunUnitTests) { + return; + } + using namespace scip_indexer; + auto metadata = [](auto &name, auto &version) { return GemMetadataInferenceTestCase::makeMetadata(name, version); }; + auto emptyNameGem = GemMetadataInferenceTestCase::makeMetadata("", "0.1"); + auto emptyVersionGem = GemMetadataInferenceTestCase::makeMetadata("sciptest", ""); + auto bothEmptyGem = GemMetadataInferenceTestCase::makeMetadata("", ""); + // TODO: Create a MockFilesystem here, add a file to that, and then test readFromConfig instead. + std::vector testCases{ + GemMetadataInferenceTestCase("Gemfile.lock", metadata("sciptest", "0.2"), {}, R"( +PATH + remote: . + specs: + sciptest (0.2) + )"), + GemMetadataInferenceTestCase("sciptest.gemspec", metadata("sciptest", "0.2"), {}, R"( + spec.name = "sciptest" + spec.version = "0.2" + )"), + // Check different fallback codepaths. + GemMetadataInferenceTestCase("Gemfile.lock", metadata("lolz", "latest"), {failedToParseGemfileLockWarning}, ""), + GemMetadataInferenceTestCase("sciptest.gemspec", metadata("sciptest", "0.1"), + {failedToParseNameFromGemspecWarning}, R"( + s.name = NOT_A_LITERAL_OOPS + s.version = "0.1" + )"), + GemMetadataInferenceTestCase("sciptest.gemspec", metadata("sciptest", "latest"), + {failedToParseVersionFromGemspecWarning}, R"( + s.name = "sciptest" + s.version = NOT_A_LITERAL_OOPS + )")}; + for (auto &testCase : testCases) { + MockFileSystem fs("/lolz"); + fs.writeFile(testCase.fileName, testCase.content); + auto [metadata, actualErrors] = GemMetadata::readFromConfig(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"; }; + ENFORCE(actualErrorSet == expectedErrorSet, "expected errors = {}\nobtained errors = {}\n", + showSet(expectedErrorSet, showError), showSet(actualErrorSet, showError)); + ENFORCE(metadata == testCase.expectedMetadata, "\nexpected metadata = {}@{}\nobtained metadata = {}@{}\n", + testCase.expectedMetadata.name(), testCase.expectedMetadata.version(), metadata.name(), + metadata.version()); + } +} // Copied from pipeline_test_runner.cc class CFGCollectorAndTyper { // TODO(varun): Copy this over to scip_test_runner.cc @@ -172,7 +242,7 @@ void formatSnapshot(const scip::Document &document, FormatOptions options, std:: }); auto formatSymbol = [](const std::string &symbol) -> string { // Strip out repeating information and placeholder names. - return absl::StrReplaceAll(symbol, {{"scip-ruby gem ", ""}, {"TODO TODO", "[..]"}}); + return absl::StrReplaceAll(symbol, {{"scip-ruby gem ", ""}, {"placeholder_name placeholder_version", "[..]"}}); }; size_t occ_i = 0; std::ifstream input(document.relative_path()); @@ -369,11 +439,9 @@ 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()}; - if (settings.gemMetadata.has_value()) { - argv.push_back("--gem-metadata"); - ENFORCE(!settings.gemMetadata.value().empty()); - argv.push_back(settings.gemMetadata.value().data()); - } + auto metadata = settings.gemMetadata.value_or("placeholder_name@placeholder_version"); + argv.push_back("--gem-metadata"); + argv.push_back(metadata.c_str()); argv.push_back(nullptr); auto parseResult = options.parse(argv.size() - 1, argv.data()); @@ -396,6 +464,10 @@ void test_one_gem(Expectations &test, const TestSettings &settings) { trees = move(namer::Namer::run(gs, move(trees), *workers, nullptr).result()); trees = move(resolver::Resolver::run(gs, move(trees), *workers).result()); + + for (auto &extension : gs.semanticExtensions) { + extension->prepareForTypechecking(gs); + } for (auto &resolvedTree : trees) { sorbet::core::MutableContext ctx(gs, core::Symbols::root(), resolvedTree.file); resolvedTree = class_flatten::runOne(ctx, move(resolvedTree)); @@ -458,6 +530,9 @@ void test_one_gem(Expectations &test, const TestSettings &settings) { // (the directory layout, any expectations of ordering, how Sorbet is run) // before setting up testing infrastructure and explicit feature support. TEST_CASE("SCIPTest") { + if (onlyRunUnitTests) { + return; + } Expectations test = Expectations::getExpectations(inputFileOrDir); TestSettings settings; @@ -507,6 +582,12 @@ int main(int argc, char *argv[]) { auto res = options.parse(argc, argv); auto unmatched = res.unmatched(); + if (unmatched.size() == 1 && unmatched.front() == "only_unit_tests") { + sorbet::test::onlyRunUnitTests = true; + doctest::Context context(argc, argv); + return context.run(); + } + if (unmatched.size() != 1) { std::fprintf(stderr, "error: runner expected single input file with .rb extension or folder"); return 1; From b1c297c56121ba21384335ef5f84fd8d1e1d0b04 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Tue, 1 Nov 2022 09:58:14 +0800 Subject: [PATCH 2/5] docs: Update README & help text based on flag inference. --- README.md | 17 +++++------------ scip_indexer/SCIPIndexer.cc | 4 +++- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 00a554669e..c47f3b21ed 100644 --- a/README.md +++ b/README.md @@ -54,28 +54,21 @@ to download and install fetch `scip-ruby`. Run `scip-ruby` along with some information about your gem. - - - If you have a `sorbet/config` file, that will be picked up automatically to determine which files to index. ```bash - # Uses the latest revision as the version - prefer this if you will index every commit - bundle exec scip-ruby --gem-metadata "my-gem-name@$(git rev-parse HEAD)" - - # Uses the latest tag as the version - prefer this if you're only indexing specific tags - bundle exec scip-ruby --gem-metadata "my-gem-name@$(git describe --tags --abbrev=0)" + bundle exec scip-ruby ``` - If you don't have a `sorbet/config` file, add an extra path argument to index all files in the project. ```bash # Uses the latest revision as the version - prefer this if you will index every commit - bundle exec scip-ruby . --gem-metadata "my-gem-name@$(git rev-parse HEAD)" - - # Uses the latest tag as the version - prefer this if you're only indexing specific tags - bundle exec scip-ruby . --gem-metadata "my-gem-name@$(git describe --tags --abbrev=0)" + bundle exec scip-ruby . ``` These commands will output a SCIP index to `index.scip` (overridable via `--index-file`). +The gem name and version will be inferred from config files (overridable via `--gem-metadata`). + The SCIP index can be uploaded to a Sourcegraph instance using the [Sourcegraph CLI](https://github.com/sourcegraph/src-cli)'s [upload command](https://docs.sourcegraph.com/cli/references/code-intel/upload). @@ -97,7 +90,7 @@ curl -L "https://github.com/sourcegraph/scip-ruby/releases/latest/download/scip- # If using in CI with 'set -e', make sure to wrap the # scip-ruby invocation in 'set +e' followed by 'set -e' # so that indexing failures are non-blocking. -./scip-ruby --index-file index.scip --gem-metadata "my-gem-name@M.N.P" +./scip-ruby ``` The generated index can be uploaded to a Sourcegraph instance diff --git a/scip_indexer/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc index 65a80aa47b..6efce3b12e 100644 --- a/scip_indexer/SCIPIndexer.cc +++ b/scip_indexer/SCIPIndexer.cc @@ -1285,7 +1285,9 @@ class SCIPSemanticExtensionProvider : public SemanticExtensionProvider { 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())( - "gem-metadata", "Metadata in 'name@version' format to be used for cross-repository code navigation.", + "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()); }; unique_ptr readOptions(cxxopts::ParseResult &providedOptions) const override { From 973600da75300943c855a19fa4f1204b0b9973bd Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Tue, 1 Nov 2022 10:19:59 +0800 Subject: [PATCH 3/5] ci: Run unit tests too. --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 82da1bd294..124cb54b8e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,6 +41,8 @@ jobs: with: name: ci-build-log path: log + - name: "🏋️‍♂️ Run unit tests" + run: rm log && ./bazel test //test/scip:unit_tests --config=dbg - name: "🏋️‍♂️ Run snapshot tests" run: rm log && ./bazel test //test/scip --config=dbg # Repo tests are kinda' broken right now because the bundle cache step needs synchronization @@ -74,4 +76,4 @@ jobs: # count = current_dependencies.count {|dep| current_specs[dep.name].first.metadata.key?("funding_uri") } # ^ boom! # We should probably file a bug against bundler but I haven't been able to repro - # the issue outside of a Bazel sandbox yet. \ No newline at end of file + # the issue outside of a Bazel sandbox yet. From 13adb41da91f304e5a1c7cc223833c0b879c58e6 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Tue, 1 Nov 2022 10:49:48 +0800 Subject: [PATCH 4/5] docs: Remove outdated comment from README. --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index c47f3b21ed..1e2938bbc0 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,6 @@ Run `scip-ruby` along with some information about your gem. - If you don't have a `sorbet/config` file, add an extra path argument to index all files in the project. ```bash - # Uses the latest revision as the version - prefer this if you will index every commit bundle exec scip-ruby . ``` From bc356dc288f5e46a6d150017b5b5ae9a0f000e71 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Tue, 1 Nov 2022 11:19:40 +0800 Subject: [PATCH 5/5] ci: Avoid deleting build log. --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 124cb54b8e..808d1f3f4b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,9 +42,9 @@ jobs: name: ci-build-log path: log - name: "🏋️‍♂️ Run unit tests" - run: rm log && ./bazel test //test/scip:unit_tests --config=dbg + run: ./bazel test //test/scip:unit_tests --config=dbg - name: "🏋️‍♂️ Run snapshot tests" - run: rm log && ./bazel test //test/scip --config=dbg + run: ./bazel test //test/scip --config=dbg # Repo tests are kinda' broken right now because the bundle cache step needs synchronization # # - name: "🏋️‍♂️ Build repo tests"