Convert recordOps to builtin functions - #604
Open
charlesstaats wants to merge 28 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors record-related identity/equality operations by replacing the legacy “add overloads per-record” approach (addRecordOps) with global open-signature builtins (alias, operator==, operator!=) and custom call-site handlers that enforce the intended record/null/type constraints. It also updates documentation and adds tests/errortests plus a small startup benchmark.
Changes:
- Introduces custom handler dispatch for open-signature builtins (translation + type inference), plus specialization support when reading certain open-signature builtins as concrete function values.
- Replaces per-record operator injection (
addRecordOps) with global==/!=open-signature builtins and a global open-signaturealias. - Adds/updates tests, errortests, and docs; adds a simple startup benchmark script.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| testsExtra/bench/startup.asy | Adds a minimal benchmark file used to measure interpreter startup/exit overhead. |
| testsExtra/bench/bench-startup.sh | Adds a script to run the startup benchmark repeatedly and report timing. |
| tests/types/recordOpEqAsValue.asy | New test verifying record ==/!= can be coerced to function values and behaves as identity equality. |
| tests/types/overrideEquals.asy | Updates override scoping test to account for global open-signature operator== and specialization behavior. |
| tests/types/builtinOps.asy | Removes now-unneeded unravel operator== for nested record equality. |
| tests/types/aliasBuiltin.asy | New tests for builtin alias across records/arrays/functions, override precedence, and scoping. |
| tests/datastructures/naivesortedTest.asy | Adjusts equality/alias assertions to reflect new builtin operator behavior. |
| tests/datastructures/hashsetTest.asy | Adjusts equality/alias assertions and fixes an EndTest() line alignment. |
| settings.cc | Minor comment punctuation change while parsing options. |
| runmath.in | Adds single-argument null-comparison helpers for record pointers and callable values. |
| runarray.in | Adds single-argument null-comparison helper for arrays. |
| Makefile.in | Adds builtin_handlers to the core compilation units. |
| exp.h | Adds specialization hook (trySpecializeToType) and declares custom handler registration/lookup + record ==/!= specialization functions. |
| exp.cc | Implements specialization fallback for casts, adds custom-handler dispatch in callExp::trans/getType, and adds open-signature detection. |
| errortests/autounravel.errors | Updates expected error location/message due to operator-based autounravel shadowing test changes. |
| errortests/autounravel.asy | Switches the autounravel shadowing case from alias to operator ==. |
| errortests/alias.errors | New expected error outputs for alias call-time constraints. |
| errortests/alias.asy | New error tests covering alias constraints (null-literal ambiguity, same-type requirement, nullable-only). |
| env.h | Removes addRecordOps declaration from protoenv. |
| env.cc | Removes protoenv::addRecordOps implementation. |
| doc/asymptote.texi | Updates docs for alias semantics and clarifies default struct == discussion; removes alias from “defined after first array” list. |
| dec.cc | Stops calling trans::addRecordOps during record field translation. |
| cmake-scripts/asy-files.cmake | Adds builtin_handlers to the core build file list for CMake builds. |
| builtin.h | Adds addOpenBuiltinFunc API (and includes exp.h for handler types). |
| builtin.cc | Registers global open-signature alias and global open-signature record ==/!= with custom handlers; updates array-op initialization guard. |
| builtin_handlers.cc | New: implements the symbol→handlers side table and custom translation/type-inference for alias and record ==/!=, plus specialization helpers. |
| application.cc | Adjusts tooManyArgs heuristic to avoid incorrectly rejecting open-signature candidates. |
| .gitignore | Adds ignores for Python tooling, shared libs, LspCpp in-source artifacts, Copilot workspace dir, and tmp/; minor formatting fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The autotools/make build writes its generated headers (run*.h, keywords.h, camp.tab.h, *.symbols.h, ...) into the source root. The interpreter sources live there too and include those headers with quotes, which the compiler resolves from each source file's own directory before any -I path -- so a leftover in-tree copy silently shadows the fresh header this out-of-source build generates, causing confusing errors deep in the build that -I ordering cannot prevent. Detect such files at configure time and abort with guidance to remove them, rather than letting the build fail cryptically.
Add a `specialize` hook for `alias`, mirroring the one for the record `operator==`, so that `bool eq(T, T) = alias;` coerces the open-signature builtin to a concrete function type for any nullable T (records, arrays, and function types). The shared `bool(T, T)` operand-type check used by both the `==`/`!=` and `alias` specialize hooks is factored out into `binaryBoolOperandType`.
Note in the manual that the built-in `==`, `!=`, and `alias` require both operands to have the same type and perform no implicit casting, so comparing two different structure types is an error even when a cast between them exists. This documents a behavior change from the previous per-record comparison operators, now that they are a single open-signature `==`/`!=`/`alias`: formerly such a comparison would cast one operand, silently comparing it against a freshly created object and thus always yielding false; it is now a compile-time error. Define an `operator ==` for the pair if cross-type comparison is intended. Add errortests/recordEq to lock in that record `==`/`!=` between different types (with and without a cast in scope) is rejected.
callExp::getType() dispatched to a custom getType handler whenever regular resolution failed, so a still-open alias call whose argument itself errored returned getAliasType's unconditional bool instead of the real error --- yielding a bogus "cannot cast 'bool' to 'int'". Gate the handler on resolvedToOpenSignature() (mirroring trans()) and return a concrete or errored type as-is, with the error check first so the guard holds regardless of how a failed resolution interacts with resolvedToOpenSignature(). Add a regression test.
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
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.
The
addRecordOpsbehavior was always unfortunate. This CL convertsalias,operator==(comparing records), andoperator!=(comparing record) to builtin functions with open signatures, rather than adding new overloads every time a record is defined.Some behavior notes:
plainmodule does not increase, and may even decrease although it was within the noise. (I added a benchmark to check this.)aliasfunction is defined for all nullable types, including records (structs), arrays, and functions. However, it can only be called if both sides have the same type (no casting), or if exactly one of the two operands is anullliteral.operator==andoperator!=functions are defined when both arguments have the same struct type (no casting), or when one argument is a struct and the other is anullliteral. Theoperator==function in this context gives the same results asalias, whileoperator!=is, of course, its negation.autounravel), while the default operators are available in any scope.Tis a struct, then the codebool f(T, T) = operator==will work as expected. Iffandgare both constructed this way, thenalias(f, g)will be true. Unlike the defaultoperator==, the new functionfperforms typecasting on its arguments. The same holds foroperator!=.aliasfunction cannot be cast in the same way, since I did not see a good use case for it and making it work is mildly invasive.