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] Fix/tighten up URI/path conversions, use VirtualizableFilesystem pervasively in DSLX #1737

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cdleary
Copy link
Collaborator

@cdleary cdleary commented Nov 21, 2024

Review note:

  • most of this change is plumbing to ensure consistency with respect to the VirtualizableFilesystem layer and resulting consolidation (e.g. before we had get_contents lambda which is now subsumed by the VirtualizableFilesystem, and there are a few variants for common test uses of get_contents, like UniformContentFilesystem).
  • the remainder of it is using a LspUri type to ensure types represent the conversion between filesystem paths and LSP URIs

We now have a wrapper type to indicate when things have been checked to be in URI form, and that type offers convenience APIs for conversion to string_view for the underlying and to std::filesystem::path to strip off the URI prefix.

Previously we ran into trouble in #1731 because the stdlib path was a relative path and pointed directly at the filesystem, and thus was clashing with the expectation that everything would be in URI form; i.e. a module would import a stdlib module and then that would have a filesystem (non-URI) path, and that would potentially import other stdlib modules with more filesystem (non-URI) paths.

We check as invariant in the file table that no URIs are held there -- the conversion into and out of URI form happens on the import boundary.

This PR also plumbs virtualizable filesystem into the error printing routines so they don't try to look at the real filesystem when they should be looking at the virtualized filesystem contents, e.g. in any error reporting done for parse errors encounterd by the LSP. Now that the LSP-parsed span filenames look indistinguishable from real filesystem filenames we have to be more careful to always try to use the virtual filesystem abstraction instead of xls/common/file/filesystem.h routines in retrieving file contents.

This, as a result, plumbs the virtual filesystem in everywhere to places that GetContents is called to ensure consistency across the DSLX facilities. In general we shouldn't need to rely on //xls/common/file:filesystem as a dep except in tools that write output files (as the VirtualizableFileSystem doesn't have SetContents). Note that usually there is an ImportData around somewhere, as it is a fairly primordial context-like object, and there is a vfs() hanging off of that.

Ran clang-tidy and bant dwyu

@cdleary cdleary force-pushed the cdleary/2024-11-20-dslx-path-in-lsp branch from 00b8dd2 to eb5ebb5 Compare November 21, 2024 09:27
@cdleary cdleary marked this pull request as ready for review November 21, 2024 09:27
@hzeller
Copy link
Member

hzeller commented Nov 21, 2024

Very nice!

Running xls/dev_tools/run-clang-tidy-cached.sh, I do get a bunch of missing includes for some of the referenced filesystem classes or some of the system includes such as <string_view>, so maybe that needs some attention (grep misc-include-cleaner xls_clang-tidy.out).

(I found that our current way of building the compilation DB has issues (at least on Linux) to properly succeed because it is confused about the dylib stuff, so I found on Linux I have to comment out in xls/public/BUILD while building the compilation DB/running clang-tidy-cached)

#libxls_dylib_binary()
#pytype_test_test_c_api_symbols()

)

xls/dslx/frontend/ast_cloner_test.cc Show resolved Hide resolved
xls/dslx/virtualizable_file_system.h Show resolved Hide resolved
xls/dslx/lsp/lsp_uri.h Show resolved Hide resolved
xls/dslx/lsp/lsp_uri.cc Show resolved Hide resolved
@cdleary
Copy link
Collaborator Author

cdleary commented Nov 22, 2024

Very nice!

Running xls/dev_tools/run-clang-tidy-cached.sh, I do get a bunch of missing includes for some of the referenced filesystem classes or some of the system includes such as <string_view>, so maybe that needs some attention (grep misc-include-cleaner xls_clang-tidy.out).

Fixed almost all actionable clang-tidies outside of contrib, PTAL.

We now have a wrapper type to indicate when things have been checked to be in
URI form, and that type offers convenience APIs for conversion to `string_view`
for the underlying and to `std::filesystem::path` to strip off the URI prefix.

We also add a hook to the virtual filesystem layer to allow the LSP filesystem
implementation to validate that the "paths" being resolved are in fact all in
canonical URI form.

Previously we ran into trouble because the stdlib path was a relative path and
pointed directly at the filesystem, and thus was clashing with the expectation
that everything would be in URI form; i.e. a module would import a stdlib
module and then that would have a filesystem (non-URI) path, and that would
potentially import other stdlib modules with more filesystem (non-URI) paths.

We check as invariant in the file table that no URIs are held there -- the
conversion into and out of URI form happens on the import boundary.

This PR also plumbs virtualizable filesystem into the error printing routines
so they don't try to look at the real filesystem when they should be looking at
the virtualized filesystem contents, e.g. in any error reporting done for parse
errors encounterd by the LSP. Now that the LSP-parsed span filenames look
indistinguishable from real filesystem filenames we have to be a bit more
careful to use the virtual filesystem abstraction instead of the real
filesystem in retrieving file contents.

This also plumbs the virtual filesystem in everywhere to places that
GetContents is called to ensure consistency across the DSLX facilities. In
general we shouldn't need to rely on `//xls/common/file:filesystem` as a dep
except in tools that write output files (as the VirtualizableFileSystem doesn't
have SetContents).

Ran clang-format and clang-tidy and bant
@cdleary cdleary force-pushed the cdleary/2024-11-20-dslx-path-in-lsp branch from 06cf7bf to 95c6762 Compare November 22, 2024 01:53
@meheff
Copy link
Collaborator

meheff commented Nov 22, 2024

Running xls/dev_tools/run-clang-tidy-cached.sh, I do get a bunch of missing includes for some of the referenced filesystem classes or some of the system includes such as <string_view>, so maybe that needs some attention (grep misc-include-cleaner xls_clang-tidy.out).

The github PR to google internal submission is already cumbersome so I think we should avoid adding the additional requirement of keeping things include clean. Include cleanliness, in general, is of small value (not zero, but small). I don't think it is worth people's time to either check for cleanliness on PR's or go much out of your way to fix them. We have tools internally which can better handle these cleanups such as automatically flagging them in code review and transforming the code automatically. We can do this on a regular cadence internally if we care enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants