build(deps): bump js-yaml from 3.10.0 to 3.13.1 in /progtest #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dependabot
bot
force-pushed
the
dependabot/npm_and_yarn/progtest/js-yaml-3.13.1
branch
2 times, most recently
from
519889d to
62e1f92
Compare
mikegerwitz
pushed a commit
that referenced
this pull request
Apr 9, 2022
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 5a816a4, 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/
mikegerwitz
pushed a commit
that referenced
this pull request
Nov 11, 2022
This exposes the internal rendering of `ListDisplayWrapper::fmt` such that we can output a list without actually creating a list. This is used in an upcoming change for =ele_parse!= so that Sum NTs can render the union of all the QNames that their constituent NTs match on, recursively, as a single list, without having to create an ephemeral collection only for display. If Rust supports const functions for arrays/Vecs in the future, we could generate this at compile-time, if we were okay with the (small) cost, but this solution seems just fine. But output may be even _more_ performant since they'd all be adjacent in memory. This is used in these secenarios: 1. Diagnostic messages; 2. Error messages (overlaps with #1); and 3. `Display::fmt` of the `ParseState`s themselves. The reason that we want this to be reasonably performant is because #3 results in a _lot_ of output---easily GiB of output depending on what is being traced. Adding heap allocations to this would make it even slower, since a description is generated for each individual trace. Anyway, this is a fairly simple solution, albeit a little bit less clear, and only came after I had tried a number of other different approaches related to recursively constructing QName lists at compile time; they weren't worth the effort when this was so easy to do. DEV-7145
mikegerwitz
pushed a commit
that referenced
this pull request
Nov 11, 2022
This includes when on the last state / expecting a close.
Previously, there were a couple major issues:
1. After parsing an NT, we can't allow preemption because we must emit a
dead state so that we can remove the NT from the stack, otherwise
they'll never close (until the parent does) and that results in
unbounded stack growth for a lot of siblings. Therefore, we cannot
preempt on `Text`, which causes the NT to receive it, emit a dead
state, transition away from the NT, and not accept another NT of the
same type after `Text`.
2. When encountering an unknown element, the error message stated that a
closing tag was expected rather than one of the elements accepted by the
final NT.
For #1, this was solved by allowing the parent to transition back to the NT
if it would have been matched by the previous NT. A future change may
therefore allow us to remove repetition handling entirely and allow the
parent to deal with it (maybe).
For #2, the trouble is with the parser generator macro---we don't have a
good way of knowing the last NT, and the last NT may not even exist if none
was provided. This solution is a compromise, after having tried and failed
at many others; I desperately need to move on, and this results in the
correct behavior and doesn't sacrifice performance. But it can be done
better in the future.
It's also worth noting for #2 that the behavior isn't _entirely_ desirable,
but in practice it is mostly correct. Specifically, if we encounter an
unknown token, we're going to blow through all NTs until the last one, which
will be forced to handle it. After that, we cannot return to a previous NT,
and so we've forefitted the ability to parse anything that came before it.
NIR's grammar is such that sequences are rare and, if present, there's
really only ever two NTs, and so this awkward behavior will rarely cause
practical issues. With that said, it ought to be improved in the future,
but let's wait to see if other parts of the lowering pipeline provide more
appropriate places to handle some of these things (even though it really
ought to be handled at the grammar level).
But I'm well out of time to spend on this. I have to move on.
DEV-7145
Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 3.10.0 to 3.13.1. - [Release notes](https://github.com/nodeca/js-yaml/releases) - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](nodeca/js-yaml@3.10.0...3.13.1) Signed-off-by: dependabot[bot] <[email protected]>
dependabot
bot
force-pushed
the
dependabot/npm_and_yarn/progtest/js-yaml-3.13.1
branch
from
62e1f92 to
262977b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bumps js-yaml from 3.10.0 to 3.13.1.
Changelog
Sourced from js-yaml's changelog.
Commits
665aadd3.13.1 releasedda8ecf2Browser files rebuildb2f9e88Merge pull request #480 from nodeca/toStringe18afbfFix possible code execution in (already unsafe) load()9d4ce5e3.13.0 releasedf64c673Browser files rebuilda567ef3Restrict data types for object keys59b6e76Fix test namee4267fc3.12.2 released7231a49Browser files rebuildDependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)@dependabot use these labelswill set the current labels as the default for future PRs for this repo and language@dependabot use these reviewerswill set the current reviewers as the default for future PRs for this repo and language@dependabot use these assigneeswill set the current assignees as the default for future PRs for this repo and language@dependabot use this milestonewill set the current milestone as the default for future PRs for this repo and languageYou can disable automated security fix PRs for this repo from the Security Alerts page.