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 was broken by the previous fix, because I had cast to a numeric value
before invoking `set_defaults`, which needs the empty string retained so
that it knows whether a default ought to be applied.
This also ensures that `set_values` will always return a numeric value when
that default is applied.
DEV-10484
This has been a problem for...ever, but the old classification system (and
calculations) had `||0` for ever variable reference, whereas the new one
does not; NaNs result in undefined behavior in the new classification
system, since those values are not expected to exist.
This ought to have automated tests, but it will be rewritten in TAMER.
DEV-10484
This was originally my plan with the new classification system, but it was
undone because I had hoped to punt on the somewhat controversial
issue. Unfortunately, I see no other way. Here I attempt to summarize the
reasons why, many of which are specific to the design decisions of TAME.
Keep in mind that TAME is a domain-specific language (DSL) for writing
insurance rating systems. It should act intuitively for our use case, while
still being mathematically sound.
If you still aren't convinced, please see the link at the bottom.
Target Language Semantics (ECMAScript)
--------------------------------------
First: let's establish what happens today. TAME compiles into ECMAScript,
which uses IEEE 754-2008 floating-point arithmetic. Here we have:
x/0 = Infinity, x > 0;
x/0 = -Infinity, x < 0;
0/0 = NaN, x = 0.
This is immediately problematic: TAME's calculations must produce concrete
real numbers, always. NaN is not valid in its domain, and Infinity is of no
practical use in our computational model (TAME is build for insurance rating
systems, and one will never have infinite premium). Put plainly: the
behavior is undefined in TAME when any of these values are yielded by an
expression.
Furthermore, we have _three different possible situations_ depending on
whether the numerator is positive, negative, or zero. This makes it more
difficult to reason about the behavior of the system, for values we do not
want in the first place.
We then have these issues in ECMAScript:
Infinity * 0 = NaN.
-Infinity * 0 = NaN.
NaN * 0 = NaN.
These are of particular concern because of how predicates work in TAME,
which will be discussed further below. But it is also problematic because
of how it propagates: once you have NaN, you'll always have NaN, unless you
break out of the situation with some control structure that avoids using it
in an expression at all.
Let's now consider predicates:
NaN > 0 = false.
NaN < 0 = false.
NaN === 0 = false.
NaN === NaN = false.
These will be discussed in terms of classification predicates (matches).
We also have issues of serialization:
JSON.stringify(Infinity) = "null".
JSON.stringify(NaN) = "null".
These means that these values are difficult to transfer between systems,
even if we wanted them.
TAME's Predicates
-----------------
TAME has a classification system based on first-order logic, where ⊥ is
represented by 0 and ⊤ is represented by 1. These classifications are used
as predicates to calculations via the @class attribute of a rate block. For
example:
<rate-each class="property" generates="propValue" index="k">
<c:quotient>
<c:value-of name="buildingTiv" index="k" />
<c:value-of name="tivPropDivisor" index="k" />
</c:quotient>
</rate>
As can be observed via the Summary Page, this calculation compiles into the
following mathematical expression:
∑ₖ(pₖ(tₖ/dₖ)),
that is—the quotient is then multiplied by the value of the `property`
classification, which is a 0 or 1 respectively for that index.
Let's say that tivPropDivisor were defined in this way:
<rate-each class="property" generates="tivPropDivisor" index="k">
<!--- ... logic here ... -->
</rate>
It does not matter what the logic here is. Observe that the predicate here
is `property` as well, which means that, if this risk is not a property
risk, then `tivPropDivisor` will be `0`.
Looking back at `propValue`, let's say that we do have a property risk, and
that `buildingTiv` is `[100_000, 200_000]` and `tivPropDivisor` is 1000. We
then have:
1(100,000 / 1000) + 1(200,000 / 1000)) = 300.
Consider instead what happens if `property` is 0. Since we have no property
locations, we have `[0, 0]` as `buildingTiv` and `tivPropDivisor` is 0.
0(0/0) + 0(0/0)) = 0(NaN + NaN) = NaN.
This is clearly not what was intended. The predicate is expected to be
_strongly_ zero, as if using an Iverson bracket:
((0/0)[0] + (0/0)[0]) = 0.
Of course, one option is to redefine TAME such that we use Iverson's
convention in place of summation, however this is neither necessary nor
desirable given that
(a) NaN is not valid within the domain of any TAME expression, and
(b) Summation is elegantly generalized and efficiently computed using
vector arithmetic and SIMD functions.
That is: there's no use in messing with TAME's computational model for a
valid that should be impossible to represent.
Short-Circuiting Computation
----------------------------
There's another way to look at it, though: that we intended to skip the
computation entirely, and so it doesn't matter what the quotient is. If the
compiler were smart enough (and maybe one day it will be), it would know
that the predicate of `tivPropDivisor` and `propValue` are the same and so
there is no circumstance under which we would compute `propValue` and have
`tivPropDivisor` be 0.
The problem is: that short-circuiting is employed as an _optimization_, and
is an implementation detail. Mathematically, the expression is unchanged,
and is still invalid within TAME's domain. It is unrepresentable, and so
this is not an out.
But let's pretend that it was defined that way, which would yield this:
{ ∑ₖ(pₖ(tₖ/dₖ)), ∀x∈p(x = 1);
propValue = <
{ 0, otherwise.
This is the optimization that is employed, but it's still not mathematically
correct! What happens if p₀ = 1, but p₁ = 0? Then we have:
1(100,000/1000) + 0(0/0) = 100 + NaN = NaN,
but the _intent_ was clearly to have 100 + 0 = 100, and so we return to the
original problem once again.
Classification Predicates and Intent
------------------------------------
Classifications are used as predicates for equations, but classifications
_themselves_ have predicates in the form of _matches_. Consider, for
example, a classification that may be used in an assertion to prevent
negative premium from being generated:
<t:assert failure="premBuilding must not be negative for any index">
<t:match-gte value="premBuilding" value="#0" />
</t:assert>
Simple enough—the system will fail if the premium for a given building is
below $0.
But what happens if premBuilding is calculated as so?
<rate-each class="property" yields="premBuildingTotal"
generates="premBuilding" index="k">
<c:product>
<c:value-of name="propValue" index="k" />
<c:value-of name="propRate" index="k" />
</c:product>
</rate-each>
Alas, if `property` is false for any index, then we know that `propValue` is
NaN, and NaN * x = NaN, and so `premBuilding` is NaN.
The above assertion will compile the match into the first-order sentence
∀x∈b(x > 0).
Unfortunately, NaN is not greater than, less than, equal to, or any other
sort of thing to 0, and so _this assertion will trigger_. This causes
practical problems with the `_premium_` template, which has an
`@allow-zero@` argument to permit zero premium.
Consider this real-world case that I found (variables renamed), to avoid a
strawman:
<t:premium class="loc" round="cent"
yields="locInitialTotal"
generates="locInitial" index="k"
allow-zero="true"
desc="...">
<c:value-of name="premAdditional" />
<c:quotient>
<c:value-of name="premLoc" index="k" />
<c:value-of name="premTotal" />
</c:quotient>
</t:premium>
This appears to be responsible for splitting up `premAdditional` relative to
the total premium contribution of each location. It explicitly states that
it wants to permit a zero value. The intent of this block is clear: a value
of 0 is explicitly permitted and _expected_.
But if `premTotal` is for whatever reason 0—whether it be due to a test
case or some unexpected input—then it'll yield a NaN and make the entire
expression NaN. Or if `premAdditional` or `premLoc` are tainted by a NaN,
the same result will occur. The assertion will trigger. And, indeed, this
is what I'm seeing with test cases against the new classification system.
What about Infinity? Is it intuitive that, should `propValue` in the
previous example be positive and `propRate` be 0, that we would, rather than
producing a very small value, produce an infinitely large one? Does that
match intuition? Remember, this system is a domain-specific language for
_our_ purposes—it is not intended to be used to model infinities.
For example, say we had this submission because the premium exceeds our
authority to write with some carrier:
<t:submit reason="Premium exceeds authority">
<t:match-gt name="premBuilding" value="#100k" />
</t:submit>
If we had
(100,000 / 0) = ∞,
then this submit reason would trigger. Surely that was not intended, since
we have `property` as a predicate and `propRate` with the same predicate,
implying that the answer we _actually_ want is 0! In that case, what we
_probably_ want to trigger is something like
<rate yields="premFinal">
<t:maxreduce>
<c:value-of name="premBuildingTotal" />
<c:value-of name="#500" />
</t:maxreduce>
</rate>,
in order to apply a minimum premium of $500. But if `premBuildingTotal` is
Infinity, then you won't get that—you'll get Infinity, which is of course
nonsense.
And nevermind -Infinity.
Why Wasn't This a Problem Before?
---------------------------------
So why bring this up now? Why have we survived a decade without this?
We haven't, really—these bugs have been hidden. But the old classification
system covered them up; predicates would implicitly treat missing values as
0 by enclosing them in `(x||0)` in the compiled code. Observe this
ECMAScript code:
NaN || 0 = 0.
Consequently, the old classification system absorbed bad values and treated
them implicitly as 0. But that was a bug, and had to be removed; it meant
that missing indexes in classifications would trigger predicates that were
not intended to be triggered, if they matched against 0, or matched against
a value less than some number larger than zero. (See
`core/test/core/class` for examples.)
The new classification system does not perform such defaulting. _But it
also does not expect to receive values outside of its valid domain._
Consequently, _NaN and Infinity lead to undefined behavior_, and the
current implementation causes the predicate to match (NaN < 0) and therefore
fail.
The reason for this is because that this implementation is intended to
convey precisely the computation necessary for the classification system, as
formally defined, so that it can be later optimized even further. Checking
for values outside the domain not only should not be necessary, but it would
prevent such future optimizations.
Furthermore, parameters used to compile into (param||0), to account for
missing values or empty strings. This changed somewhat recently with
5a816a4701, which pre-cast all inputs and
allowed relaxing many of those casts since they were both wasteful and no
longer necessary.
Given that, for all practical purposes, 0/0=0 in the system <1yr ago.
Infinity, of course, is a different story, since (Infinity||0)=Infinity;
this one has always been a problem.
Let's Just Fail
---------------
Okay, so we cannot have a valid expression, so let's just fail.
We could mean that in two different ways:
1. Fail at runtime if we divide by 0; or
2. Fail at compile-time if we _could_ divide by 0.
Both of these have their own challenges.
Let's dismiss #2 right off the bat for now, because until we have TAMER,
that's not really feasible. We need something today. We will discuss that
in the future.
For #1—we cannot just throw an error and halt computation, because if the
`canterm` flag passed into the system is `false`, then _computation must
proceed and return all results_. Terminating classifications are checked
after returning rather than throwing errors.
Since we have to proceed with computation, then the computations have to be
valid, and so we're left with the same problem again—we cannot have
undefined behavior.
One could argue that, okay, we have undefined behavior, but we're going to
fail because of the assertion anyway! That's potentially defensible, but it
is at the moment undesirable, because we get so many failures. And,
relative to the section below, it's not clear to me what benefit we get from
that behavior other than making things more difficult for ourselves.
Furthermore, such an assertion would have to be defined for every
calculation that performs a quotient, and would have to set some
intermediate flag in the calculation which would then have to be checked for
after-the-fact. This muddies the generated calculation, which causes
problems for optimizations, because it requires peering into state of the
calculation that may be hidden or optimized away.
If we decide that calculations must be valid because we cannot fail, and we
have to stick with the domain of calculations, then `x/0` must be
_something_ within that domain.
x/0=0 Makes Sense With the Current System
-----------------------------------------
Let's take a step back. Consider a developer who is unaware that
NaN/Infinity are permitted in the system—they just know that division by
zero is a bad thing to do because that's what they learned, and they want to
avoid it in their code.
Consider that they started with this:
<rate-each class="property" generates="propValue" index="k">
<c:quotient>
<c:value-of name="buildingTiv" index="k" />
<c:value-of name="tivPropDivisor" index="k" />
</c:quotient>
</rate>
They have inspected the output of `tivPropDivisor` and see that it is
sometimes 0. They understand that `property` is a predicate for the
calculation, and so reasonably think that they could do something like this:
<classify as="nonzero-tiv-prop-divisor" ...>
<t:match-ne on="tivPropDivisor" value="#0" />
</classify>
and then change the rate-each to
<rate-each class="property nonzero-tiv-prop-divisor" ...>.
Except that, of course, we know that will have no effect, because a NaN is a
NaN. This is not intuitive.
So they'd have to do this:
<rate-each class="property" generates="propValue" index="k">
<c:cases>
<c:case>
<t:when-ne name="tivPropDivisor" value="#0" />
<c:quotient>
<c:value-of name="buildingTiv" index="k" />
<c:value-of name="tivPropDivisor" index="k" />
</c:quotient>
</c:case>
<c:otherwise>
<c:value-of name="#0" />
</c:otherwise>
</c:cases>
</rate>.
But for what purpose? What have we gained over simply having x/0=0, which
does this for you?
The reason why this is so unintuitive is because 0 is the default case in
every other part of the system. If something doesn't match a predicate, the
value becomes 0. If a value at an index is not defined, it is implicitly
zero. A non-matching predicate is 0.
This is exploited for reducing values using summation. So the behavior of
the system with regards to 0 is always on the mind of the developer. If we
add it in another spot, they would think nothing of it.
It would be nice if it acted as an identity in a monoidic operation,
e.g. as 0 for sums but as 1 for products, but that's not how the system
works at all today. And indeed such a thing could be introduced using a
special template in place of `c:value-of` that copies the predicates of the
referenced value and does the right thing.
The _danger_, of course, is that this is _not_ how the system as worked, and
so changing the behavior has the risk of breaking something that has relied
on undefined behavior for so long. This is indeed a risk, but I have taken
some confident in (a) all the test cases for our system pass despite a
significant number of x/0=0 being triggered due to limited inputs, and (b)
these situations are _not correct today_, resulting in `null` in serialized
result data because `JSON.stringify([NaN, Infinity]) === "[null, null]"`.
Given all of that, predictable incorrect behavior is better than undefined
behavior.
So x/0=0 Isn't Bad?
-------------------
No, and it's mathematically sound. This decision isn't unprecedented—
Coq, Lean, Agda, and other theorem provers define x/0=0. APL originally
defined x/0=1, but later switched to 0. Other languages do their own thing
depending on what is right for their particular situation.
Division is normally derived from
a × a⁻¹ = 1, a ≠ 0.
We're simply not using that definition—when we say "quotient", or use the
`/` symbol, we mean a _different_ function (`div`, in the compiled JS),
where we have an _additional_ axiom that
a / 0 = 0.
And, similarly,
0⁻¹ = 0.
So we've taken a _normally undefined_ case and given it a definition. No
inconsistency arises.
In fact, this makes _sense_ to do, because _this is what we want_. The
alternative, as mentioned above, is a lot of boilerplate—checking for 0 any
time we want to do division. Complicating the compiler to check for those
cases. And so on. It's easier to simple state that, in TAME, quotients
have this extra convenient feature whereby you don't have to worry about
your denominator being zero because it'll act as though you enclosed it in a
case statement, and because of that, all your code continues to operate in
an intuitive way.
I really recommend reading this blog post regarding the Lean theorem prover:
https://xenaproject.wordpress.com/2020/07/05/division-by-zero-in-type-theory-a-faq/
This is intended to be set via the configure script, and is being added
primarily for the upcoming flag to enable the legacy classification
system. This is only used for the XSLT-based compiler.
preproc:symtable-process-symbols is run on each pass (e.g. during initial
processing and after each template expansion) to introduce new symbols into
the symbol table from imports and newly discovered symbols.
This processing was previously optimized a bit using maps to reduce the cost
of symbol table lookups, but the processing was still inefficient, relying
on XSLT1-style processing (as originally written) for deduplication. This
now uses `for-each-group` and `perform-sort` to offload the expensive
computation onto Saxon, which is much more efficient.
Symbol table processing has long been a culprit, but I hadn't attempted to
optimize further in recent months because of TAMER work. Since TAMER has
been on pause for a few months with other things needing my attention, I
needed to provide a short-term performance improvement to keep up with
increasing build times.
DEV-11716
This provides logging that can be used to analyze jobs. See `tamed --help`
for some examples. More to come.
You'll notice that one of the examples reprents package build time in
_minutes_. This is why TAMER is necessary; as of the time of writing, the
longest-building package is nearly five and a half minutes, and there are a
number of packages that take a minute or more. But, there are potentially
other optimizations that can be done. And this is _after_ many rounds of
optimizations over the years. (TAME was not originally built for what it is
currently being used for.)
This is something that I've wanted to do for quite some time, but for good
reason, have been avoiding.
`tamed --report` is fairly basic right now, but allows you to see what each
of the runners are doing. This will be expanded further to gather data for
further analysis.
The thing that I was avoiding was a status line during the build to
summarize what the runners are doing, since it's nearly impossible to do so
from the build output with multiple runners. This will not only allow me to
debug more easily, but will keep the output plainly visible to developers at
all times in the hope that it can help them improve the build times
themselves in certain cases.
It is currently gated behind TAMED_TUI, since, while it works well overall,
it is imperfect, and will cause artifacts from build output partly
overwriting the status line, and may even occasionally clobber the PS1 by
erasing the line. This will be improved upon in the future; something is
better than nothing.
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
This nearly completely integrates the new Parser with xir::tree, but does
not yet compose AttrParseState. I also need to determine what to do with
`parse()` and, further, make `parser_from` generic as part of mod parse.
If we take a moment to reflect on all of the changes, this struggle has been
a roundabout way of converting tree's parser into parse::Parser; providing
a trait for Stack (as ParseState); beginning parser decomposition; and
moving some common logic into Parser. The composition of parsers is the
final piece to be realized.
This could have been a lot less work if I really understood exactly what I
wanted to do up front, but as was mentioned in previous commits, I was
really confusing myself trying to maintain API BC in ways that I should not
have for XmloReader. More on that will be coming soon as well.
DEV-11268
This will allow Parser to operate on both owned and &mut values, and is the
same approach that Rust's built-in iterators take.
This is at first quite surprising, and I often forget that this is a
feature, and, as a bonus, an attractive way to avoid lifetimes in struct
definitions when generics are used for the type that may become a
reference.
DEV-11268
This isn't currently used by anything, and this is collecting, which does
not fit well with the streaming model. AttrList was originally written for
Element parsing, and the isolated attr parser was written for test cases,
before it was fully decided how this system ought to work.
Instead, if AttrList is in fact needed, we can either collect (ideally not)
or implement Extend for AttrList. (Or create TryExtend.)
DEV-11268
This removes the layer of encapsulation that was hiding Stack, which is the
actual parser. The new layer of encapsulation is parse::Parser, which will
be introduced here soon. Baby steps, so it's clear how this evolves.
DEV-11268
The old Parsed was renamed to ParseStatus to be used by Parser, and Parser
converts it into Parsed, which has the same variants as it did before and
has all but the Done variant, since it's not possible for Parser to yield
it.
DEV-11268
This removes Option from ParseState, as mentioned in previous commits.
This is ideal because it not only removes a layer of abstraction, but also
makes the intent very clear; the use of None was too tied to the concept of
an Iterator, which is the concern of Parser, _not_ ParseState.
This is now similar to tree::Parsed, which will help with that refactoring
shortly.
The Done variant is not accessible outside of Parser, since it always
coverts it to None (to halt iteration); given that, we should have another
public-facing type, as was also mentioned in a previous commit.
DEV-11268
This also renames related types.
See previous commits for more in formation. In essence, this trait
represents the reification of all parser state. The omission of "r" in the
name ParseState is intentional, since it indicates the state of a current
parse. We'll see whether that naming ends up being too confusing; it's easy
enough to change.
DEV-11268
This just leaves Parser, which is what I started with, but I wasn't sure how
far I was going to take this. I went against my usual judgment in creating
a trait that I may not need, in an attempt to try to reason about the API
that I wanted, because it wasn't yet clear at the time whether the Parser
ought to be generic.
Since then (as detailed in the last commit), this has become more of a
coordinator/mediator, and the real parser is actually TokenStreamState,
which will be renamed shortly.
DEV-11268
This begins to integrate the isolated AttrParser. The next step will be
integrating it into the larger XIRT parser.
There's been considerable delay in getting this committed, because I went
through quite the struggle with myself trying to determine what balance I
want to strike between Rust's type system; convenience with parser
combinators; iterators; and various other abstractions. I ended up being
confounded by trying to maintain the current XmloReader abstraction, which
is fundamentally incompatible with the way the new parsing system
works (streaming iterators that do not collect or perform heap
allocations).
There'll be more information on this to come, but there are certain things
that will be changing.
There are a couple problems highlighted by this commit (not in code, but
conceptually):
1. Introducing Option here for the TokenParserState doesn't feel right, in
the sense that the abstraction is inappropriate. We should perhaps
introduce a new variant Parsed::Done or something to indicate intent,
rather than leaving the reader to have to read about what None actually
means.
2. This turns Parsed into more of a statement influencing control
flow/logic, and so should be encapsulated, with an external equivalent
of Parsed that omits variants that ought to remain encapsulated.
3. TokenStreamState is true, but these really are the actual parsers;
TokenStreamParser is more of a coordinator, and helps to abstract away
some of the common logic so lower-level parsers do not have to worry
about it. But calling it TokenStreamState is both a bit
confusing and is an understatement---it _does_ hold the state, but it
also holds the current parsing stack in its variants.
Another thing that is not yet entirely clear is whether this AttrParser
ought to care about detection of duplicate attributes, or if that should be
done in a separate parser, perhaps even at the XIR level. The same can be
said for checking for balanced tags. By pushing it to TokenStream in XIR,
we would get a guaranteed check regardless of what parsers are used, which
is attractive because it reduces the (almost certain-to-otherwise-occur)
risk that individual parsers will not sufficiently check for semantically
valid XML. But it does _potentially_ match error recovery more
complicated. But at the same time, perhaps more specific parsers ought not
care about recovery at that level.
Anyway, point being, more to come, but I am disappointed how much time I'm
spending considering parsing, given that there are so many things I need to
move onto. I just want this done right and in a way that feels like it's
working well with Rust while it's all in working memory, otherwise it's
going to be a significant effort to get back into.
DEV-11268
This stores the last seen Span and uses that when reporting EOF, so that the
user will be able to be notified of where exactly the problem occurred.
When I get into creating combinators, it'll be the responsibility of those
combinators to ensure that any None return value will be supplemented by its
own last span.
DEV-11268
This permits retrieving a Span from any Token variant. To support this,
rather than having this return an Option, Token::AttrEnd was augmented with
a Span; this results in a much simpler and friendlier API.
DEV-11268
This removes XIRT support for attribute fragments. The reason is that
because this is a write-only operation---fragments are used to concatenate
SymbolIds without reallocation, which can only happen if we are generating
XIR internally.
Given that this cannot happen during read, it was a mistake to complicate
the parsers. But it makes sense why I did originally, given that the XIRT
parser was written for simplifying test cases. But now that we want parsers
for real, and are writing production-quality parsers, this extra complexity
is very undesirable.
As a bonus, we also avoid any potential for heap allocations related to
attributes. Granted, they didn't _really_ exist to begin with, but it was
part of XIRT, and was ugly.
DEV-11268
The XIRT parser was initially written for test cases, so that unit tests
should assert more easily on generated token streams (XIR). While it was
planned, it wasn't clear what the eventual needs would be, which were
expected to differ. Indeed, loading everything into a generic tree
representation in memory is not appropriate---we should prefer streaming and
avoiding heap allocations when they’re not necessary, and we should parse
into an IR rather than a generic format, which ensures that the data follow
a proper grammar and are semantically valid.
When parsing attributes in an isolated context became necessary for the
aforementioned task, the state machine of the XIRT parser was modified to
accommodate. The opposite approach should have been taken---instead of
adding complexity and special cases to the parser, and from a complex parser
extracting a simple one (an attribute parser), we should be composing the
larger (full XIRT) parser from smaller ones (e.g. attribute, child
elements).
A combinator, when used in a functional sense, refers not to combinatory
logic but to the composition of more complex systems from smaller ones. The
changes made as part of this commit begin to work toward combinators, though
it's not necessarily evident yet (to you, the reader) how that'll work,
since the code for it hasn't yet been written; this is commit is simply
getting my work thusfar introduced so I can do some light refactoring before
continuing on it.
TAMER does not aim to introduce a parser combinator framework in its usual
sense---it favors, instead, striking a proper balance with Rust’s type
system that permits the convenience of combinators only in situations where
they are needed, to avoid having to write new parser
boilerplate. Specifically:
1. Rust’s type system should be used as combinators, so that parsers are
automatically constructed from the type definition.
2. Primitive parsers are written as explicit automata, not as primitive
combinators.
3. Parsing should directly produce IRs as a lowering operation below XIRT,
rather than producing XIRT itself. That is, target IRs should consume
XIRT and produce parse themselves immediately, during streaming.
In the future, if more combinators are needed, they will be added; maybe
this will eventually evolve into a more generic parser combinator framework
for TAME, but that is certainly a waste of time right now. And, to be
honest, I’m hoping that won’t be necessary.
There are a number of reasons for this, where the benefits do not make up
for the losses.
First: this is actually invoking cargo. Not only is this not necessary, but
it's not desirable: cargo by default hits the network and does all sorts of
other stuff, when all we want to do is invoke the executable. So the tests
aren't really testing the right thing in that sense. See the previous
commit for more information.
The way it invokes cargo is different than the way the Makefile invokes
cargo, so on my system, it's actually invoking a _different cargo_! This is
causing problems, in particular with lock files, which causes my tests to
fail.
Importantly, this also removes a _lot_ of dependencies, which removes a lot
of supplier chain risk and a lot of code to audit. This provides
significant security benefits, especially given that what was being tested
was rather small, and could be done in a shell script.
TAMER will receive significant system testing later on. But for now, none
of this was worth it.
Further audits of dependencies will come later on. I've always been fairly
insistent on keeping the dependency graph small and auditable, but recent
supply chain attacks have given me a better way to rationalize the security
risk. Further, I'm the only one on this project right now.
Mike Gerwitz