v0.19 native nullable types #1496
Labels
No labels
UX
active development
backlog
blocker
bootstrap
bounty
bug
dependencies
discussion
documentation
duplicate
enhancement
flaky test
help wanted
invalid
javascript
question
release
tendentious
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
mighty-gerbils/gerbil!1496
Loading…
Reference in a new issue
No description provided.
Delete branch "v0.19-nullable-types"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This has been pending for a long while, ever since the introduction of the :? type annotation sigils coz it was quite tricky and hairy... but not for astra. It is now a reality, the compiler now natively understands nullable types and does not do unsafe shit because you forgot to check a nullable reference, and you can use (Maybe T) to declare a nullable type return.
also updates the gambit pin which was broken and includes unwind-protect! and fixes a type bug, cherry picked ,from my wip branch on ensembles.
Implemented by astra with guidance and oversight from the gerbilosaurus rex.
I'll review manually, but in the mean time, here's what Sol says:
I would request changes, for one substantive correctness hole. Overall the nullable-type machinery itself looks well thought out: a distinct optimizer type, covariance/subtyping rules, SSXI preservation, conservative treatment of mutable bindings and pretty extensive tests. (cons.io git)
Blocking: nullable intermediate dotted slots lose their nullability. The new code correctly annotates a terminal dotted slot read such as
holder.itemas(nullable ...)when the slot contract contains:?. But when the same slot is an intermediate component,%%ref-dottedsimply resolves its underlying type and recurses on#'(accessor object);%%app-dottedand%%set-dotted!do the same thing. (cons.io git) Worse,get-slot-accessoris deliberately an unchecked accessor and can expand all the way down to##unchecked-structure-ref. (cons.io git)So I think this defeats the central safety property of the PR. Consider:
a.bmay be#f, but whilea.balone receives nullable metadata, ina.b.xthebcontract is forgotten andxis fetched from the result as though it were definitely aB. The tests cover terminal nullable slots (holder.item) but I don't see a nested nullable receiver case. (cons.io git)There is already syntax capable of making the intended nil check explicit (
a.b.?x, by the expansion logic). So I don't have a strong opinion whethera.b.xshould be rejected or automatically checked. But it absolutely should not silently turn:? Bback intoBand perform an unchecked access. The same regression tests should cover read, method/application and set! through an intermediate nullable slot.Minor:
optimizer-current-typesthrows away the base of a nullable type. Its generic!type?case returns(!type-id t), which for every!nullable-typeis simply'nullable. Thus(nullable Foo::t)and(nullable Bar::t)become indistinguishable in this introspection output. You already haveoptimizer-type-expression, which preserves exactly this structure. An!nullable-type?clause before the generic!type?clause seems appropriate. (cons.io git) I don't see this affecting optimization correctness, so I'd call it a diagnostic/debugging bug, not a blocker.The committed
nullable-types.mdneeds a serious haircut. It contains useful design documentation, but also/tmp/opencode/...logs, a/home/vyzo/...path, stash hashes, interrupted-work recovery history, statements such as “the user approved committing this follow-up”, whether a branch was pushed and other session archaeology. (cons.io git) I would retain the semantics, invariants, examples, test commands and perhaps concise verification results, and remove the AI/worktree diary. That stuff will be meaningless noise six months from now.One test-coverage note: the new
kqueuenullable declaration/test was not actually run or native-compiled because the work was performed on Linux; the PR itself records this. (cons.io git) Since this is mostly metadata rather than an implementation change I wouldn't necessarily block on it, but ideally BSD CI should exercise it before release.I found no problem with the exact-
Maybesignature comparison I initially suspected: that path is reconciling inherited interface declarations, not checking covariant implementation returns. Likewise, the nullable interface adaptation and the@type.returnmachinery appear deliberately conservative rather than unsound.So my verdict is: good underlying design, but I would not merge until the intermediate-dotted-access case is resolved. That one lands squarely on the PR's advertised guarantee that forgetting to check a nullable reference must not lead to unsafe code. (cons.io git)
great review by sol. For 1, we discussed with astra, and decided to emit the checked accessor for chains that include a nullable dereference.
addressed review findings 1-3, 4 is out of scope as i don't have a bsd box.
Maybe the clanker can cook an automatic job with qemu or something to check BSD once in a while?
Sol comments:
Yes. The update at
8dd21144a8fixes the issues I raised in the first pass, and the intermediate nullable receiver fix is real, not cosmetic. But I think there is still one blocking hole, closely related to the one just fixed.The good news first:
h.item.x,h.item.proc, setters throughh.item, deeper chains, structs, inherited slots and the system-class offset case now propagate the nullable status to the next receiver and select checked accessors/mutators. The added test is substantial and exercises exactly those cases. (cons.io git) Theoptimizer-current-typesproblem is also fixed, andnullable-types.mdhas been cleaned up into useful design documentation rather than a work-session transcript. (cons.io git)However, a nullable binding itself still seems to lose its nullability in the dotted macro environment.
type-envhas only(var type checked? super).with-classknowsmaybe?and correctly puts(nullable Type::t)into the optimizer annotation, but when it constructs thetype-env, it stores only the ordinary class pluschecked?;maybe?disappears. Note thatchecked?cannot substitute for it because both:and:?bindings are checked. (cons.io git)Then all three dotted walkers initialize the receiver as non-nullable:
They only set it to true after traversing a nullable slot. (cons.io git)
So I believe this is still unsafe:
Calling any of those with
#fpasses the:? Itementry contract, as intended, but the first dotted hop seeschecked-receiver? = #fand asksget-slot-accessorfor the unchecked accessor. For offset-based classes that literally produces##unchecked-structure-ref; ordinary unchecked accessors likewise stay unchecked. (cons.io git)And the optimizer doesn't repair this downstream: an unchecked
!accessorskips argument checking, and optimized final/struct accesses lower to%#struct-unchecked-ref; the generic fallback isunchecked-slot-ref. (cons.io git)The new tests don't catch this. They comprehensively test nullable slots used as receivers, e.g.
h.item.x. The closest direct nullable local is:but
item.xis reached only after(and item ...)has refined the stable local to non-null, so that is exactly the safe case. (cons.io git)So my revised verdict is: the original blocker is fixed correctly, but I would still request changes for the root-binding analogue of the same bug.
I suspect the clean fix is to put a
nullable?/maybe?bit intotype-envand initializechecked-receiver?from that, then add direct-base tests for read/call/set and#f. That also makes the invariant pleasantly simple:After that, I think this PR is looking quite solid.
addressed the review issue and rebootstrapped.
7d057c8a7btoe179c50f63