This repository has been archived by the owner on Dec 13, 2021. It is now read-only.
Update to codespan and codespan-reporting 0.6 #160
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a fairly big change due to the different representation of `Span` in `codespan` 0.6. Previously all files were represented in a contiguous 32bit space as if they were concatenate in to a single file. This is also how the Rust compiler and Arret's ancient bespoke system used to work. Now, there is a separate `FileId` that identifies the file while the `Span` is an offset within the file. This makes the `codespan` API cleaner but presents a couple of disadvantages: 1. In some cases we don't have a `FileId`. This comes up a lot in tests and also in the REPL when we want to parse for syntax highlighting. In these cases we never report diagnostics so we don't need a `FileId`. Previously we would just create our own `Span`s that were offset from 0 and hope we didn't accidentally diagnose with them. We could create a `codespan::Files` every time we parse but this adds overhead, especially because `codespan` isn't thread safe so it requires locking. We use `Option<FileId>` in our custom `Span` type to get around the cases where we don't want to commit to a full file. 2. This blows up the size of our `Span` type from 8 bytes to 13 bytes (start + end + `FileId` + `Option` overhead). This will be rounded up to 16 bytes in most situations due to alignment and padding. This can be reduced to 12 bytes by making `FileId` `NonZeroU32` upstream. There also the complication of `Files` no longer being parameterized on string types. This comes with two other downsides: 1. We can no longer use `Cow<str>` to directly share the `&'static str` from our loaded RFI libraries. 2. Because we now need the `FileId` to build our `Span` we can't parse the input until we add it to `Files`. This means we need to borrow the source back from `Files` after we add it. However, because we can't use `Arc` this means we need to hold a read lock on `Files` while we parse. This effectively means we can only load + parse a single file at a time across all threads. For our integration tests that need to parse a lot of files up front this causes contention. Additionally, there is no way to create a `codespan::Span` in a const context anymore. This required replacing the `EMPTY_SPAN` constant with an `empty_span()` function. On the plus side this brings us back onboard with `codepan` which should help with LSP support. There also seems to be better rendering of diagnostics across multiple files.
This was referenced Jan 4, 2020
This is enabled by brendanzab/codespan#143 which requires us to take a Git dependency until `codespan` 0.7 is released.
This allows us to parameterise `Files` with a new `SourceText` that can both avoid cloning RFI type names and holding a read lock on `Files` during parsing. This is the last of the major regressions since `codespan` 0.3 besides the unavoidable increase in the size of `Span`.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fairly big change due to the different representation of
Spanincodespan0.4.Previously all files were represented in a contiguous 32bit space as if they were concatenate in to a single file. This is also how the Rust compiler and Arret's ancient bespoke system used to work. Now, there is a separate
FileIdthat identifies the file while theSpanis an offset within the file.This makes the
codespanAPI cleaner but presents a couple of disadvantages:In some cases we don't have a
FileId. This comes up a lot in tests and also in the REPL when we want to parse for syntax highlighting. In these cases we never report diagnostics so we don't need aFileId. Previously we would just create our ownSpans that were offset from 0 and hope we didn't accidentally diagnose with them.We could create a
codespan::Filesevery time we parse but this adds overhead, especially becausecodespanisn't thread safe so it requires locking.We use
Option<FileId>in our customSpantype to get around the cases where we don't want to commit to a full file.This increases the size of our
Spantype from 8 bytes to 12 bytes (start + end +Option<FileId>).On the plus side this brings us back onboard with
codepanwhich should help with LSP support. There also seems to be better rendering of diagnostics across multiple files.