Skip to content

Convert recordOps to builtin functions - #604

Open
charlesstaats wants to merge 28 commits into
masterfrom
aliasBuiltin
Open

Convert recordOps to builtin functions#604
charlesstaats wants to merge 28 commits into
masterfrom
aliasBuiltin

Conversation

@charlesstaats

Copy link
Copy Markdown
Contributor

The addRecordOps behavior was always unfortunate. This CL converts alias, operator== (comparing records), and operator!= (comparing record) to builtin functions with open signatures, rather than adding new overloads every time a record is defined.

Some behavior notes:

  • The compilation time for the plain module does not increase, and may even decrease although it was within the noise. (I added a benchmark to check this.)
  • The alias function 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 a null literal.
  • The operator== and operator!= functions are defined when both arguments have the same struct type (no casting), or when one argument is a struct and the other is a null literal. The operator== function in this context gives the same results as alias, while operator!= is, of course, its negation.
  • All three operators can be overridden; however, the override will be limited to its scope (which can be enlarged using autounravel), while the default operators are available in any scope.
  • If T is a struct, then the code bool f(T, T) = operator== will work as expected. If f and g are both constructed this way, then alias(f, g) will be true. Unlike the default operator==, the new function f performs typecasting on its arguments. The same holds for operator!=.
  • The alias function 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-signature alias.
  • 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.

Comment thread exp.h Outdated
Comment thread builtin_handlers.cc Outdated
Comment thread testsExtra/bench/bench-startup.sh Outdated
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.

Comment thread exp.cc
Comment thread builtin_handlers.cc Outdated
Comment thread doc/asymptote.texi Outdated
Comment thread cmake-scripts/generated-files.cmake Outdated
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants