Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[DSLX:LS] Diagnostics updates for dependent modules. #1666

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions xls/codegen/vast/dslx_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,10 @@ DslxBuilder::DslxBuilder(
: additional_search_paths_(
WrapOptionalPathInVector(additional_search_path)),
dslx_stdlib_path_(dslx_stdlib_path),
import_data_(dslx::CreateImportData(
dslx_stdlib_path_, additional_search_paths_,
/*enabled_warnings=*/dslx::kDefaultWarningsSet)),
import_data_(
dslx::CreateImportData(dslx_stdlib_path_, additional_search_paths_,
/*enabled_warnings=*/dslx::kDefaultWarningsSet,
std::make_unique<dslx::RealFilesystem>())),
module_(std::string(main_module_name), /*fs_path=*/std::nullopt,
import_data_.file_table()),
resolver_(resolver),
Expand Down Expand Up @@ -527,7 +528,8 @@ absl::StatusOr<dslx::Import*> DslxBuilder::GetOrImportModule(
return dslx::TypecheckModule(
module, &import_data_, &warnings_);
},
import_tokens, &import_data_, dslx::Span::Fake()));
import_tokens, &import_data_, dslx::Span::Fake(),
import_data_.vfs()));

auto* name_def = resolver_->MakeNameDef(*this, dslx::Span::Fake(), tail);
VLOG(2) << "Creating import node via name definition `"
Expand Down Expand Up @@ -734,7 +736,8 @@ absl::StatusOr<std::string> DslxBuilder::FormatModule() {
const std::string file_name = module_.name() + ".x";
auto import_data =
dslx::CreateImportData(dslx_stdlib_path_, additional_search_paths_,
/*enabled_warnings=*/dslx::kDefaultWarningsSet);
/*enabled_warnings=*/dslx::kDefaultWarningsSet,
std::make_unique<dslx::RealFilesystem>());
dslx::Fileno fileno = import_data.file_table().GetOrCreate(file_name);
dslx::Scanner scanner(import_data.file_table(), fileno, text);
dslx::Parser parser(module_.name(), &scanner);
Expand Down
3 changes: 2 additions & 1 deletion xls/dev_tools/repl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,8 @@ absl::Status CommandReload() {
dslx::ImportData import_data(dslx::CreateImportData(
/*dslx_stdlib_path=*/"",
/*additional_search_paths=*/{},
/*enabled_warnings=*/dslx::kDefaultWarningsSet));
/*enabled_warnings=*/dslx::kDefaultWarningsSet,
std::make_unique<dslx::RealFilesystem>()));

dslx::FileTable& file_table = import_data.file_table();

