This provides much more clarity as to what is going on. Further, it's less
ambiguous, since I'm about to introduce a new type of xmlo lowering into XIR
for writing the actual xmlo files.
DEV-7145
This allows `XmlXirReader` to be used in a `Lower` operation, just as
everything else, bringing me one step closer to a pipeline that can be
concisely represented; this is finally beginning to unify in a clear way,
though it is still a bit of a mess.
This causes `XmlXirReader` to _act_ like a `parse::Parser` in that it yields
a `ParsedResult`, but it does not use `parse::Parser` itself; that was the
_original_ plan: convert it into a `ParseState` where `XmlXirReader` became
a context, and force `Parser` to yield by feeding it a stream of tokens with
`repeat`, but that ended up performing poorly relative to this change. I
did some investigation, which I might write about in the future, but for
now, this solution works just fine.
DEV-7145
This abstraction has grown quite a bit, and it's time to start formalizing
it a bit. This split doesn't change any behavior, but it does start to make
it easier to reason about by clearly stating the broad components and how
they interact with one-another.
This doesn't yet move the tests; those will come next, but they are very
few. The reason I gave previously for this was because (a) they're tested
indirectly via the systems that utilize them and (b) because the abstraction
was not yet settled on the process was already very expensive. No test
coverage was lost---it's only that failures were potentially harder to debug
on test failures, but in practice not even this was true, because the deeply
expressive types all but ensured that, if it compiles, it will function in a
way that is expected. Unit tests and documentation for this system will be
added once I'm sure that this abstraction is in a proper state.
DEV-7145
This also modifies `poc` such that `Lower` is invoked as an associated
function rather than a method to emphasize the pattern that is forming, so
that it can be later abstracted away.
DEV-11864
The `while_ok` can just be implied with a lowering operation, and that
reduces the name complexity so that we can maybe introduce even more
specialized methods without resulting in a huge sentence as a name.
DEV-11864
This finally uses `parse` all the way up to aggregation into the ASG, as can
be seen by the mess in `poc`. This will be further simplified---I just need
to get this committed so that I can mentally get it off my plate. I've been
separating this commit into smaller commits, but there's a point where it's
just not worth the effort anymore. I don't like making large changes such
as this one.
There is still work to do here. First, it's worth re-mentioning that
`poc` means "proof-of-concept", and represents things that still need a
proper home/abstraction.
Secondly, `poc` is retrieving the context of two parsers---`LowerContext`
and `Asg`. The latter is desirable, since it's the final aggregation point,
but the former needs to be eliminated; in particular, packages need to be
worked into the ASG so that `found` can be removed.
Recursively loading `xmlo` files still happens in `poc`, but the compiler
will need this as well. Once packages are on the ASG, along with their
state, that responsibility can be generalized as well.
That will then simplify lowering even further, to the point where hopefully
everything has the same shape (once final aggregation has an abstraction),
after which we can then create a final abstraction to concisely stitch
everything together. Right now, Rust isn't able to infer `S` for
`Lower<S, LS>`, which is unfortunate, but we'll be able to help it along
with a more explicit abstraction.
DEV-11864
This is intended to describe, to the user, the state that the parser is
in. This will be used to convey additional information for general parser
errors, but it should also probably be integrated into parsers' individual
errors as well when appropriate.
This is something I expected to add at some point, but I wanted to add them
because, when dealing with lowering errors, it can be difficult to tell
what parser the error originated from.
DEV-11864
The `*_iter_while_ok` functions now compose like monads, flattening `Result`
at each step and drastically simplifying handling of error types. This also
removes the bunch of `?`s at the end of the expression, and allows me to use
`?` within the callback itself.
I had originally not used `Result` as the return type of the callback
because I was not entirely sure how I was going to use them, but it's now
clear that I _always_ use `Result` as the return type, and so there's no use
in trying to be too accommodating; it can always change in the future.
This is desirable not just for cleanup, but because trying to refactor
`asg_builder` into a pair of `Parser`s is really messy to chain without
flattening, especially given some state that has to leak temporarily to the
caller. More on that in a future commit.
DEV-11864
This was always the intent, but I didn't have a higher-level object
yet. This removes all the awkwardness that existed with working the root
in as an identifier.
DEV-11864
This wraps `Ident` in a new `Object` variant and modifies `Asg` so that its
nodes are of type `Object`.
This unfortunately requires runtime type checking. Whether or not that's
worth alleviating in the future depends on a lot of different things, since
it'll require my own graph implementation, and I have to focus on other
things right now. Maybe it'll be worth it in the future.
Note that this also gets rid of some doc examples that simply aren't worth
maintaining as the API evolves.
DEV-11864
A previous commit mentioned that there's not a place for `Dim`, and
duplicated it between `asg` and `xmlo`. Well, `Dtype` is also needed in
both, and so here's a home for now.
`Dtype` has always been an inappropriate detail for the system and will one
day be removed entirely in favor of higher-level types; the machine
representation is up to the compiler to decide.
DEV-11864
asg_builder is about to be replaced, but in the process of simplifying the
destination IR (the ASG), I'm moving things into the proper place. This
never belonged here---it belongs with the actual lowering operation.
Previously, this was not reasoned about in terms of a lowering operation,
and was written when I was first introducing myself to Rust and trying to
get a proof-of-concept linker working.
DEV-11864
This matches xmlo::Dim, and could be the same thing, if we can find a home
for it in the future; it's not worth creating such a home right now when I'm
not yet sure what else ought to live there; the duplication may be fine.
The conversion from xmlo needs to be moved, and `Dim` is going to be used
for more than just identifiers (expressions will have type inference
performed).
DEV-11864
This allows retrieving and providing a context to a `Parser`. This is
intended for use with an aggregating parser, in particular to construct the
ASG and return it.
This is a component of a change that replaces `asg_builder` with a
`Parser`-based lowering into the ASG, but there are still changes that need
to be made to simplify things and complete its integration.
DEV-11864
Previously, since the graph contained only identifiers, discovered roots
were stored in a separate vector and exposed to the caller. This not only
leaked details, but added complexity; this was left over from the
refactoring of the proof-of-concept linker some time ago.
This moves the root management into the ASG itself, mostly, with one item
being left over for now in the asg_builder (eligibility classifications).
There are two roots that were added automatically:
- __yield
- __worksheet
The former has been removed and is now expected to be explicitly mapped in
the return map, which is now enforced with an extern in `core/base`. This
is still special, in the sense that it is explicitly referenced by the
generated code, but there's nothing inherently special about it and I'll
continue to generalize it into oblivion in the future, such that the final
yield is just a convention.
`__worksheet` is the only symbol of type `IdentKind::Worksheet`, and so that
was generalized just as the meta and map entries were.
The goal in the future will be to have this more under the control of the
source language, and to consolodate individual roots under packages, so that
the _actual_ roots are few.
As far as the actual ASG goes: this introduces a single root node that is
used as the sole reference for reachability analysis and topological
sorting. The edges of that root node replace the vector that was removed.
DEV-11864
In the actual implementation (outside of tests), this is always looking up
before adding the symbol. This will simplify the API, while still retaining
errors, since the identifier will fail the state transition if the
identifier did not exist before attempting to set a fragment. So while this
is slower in microbenchmarks, this has no effect on real-world performance.
Further, I'm refactoring toward a streaming ASG aggregation, which is a lot
easier if we do not need to perform lookups in a separate step from the
ASG's primitives.
DEV-11864
`PartialEq` remains, and is all that is needed. See previous commit
regarding the removal of this same bound from `Context`.
This can be re-added if it ends up actually being necessary. But Tokens are
ephemeral and used only in lowering pipelines, using pattern matching.
DEV-11864
These traits are no longer necessary now that I'm using concrete types; they
just add unnecessary noise and confusion as I attempt to further refactor.
Don't abstract prematurely.
DEV-11864
This removes the generic on the Asg (which was formerly BaseAsg),
hard-coding `IdentObject`, which will further evolve. This makes the IR an
actual concrete IR rather than an abstract data structure.
These tests bring me back a bit, since they were written as I was still
becoming familiar with Rust.
DEV-11864
This is the beginning of an incremental refactoring to remove generics, to
simplify the ASG. When I initially wrote the linker, I wasn't sure what
direction I was going in, but I was also negatively influenced by more
traditional approaches to both design and unit testing.
If we're going to call the ASG an IR, then it needs to be one---if the core
of the IR is generic, then it's more like an abstract data structure than
anything. We can abstract around the IR to slice it up into components that
are a little easier to reason about and understand how responsibilities are
segregated.
DEV-11864
This is unnecessarily restrictive, since we do not require anything further
than `PartialEq` for the situations where we care about equality (tests).
DEV-11864
This is too restrictive, especially for parsers that fold into something,
like the ASG, which may exist prior to invoking the parser.
This moves the trait bound to the functions that actually need it. Those
obviously cannot be used if the Context does not implement `Default`, but
I'll provide alternative conveniences.
DEV-11864
RSG (Ryan Specialty Group) recently announced a rename to Ryan Specialty (no
"Group"), but I'm not sure if the legal name has been changed yet or not, so
I'll wait on that.
These are no longer TODOs---they represent invalid tokens.
I'm going to put effort into providing further context with the diagnostic
system [right now] because these are internal errors caused by either
miscompilation or an incomplete reader.
DEV-10936
This was missed when removing it from other Display impls when the new
diagnostic system was introduced. Raw `Span`s display byte offsets and the
context, which is no longer desirable as part of an error message.
DEV-10936
I had waited to provide more documentation until I was sure that the
abstraction was not going to change significantly; there was a lot of
refactoring in prior commits.
DEV-12151
This moves construction out of `From` and into separate associated
functions, which can be further simplified in a bit.
We also need unit tests for this, since this still relies on integration
tests due to the cost of the aggressive and tight refactoring iterations.
DEV-12151
Previously, when adjacent duplicate spans were both resolved, if one failed,
the other certainly would, which would result in duplicate labels each
squash. Elided spans do not have syslabels, and so this is no longer a
concern.
DEV-12151
This was removed in a previous commit while working on simplifying the
implementation, with the hope of returning to it once things were in a
better place. They are, so let's bring it back.
DEV-12151
`SpanLabel` was created during a very early refactoring of this system, and
I've just been fighting with it sense. This removes it, and simplifies
some things in the process.
It also makes clear that `Level` is never optional and removes the awkward
`Level::default` that was there previously; the default is now the lowest
level, which will always be able to be escalated.
DEV-12151
This does what the original proof-of-concept implementation did---skip a
span that was just processed, since it'll be squashed into the previous
anyway. These duplicate spans originate from the diagnostic system when
producing supplemental help information.
DEV-12151
Tests are large and will be getting larger. The source will also grow as
it's better documented and cleaned up. It's getting more difficult to
navigate efficiently and concurrently modify implementation and tests, and
parsing via LSP is getting slower with certain types of changes.
DEV-12151
Alright, starting to settle on an abstraction now, and things are coming
together. This gives us line numbers in the previously-empty gutter, and
widens the gutter to accommodate. Gutters are normalized across
sections. Sections are not yet collapsed for sequential line numbers in the
same context.
Exciting!
Here's an example, on an xmlo file:
error: expected closing tag for `preproc:symtable`
--> /home/.../foo.xmlo:16:4
|
16 | <preproc:symtable xmlns:map="http://www.w3.org/2005/xpath-functions/map">
| ----------------- note: element `preproc:symtable` is opened here
--> /home/.../foo.xmlo:11326:4
|
11326 | </preproc:wrong>
| ^^^^^^^^^^^^^^^^ error: expected `</preproc:symtable>`
DEV-12151
The `Section` itself is now responsible for outputting the gutter, which
puts us in a position to be able to apply consistent formatting without
having to propagate width data to every line variant.
Now `SourceLine` _does_ actually correspond to a line of output, which will
allow for better formatting (e.g. collapsing padding) and, importantly,
proper management of gutters.
Note that the seemingly unnecessary `SectionSourceLine` allows for a subtle
consistent formatting for all variants' gutters in `SectionLine`, which will
allow us to hoist that rendering out in the next commit. The other option
was to include a trailing space for padding and marks, but that is not only
sloppy and undesirable, but asking for confusion, especially in editors (like
mine) that trim trailing whitespace.
DEV-12151
If a column isn't present, it degrades to displaying labels like footnotes
anyway, so this simplifies the system rather than catering to a rare
case. With that said, this does lose functionality, since it does not
render the source line at all, even though we _could_ do so.
I may re-introduce that rendering after some further refactoring,
specifically for gutters.
DEV-12151
Using a byte vector just makes life more difficult with regard to preparing
the diagnostic reports. We're already validating UTF-8 data for column
generation, which is necessary for a robust report, so let's just store it
as a String to begin with.
DEV-12151
Note that, if a span is first encountered with a mark but with _no_ label,
the first label (if collapsed) will be on the next line. This allows a span
to be marked without extra visual noise if it's not necessary, and to be
able to trust that it'll stay that way.
Until coloring is introduced, this may or may not be easier to read
depending on context.
This is also not yet taking into account where on the line it begins, and so
may render poorly if the span is at the end of a line. That will be fixed
later on.
DEV-12151
This is now visible in the diagnostic output. Example at this point in
time, on an xmlo file for one of our smallest systems:
error: expected closing tag for `preproc:symtable`
--> /home/.../foo.xmlo:16:4
|
| <preproc:symtable xmlns:map="http://www.w3.org/2005/xpath-functions/map">
| -----------------
= note: element `preproc:symtable` is opened here
--> /home/.../foo.xmlo:11326:4
|
| </preproc:wrong>
| ^^^^^^^^^^^^^^^^
= error: expected `</preproc:symtable>`
DEV-12151
Looking more and more Rust-like. Shameless copy.
TBH I forget what character it uses for help, but it's easy enough to
change.
Also, to be clear: this is modeled after Rust, but it's not a requirement of
mine that it look exactly like it. I just like the general style; I'll
surely deviate over time, as appropriate (or as I feel like it).
DEV-12151
This has the effect of highlighting the columns of the source lines using
'^' as an underline.
The next step will be to have the underline character depend on the
`Level`.
If this commit message doesn't sound all that exciting, given what it
finally achieved after all this time, it's because I'm exhausted, and my
prototype has already taken my excitement. But this is significant, given
all the work leading up to it.
There is some code cleanup needed and some unit tests that ought to be
written rather than relying on integration, but considering how much this is
being refactored, I don't want to add to that refactoring cost just yet
before gutters are introduced and I know things are settled for now.
DEV-12151
This has been a lot of refactoring for something that I prototyped a week
ago, and the prototype is still further along in its output formatting (it
has line numbering in gutters and span markings).
But, this has come a long way, and I'm happy with it overall, though I'm not
happy with my slow pace and struggle to maintain focus. But those are
personal issues.
This leaves a lot to be desired, but at the same time is still really
helpful. There's a couple notable TODOs regarding pointless allocation and
UTF8 re-checking, but otherwise, the feature-related steps are:
- Gutters with line numbers; and
- Marking columns associated with the span.
DEV-12151
Rather than squashing as a separate operation, and explicitly denoting when
it occurred, we'll just always squash, as was done before these changes. It
doesn't really make sense to make this optional and there's not any value in
keeping the decision around.
This also sets us up favorably for future changes: it creates a vector of
labels, which can be analyzed later to determine how to best lay out marks
and labels.
DEV-12151
Just renames the lifetime to refer to the `Diagnostic`, rather than a
`Label` returned by it, which was all `'l` was previously used for.
Note that many labels have a `'static` lifetime; this doesn't change that or
somehow cause it to reallocate; the label must life _for at least `'d`_.
DEV-12151
Rather than rendering the diagnostic `Display` message to a string only to
copy it to yet another buffer later on, this simply stores a reference to
the `Diagnostic` that was provided. This also adds a type to the `Report`
associating it with the provided `Diagnostic`, which does seem appropriate,
given that the report was produced for it.
I should probably rename '{l=>d} now.
DEV-12151
Rather than writing to the provided `Write` object, this produces a `Report`
object. While a lifetime still exists for the diagnostic data (labels,
specifically), I was able to remove the other lifetime resulting from
`ResolvedSpan` by transferring ownership of the data to the `Report`
itself. Once actual source lines are integrated shortly, `Report` will
include those as well.
This has been a tedious process, but it's coming together. Hopefully these
commits documenting the progressive and ugly refactoring are found useful by
some reader in the future.
DEV-12151
The line number was getting special treatment that is simply not worth the
cost (with regards to how burdensome it is on the type definitions). This
simplifies things quite a bit.
If we want header customization in the future, we can worry about that in a
different way, or allow the header as a whole to be swapped out, rather than
its constituents.
DEV-12151
`HeadingColNum` is no longer constructed by `HeadingLineNum`. This both
narrows the types and required data (e.g. removing dummy values in test
cases), and reduces the coupling (by favoring composition, but still coupled
with the concrete type).
DEV-12151
I'm unhappy with the current state of this, which is why I haven't settled
on docs or unit tests for these changes yet (though note that the
integration tests do cover these changes)---this is still a prototype
refactoring.
In particular, this needs to do more lowering---the `ResolvedSpan` and
`MaybeResolvedSpan` need to be eliminated and lowered into exactly what is
needed so that we can stop reasoning about them and propagating them.
Further, having lines and columns lazily evaluate themselves for
display---based on `MaybeResolvedSpan`---adds extra generics that shouldn't
be necessary; they should be pre-computed and store the concrete data they
need in variants. Display shouldn't involve computation beyond formatting
of pre-computed data.
That was always the plan, but this refactoring has been incremental.
Anyway: this is in a working and integration-tested state, but it's going to
change.
DEV-12151
This generalizes the types a bit more and introduces unit tests. Note that
these are still also covered by integration tests.
The next step will be to finish generalizing
`<VisualReporter as Reporter>::render`, after which I'll get back to the
task of outputting the source line along with markings and labels.
DEV-12151
This is just to provide clarity. `ctx` is not so widely used that we
benefit from such a short identifier, and it's not worth the cognitive
burden of people unfamiliar with what it may mean.
DEV-12151
This is redundant with the `Endpoints` variant, although it did read
better. It's just another case to have to handle.
I was originally going to use `std::ops::RangeInclusive` for `Endpoints`,
however that struct also contains an extra bool indicating whether it was
exhausted (as an iterator), which isn't appropriate for this.
DEV-12151
This logic is still covered by the integration tests; I'll be adding unit
tests once it's decoupled to the point where that's possible, which should
be shortly, and after I make sure this is the route I do want to go down.
DEV-12151
This simplifies types and error handling since we will always have at least
one line, provided that the span is within the range of the context. To
ensure that, this patch introduces a new error.
DEV-12151
I did not initially introduce lifetimes because I wasn't sure how the system
was going to evolve, but now lifetimes are going to be needed in a number of
contexts. The core of TAMER is able to avoid lifetimes in most instances
because of its internment system, but its use is not appropriate for the
diagnostic system's buffers (beyond sourcing strings from already-interned
data).
DEV-12151
Determining the column number is not as simple as performing byte
arithmetic, because certain characters have different widths. Even if we
only accepted ASCII, control characters aren't visible to the user.
This uses the unicode-width crate as an alternative to POSIX wcwidth, to
determine (hopefully) the number of fixed-width cells that a unicode
character will take up on a terminal. For example, control characters are
zero-width, while an emoji is likely double-width. See test cases for more
information on that.
There is also the unicode-segmentation crate, which can handle extended
grapheme clusters and such, but (a) we'll be outputting the line to the
terminal and (b) there's no guarantee that the user's editor displays
grapheme clusters as a single column. LSP measures in UTF-16,
apparently. I use both Emacs and Vim from a terminal, so unicode-width
applies to me. There's too much variation to try to solve that right now.
The columns can be considered a visual span---this gives us enough
information to draw line annotations, which will happen soon.
Here are some useful links:
- https://hsivonen.fi/string-length/
- https://unicode.org/reports/tr29/
- https://github.com/rust-analyzer/rowan/issues/17
- https://www.reddit.com/r/rust/comments/gpw2ra/how_is_the_rust_compiler_able_to_tell_the_visible/
DEV-10935
This does not yet resolve columns, and omits the length of the span, but
it's starting to come together.
This is particularly exciting for me to see because I've been wanting line
numbers in TAME error messages for over a decade.
DEV-10935
This does adds support for rewinding the underlying buffer when necessary to
read a span that occurs earlier within the same context (which could also
include the same span read twice).
As part of this change, I cleaned up the code a bit. Working with this
system can be confusing with the different meanings of the byte offsets and
the different ways of interpreting lines relative to the span that is
provided. There's not a lot of code here, but it represents a lot of work
to get right.
This works, but it's ugly and requires some cleanup. It shows that there
are some interesting considerations when determining how to best represent
the location of spans to the user in a way that is intuitive.
This is not yet integrated with the reporter, which will require a layer to
load a `Context` from disk.
DEV-10935
This is a POC, minimal-effort integration that also creates the TamecError
sum type analogous to TameldError.
I'll work on reducing the boilerplate in the future.
A note regarding the type and boilerplate vs. dynamic dispatch, for any
future readers: the purpose of this is to be explicit about the error types
so that the system is self-documenting and it forces and understanding of
its error conditions. `Box<dyn Error>` is basically "eh idk anything can
happen!", which is not what I'm interested in having.
DEV-10935
This is a working concept that will continue to evolve. I wanted to start
with some basic output before getting too carried away, since there's a lot
of potential here.
This is heavily influenced by Rust's helpful diagnostic messages, but will
take some time to realize a lot of the things that Rust does. The next step
will be to resolve line and column numbers, and then possibly include
snippets and underline spans, placing the labels alongside them. I need to
balance this work with everything else I have going on.
This is a large commit, but it converts the existing Error Display impls
into Diagnostic. This separation is a bit verbose, so I'll see how this
ends up evolving.
Diagnostics are tied to Error at the moment, but I imagine in the future
that any object would be able to describe itself, error or not, which would
be useful in the future both for the Summary Page and for query
functionality, to help developers understand the systems they are writing
using TAME.
Output is integrated into tameld only in this commit; I'll add tamec
next. Examples of what this outputs are available in the test cases in this
commit.
DEV-10935
We can just use PathSymbolId directly and simplify things. Typing can (and
should) happen on the symbol itself, and if we want a separate symbol type,
it ought to have its own interner.
For now, it doesn't, and having this extra type is just a PITA.
DEV-10935
There's no use in complicating the error handling here when we'd just
default to `UNKNOWN_SPAN` anyway when trying to render it. `UNKNOWN_SPAN`
didn't exist at the time of writing.
DEV-10935
This entirely removes the old XmloReader that has since been replaced with a
XIR-based reader.
I had been holding off on this because the new reader is slower, pending
performance optimizations (which I'll do a little later on), however the
performance loss is of no practical consideration and only affects the
linker, which is still fast.
Therefore, it's better to get this old code out of the way to simplify
refactoring going forward. In particular, I'm working on the diagnostic
system.
This is a little sad, in a way---this is some of my first Rust code that I'm
deleting.
DEV-10935
This does not deal directly with XIRF (that's composed into a pipeline
outside of this parser).
I'd like to clean up further...perhaps I should retire the
wip-xmlo-xir-reader flag now, despite the minor performance regression (see
previous recent commits for explanation).
DEV-10935
This aggregates all non-panic errors that can occur during link time, making
`Box<dyn Error>` unnecessary. I've been wanting to do this for a long time,
so it's nice seeing this come together. This is a powerful tool, in that we
know, at compile time, all errors that can occur, and properly report on
them and compose them. This method of error composition ensures that all
errors have a chance to be handled within their context, though it'll take
time to do so in a decent way.
This just maintains compatibility with the dynamic dispatch that was
previous occurring. This work is being done to introduce the initial
diagnostic system, which was really difficult/confusing to do without proper
errors types at the top level, considering the toplevel is responsible for
triggering the diagnostic reporting.
The cycle error is in particular going to be interesting once the system is
in place, especially once it provides spans in the future, since it will
guide the user through the code to understand how the cycle formed.
More to come.
DEV-10935
tamec and tameld will now both introduce a `Context` to XIR, which will use
it to create spans.
Here's an example of an error, now that it's all working well together:
$ target/release/tameld --emit xmle -o /dev/null path/to/package.xmlo
error: invalid preproc:sym/@dim `9` at [/../path/to/package.xmlo offset 1175451-1175452]
A future task will make this human-readable by producing line and column
numbers, and perhaps even a snippet (if not now, then eventually).
It's exciting to see this coming together finally.
DEV-10934
There's a bit to unpack here. Some of the spans originate from quick-xml's
error handling, but in coming up with test cases to try to trigger errors, I
found that quick-xml is far too permissive in what it accepts, and
oughtright dangerous in some situations.
I feel like the writing is on the wall for quick-xml, but I'll probably wait
until replacing `xmlo` with a more efficient format before deciding whether
to use a different library or implement parsing ourselves. There's a lot of
factors to consider, and a library would have to not only be correct and
performant, but provide useful information for span generation.
But for now, I have other more important things to work on, like a
functioning compiler. So while quick-xml is around, I'll just have to do
the best I can to provide a correct parser with useful errors.
DEV-10934
This is a large change, and was a bit of a tedious one, given the
comprehensive tests.
This introduces proper offsets and lengths for spans, with the exception of
some quick-xml errors that still need proper mapping. Further, this still
uses `UNKNOWN_CONTEXT`, which will be resolved shortly.
This also introduces `SpanlessError`, which `Error` explicitly _does not_
implement `From<SpanlessError>` for---this forces the caller to provide a
span before the error is compatable with the return value, ensuring that
spans will actually be available rather than forgotten for errors. This is
important, given that errors are generally less tested than the happy path,
and errors are when users need us the most (so, need span information).
Further, I had to use pointer arithmetic in order to calculate many of the
spans, because quick-xml does not provide enough information. There's no
safety considerations here, and the comprehensive unit test will ensure
correct behavior if the implementation changes in the future.
I would like to introduce typed spans at some point---I made some
opinionated choices when it comes to what the spans ought to
represent. Specifically, whether to include the `<` or `>` with the open
span (depends), whether to include quotes with attribute values (no),
and some other details highlighted in the test cases. If we provide typed
spans, then we could, knowing the type of span, calculate other spans on
request, e.g. to include or omit quotes for attributes. Different such
spans may be useful in different situations when presenting information to
the user.
This also highlights gaps in the tokens emitted by XIR, such as whitespace
between attributes, the `=` between name and value, and so on. These are
important when it comes to code formatting, so that we can reliably
reconstruct the XML tree, but it's not important right now. I anticipate
future changes would allow the XIR reader to be configured (perhaps via
generics, like a strategy-type pattern) to optionally omit these tokens if
desired.
Anyway, more to come.
DEV-10934
When wip-frontends is on, this will parse the input file using XIR and then
immediately output it again. This makes the necessary changes to be able to
read every source file we have in our largest project, such that the output
is identical after having been formatted with `xmllint --format -` (there
are differences because e.g. whitespace between attributes is not yet
maintained).
This is performant too, with times remaining essentially identical despite
the additional work.
DEV-10413
This resolves the performance issues caused by Rust's failure to elide the
ElementStack (ArrayVec) memcpys on move.
Since XIRF is invoked tens of millions of times in some cases for larger
systems, prior to this change, failure to optimize away moves for XIRF
resulted in tens of millions of memcpys. This resulted in linking of one
program going from 1s -> ~15s. This change reduces it to ~2.5s with the
wip-xmlo-xir-reader flag on, with the extra time coming from elsewhere (the
subject of future changes).
In particular, this change introduces a new mutable reference to
`ParseState::parse_token`, which is a reference to a `Context` owned by the
caller (e.g. `Parser`). In the case of XIRF, this means that
`Parser<flat::State, _>` will own the `ElementStack`/`ArrayVec` instead of
`flat::State`; this allows the latter to remain pure and benefit from Rust's
move optimizations, without sacrificing the otherwise-pure implementation.
ParseStates that do not need a mutable context can use `NoContext` and
remain pure.
DEV-12024
This makes the necessary tweaks to have the entire linker work end-to-end
and produce a compatible xmle file (that is, identical except for
nondeterministic topological ordering). That's good, and finally that can
get off of my plate.
What's disappointing, and what I'll have more information on in future
commits, is how slow it is.
The linking of our largest package goes from ~1s -> ~15s with this
change. The reason is because of tens of millions of `memcpy` calls. Why?
The ParseState abstraction is pure and passes an owned `self` around, and
Parser replaces its own reference using this:
let result;
TransitionResult(Transition(self.state), result) =
take(&mut self.state).parse_token(tok);
Naively, this would store a copy of the old state in `result`, allocate a
new ParseState for `self.state`, pass the original or a copy to
`parse_token`, and then overwrite `self.state` with the new ParseState that
is returned once it is all over.
Of course, that'd be devastating. What we want to happen is for Rust to
realize that it can just pass a reference to `self.state` and perform no
copying at all.
For certain parsers, this is exactly what happens. Great!
But for XIRF, it we have this:
/// Stack of element [`QName`] and [`Span`] pairs,
/// representing the current level of nesting.
///
/// This storage is statically allocated,
/// allowing XIRF's parser to avoid memory allocation entirely.
type ElementStack<const MAX_DEPTH: usize> = ArrayVec<(QName, Span), MAX_DEPTH>;
/// XIRF document parser state.
///
/// This parser is a pushdown automaton that parses a single XML document.
#[derive(Debug, Default, PartialEq, Eq)]
pub enum State<const MAX_DEPTH: usize, SA = AttrParseState>
where
SA: FlatAttrParseState,
{
/// Document parsing has not yet begun.
#[default]
PreRoot,
/// Parsing nodes.
NodeExpected(ElementStack<MAX_DEPTH>),
/// Delegating to attribute parser.
AttrExpected(ElementStack<MAX_DEPTH>, SA),
/// End of document has been reached.
Done,
}
ParseState contains an ArrayVec, and its implementation details are causes
LLVM _not_ to elide the `memcpy`. And there's a lot of them.
Considering that ParseState is supposed to use only statically allocated
memory and be zero-copy, this is rather ironic.
Now, this _could_ be potentially fixed by not using ArrayVec; removing
it (and the corresponding checks for balanced tags) gets us down to
2s (which still needs improvement), but we can't have a core abstraction in
our system resting on a house of cards. What if the optimization changes
between releases and suddenly linking / building becomes shit slow? That's
too much of a risk.
Further, having to limit what abstractions we use just to appease the
compiler to optimize away moves is very restrictive.
The better option seems like to go back to what I used to do: pass around
`&mut self`. I had moved to an owned `self` to force consideration of _all_
state transitions, but I can try to do the same thing in a different type of
way using mutable references, and then we avoid this problem. The
abstraction isn't pure (in the functional sense) anymore, but it's safe and
isn't relying on delicate inlining and optimizer implementation details to
have a performant system.
More information to come.
DEV-10863
This concludes the bulk of the header parsing, though there are surely going
to be other issues when I try to read a real xmlo file, such as
whitespace. That is something I expect that I'd rather handle as part of
XIRF, but maybe I'll initially ignore it here just to get it working. We'll
see.
DEV-10863
This parses the symbol dependency list (adjacency list).
I'm noticing some glaring issues in error handling, particularly that the
token being parsed while an error occurs is not returned and so recovery is
impossible. I'll have to address that later on, after I get this parser
completed.
Another previous question that I had a hard time answering in prior months
was how I was going to compose boilerplate parsers, e.g. handling the
parsing of single-attribute elements and such. A pattern is clearly taking
shape, and with the composition of parsers more formalized, that'll be able
to be abstracted away. But again, that's going to wait until after this
parser is actually functioning. Too many delays so far.
DEV-10863
Ideally this would just be an attribute, but I guess I never got around to
making that change in the compiler and I don't want a detour right now.
DEV-10863
I clearly was not paying attention to what was correct behavior here, since
the tests also verified the wrong behavior: rather than taking the last
processed attribute span, we should be taking the span of the opening
tag for the `preproc:sym` node.
DEV-10863
This simply removes boilerplate.
This will receive concrete examples once I come up with docs for the entire
module; there's boilerplate involved in testing and documenting this in
isolation and the time investment is not worth it yet until I'm certain that
this will not be changed.
DEV-10863
This integrates much of the work done so far to parse into a
`XmloEvent::SymDecl`. The attribute parsing _is_ verbose, and I do intend
to abstract it away later on, but I'm going to wait on that for now.
The new reader should be finishing up soon, which is really exciting, since
I started working on this months ago (before having to take a break on
TAMER); I'm anticipating strong performance gains in the reader, and this is
a test that will tell us how the compiler will perform moving forward with
the abstractions that I've spent so much time on.
DEV-10863
This introduces a new method similar to the previous `delegate`, but with
another closure that allows for handling lookahead tokens from the child
parser.
Admittedly, this isn't exactly what I was going for---a list of arguments
isn't exactly self-documenting, especially with the brevity when the
arguments line up---but this was easy to do and so I'll run with this for
now.
This also modified `delegate` to accept a context, even though it wasn't
necessary, both for consistency with its lookup counterpart and for brevity
with the `into` argument (allowing, in our case, to just pass the name of
the variant, rather than a closure).
I'm not going to handle the actual starting and accepting state stitching
abstraction for now; I'd like to observe future boilerplate more before I
consider the best way to handle it, though I do have some ideas.
DEV-10863
This is the delegation portion of what I've come to call "state
stitching"---wiring together two state machines that recognize the same
input tokens.
This handles the delegation of tokens once the parser has been entered, but
does not yet handle the actual stitching part of it: wiring the start and
accepting states of the child parser to the parent.
This is indirectly tested by the XmloReader, but it will receive its own
tests once I further finalize this concept. I'm playing around with some
ideas. With that said, a quick visual inspection together with the
guarantees provided by the type system should convince any familiar reader
of its correctness.
DEV-10863
This wasn't the simplest thing to start with, but I wanted to explore
something with a higher level of complexity. There is some boilerplate to
observe here, including:
1. The state stitching (as I guess I'm calling it now) of SymtableState
with XmloReaderState is all boilerplate and requires no lookahead,
presenting an abstraction opportunity that I was holding off on
previously (attr parsing for XIRF requires lookahead).
2. This is simply collecting attributes into a struct. This can be
abstracted away in the future.
3. Creating stub parsers to verify that generics are stitched rather than
being tightly coupled with another state is boilerplate that maybe can
be abstracted away after a pattern is observed in future tests.
DEV-10863
This does some cleanup and adds `parse::Object` for use in disambiguating
`From` for `ParseStatus`, allowing the `Transition` API to be much more
flexible in the data it accepts and automatically converts. This allows us
to concisely provide raw output data to be wrapped, or provide `ParseStatus`
directly when more convenient.
There aren't yet examples in the docs; I'll do so once I make sure this API
is actually utilized as intended.
DEV-10863
This replaces u8 and will be used for the new XmloReader.
Previously I wasn't sure what direction TAMER was going to go in with
regards to dimensionality, but I do not expect that higher dimensions will
be supported, and if they are, they'd very likely compile down to lower ones
and create an illusion of higher-dimensionality.
Whatever the future holds, it's not used today, and I'd rather these types
be correct.
ASG needs changing too, but one step at a time.
DEV-10863
This converts the tuple type alias into a newtype, so that we may provide
our own implementations.
This differs from a previous approach that I took, which involved making
this type `Result<(S, T), (S, E)>` so that the return values composed well
with other functions. But the reality is that this is used only by other
`ParseState`s and `Parser`, so it's unnecessary.
However, this is also an attempt to utilize the new Try and FromResidual
traits; note how the Try associated types match precisely what I was trying
to do before, though they're used as intermediate types. I'll see how this
evolves.
DEV-10863
This allows the Results to compose and, importantly, is compatible with
`?` without having to put in any extra effort.
This makes puts the caller in an awkward spot, so I introduced a utility
function `result_tup0_invert` for now; we'll see if that stays or evolves
differently.
DEV-10863
Since this is the object produced by this parser, this is likely the most
useful first thing to present as a summary of what `XmloReader` actually
does.
DEV-10863
This removes the flag from most of the code, which also resolves the
indentation. Not only was it bothering me, but I don't want (a) every line
modified when the module body is hoisted and (b) `rustfmt` to reformat
everything when that happens.
This means that everything will be built, even though it's not used, when
the flag is off, but I see that as a good thing.
DEV-10863
Finally we get to do some actual parsing with all of the preparatory work!
This means that we're finally ready to fully replace the old XmloReader,
provided that I'm okay with some boilerplate / lack of abstractions for
now (and I am, because all I've been doing is working on abstractions to
prepare lowering operations).
DEV-10863
This makes more sense for pattern matching. Encapsulation of these fields
is not necessary, given that it's passed around as an owned value and its
`new` method constructs it verbatim; the individual fields are
self-validating.
DEV-10863
This introduces a WIP lowering operation, abstracting away quite a bit of
the manual wiring work, which is really important to providing an API that
provides the proper level of abstraction for actually understanding what the
system is doing.
This does not yet have tests associated with it---I had started, but it's a
lot of work and boilerplate for something that is going to
evolve. Generally, I wouldn't use that as an excuse, but the robust type
definitions in play, combined with the tiny amount of actual logic, provide
a pretty high level of confidence. It's very difficult to wire these types
together and produce something incorrect without doing something obviously
bad.
Similarly, I'm holding off on proper docs too, though I did write some
information here.
More to come, after I actually get to work on the XmloReader.
On a side note: I'm happy to have made progress on this, since this wiring
is something I've been dreading and wondering about since before the Parser
abstraction even existed.
Note also that this makes parser::feed_toks private again---I don't intend
to support push parsers yet, since they're only needed internally. Maybe
for error recovery, but I'll wait to decide until it's actually needed.
DEV-10863
This begins to transition XmloReader into a ParseState. Unlike previous
changes where ParseStates were composed into a single ParseState, this is
instead a lowering operation that will take the output of one Parser and
provide it to another.
The mess in ld::poc (...which still needs to be refactored and removed)
shows the concept, which will be abstracted away. This won't actually get
to the ASG in order to test that that this works with the
wip-xmlo-xir-reader flag on (development hasn't gotten that far yet), but
since it type-checks, it should conceptually work.
Wiring lowering operations together is something that I've been dreading for
months, but my approach of only abstracting after-the-fact has helped to
guide a sane approach for this. For some definition of "sane".
It's also worth noting that AsgBuilder will too become a ParseState
implemented as another lowering operation, so:
XIR -> XIRF -> XMLO -> ASG
These steps will all be streaming, with iteration happening only at the
topmost level. For this reason, it's important that ASG not be responsible
for doing that pull, and further we should propagate Parsed::Incomplete
rather than filtering it out and looping an indeterminate number of times
outside of the toplevel.
One final note: the choice of 64 for the maximum depth is entirely
arbitrary and should be more than generous; it'll be finalized at some point
in the future once I actually evaluate what maximum depth is reasonable
based on how the system is used, with some added growing room.
DEV-10863
This introduces a (still-private) way to _push_ tokens into the parser,
rather than relying purely on a pull-based interface. Not only does this
simplify the iterator, but this is also preparing to make the new `feed_tok`
public so that parsers can be composed in more contexts. I suspect that
this method may also be useful for error recovery, since it can be used to
inject tokens into arbitrary points of a token stream.
I kept the new method private for now so that I can introduce the new API
and docs separate from this refactoring.
DEV-10863
The parsing framework originally created for XIR is now more general and
useful to other things. We'll see how this evolves.
This needs additional documentation, but I'd like to see how it changes as
I implement XmloReader and then some of the source readers first.
DEV-10863
This adds a `Token` type to `ParseState`. Everything uses `xir::Token`
currently, but `XmloReader` will use `xir::flat::Object`.
Now that this has been generalized beyond XIR, the parser ought to be
hoisted up a level.
DEV-10863
This does a couple of things: it ensures that documents one and only one
root note, and it properly handles dead transitions once parsing is
complete (allowing it to be composed).
This should make XIRF feature-complete for the time being. It does rely on
the assumption that the reader is stripping out any trailing whitespace, so
I guess we'll see if that's true as we proceed.
DEV-10863
I'm not rendering errors yet in practice, so this wouldn't have been
noticed, but we want error messages to reference the final byte in a file on
EOF, not the offset of the last-encountered token, which would be confusing.
This doesn't _directly_ pertain to what I'm working on; I just happened to
notice it.
DEV-10863
XIRF introduced the concept of `Transition` to help document code and
provide mental synchronization points that make it easier to reason about
the system. I decided to hoist this into XIR's parser itself, and have
`parse_token` accept an owned state and require a new state to be returned,
utilizing `Transition`.
Together with the convenience methods introduced on `Transition` itself,
this produces much clearer code, as is evidenced by tree::Stack (XIRT's
parser). Passing an owned state is something that I had wanted to do
originally, but I thought it'd lead to more concise code to use a mutable
reference. Unfortunately, that concision lead to code that was much more
difficult than necessary to understand, and ended up having a net negative
benefit by leading to some more boilerplate for the nested types (granted,
that could have been alleviated in other ways).
This also opens up the possibility to do something that I wasn't able to
before, which was continue to abstract away parser composition by stitching
their state machines together. I don't know if this'll be done immediately,
but because the actual parsing operations are now able to compose
functionally without mutability getting the way, the previous state coupling
issues with the parent parser go away.
DEV-10863
This introduces XIR Flat (XIRF), which is conceptually between XIR and
XIRT. This provides a more appropriate level of abstraction for further
lowering operations to parse against, and removes the need for other parsers
to perform their own validations (inappropriately) to ensure well-formed
XML.
There is still some cleanup worth doing, including moving some of the
parsing responsibility up a level back into the XIR parser.
DEV-10863
This behavior is unchanged, but it allows us to create more constant spans
for testing. For example:
const S = DUMMY_SPAN.offset_add(1).unwrap();
This, in turn, will allow for removing lazy_static! for tests that use it
for span generation.
DEV-10863
Petgraph was previously held back due to petgraph-graphml. I'd like to
transition away from that at some point, given that it's tied to petgraph
and also pulls in xmlns, on top of quick-xml and our XIR, but that can come
down the line.
The Options here are awkward and will be able to go away in the new reader
and in AsgBuilder once it has a proper state machine.
This gets rid of some of the initial migratory work for the new reader,
because PackageAttrs is gone. I'm going to wait to update this to the new
way until I get further into this.
DEV-11449
I'm finally back to TAMER development.
The original plan, some time ago, was to gate an entirely new XmloReader
behind a feature flag (wip-xmlo-xir-reader), and go from there, leaving the
existing implementation untouched. Unfortunately, it became too difficult
and confusing to marry the old aggregate API with the new streaming one.
AsgBuilder is the only system interacting with XmloReader, so I decided (see
previous commits) to just go the route of refactoring the existing
one. I'm not yet sure if I'll continue to progressively refactor this one
and eliminate the two separate implementations behind the flag, or if I'll
get this API similar and then keep the flag and reimplement it. But I'll
know soon.
DEV-11449
This is simply not worth it; the size is not going to be the bottleneck (at
least any time soon) and the generic not only pollutes all the things that
will use ASG in the near future, but is also incompatible with the SymbolId
default that is used everywhere; if we have to force it to 32 bits anyway,
then we may as well just default it right off the bat.
I thought that this seemed like a good idea at the time, and saving bits is
certainly tempting, but it was premature.
It's a bit odd that I've done next to nothing with TAMER for the past week
or so, and decided to do this one small thing before I go on break for the
holidays, but I felt compelled to do _something_. Besides, this gets me in
a better spot for the inevitable mental planning and writing I'll be doing
over the holidays.
This move was natural, given what this has evolved into---it has nothing to
do with the concept of a "tree", and the modules imports emphasized that
fact given the level of inappropriate nesting.
Now that the parser has been simplified by removing attributes, we can
further simplify the state transitions to make it more clear what further
refactoring can be done.
DEV-11339
More information can be found in the prior commit message, but I'll
summarize here.
This token was introduced to create a LL(0) parser---no tokens of
lookahead. This allowed the underlying TokenStream to be freely passed to
the next system that needed it.
Since then, Parser and ParseState were introduced, along with
ParseStatus::Dead, which introduces the concept of lookahead for a single
token---an LL(1) grammar.
I had always suspected that this would happen, given the awkwardness of
AttrEnd; it was just a matter of time before the right abstraction
manifested itself to handle lookahead.
DEV-11339
Note that AttrParse{r=>}State needs renaming, and Stack will get a better
name down the line too. This commit message is accurate, but confusing.
This performs the long-awaited task of trying to observe, concretely, how to
combine two automata. This has the effect of stitching together the state
machines, such that the union of the two is equivalent to the original
monolith.
The next step will be to abstract this away.
There are some important things to note here. First, this introduces a new
"dead" state concept, where here a dead state is defined as an _accepting_
state that has no state transitions for the given input token. This is more
strict than a dead state as defined in, for example, the Dragon Book, where
backtracking may occur.
The reason I chose for a Dead state to be accepting is simple: it represents
a lookahead situation. It says, "I don't know what this token is, but I've
done my job, so it may be useful in a parent context". The "I've done my
job" part is only applicable in an accepting state.
If the parser is _not_ in an accepting state, then an unknown token is
simply an error; we should _not_ try to backtrack or anything of the sort,
because we want only a single token of lookahead.
The reason this was done is because it's otherwise difficult to compose the
two parsers without requiring that AttrEnd exist in every XIR stream; this
has always been an awkward delimiter that was introduced to make the parser
LL(0), but I tried to compromise by saying that it was optional. Of course,
I knew that decision caused awkward inconsistencies, I had just hoped that
those inconsistencies wouldn't manifest in practical issues.
Well, now it did, and the benefits of AttrEnd that we had in the previous
construction do not exist in this one. Consequently, it makes more sense to
simply go from LL(0) to LL(1), which makes AttrEnd unnecessary, and a future
commit will remove it entirely.
All of this information will be documented, but I want to get further in
the implementation first to make sure I don't change course again and
therefore waste my time on docs.
DEV-11268
These were missed from a couple of commits ago, after I recalled that I
could now simplify the Stack variants; they were made more complicated due
to isolated attribute parsing.
These progressive refactorings do a good job illustrating why composing
parsers is better than a monolith---the complexity of the parsers is
significantly reduced, and the number of combinations of states are also
greatly reduced, which allows us to reason about them in isolation.
DEV-11268
This was added only for isolated attribute parsing. Of course, this does
mean that a new union type will be needed when combining the two parsers,
depending on the desired resolution, but that'll come at a later time and
possibly in a more general way.
DEV-11268
Mike Gerwitz