Expand Down
1 change: 1 addition & 0 deletions xls/dslx/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ cc_library(
":import_record",
":interp_bindings",
":warning_kind",
"//xls/common/file:filesystem",
"//xls/common/status:ret_check",
"//xls/common/status:status_macros",
"//xls/dslx/bytecode:bytecode_cache_interface",
Expand Down
6 changes: 3 additions & 3 deletions xls/dslx/cpp_transpiler/cpp_transpiler_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ absl::Status RealMain(const std::filesystem::path& module_path,
std::string_view namespaces) {
XLS_ASSIGN_OR_RETURN(std::string module_text, GetFileContents(module_path));

ImportData import_data(
CreateImportData(dslx_stdlib_path, /*additional_search_paths=*/dslx_paths,
kDefaultWarningsSet));
ImportData import_data(CreateImportData(
dslx_stdlib_path, /*additional_search_paths=*/dslx_paths,
kDefaultWarningsSet, std::make_unique<RealFilesystem>()));
XLS_ASSIGN_OR_RETURN(TypecheckedModule module,
ParseAndTypecheck(module_text, std::string(module_path),
"source", &import_data));
Expand Down
11 changes: 7 additions & 4 deletions xls/dslx/create_import_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,26 @@ namespace xls::dslx {
ImportData CreateImportData(
const std::filesystem::path& stdlib_path,
absl::Span<const std::filesystem::path> additional_search_paths,
WarningKindSet warnings) {
ImportData import_data(stdlib_path, additional_search_paths, warnings);
WarningKindSet warnings, std::unique_ptr<VirtualizableFilesystem> vfs) {
ImportData import_data(stdlib_path, additional_search_paths, warnings,
std::move(vfs));
import_data.SetBytecodeCache(std::make_unique<BytecodeCache>(&import_data));
return import_data;
}

ImportData CreateImportDataForTest() {
ImportData import_data(xls::kDefaultDslxStdlibPath,
/*additional_search_paths=*/{}, kDefaultWarningsSet);
/*additional_search_paths=*/{}, kDefaultWarningsSet,
std::make_unique<RealFilesystem>());
import_data.SetBytecodeCache(std::make_unique<BytecodeCache>(&import_data));
return import_data;
}

std::unique_ptr<ImportData> CreateImportDataPtrForTest() {
auto import_data = absl::WrapUnique(
new ImportData(xls::kDefaultDslxStdlibPath,
/*additional_search_paths=*/{}, kDefaultWarningsSet));
/*additional_search_paths=*/{}, kDefaultWarningsSet,
std::make_unique<RealFilesystem>()));
import_data->SetBytecodeCache(
std::make_unique<BytecodeCache>(import_data.get()));
return import_data;
Expand Down
2 changes: 1 addition & 1 deletion xls/dslx/create_import_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace xls::dslx {
ImportData CreateImportData(
const std::filesystem::path& stdlib_path,
absl::Span<const std::filesystem::path> additional_search_paths,
WarningKindSet warnings);
WarningKindSet warnings, std::unique_ptr<VirtualizableFilesystem> vfs);

// Creates an ImportData with reasonable defaults (standard path to the stdlib
// and no additional search paths).
Expand Down
3 changes: 2 additions & 1 deletion xls/dslx/dslx_fmt.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ absl::Status RealMain(std::string_view input_path,

ImportData import_data =
CreateImportData(xls::kDefaultDslxStdlibPath,
/*additional_search_paths=*/{}, kNoWarningsSet);
/*additional_search_paths=*/{}, kNoWarningsSet,
std::make_unique<RealFilesystem>());
std::string formatted;
if (mode == "autofmt") {
std::vector<CommentData> comments;
Expand Down
18 changes: 18 additions & 0 deletions xls/dslx/import_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "absl/strings/str_format.h"
#include "absl/strings/str_join.h"
#include "absl/strings/str_split.h"
#include "xls/common/file/filesystem.h"
#include "xls/common/status/ret_check.h"
#include "xls/common/status/status_macros.h"
#include "xls/dslx/bytecode/bytecode_cache_interface.h"
Expand All @@ -46,6 +47,19 @@

namespace xls::dslx {

absl::Status RealFilesystem::FileExists(const std::filesystem::path& path) {
return xls::FileExists(path);
}

absl::StatusOr<std::string> RealFilesystem::GetFileContents(
const std::filesystem::path& path) {
return xls::GetFileContents(path);
}

absl::StatusOr<std::filesystem::path> RealFilesystem::GetCurrentDirectory() {
return xls::GetCurrentDirectory();
}

/* static */ absl::StatusOr<ImportTokens> ImportTokens::FromString(
std::string_view module_name) {
return ImportTokens(absl::StrSplit(module_name, '.'));
Expand Down Expand Up @@ -185,6 +199,10 @@ absl::Status ImportData::AddToImporterStack(
}
}

if (importer_stack_observer_ != nullptr) {
importer_stack_observer_(importer_span, imported);
}

VLOG(3) << "Adding import span to stack: "
<< importer_span.ToString(file_table());
importer_stack_.push_back(new_import_record);
Expand Down
63 changes: 56 additions & 7 deletions xls/dslx/import_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,33 @@

namespace xls::dslx {

// A virtualizable filesystem seam so that we can perform imports using either
// the real filesystem or a virtual filesystem.
//
// This is useful for use-cases like language servers where a mix of real
// filesystem and virtualized filesystem contents (i.e. with temporary edits in
// working buffers) are used.
class VirtualizableFilesystem {
public:
virtual ~VirtualizableFilesystem() = default;

virtual absl::Status FileExists(const std::filesystem::path& path) = 0;

virtual absl::StatusOr<std::string> GetFileContents(
const std::filesystem::path& path) = 0;

virtual absl::StatusOr<std::filesystem::path> GetCurrentDirectory() = 0;
};

class RealFilesystem : public VirtualizableFilesystem {
public:
~RealFilesystem() override = default;
absl::Status FileExists(const std::filesystem::path& path) override;
absl::StatusOr<std::string> GetFileContents(
const std::filesystem::path& path) override;
absl::StatusOr<std::filesystem::path> GetCurrentDirectory() override;
};

// An entry that goes into the ImportData.
class ModuleInfo {
public:
Expand Down Expand Up @@ -104,12 +131,11 @@ class ImportTokens {
std::vector<std::string> pieces_;
};

// Wrapper around a {subject: module_info} mapping that modules can be imported
// into.
// Use the routines in create_import_data.h to instantiate an object.
// Wrapper around a `{subject: module_info}` mapping that modules can be
// imported into.
class ImportData {
public:
// All instantiations of ImportData should pass a stdlib_path as below.
// Use the routines in `create_import_data.h` to instantiate an object.
ImportData() = delete;

bool Contains(const ImportTokens& target) const {
Expand All @@ -124,6 +150,17 @@ class ImportData {
absl::Status AddToImporterStack(const Span& importer_span,
const std::filesystem::path& imported);

// Registers a callback to be notified when we see that an importing span is
// beginning to import a filesystem path. This is useful for tracking the DAG
// of dependencies.
//
// Recursion checks are performed before this callback is invoked, so the
// callback results should all be validated to be DAG compliant.
void SetImporterStackObserver(
std::function<void(const Span&, const std::filesystem::path&)> f) {
importer_stack_observer_ = std::move(f);
}

// This pops the entry from the import stack and verifies it's the latest
// entry, returning an error iff it is not.
absl::Status PopFromImporterStack(const Span& import_span);
Expand Down Expand Up @@ -205,22 +242,29 @@ class ImportData {
FileTable& file_table() { return file_table_; }
const FileTable& file_table() const { return file_table_; }

VirtualizableFilesystem& vfs() const { return *vfs_; }

private:
friend ImportData CreateImportData(const std::filesystem::path&,
absl::Span<const std::filesystem::path>,
WarningKindSet);
WarningKindSet,
std::unique_ptr<VirtualizableFilesystem>);

friend std::unique_ptr<ImportData> CreateImportDataPtr(
const std::filesystem::path&, absl::Span<const std::filesystem::path>,
WarningKindSet);

friend ImportData CreateImportDataForTest();
friend std::unique_ptr<ImportData> CreateImportDataPtrForTest();

ImportData(std::filesystem::path stdlib_path,
absl::Span<const std::filesystem::path> additional_search_paths,
WarningKindSet enabled_warnings)
WarningKindSet enabled_warnings,
std::unique_ptr<VirtualizableFilesystem> vfs)
: stdlib_path_(std::move(stdlib_path)),
additional_search_paths_(additional_search_paths),
enabled_warnings_(enabled_warnings) {}
enabled_warnings_(enabled_warnings),
vfs_(std::move(vfs)) {}

// Attempts to find a module owned by this ImportData according to the
// filename present in "span". Returns a NotFound error if a corresponding
Expand All @@ -240,8 +284,13 @@ class ImportData {
WarningKindSet enabled_warnings_;
std::unique_ptr<BytecodeCacheInterface> bytecode_cache_;

std::function<void(const Span&, const std::filesystem::path&)>
importer_stack_observer_;

// See comment on AddToImporterStack() above.
std::vector<ImportRecord> importer_stack_;

std::unique_ptr<VirtualizableFilesystem> vfs_;
};

} // namespace xls::dslx
Expand Down
23 changes: 12 additions & 11 deletions xls/dslx/import_routines.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
#include "absl/strings/str_join.h"
#include "absl/types/span.h"
#include "xls/common/config/xls_config.h"
#include "xls/common/file/filesystem.h"
#include "xls/common/file/get_runfile_path.h"
#include "xls/common/status/ret_check.h"
#include "xls/common/status/status_macros.h"
Expand All @@ -48,7 +47,8 @@ namespace xls::dslx {
static absl::StatusOr<std::filesystem::path> FindExistingPath(
const ImportTokens& subject, const std::filesystem::path& stdlib_path,
absl::Span<const std::filesystem::path> additional_search_paths,
const Span& import_span, const FileTable& file_table) {
const Span& import_span, const FileTable& file_table,
VirtualizableFilesystem& vfs) {
absl::Span<std::string const> pieces = subject.pieces();
std::optional<std::string> subject_parent_path;
const absl::flat_hash_set<std::string> builtins = {
Expand All @@ -66,13 +66,13 @@ static absl::StatusOr<std::filesystem::path> FindExistingPath(
std::vector<std::string> attempted;

// Helper that tries to see if "path" is present relative to "base".
auto try_path = [&attempted](const std::filesystem::path& base,
const std::filesystem::path& path)
auto try_path = [&attempted, &vfs](const std::filesystem::path& base,
const std::filesystem::path& path)
-> std::optional<std::filesystem::path> {
auto full_path = std::filesystem::path(base) / path;
VLOG(3) << "Trying path: " << full_path;
attempted.push_back(std::string{full_path});
if (FileExists(full_path).ok()) {
if (vfs.FileExists(full_path).ok()) {
VLOG(3) << "Found existing file for import path: " << full_path;
return full_path;
}
Expand Down Expand Up @@ -104,7 +104,7 @@ static absl::StatusOr<std::filesystem::path> FindExistingPath(
VLOG(3) << "Attempting runfile-based import path via " << subject_path;
if (absl::StatusOr<std::string> runfile_path =
GetXlsRunfilePath(GetXLSRootDir() / subject_path);
runfile_path.ok() && FileExists(*runfile_path).ok()) {
runfile_path.ok() && vfs.FileExists(*runfile_path).ok()) {
return *runfile_path;
}

Expand All @@ -121,7 +121,7 @@ static absl::StatusOr<std::filesystem::path> FindExistingPath(
<< *subject_parent_path;
if (absl::StatusOr<std::string> runfile_path = GetXlsRunfilePath(
absl::StrCat(GetXLSRootDir(), *subject_parent_path));
runfile_path.ok() && FileExists(*runfile_path).ok()) {
runfile_path.ok() && vfs.FileExists(*runfile_path).ok()) {
return *runfile_path;
}
}
Expand All @@ -138,13 +138,14 @@ static absl::StatusOr<std::filesystem::path> FindExistingPath(
"attempted: [ %s ]; working "
"directory: \"%s\"; stdlib directory: \"%s\"",
import_span.ToString(file_table), absl::StrJoin(attempted, " :: "),
GetCurrentDirectory().value(), stdlib_path));
vfs.GetCurrentDirectory().value(), stdlib_path));
}

absl::StatusOr<ModuleInfo*> DoImport(const TypecheckModuleFn& ftypecheck,
const ImportTokens& subject,
ImportData* import_data,
const Span& import_span) {
const Span& import_span,
VirtualizableFilesystem& vfs) {
XLS_RET_CHECK(import_data != nullptr);
if (import_data->Contains(subject)) {
VLOG(3) << "DoImport (cached) subject: " << subject.ToString();
Expand All @@ -157,13 +158,13 @@ absl::StatusOr<ModuleInfo*> DoImport(const TypecheckModuleFn& ftypecheck,
XLS_ASSIGN_OR_RETURN(std::filesystem::path found_path,
FindExistingPath(subject, import_data->stdlib_path(),
import_data->additional_search_paths(),
import_span, file_table));
import_span, file_table, vfs));

XLS_RETURN_IF_ERROR(import_data->AddToImporterStack(import_span, found_path));
absl::Cleanup cleanup = absl::MakeCleanup(
[&] { CHECK_OK(import_data->PopFromImporterStack(import_span)); });

XLS_ASSIGN_OR_RETURN(std::string contents, GetFileContents(found_path));
XLS_ASSIGN_OR_RETURN(std::string contents, vfs.GetFileContents(found_path));

absl::Span<std::string const> pieces = subject.pieces();
std::string fully_qualified_name = absl::StrJoin(pieces, ".");
Expand Down
15 changes: 9 additions & 6 deletions xls/dslx/import_routines.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,27 +28,30 @@ namespace xls::dslx {
// Type-checking callback lambda.
using TypecheckModuleFn = std::function<absl::StatusOr<TypeInfo*>(Module*)>;

// Imports the module identified (globally) by 'subject'.
// Imports the module identified (globally) by `subject`.
//
// Importing means: locating, parsing, typechecking, and caching in the import
// cache.
//
// Resolves against an existing import in 'cache' if it is present.
// Resolves against an existing import in `import_data` if it is present.
//
// Args:
// ftypecheck: Function that can be used to get type information for a module.
// subject: Tokens that globally uniquely identify the module to import; e.g.
// something built-in like ('std',) for the standard library or something
// fully qualified like ('xls', 'lib', 'math').
// cache: Cache that we resolve against so we don't waste resources
// something built-in like `{"std"}` for the standard library or something
// fully qualified like `{"xls", "lib", "math"}`.
// import_data: Cache that we resolve against so we don't waste resources
// re-importing things in the import DAG.
// import_span: Indicates the "importer" (i.e. the AST node, lexically) that
// caused this attempt to import.
//
// Returns:
// The imported module information.
absl::StatusOr<ModuleInfo*> DoImport(const TypecheckModuleFn& ftypecheck,
const ImportTokens& subject,
ImportData* import_data,
const Span& import_span);
const Span& import_span,
VirtualizableFilesystem& vfs);

} // namespace xls::dslx

Expand Down
Loading
Loading