Align error suggestions across builtins and runners#308
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (81)
📝 WalkthroughWalkthroughAdds centralized error message and suggestion resources, extends thrown-value and error-helper APIs to carry suggestion metadata, threads suggestion data into error-detail formatting and tooling output, and replaces many hardcoded error strings with standardized SError/SSuggest pairs across runtime, builtins, values, and tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant ThrowSite as Throwing code
participant ErrorHelper as ErrorHelper
participant ThrowValue as TGocciaThrowValue
participant Runner as Runner/REPL/Loader
participant Formatter as FormatThrowDetail
participant Output as Console/File
ThrowSite->>ErrorHelper: ThrowTypeError(..., ASuggestion?)
ErrorHelper->>ThrowValue: Create(ErrorObject, ASuggestion)
ThrowValue-->>Runner: exception propagates
Runner->>Formatter: FormatThrowDetail(Value, File, Source, UseColor, ASuggestion)
Formatter-->>Runner: formatted string (includes suggestion when present)
Runner->>Output: WriteLn / write file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
units/Goccia.Values.Iterator.Lazy.pas (1)
552-557:⚠️ Potential issue | 🟠 MajorPreserve the intended TypeError when iterator close throws.
On Line 555 and Line 615, direct
FSourceIterator.Closecan throw and mask the subsequentThrowTypeError(...). Use the local preservation helper so the flatMap contract error remains the surfaced exception.🛠️ Proposed fix
@@ - FSourceIterator.Close; + CloseIteratorPreservingOriginalError(FSourceIterator); ThrowTypeError(SErrorIteratorFlatMapMustReturnIterable, SSuggestIteratorFlatMapCallable); @@ - FSourceIterator.Close; + CloseIteratorPreservingOriginalError(FSourceIterator); ThrowTypeError(SErrorIteratorFlatMapMustReturnIterable, SSuggestIteratorFlatMapCallable);Also applies to: 612-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.Iterator.Lazy.pas` around lines 552 - 557, The code currently calls FSourceIterator.Close directly before ThrowTypeError which can raise and mask the intended TypeError from ThrowTypeError; update the two locations (around ResolveIterator / FInnerIterator and the other block at 612-617) to use the local exception-preservation helper (i.e., save the current exception, call FSourceIterator.Close within a try/finally that restores the saved exception) so that any exception from Close does not replace the ThrowTypeError(SErrorIteratorFlatMapMustReturnIterable, SSuggestIteratorFlatMapCallable) error; ensure the helper wraps the Close call around FSourceIterator.Close while still allowing proper cleanup.units/Goccia.Error.Detail.pas (1)
120-133:⚠️ Potential issue | 🟠 MajorFallback formatting path drops
ASuggestion
ASuggestionis only used in the source-context branch (Line 115-117). When fallback rendering is used, suggestions are silently lost, so diagnostics become inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Error.Detail.pas` around lines 120 - 133, The fallback branch that builds Result for non-source-context errors drops ASuggestion, so when TGocciaObjectValue fallback or the final else set Result (using StackValue or AThrown.ToStringLiteral.Value) they must also append the suggestion exactly as the source-context path does; update the fallback assignments of Result to include ASuggestion when provided (e.g., check ASuggestion<>'' and concatenate it with the existing Result string) so diagnostics remain consistent with the source-context branch.
🧹 Nitpick comments (12)
units/Goccia.Values.TemporalInstant.pas (1)
64-64: Consider centralizing the remaining ad hoc Temporal.Instant TypeError strings.Line 64 and Line 90 still construct hardcoded messages directly; migrating these to shared
SError*constants would fully align this unit with the PR’s standardization direction.Based on learnings: Applies to **/*.pas : Use split constant units for runtime constants instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names,Goccia.Constants.TypeNamesfor type names,Goccia.Constants.ErrorNamesfor error names,Goccia.Constants.ConstructorNamesfor constructor names, andGoccia.Constants.SymbolNamesfor symbol names.Also applies to: 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalInstant.pas` at line 64, Replace the two hardcoded Temporal.Instant TypeError messages that call ThrowTypeError (the instance using AMethod + ' called on non-Instant', SSuggestTemporalFromArg and the similar message at the other occurrence) with shared string constants located in the centralized constants unit (e.g., Goccia.Constants.ErrorNames or a new SErrorTemporalInstant constant), then update the ThrowTypeError calls to use those constants; locate the usages by searching for ThrowTypeError and the concatenation pattern (AMethod + ' called on non-Instant') and replace the literal strings with the new SError* constant names so this unit aligns with the project's standardized error constants.units/Goccia.Values.TemporalPlainYearMonth.pas (1)
103-104: Move remaining PlainYearMonth literal error messages into shared constants.Line 103, Line 161/164, and Line 171 still build ad-hoc message strings; migrating those to
Goccia.Error.Messageswould complete alignment in this file.Based on learnings: Applies to **/*.pas : Use split constant units for runtime constants instead of hardcoded string literals, especially for runtime error text.
Also applies to: 161-165, 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainYearMonth.pas` around lines 103 - 104, Replace the ad-hoc PlainYearMonth error message strings used in ThrowTypeError and other places with shared constants from the Goccia.Error.Messages unit: define appropriate constants (e.g., ErrNotPlainYearMonth, ErrPlainYearMonthSuggestFromArg, etc.) in Goccia.Error.Messages and import that unit into units/Goccia.Values.TemporalPlainYearMonth.pas, then update the calls (ThrowTypeError(AMethod + ' called on non-PlainYearMonth', SSuggestTemporalFromArg), and the messages at the locations referenced around the TGocciaTemporalPlainYearMonthValue checks at lines ~161–165 and ~171–172) to use those constants instead of string literals so all runtime error text is centralized.units/Goccia.Values.TemporalZonedDateTime.pas (1)
115-116: Replace the remaining ad-hoc Temporal error strings in changed paths.Line 115, Line 152, and Line 263 still construct literal error text instead of using
Goccia.Error.Messagesconstants, which leaves this unit partially outside the centralized error catalog.Based on learnings: Applies to **/*.pas : Use split constant units for runtime constants instead of hardcoded string literals (
Goccia.Constants.*), and keep runtime error construction standardized via shared helpers/resources.Also applies to: 152-153, 263-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalZonedDateTime.pas` around lines 115 - 116, Replace the ad-hoc literal error text used when validating ZonedDateTime values by using the centralized error/messages constants and helper: instead of calling ThrowTypeError(AMethod + ' called on non-ZonedDateTime', SSuggestTemporalFromArg) and returning TGocciaTemporalZonedDateTimeValue(AValue) with hardcoded strings, assemble the error text using the appropriate constant from Goccia.Error.Messages (or the split runtime constants in Goccia.Constants.*) and pass that into ThrowTypeError (or the shared error-construction helper) so all messages in TGocciaTemporalZonedDateTimeValue validation use the centralized catalog; update the other occurrences mentioned (the other checks around TGocciaTemporalZonedDateTimeValue) to follow the same pattern.units/Goccia.Values.TemporalPlainTime.pas (1)
77-78: Centralize the two remaining hardcoded PlainTime error strings.Line 77 and Line 118 still compose literal runtime error text; switching these to
Goccia.Error.Messagesconstants would fully align this unit with the new diagnostic model.Based on learnings: Applies to **/*.pas : Use split constant units for runtime constants instead of hardcoded string literals, including runtime diagnostic text.
Also applies to: 118-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainTime.pas` around lines 77 - 78, Two remaining hardcoded PlainTime diagnostic messages in this unit should be replaced with centralized constants from Goccia.Error.Messages: replace the literal string used in the ThrowTypeError call that builds AMethod + ' called on non-PlainTime' (the call immediately before the TGocciaTemporalPlainTimeValue(AValue) cast) and the other literal used around lines 118-119 with appropriate constants from Goccia.Error.Messages (e.g., PlainTimeCalledOnNonPlainTime and the corresponding suggestion constant instead of SSuggestTemporalFromArg if a specific suggestion constant exists); update the ThrowTypeError invocations to concatenate AMethod with the exported constant(s) so both runtime diagnostics come from the shared message unit.units/Goccia.Builtins.Semver.pas (1)
473-473: Prefer a sharedSError*constant for this argument error.Line 473 still uses an ad hoc literal string. Consider switching to a centralized error-message constant to keep SemVer diagnostics fully uniform.
Based on learnings: Applies to **/*.pas : Use split constant units for runtime constants instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names,Goccia.Constants.TypeNamesfor type names,Goccia.Constants.ErrorNamesfor error names,Goccia.Constants.ConstructorNamesfor constructor names, andGoccia.Constants.SymbolNamesfor symbol names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Semver.pas` at line 473, Replace the ad-hoc literal in the ThrowTypeError call with a shared error-message constant from the central constants unit: add Goccia.Constants.ErrorNames to the uses clause if missing and change ThrowTypeError(AMethodName + ' requires a string argument', SSuggestStringArgRequired) to use the shared constant (e.g. ThrowTypeError(AMethodName + ' ' + SErrorStringArgRequired, SSuggestStringArgRequired) or ThrowTypeError(AMethodName + SErrorStringArgRequired, SSuggestStringArgRequired)) referencing the constant name (e.g. SErrorStringArgRequired) from Goccia.Constants.ErrorNames so SemVer diagnostics use the uniform error message.units/Goccia.Values.ClassValue.pas (1)
1249-1249: Suggestion text appears mismatched for assignment errors.Line 1249, Line 1279, and Line 1285 use
SSuggestCannotDeleteNonConfigurablefor assignment failures. That hint reads delete-oriented and can confuse users during set/assign errors. Consider switching to an assignment-focused suggestion constant (or adding one).Also applies to: 1279-1279, 1285-1285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ClassValue.pas` at line 1249, The error message uses a delete-oriented suggestion constant (SSuggestCannotDeleteNonConfigurable) when throwing assignment errors in ThrowTypeError calls using Format(SErrorCannotAssignReadOnly, [AName]); update those three call sites (the ThrowTypeError invocations at lines referencing SErrorCannotAssignReadOnly) to use an assignment-focused suggestion constant instead—either replace SSuggestCannotDeleteNonConfigurable with a new constant like SSuggestCannotAssignToReadOnly (and add that resource/string constant) or with an existing assignment-oriented suggestion constant; ensure all three occurrences are updated so assignment failures show the correct suggestion text.units/Goccia.Values.TemporalPlainDateTime.pas (1)
154-159: Replace hardcoded Temporal field names withPROP_*constants in these touched paths.
CoercePlainDateTimeandDateTimeWithstill use string literals for Temporal fields ('year','month','day', etc.). Please switch these toGoccia.Constants.PropertyNamesconstants to keep Temporal property access consistent.♻️ Proposed refactor
+ Goccia.Constants.PropertyNames, - VYear := GetRequiredField('year'); - VMonth := GetRequiredField('month'); - VDay := GetRequiredField('day'); - VHour := GetOptionalField('hour', 0); - VMinute := GetOptionalField('minute', 0); - VSecond := GetOptionalField('second', 0); + VYear := GetRequiredField(PROP_YEAR); + VMonth := GetRequiredField(PROP_MONTH); + VDay := GetRequiredField(PROP_DAY); + VHour := GetOptionalField(PROP_HOUR, 0); + VMinute := GetOptionalField(PROP_MINUTE, 0); + VSecond := GetOptionalField(PROP_SECOND, 0); - GetFieldOr('year', D.FYear), GetFieldOr('month', D.FMonth), GetFieldOr('day', D.FDay), - GetFieldOr('hour', D.FHour), GetFieldOr('minute', D.FMinute), GetFieldOr('second', D.FSecond), - GetFieldOr('millisecond', D.FMillisecond), GetFieldOr('microsecond', D.FMicrosecond), - GetFieldOr('nanosecond', D.FNanosecond)); + GetFieldOr(PROP_YEAR, D.FYear), GetFieldOr(PROP_MONTH, D.FMonth), GetFieldOr(PROP_DAY, D.FDay), + GetFieldOr(PROP_HOUR, D.FHour), GetFieldOr(PROP_MINUTE, D.FMinute), GetFieldOr(PROP_SECOND, D.FSecond), + GetFieldOr(PROP_MILLISECOND, D.FMillisecond), GetFieldOr(PROP_MICROSECOND, D.FMicrosecond), + GetFieldOr(PROP_NANOSECOND, D.FNanosecond));Based on learnings: all Temporal value units must use
Goccia.Constants.PropertyNamesconstants (e.g.,PROP_YEAR,PROP_MONTH,PROP_DAY, time-unitPROP_*) instead of hardcoded literals.Also applies to: 413-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainDateTime.pas` around lines 154 - 159, Replace the hardcoded Temporal property name strings in CoercePlainDateTime and DateTimeWith with the constants from Goccia.Constants.PropertyNames (e.g., use PROP_YEAR, PROP_MONTH, PROP_DAY, PROP_HOUR, PROP_MINUTE, PROP_SECOND) wherever GetRequiredField/GetOptionalField are called; update the same replacements in the other affected block referenced (the later DateTimeWith/related code around the second occurrence) so all temporal field lookups use the PropertyNames constants instead of string literals.units/Goccia.Values.TemporalPlainMonthDay.pas (1)
65-68: Finish migrating the remaining PlainMonthDay error literals.These branches still hardcode message text, and the object-coercion errors also hardcode Temporal property names (
day,monthCode,month). That leaves this unit out of sync with the shared diagnostics work and the Temporal property-name convention. Please format these through the sharedSError*messages andPROP_DAY/PROP_MONTH_CODE/PROP_MONTHconstants instead.Based on learnings, all Temporal value unit implementations must use
Goccia.Constants.PropertyNames::{PROP_YEAR, PROP_MONTH, PROP_MONTH_CODE, PROP_DAY, PROP_CALENDAR_ID}when constructing related error messages.Also applies to: 101-127, 132-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainMonthDay.pas` around lines 65 - 68, The branch in AsPlainMonthDay currently throws a hardcoded type error message; replace that literal with the shared error message helpers (use the appropriate SError* function/constant) and construct the object-coercion/error text using the PropertyNames constants PROP_DAY, PROP_MONTH_CODE and PROP_MONTH from Goccia.Constants.PropertyNames rather than hardcoded strings; update all similar literal usages in this unit (including the other branches around the PlainMonthDay coercions and the regions noted) to use the shared SError* messages and the PROP_* constants when composing the error messages so the Temporal property-name convention is respected.units/Goccia.Evaluator.pas (1)
629-652: Keep call/construct suggestions on the shared suggestion catalog.These fallback branches still synthesize the suggestion/detail text inline, so the new suggestion-aware output can drift again across equivalent “not callable” / “not constructable” errors. Please move the suggestion prose behind shared
SSuggest*constants and keep only the contextual interpolation here.Also applies to: 2023-2033
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 629 - 652, The branches that currently embed suggestion/detail strings inline (the ThrowTypeError calls handling non-function/non-callable cases in units/Goccia.Evaluator.pas) should use the shared suggestion constants instead of synthesizing text here: replace the inline detail Format('''%s'' is of type ''%s'' which does not have method ''%s''', ...) and Format('''%s'' is of type ''%s'' and cannot be called as a function', ...) and the final SSuggestNotFunctionType usage consistency by passing the appropriate SSuggest* constant (e.g., SSuggestNotFunctionType or the shared "not constructable" suggestion constant) to ThrowTypeError alongside the contextual Format(...) primary message (using SErrorMemberNotFunction, SErrorNotFunction, SErrorValueNotFunction). Update the branches that reference MemberExpr, ACallExpression.Callee and TGocciaIdentifierExpression to use those shared SSuggest* constants (also apply same change to the other occurrences mentioned: lines 2023-2033).units/Goccia.Builtins.TestAssertions.pas (1)
544-546: PreferIsCallablefor the new function validators.These branches hard-code
TGocciaFunctionBase, which is tighter than the runtime’s callability contract and keeps spreading the non-idiomatic check through the test API surface.Based on learnings: Applies to units/**/*.pas : All values inherit from
TGocciaValue. Prefer VMT methodsGetProperty(Name),SetProperty(Name, Value),IsPrimitive, andIsCallableover type-checking at call sites. Do not useistype checks for fundamental type-system properties.Also applies to: 2470-2471, 2501-2502, 3152-3155, 3195-3198, 3399-3400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.pas` around lines 544 - 546, The code currently checks AArguments.GetElement(1) is TGocciaFunctionBase which hard-codes the concrete type; replace this type-check with the runtime callability check by calling IsCallable on the value returned by AArguments.GetElement(1) and keep the same error path: if not AArguments.GetElement(1).IsCallable then call Goccia.Values.ErrorHelper.ThrowTypeError(Format(SErrorFunctionExpectsFunctionSecond,[GetFunctionName]), SSuggestTestUsage). Update the analogous occurrences mentioned (lines near 2470, 2501, 3152, 3195, 3399) to use IsCallable instead of "is TGocciaFunctionBase".units/Goccia.Builtins.GlobalPromise.pas (1)
615-617: Use a shared message constant for the iterable TypeError pathThis branch still composes an inline message (
... + ' is not iterable'). Prefer a centralizedSError...message (withFormat(...)if needed) to keep diagnostics fully aligned with the new message catalog.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalPromise.pas` around lines 615 - 617, Replace the inline error message concatenation in the TypeError branch with a centralized message constant from the message catalog (use the appropriate SError... constant), e.g. call Goccia.Values.ErrorHelper.ThrowTypeError with Format(SErrorNotIterable, [Iterable.ToStringLiteral.Value]) instead of Iterable.ToStringLiteral.Value + ' is not iterable'; keep SSuggestNotIterable as the suggestion parameter and leave Result := nil unchanged.units/Goccia.Values.TypedArrayValue.pas (1)
1716-1717: Consider aTypedArray.from-specific suggestion constant
TypedArray.fromcurrently reusesSSuggestTypedArraySetSource. If you want cleaner UX copy, a dedicatedfromsuggestion constant would avoid cross-method wording.Also applies to: 1779-1779
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TypedArrayValue.pas` around lines 1716 - 1717, The code is reusing SSuggestTypedArraySetSource for TypedArray.from which leads to non-ideal UX copy; add a new suggestion constant (e.g., SSuggestTypedArrayFromSource) in the resource/strings module and replace SSuggestTypedArraySetSource with SSuggestTypedArrayFromSource in the ThrowTypeError calls used by TypedArray.from (the call that uses ThrowTypeError(SErrorTypedArrayFromRequiresArg, SSuggestTypedArraySetSource) and the similar occurrence around line 1779). Ensure the new constant text matches "from"-specific wording and update any unit tests or references to use the new SSuggestTypedArrayFromSource identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/ppas.sh`:
- Around line 1-72: This PR includes a generated FPC artifact build/ppas.sh (and
its Windows counterpart build/ppas.bat) that should not be committed; remove
build/ppas.sh from the commit (and ensure build/ppas.bat is also untracked if
present) and add an entry to .gitignore to exclude these transient files (e.g.,
ignore build/ppas.sh and build/ppas.bat) so they don’t reappear in future diffs;
then amend the commit or create a new commit that deletes the file(s) from the
repo while leaving the .gitignore change.
In `@units/Goccia.Builtins.GlobalArrayBuffer.pas`:
- Around line 110-116: The current length coercion extracts Num :=
AArgs.GetElement(0).ToNumberLiteral and then manually validates/truncates it
(using Trunc and Trunc checks) which bypasses the canonical ToIndex logic;
replace this manual path by calling the standard ConstructorToIndex helper to
compute Len (and handle NaN, Infinity, negative, fractional, and large values)
and use its result for ArrayBuffer sizing; keep the ThrowRangeError call only
for constructor-level range failures if ConstructorToIndex indicates an error,
and update references to Num/Llen accordingly so Len is derived from
ConstructorToIndex instead of Trunc(Num.Value).
In `@units/Goccia.Builtins.GlobalMap.pas`:
- Around line 54-55: Map.groupBy currently rejects any input that is not a
TGocciaArrayValue by calling ThrowTypeError with
SErrorMapGroupByRequiresIterable; update the validation to accept either a
TGocciaArrayValue or any object that implements the iterator protocol your
runtime uses (e.g., checks for the iterator interface/type or a
GetIterator/MoveNext/Current pattern) before throwing. Locate the check using
AArgs.GetElement(0) and TGocciaArrayValue and expand it to also detect the
iterable/iterator type (or alternatively narrow the error text to explicitly say
“array” by keeping the array-only check and changing
SErrorMapGroupByRequiresIterable/SSuggestNotIterable), then only call
ThrowTypeError if neither condition is true so valid iterables do not get
rejected.
In `@units/Goccia.Builtins.GlobalString.pas`:
- Around line 138-139: The error message always reports 'undefined' even when
RawValue is null; update the check around RawValue in the conversion to compute
the correct kind string and pass that into ThrowTypeError: inspect RawValue
using the existing type checks (TGocciaUndefinedLiteralValue vs
TGocciaNullLiteralValue) to set a local kind variable to 'undefined' or 'null'
and then call ThrowTypeError(Format(SErrorCannotConvertToObject, [kind]),
SSuggestCheckNullBeforeAccess) so the reported value matches the actual
RawValue; target the code that references RawValue,
TGocciaUndefinedLiteralValue, TGocciaNullLiteralValue, ThrowTypeError,
SErrorCannotConvertToObject and SSuggestCheckNullBeforeAccess.
In `@units/Goccia.Builtins.GlobalSymbol.pas`:
- Line 180: Replace the incorrect prototype-receiver suggestion used when
throwing the keyFor type error: in the ThrowTypeError call that currently passes
SErrorSymbolKeyForRequiresSymbol and SSuggestSymbolThisType, swap
SSuggestSymbolThisType for a Symbol.keyFor-specific suggestion constant (or add
one if missing) so the thrown error uses a keyFor-argument-type suggestion;
update the ThrowTypeError invocation (the site using ThrowTypeError,
SErrorSymbolKeyForRequiresSymbol) to pass the new/appropriate suggestion token
instead of SSuggestSymbolThisType.
In `@units/Goccia.Builtins.Math.pas`:
- Around line 1023-1025: In the non-number branch of Math.sumPrecise replace the
incorrect suggestion token: where ThrowTypeError is called with
SErrorMathSumPreciseNotNumber and SSuggestNotIterable, change
SSuggestNotIterable to the number-specific suggestion token (e.g.,
SSuggestNotNumber) so the call becomes
Goccia.Values.ErrorHelper.ThrowTypeError(SErrorMathSumPreciseNotNumber,
SSuggestNotNumber); this affects the check that casts AElement to
TGocciaNumberLiteralValue before assigning NumVal.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 541-550: The test-suggestion text (SSuggestTestUsage) added where
ErrorHelper.ThrowTypeError is called is lost because the TGocciaThrowValue
reporting/formatting path doesn't include suggestion strings; update the
TGocciaThrowValue formatter (the class/method responsible for producing the
thrown value message used by runTests) so it detects and includes any suggestion
text provided (e.g., append or include a suggestion field when TGocciaThrowValue
is constructed or when its ToString/Format method runs), and ensure throws
originating from Goccia.Values.ErrorHelper.ThrowTypeError preserve and surface
that suggestion in runTests output.
In `@units/Goccia.Builtins.TOML.pas`:
- Line 87: The ThrowSyntaxError call currently passes SSuggestStringArgRequired
which is misleading for TOML parse failures; update the call at the
ThrowSyntaxError(E.Message, SSuggestStringArgRequired) site to use the
TOML-specific suggestion constant (e.g., SSuggestTOMLSyntaxError or the
project's existing TOML parse suggestion constant) so syntax/parse errors
produce the correct guidance; keep the ThrowSyntaxError and E.Message parameters
unchanged and only swap the suggestion constant.
In `@units/Goccia.Builtins.YAML.pas`:
- Line 90: The parser is raising syntax errors with the wrong suggestion
constant (SSuggestStringArgRequired) in the ThrowSyntaxError calls at the YAML
parser locations (calls to ThrowSyntaxError around lines where the exception is
raised); update those ThrowSyntaxError calls in units/Goccia.Builtins.YAML.pas
(the two occurrences referencing SSuggestStringArgRequired) to omit the
misleading suggestion or pass a neutral/empty suggestion value instead (i.e.,
remove SSuggestStringArgRequired from the ThrowSyntaxError call or replace it
with a neutral suggestion constant such as an empty string or SSuggestNone if
available) so the error message no longer directs users incorrectly.
In `@units/Goccia.Values.ArrayValue.pas`:
- Around line 408-410: The TGocciaArrayValue.ThrowError method currently raises
TGocciaError directly; change it to delegate to the project error helpers in
Goccia.Values.ErrorHelper (e.g., call ThrowError, or
ThrowTypeError/ThrowRangeError as appropriate) instead of using
TGocciaError.Create so all runtime array validation errors flow through the
standard helper pipeline; update the method to format the message (using
Format(AMessage, AArgs)) and pass that text plus the ASuggestion into the
correct helper routine and ensure the ErrorHelper unit is referenced so no
direct TGocciaError construction remains.
In `@units/Goccia.Values.FFIPointer.pas`:
- Line 147: Replace hard-coded property name string literals used in formatted
error messages with the corresponding property-name constants: change the
'isNull' argument in the Format call passed to ThrowTypeError to use
PROP_IS_NULL, and likewise replace any 'address' literal (e.g., the similar
Format/ThrowTypeError at the other location) with PROP_FFI_ADDRESS; import or
reference the constant container (Goccia.Constants.PropertyNames) if not already
available so the Format calls in the ThrowTypeError/Format expressions use
PROP_IS_NULL and PROP_FFI_ADDRESS instead of literal strings.
In `@units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 130-136: Replace the hardcoded Temporal property names by using
the constants from Goccia.Constants.PropertyNames: change string literals passed
to Obj.GetProperty ('year','month','day') to the corresponding constants
(PROP_YEAR, PROP_MONTH, PROP_DAY) and update the ThrowTypeError messages that
currently embed "year, month, day" to build the message using those same
constants (keeping the existing use of ThrowTypeError, SSuggestTemporalFromArg,
Obj.GetProperty and TGocciaUndefinedLiteralValue). Ensure all references are
consistent so property access and the error text rely on the PropertyNames
constants rather than literal strings.
In `@units/Goccia.Values.TextEncoderValue.pas`:
- Line 239: The ThrowTypeError call currently uses SSuggestTextEncoderThisType
while validating encodeInto argument/destination shape; update the three
ThrowTypeError calls (those invoking ThrowTypeError with
SErrorTextEncoderEncodeIntoArgs and SSuggestTextEncoderThisType) to use a more
appropriate argument/destination-focused suggestion constant (e.g.,
SSuggestTextEncoderEncodeIntoArgs or SSuggestTextEncoderDestinationType) so the
thrown error points users at the invalid argument/destination rather than
suggesting a problem with this; modify the ThrowTypeError invocations in the
encodeInto validation branches in Goccia.Values.TextEncoderValue.pas to
reference the new suggestion symbol and ensure the error message remains
consistent with SErrorTextEncoderEncodeIntoArgs.
In `@units/Goccia.VM.pas`:
- Line 2413: The ThrowTypeError calls currently pass the wrong suggestion
constant (SSuggestNotFunctionType) for module/import failures; update both
ThrowTypeError(SErr... SErrorModuleNotAvailableInVM, SSuggestNotFunctionType)
occurrences to use the module-availability suggestion constant instead (e.g.,
SSuggestModuleNotAvailable) so the suggestion matches
SErrorModuleNotAvailableInVM.
- Around line 3436-3437: The current ThrowTypeError call always uses
SSuggestCheckNullBeforeAccess which is inappropriate for non-null primitives;
change this to select the suggestion based on the value kind(s) before throwing.
Locate the ThrowTypeError usage (symbols: ThrowTypeError, Format,
SErrorCannotUseInOperator, AKey, AObject, SSuggestCheckNullBeforeAccess) and
replace the fixed suggestion with a small branch that inspects AObject (and/or
AKey) kind (e.g., IsBoolean/IsNumber/IsString/IsObject/IsNull) and picks an
appropriate suggestion string for primitives versus null/object access (or a
generic "ensure value is an object or primitive" message) before calling
ThrowTypeError. Ensure the formatted error text still uses
SErrorCannotUseInOperator and only the suggestion token differs.
- Around line 345-346: The suggestion constant used with
ThrowTypeError(Format(SErrorCannotAssignReadOnly, [AName]),
SSuggestCannotDeleteNonConfigurable) is incorrect for a read-only assignment
error; replace SSuggestCannotDeleteNonConfigurable with a
read-only/assignment-specific suggestion constant (e.g.,
SSuggestCannotAssignReadOnly or SSuggestCannotWriteToReadOnly) so the second
argument matches SErrorCannotAssignReadOnly, and if such a constant doesn't
exist, add a properly named suggestion constant and use it in the ThrowTypeError
call.
---
Outside diff comments:
In `@units/Goccia.Error.Detail.pas`:
- Around line 120-133: The fallback branch that builds Result for
non-source-context errors drops ASuggestion, so when TGocciaObjectValue fallback
or the final else set Result (using StackValue or AThrown.ToStringLiteral.Value)
they must also append the suggestion exactly as the source-context path does;
update the fallback assignments of Result to include ASuggestion when provided
(e.g., check ASuggestion<>'' and concatenate it with the existing Result string)
so diagnostics remain consistent with the source-context branch.
In `@units/Goccia.Values.Iterator.Lazy.pas`:
- Around line 552-557: The code currently calls FSourceIterator.Close directly
before ThrowTypeError which can raise and mask the intended TypeError from
ThrowTypeError; update the two locations (around ResolveIterator /
FInnerIterator and the other block at 612-617) to use the local
exception-preservation helper (i.e., save the current exception, call
FSourceIterator.Close within a try/finally that restores the saved exception) so
that any exception from Close does not replace the
ThrowTypeError(SErrorIteratorFlatMapMustReturnIterable,
SSuggestIteratorFlatMapCallable) error; ensure the helper wraps the Close call
around FSourceIterator.Close while still allowing proper cleanup.
---
Nitpick comments:
In `@units/Goccia.Builtins.GlobalPromise.pas`:
- Around line 615-617: Replace the inline error message concatenation in the
TypeError branch with a centralized message constant from the message catalog
(use the appropriate SError... constant), e.g. call
Goccia.Values.ErrorHelper.ThrowTypeError with Format(SErrorNotIterable,
[Iterable.ToStringLiteral.Value]) instead of Iterable.ToStringLiteral.Value + '
is not iterable'; keep SSuggestNotIterable as the suggestion parameter and leave
Result := nil unchanged.
In `@units/Goccia.Builtins.Semver.pas`:
- Line 473: Replace the ad-hoc literal in the ThrowTypeError call with a shared
error-message constant from the central constants unit: add
Goccia.Constants.ErrorNames to the uses clause if missing and change
ThrowTypeError(AMethodName + ' requires a string argument',
SSuggestStringArgRequired) to use the shared constant (e.g.
ThrowTypeError(AMethodName + ' ' + SErrorStringArgRequired,
SSuggestStringArgRequired) or ThrowTypeError(AMethodName +
SErrorStringArgRequired, SSuggestStringArgRequired)) referencing the constant
name (e.g. SErrorStringArgRequired) from Goccia.Constants.ErrorNames so SemVer
diagnostics use the uniform error message.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 544-546: The code currently checks AArguments.GetElement(1) is
TGocciaFunctionBase which hard-codes the concrete type; replace this type-check
with the runtime callability check by calling IsCallable on the value returned
by AArguments.GetElement(1) and keep the same error path: if not
AArguments.GetElement(1).IsCallable then call
Goccia.Values.ErrorHelper.ThrowTypeError(Format(SErrorFunctionExpectsFunctionSecond,[GetFunctionName]),
SSuggestTestUsage). Update the analogous occurrences mentioned (lines near 2470,
2501, 3152, 3195, 3399) to use IsCallable instead of "is TGocciaFunctionBase".
In `@units/Goccia.Evaluator.pas`:
- Around line 629-652: The branches that currently embed suggestion/detail
strings inline (the ThrowTypeError calls handling non-function/non-callable
cases in units/Goccia.Evaluator.pas) should use the shared suggestion constants
instead of synthesizing text here: replace the inline detail Format('''%s'' is
of type ''%s'' which does not have method ''%s''', ...) and Format('''%s'' is of
type ''%s'' and cannot be called as a function', ...) and the final
SSuggestNotFunctionType usage consistency by passing the appropriate SSuggest*
constant (e.g., SSuggestNotFunctionType or the shared "not constructable"
suggestion constant) to ThrowTypeError alongside the contextual Format(...)
primary message (using SErrorMemberNotFunction, SErrorNotFunction,
SErrorValueNotFunction). Update the branches that reference MemberExpr,
ACallExpression.Callee and TGocciaIdentifierExpression to use those shared
SSuggest* constants (also apply same change to the other occurrences mentioned:
lines 2023-2033).
In `@units/Goccia.Values.ClassValue.pas`:
- Line 1249: The error message uses a delete-oriented suggestion constant
(SSuggestCannotDeleteNonConfigurable) when throwing assignment errors in
ThrowTypeError calls using Format(SErrorCannotAssignReadOnly, [AName]); update
those three call sites (the ThrowTypeError invocations at lines referencing
SErrorCannotAssignReadOnly) to use an assignment-focused suggestion constant
instead—either replace SSuggestCannotDeleteNonConfigurable with a new constant
like SSuggestCannotAssignToReadOnly (and add that resource/string constant) or
with an existing assignment-oriented suggestion constant; ensure all three
occurrences are updated so assignment failures show the correct suggestion text.
In `@units/Goccia.Values.TemporalInstant.pas`:
- Line 64: Replace the two hardcoded Temporal.Instant TypeError messages that
call ThrowTypeError (the instance using AMethod + ' called on non-Instant',
SSuggestTemporalFromArg and the similar message at the other occurrence) with
shared string constants located in the centralized constants unit (e.g.,
Goccia.Constants.ErrorNames or a new SErrorTemporalInstant constant), then
update the ThrowTypeError calls to use those constants; locate the usages by
searching for ThrowTypeError and the concatenation pattern (AMethod + ' called
on non-Instant') and replace the literal strings with the new SError* constant
names so this unit aligns with the project's standardized error constants.
In `@units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 154-159: Replace the hardcoded Temporal property name strings in
CoercePlainDateTime and DateTimeWith with the constants from
Goccia.Constants.PropertyNames (e.g., use PROP_YEAR, PROP_MONTH, PROP_DAY,
PROP_HOUR, PROP_MINUTE, PROP_SECOND) wherever GetRequiredField/GetOptionalField
are called; update the same replacements in the other affected block referenced
(the later DateTimeWith/related code around the second occurrence) so all
temporal field lookups use the PropertyNames constants instead of string
literals.
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 65-68: The branch in AsPlainMonthDay currently throws a hardcoded
type error message; replace that literal with the shared error message helpers
(use the appropriate SError* function/constant) and construct the
object-coercion/error text using the PropertyNames constants PROP_DAY,
PROP_MONTH_CODE and PROP_MONTH from Goccia.Constants.PropertyNames rather than
hardcoded strings; update all similar literal usages in this unit (including the
other branches around the PlainMonthDay coercions and the regions noted) to use
the shared SError* messages and the PROP_* constants when composing the error
messages so the Temporal property-name convention is respected.
In `@units/Goccia.Values.TemporalPlainTime.pas`:
- Around line 77-78: Two remaining hardcoded PlainTime diagnostic messages in
this unit should be replaced with centralized constants from
Goccia.Error.Messages: replace the literal string used in the ThrowTypeError
call that builds AMethod + ' called on non-PlainTime' (the call immediately
before the TGocciaTemporalPlainTimeValue(AValue) cast) and the other literal
used around lines 118-119 with appropriate constants from Goccia.Error.Messages
(e.g., PlainTimeCalledOnNonPlainTime and the corresponding suggestion constant
instead of SSuggestTemporalFromArg if a specific suggestion constant exists);
update the ThrowTypeError invocations to concatenate AMethod with the exported
constant(s) so both runtime diagnostics come from the shared message unit.
In `@units/Goccia.Values.TemporalPlainYearMonth.pas`:
- Around line 103-104: Replace the ad-hoc PlainYearMonth error message strings
used in ThrowTypeError and other places with shared constants from the
Goccia.Error.Messages unit: define appropriate constants (e.g.,
ErrNotPlainYearMonth, ErrPlainYearMonthSuggestFromArg, etc.) in
Goccia.Error.Messages and import that unit into
units/Goccia.Values.TemporalPlainYearMonth.pas, then update the calls
(ThrowTypeError(AMethod + ' called on non-PlainYearMonth',
SSuggestTemporalFromArg), and the messages at the locations referenced around
the TGocciaTemporalPlainYearMonthValue checks at lines ~161–165 and ~171–172) to
use those constants instead of string literals so all runtime error text is
centralized.
In `@units/Goccia.Values.TemporalZonedDateTime.pas`:
- Around line 115-116: Replace the ad-hoc literal error text used when
validating ZonedDateTime values by using the centralized error/messages
constants and helper: instead of calling ThrowTypeError(AMethod + ' called on
non-ZonedDateTime', SSuggestTemporalFromArg) and returning
TGocciaTemporalZonedDateTimeValue(AValue) with hardcoded strings, assemble the
error text using the appropriate constant from Goccia.Error.Messages (or the
split runtime constants in Goccia.Constants.*) and pass that into ThrowTypeError
(or the shared error-construction helper) so all messages in
TGocciaTemporalZonedDateTimeValue validation use the centralized catalog; update
the other occurrences mentioned (the other checks around
TGocciaTemporalZonedDateTimeValue) to follow the same pattern.
In `@units/Goccia.Values.TypedArrayValue.pas`:
- Around line 1716-1717: The code is reusing SSuggestTypedArraySetSource for
TypedArray.from which leads to non-ideal UX copy; add a new suggestion constant
(e.g., SSuggestTypedArrayFromSource) in the resource/strings module and replace
SSuggestTypedArraySetSource with SSuggestTypedArrayFromSource in the
ThrowTypeError calls used by TypedArray.from (the call that uses
ThrowTypeError(SErrorTypedArrayFromRequiresArg, SSuggestTypedArraySetSource) and
the similar occurrence around line 1779). Ensure the new constant text matches
"from"-specific wording and update any unit tests or references to use the new
SSuggestTypedArrayFromSource identifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66cf807e-b410-42da-8ee6-a72c2c06030c
📒 Files selected for processing (81)
BenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprbuild/ppas.shunits/Goccia.Application.pasunits/Goccia.Builtins.DisposableStack.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalArrayBuffer.pasunits/Goccia.Builtins.GlobalFFI.pasunits/Goccia.Builtins.GlobalMap.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.Builtins.GlobalPromise.pasunits/Goccia.Builtins.GlobalProxy.pasunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Builtins.GlobalRegExp.pasunits/Goccia.Builtins.GlobalString.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.Globals.pasunits/Goccia.Builtins.JSON.pasunits/Goccia.Builtins.JSON5.pasunits/Goccia.Builtins.JSONL.pasunits/Goccia.Builtins.Math.pasunits/Goccia.Builtins.Performance.pasunits/Goccia.Builtins.Semver.pasunits/Goccia.Builtins.TOML.pasunits/Goccia.Builtins.Temporal.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Builtins.YAML.pasunits/Goccia.DisposalTracker.pasunits/Goccia.Error.Detail.pasunits/Goccia.Error.Messages.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.Evaluator.Arithmetic.pasunits/Goccia.Evaluator.Assignment.pasunits/Goccia.Evaluator.TypeOperations.pasunits/Goccia.Evaluator.pasunits/Goccia.ImportMeta.pasunits/Goccia.Scope.pasunits/Goccia.Temporal.Options.pasunits/Goccia.URI.pasunits/Goccia.Utils.Arrays.pasunits/Goccia.VM.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.ClassValue.pasunits/Goccia.Values.Error.pasunits/Goccia.Values.ErrorHelper.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.FunctionValue.pasunits/Goccia.Values.Iterator.Concat.pasunits/Goccia.Values.Iterator.Generic.pasunits/Goccia.Values.Iterator.Lazy.pasunits/Goccia.Values.Iterator.Zip.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectPropertyDescriptor.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.ProxyValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pasunits/Goccia.Values.SymbolValue.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TemporalPlainYearMonth.pasunits/Goccia.Values.TemporalZonedDateTime.pasunits/Goccia.Values.TextDecoderValue.pasunits/Goccia.Values.TextEncoderValue.pasunits/Goccia.Values.ToPrimitive.pasunits/Goccia.Values.TypedArrayValue.pasunits/Goccia.Values.URLSearchParamsValue.pasunits/Goccia.Values.URLValue.pasunits/Goccia.Values.Uint8ArrayEncoding.pas
frostney
force-pushed
the
t3code/align-error-suggestions
branch
from
2a3f19b to
7532357
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Values.TemporalPlainDateTime.pas (1)
431-446:⚠️ Potential issue | 🟠 MajorHandle unsupported
withPlainTimeargument kinds explicitly.At Line 431, non-
undefinedvalues that are neitherTGocciaTemporalPlainTimeValuenor string currently fall through and return midnight (Line 448) instead of throwing. Please add an explicitelseerror branch so invalid inputs don’t silently coerce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainDateTime.pas` around lines 431 - 446, The current Arg handling in the withPlainTime path lets non-undefined, non-TGocciaTemporalPlainTimeValue and non-TGocciaStringLiteralValue fall through to midnight; update the conditional block that checks Arg (the Assigned(Arg) ... branch) to add a final else that calls ThrowTypeError with an appropriate message for invalid withPlainTime argument kinds (use the same ThrowTypeError mechanism as used for time parsing errors), e.g. indicate an unsupported argument type for withPlainTime; reference TGocciaTemporalPlainTimeValue, TGocciaStringLiteralValue, TGocciaUndefinedLiteralValue, TryParseISOTime, ThrowTypeError, SErrorInvalidTimeString and SSuggestTemporalISOFormat so the fix is placed in the same branch that currently handles those cases.
♻️ Duplicate comments (1)
units/Goccia.Values.TemporalPlainDate.pas (1)
67-68:⚠️ Potential issue | 🟡 MinorReplace hardcoded Temporal property-name literals with shared constants.
CoercePlainDatestill hardcodesyear/month/dayin both property access and error text, which can drift from the shared property-name source.♻️ Proposed fix
uses SysUtils, + Goccia.Constants.PropertyNames, Goccia.Error.Messages, Goccia.Error.Suggestions, @@ - V := Obj.GetProperty('year'); + V := Obj.GetProperty(PROP_YEAR); if (V = nil) or (V is TGocciaUndefinedLiteralValue) then - ThrowTypeError(AMethod + ' requires year, month, day properties', SSuggestTemporalFromArg); - VMonth := Obj.GetProperty('month'); + ThrowTypeError( + Format('%s requires %s, %s, %s properties', [AMethod, PROP_YEAR, PROP_MONTH, PROP_DAY]), + SSuggestTemporalFromArg); + VMonth := Obj.GetProperty(PROP_MONTH); if (VMonth = nil) or (VMonth is TGocciaUndefinedLiteralValue) then - ThrowTypeError(AMethod + ' requires year, month, day properties', SSuggestTemporalFromArg); - VDay := Obj.GetProperty('day'); + ThrowTypeError( + Format('%s requires %s, %s, %s properties', [AMethod, PROP_YEAR, PROP_MONTH, PROP_DAY]), + SSuggestTemporalFromArg); + VDay := Obj.GetProperty(PROP_DAY); if (VDay = nil) or (VDay is TGocciaUndefinedLiteralValue) then - ThrowTypeError(AMethod + ' requires year, month, day properties', SSuggestTemporalFromArg); + ThrowTypeError( + Format('%s requires %s, %s, %s properties', [AMethod, PROP_YEAR, PROP_MONTH, PROP_DAY]), + SSuggestTemporalFromArg);Based on learnings: Temporal units must use
Goccia.Constants.PropertyNamesconstants (PROP_YEAR,PROP_MONTH,PROP_DAY) for Temporal property access and related error-message references.Also applies to: 128-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainDate.pas` around lines 67 - 68, CoercePlainDate currently hardcodes the Temporal property-name literals "year", "month", and "day" in property access and error messages; update it to reference the shared constants PROP_YEAR, PROP_MONTH, and PROP_DAY from Goccia.Constants.PropertyNames instead. Replace any direct string keys when reading properties and any occurrences inside Goccia.Error.Messages/Goccia.Error.Suggestions text construction with those constants (e.g., in CoercePlainDate and the related validation/formatting code around the other Temporal plain-date helpers), ensuring you import or qualify Goccia.Constants.PropertyNames and use the constants in both lookups and error-message formatting so the names remain in sync.
🧹 Nitpick comments (1)
units/Goccia.Values.TemporalPlainMonthDay.pas (1)
101-127: Keep Temporal property names sourced fromPROP_*in the new error text.These messages inline
day,monthCode, andmontheven though this unit already importsPROP_DAY,PROP_MONTH_CODE, andPROP_MONTH. Using the constants in the message construction keeps the diagnostics aligned with the rest of the Temporal units and avoids string drift.♻️ Suggested cleanup
- ThrowTypeError(AMethod + ' requires day property', SSuggestTemporalFromArg); + ThrowTypeError( + Format('%s requires %s property', [AMethod, PROP_DAY]), + SSuggestTemporalFromArg); ... - ThrowTypeError(AMethod + ' requires monthCode or month property', SSuggestTemporalFromArg); + ThrowTypeError( + Format('%s requires %s or %s property', + [AMethod, PROP_MONTH_CODE, PROP_MONTH]), + SSuggestTemporalFromArg);Based on learnings, Temporal units should use
Goccia.Constants.PropertyNames::{PROP_YEAR, PROP_MONTH, PROP_MONTH_CODE, PROP_DAY, PROP_CALENDAR_ID}when constructing related error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainMonthDay.pas` around lines 101 - 127, Replace the inline property names in the thrown error texts with the existing PROP_* constants: change the ThrowTypeError that currently concatenates ' requires day property' to use PROP_DAY (e.g. Format or concatenation with PROP_DAY) where VDay is validated; change the message in the else branch that says ' requires monthCode or month property' to reference PROP_MONTH_CODE and PROP_MONTH instead of literal names; and ensure any other error messages related to monthCode/month consistency (the ThrowTypeError/ThrowRangeError calls inside the VMonthCode handling that reference monthCode/mismatch) similarly use PROP_MONTH_CODE or PROP_MONTH constants; locate these usages around symbols VDay, VMonthCode, V, MonthCodeStr, MonthPart and update message construction to use the PROP_* constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Builtins.JSONL.pas`:
- Around line 150-153: Replace misleading suggestion constants in the JSONL
argument-type error paths: where ThrowTypeError is called with
SErrorJSONLParseArgType and SSuggestStringArgRequired, remove the string-only
hint (pass an empty suggestion or a neutral hint) so the error doesn’t
incorrectly force “string” only; likewise, in the EGocciaJSONLInvalidInputError
handler replace SSuggestJSONFormat with an empty suggestion (or neutral hint) so
non-typed-array invalid inputs aren’t given an incorrect JSON format suggestion;
update both occurrences (the shown block and the similar block at the later
occurrence) to use the neutral/empty suggestion instead of
SSuggestStringArgRequired/SSuggestJSONFormat.
In `@units/Goccia.Scope.pas`:
- Around line 240-243: Replace direct instantiation and raising of
TGocciaSyntaxError/TGocciaTypeError/TGocciaReferenceError in Goccia.Scope.pas
with the centralized helpers from Goccia.Values.ErrorHelper: use
ThrowSyntaxError, ThrowTypeError or ThrowReferenceError respectively. For each
site (e.g., the block that currently does
TGocciaSyntaxError.Create(Format(SErrorIdentifierAlreadyDeclared, [AName]),
ALine, AColumn, '', nil) followed by Error.Suggestion :=
SSuggestAlreadyDeclared; raise Error) call ThrowSyntaxError with the same
formatted message, ALine and AColumn and set the suggestion through the helper
parameters or by passing the suggestion text as the helper supports; do the
analogous replacement for the other ranges (around lines mentioned: 295-305,
322-325, 335-338, 344-347) replacing TGocciaTypeError and TGocciaReferenceError
usages with ThrowTypeError and ThrowReferenceError while preserving message,
line, column and suggestion values. Ensure you include the appropriate uses
clause for Goccia.Values.ErrorHelper if not already present.
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 65-68: The current generic suggestion SSuggestTemporalFromArg is
too broad; update AsPlainMonthDay to call ThrowTypeError with a narrower
suggestion when the receiver is the wrong type (e.g., "AsPlainMonthDay called on
non-PlainMonthDay — pass a PlainMonthDay Temporal object") instead of
SSuggestTemporalFromArg, and update the MonthDayToPlainDate-related checks to
throw a distinct error message when the input lacks a year property (e.g.,
"MonthDayToPlainDate requires an object with a numeric 'year' property") so
users are not steered toward inputs those branches will still reject; also
replace similar SSuggestTemporalFromArg usages in the other related helper
checks (the other occurrences flagged) with these specific wrong-receiver vs
missing-year messages.
---
Outside diff comments:
In `@units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 431-446: The current Arg handling in the withPlainTime path lets
non-undefined, non-TGocciaTemporalPlainTimeValue and
non-TGocciaStringLiteralValue fall through to midnight; update the conditional
block that checks Arg (the Assigned(Arg) ... branch) to add a final else that
calls ThrowTypeError with an appropriate message for invalid withPlainTime
argument kinds (use the same ThrowTypeError mechanism as used for time parsing
errors), e.g. indicate an unsupported argument type for withPlainTime; reference
TGocciaTemporalPlainTimeValue, TGocciaStringLiteralValue,
TGocciaUndefinedLiteralValue, TryParseISOTime, ThrowTypeError,
SErrorInvalidTimeString and SSuggestTemporalISOFormat so the fix is placed in
the same branch that currently handles those cases.
---
Duplicate comments:
In `@units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 67-68: CoercePlainDate currently hardcodes the Temporal
property-name literals "year", "month", and "day" in property access and error
messages; update it to reference the shared constants PROP_YEAR, PROP_MONTH, and
PROP_DAY from Goccia.Constants.PropertyNames instead. Replace any direct string
keys when reading properties and any occurrences inside
Goccia.Error.Messages/Goccia.Error.Suggestions text construction with those
constants (e.g., in CoercePlainDate and the related validation/formatting code
around the other Temporal plain-date helpers), ensuring you import or qualify
Goccia.Constants.PropertyNames and use the constants in both lookups and
error-message formatting so the names remain in sync.
---
Nitpick comments:
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 101-127: Replace the inline property names in the thrown error
texts with the existing PROP_* constants: change the ThrowTypeError that
currently concatenates ' requires day property' to use PROP_DAY (e.g. Format or
concatenation with PROP_DAY) where VDay is validated; change the message in the
else branch that says ' requires monthCode or month property' to reference
PROP_MONTH_CODE and PROP_MONTH instead of literal names; and ensure any other
error messages related to monthCode/month consistency (the
ThrowTypeError/ThrowRangeError calls inside the VMonthCode handling that
reference monthCode/mismatch) similarly use PROP_MONTH_CODE or PROP_MONTH
constants; locate these usages around symbols VDay, VMonthCode, V, MonthCodeStr,
MonthPart and update message construction to use the PROP_* constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5651445e-8522-47f1-ac76-e6d5232e1411
📒 Files selected for processing (81)
.gitignoreBenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprunits/Goccia.Application.pasunits/Goccia.Builtins.DisposableStack.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalArrayBuffer.pasunits/Goccia.Builtins.GlobalFFI.pasunits/Goccia.Builtins.GlobalMap.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.Builtins.GlobalPromise.pasunits/Goccia.Builtins.GlobalProxy.pasunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Builtins.GlobalRegExp.pasunits/Goccia.Builtins.GlobalString.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.Globals.pasunits/Goccia.Builtins.JSON.pasunits/Goccia.Builtins.JSON5.pasunits/Goccia.Builtins.JSONL.pasunits/Goccia.Builtins.Math.pasunits/Goccia.Builtins.Performance.pasunits/Goccia.Builtins.Semver.pasunits/Goccia.Builtins.TOML.pasunits/Goccia.Builtins.Temporal.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Builtins.YAML.pasunits/Goccia.DisposalTracker.pasunits/Goccia.Error.Detail.pasunits/Goccia.Error.Messages.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.Evaluator.Arithmetic.pasunits/Goccia.Evaluator.Assignment.pasunits/Goccia.Evaluator.TypeOperations.pasunits/Goccia.Evaluator.pasunits/Goccia.ImportMeta.pasunits/Goccia.Scope.pasunits/Goccia.Temporal.Options.pasunits/Goccia.URI.pasunits/Goccia.Utils.Arrays.pasunits/Goccia.VM.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.ClassValue.pasunits/Goccia.Values.Error.pasunits/Goccia.Values.ErrorHelper.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.FunctionValue.pasunits/Goccia.Values.Iterator.Concat.pasunits/Goccia.Values.Iterator.Generic.pasunits/Goccia.Values.Iterator.Lazy.pasunits/Goccia.Values.Iterator.Zip.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectPropertyDescriptor.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.ProxyValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pasunits/Goccia.Values.SymbolValue.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TemporalPlainYearMonth.pasunits/Goccia.Values.TemporalZonedDateTime.pasunits/Goccia.Values.TextDecoderValue.pasunits/Goccia.Values.TextEncoderValue.pasunits/Goccia.Values.ToPrimitive.pasunits/Goccia.Values.TypedArrayValue.pasunits/Goccia.Values.URLSearchParamsValue.pasunits/Goccia.Values.URLValue.pasunits/Goccia.Values.Uint8ArrayEncoding.pas
✅ Files skipped from review due to trivial changes (16)
- .gitignore
- units/Goccia.Builtins.GlobalArrayBuffer.pas
- ScriptLoader.dpr
- units/Goccia.ImportMeta.pas
- units/Goccia.Values.ObjectPropertyDescriptor.pas
- units/Goccia.Evaluator.Arithmetic.pas
- units/Goccia.Values.Iterator.Lazy.pas
- units/Goccia.Builtins.GlobalSymbol.pas
- units/Goccia.Values.SymbolValue.pas
- units/Goccia.Values.ToPrimitive.pas
- units/Goccia.Utils.Arrays.pas
- units/Goccia.Values.URLValue.pas
- units/Goccia.Builtins.GlobalMap.pas
- units/Goccia.Builtins.GlobalRegExp.pas
- units/Goccia.Builtins.GlobalObject.pas
- units/Goccia.Error.Messages.pas
🚧 Files skipped from review as they are similar to previous changes (46)
- units/Goccia.Application.pas
- units/Goccia.Values.FunctionValue.pas
- units/Goccia.Evaluator.Assignment.pas
- units/Goccia.Values.MapValue.pas
- TestRunner.dpr
- units/Goccia.Values.Iterator.Concat.pas
- units/Goccia.Builtins.YAML.pas
- units/Goccia.Builtins.GlobalFFI.pas
- units/Goccia.Values.FunctionBase.pas
- units/Goccia.Values.Iterator.Generic.pas
- units/Goccia.Builtins.GlobalString.pas
- units/Goccia.Evaluator.TypeOperations.pas
- units/Goccia.Temporal.Options.pas
- units/Goccia.Error.Detail.pas
- units/Goccia.Values.ProxyValue.pas
- units/Goccia.DisposalTracker.pas
- units/Goccia.Values.TemporalPlainTime.pas
- units/Goccia.Builtins.TOML.pas
- units/Goccia.Values.TextDecoderValue.pas
- units/Goccia.Values.SharedArrayBufferValue.pas
- units/Goccia.Builtins.GlobalProxy.pas
- units/Goccia.Builtins.Globals.pas
- units/Goccia.Builtins.JSON5.pas
- units/Goccia.Values.Error.pas
- units/Goccia.Values.Uint8ArrayEncoding.pas
- units/Goccia.Values.NumberObjectValue.pas
- units/Goccia.Builtins.Semver.pas
- units/Goccia.Builtins.GlobalReflect.pas
- units/Goccia.Builtins.JSON.pas
- units/Goccia.Builtins.TestAssertions.pas
- units/Goccia.Builtins.GlobalArray.pas
- units/Goccia.Values.IteratorValue.pas
- units/Goccia.Values.ArrayValue.pas
- units/Goccia.Values.TemporalPlainYearMonth.pas
- units/Goccia.Builtins.GlobalPromise.pas
- units/Goccia.Values.StringObjectValue.pas
- units/Goccia.Values.TemporalZonedDateTime.pas
- units/Goccia.Values.ArrayBufferValue.pas
- units/Goccia.Values.TypedArrayValue.pas
- units/Goccia.Values.URLSearchParamsValue.pas
- units/Goccia.VM.pas
- units/Goccia.Values.ObjectValue.pas
- units/Goccia.Builtins.Temporal.pas
- units/Goccia.Values.ClassValue.pas
- units/Goccia.Values.TemporalDuration.pas
- units/Goccia.Error.Suggestions.pas
Comment on lines
+240
to
243
| Error := TGocciaSyntaxError.Create(Format(SErrorIdentifierAlreadyDeclared, [AName]), ALine, AColumn, '', nil); | ||
| Error.Suggestion := SSuggestAlreadyDeclared; | ||
| raise Error; | ||
| end; |
Contributor
There was a problem hiding this comment.
Use Goccia.Values.ErrorHelper for these runtime error paths.
These changed scope-runtime throws are constructed directly via TGocciaSyntaxError/TGocciaTypeError/TGocciaReferenceError. Please route them through ThrowSyntaxError / ThrowTypeError / ThrowReferenceError helpers to keep runtime error construction consistent with repository policy.
As per coding guidelines: "All runtime errors must use error helper functions (ThrowTypeError, ThrowRangeError, ThrowReferenceError, ThrowSyntaxError, ThrowError, ThrowDataCloneError) from Goccia.Values.ErrorHelper.pas. Never create TGocciaError directly."
Also applies to: 295-305, 322-325, 335-338, 344-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Scope.pas` around lines 240 - 243, Replace direct instantiation
and raising of TGocciaSyntaxError/TGocciaTypeError/TGocciaReferenceError in
Goccia.Scope.pas with the centralized helpers from Goccia.Values.ErrorHelper:
use ThrowSyntaxError, ThrowTypeError or ThrowReferenceError respectively. For
each site (e.g., the block that currently does
TGocciaSyntaxError.Create(Format(SErrorIdentifierAlreadyDeclared, [AName]),
ALine, AColumn, '', nil) followed by Error.Suggestion :=
SSuggestAlreadyDeclared; raise Error) call ThrowSyntaxError with the same
formatted message, ALine and AColumn and set the suggestion through the helper
parameters or by passing the suggestion text as the helper supports; do the
analogous replacement for the other ranges (around lines mentioned: 295-305,
322-325, 335-338, 344-347) replacing TGocciaTypeError and TGocciaReferenceError
usages with ThrowTypeError and ThrowReferenceError while preserving message,
line, column and suggestion values. Ensure you include the appropriate uses
clause for Goccia.Values.ErrorHelper if not already present.
Owner
Author
There was a problem hiding this comment.
Not actioned — scope errors use TGocciaError subclasses (TGocciaSyntaxError, TGocciaReferenceError, TGocciaTypeError) which carry source position (line, column, filename, source lines) directly in the constructor. The ErrorHelper.Throw* functions create TGocciaThrowValue instead, which relies on call stack trace extraction for location. These are architecturally different: TGocciaError is for errors with known source positions (parser, scope), TGocciaThrowValue is for runtime JS errors. Switching would lose the precise source position that makes scope errors point to the exact variable declaration.
Contributor
There was a problem hiding this comment.
@frostney, understood — that's a meaningful architectural distinction. TGocciaError subclasses in Goccia.Scope.pas carry precise source position in their constructors, whereas ErrorHelper.Throw* produces TGocciaThrowValue whose location comes from call-stack extraction. Conflating them would lose the exact variable/declaration position in scope errors. I'll update my understanding accordingly.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Scope.pas:240-243
Timestamp: 2026-04-15T20:56:24.170Z
Learning: In frostney/GocciaScript (units/Goccia.Scope.pas and the parser layer), TGocciaSyntaxError, TGocciaReferenceError, and TGocciaTypeError are intentionally constructed directly (not via ErrorHelper.Throw* helpers) because they carry precise source position (line, column, filename, source lines) in their constructors. ErrorHelper.Throw* functions produce TGocciaThrowValue, which relies on call-stack trace extraction for location and is intended for runtime JS errors. Do NOT flag direct TGocciaError subclass construction in Goccia.Scope.pas (DefineLexicalBinding, AssignLexicalBinding, GetLexicalBinding) or in parser code as violating the "use ErrorHelper, never construct TGocciaError directly" rule — these are architecturally distinct paths where the source position is known at throw time.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: units/Goccia.Compiler.Statements.pas:569-570
Timestamp: 2026-04-07T17:12:43.685Z
Learning: In frostney/GocciaScript (units/Goccia.Compiler.Statements.pas, units/Goccia.Engine.Backend.pas, units/Goccia.Scope.Redeclaration.pas), the `EmitGlobalDefine` / `OP_DEFINE_GLOBAL_CONST` calls in `CompileVariableDeclaration`, `CompileClassDeclaration`, `CompileDestructuringDeclaration`, and `CompileEnumDeclaration` intentionally execute *after* the RHS/initializer has been evaluated. Redeclaration protection is handled up-front by `CheckTopLevelRedeclarations` (an AST-level pre-flight walk) called from `TGocciaBytecodeBackend.CompileToModule` (when `GlobalBackedTopLevel` is true) and from `TGocciaEngine.Execute` (after parsing, before execution). This catches all top-level `let`/`const`/class/enum redeclarations before any RHS side effects can run, matching V8 early-error behaviour. Do NOT re-flag the post-evaluation placement of `EmitGlobalDefine` as a side-effect ordering bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Values.ArrayValue.pas:408-410
Timestamp: 2026-04-15T15:41:26.217Z
Learning: In frostney/GocciaScript (units/Goccia.Values.ArrayValue.pas), `TGocciaArrayValue.ThrowError` is a pre-existing internal engine assertion helper that raises `TGocciaError` (a Pascal exception for engine-internal validation). It is architecturally distinct from `Goccia.Values.ErrorHelper` helpers (`ThrowTypeError`, `ThrowRangeError`, etc.) which produce `TGocciaThrowValue` — a JS-visible error catchable from JavaScript `try…catch`. Direct `TGocciaError.Create` usage inside `TGocciaArrayValue.ThrowError` (including the overload with `ASuggestion`) is intentional and must NOT be flagged as violating the "use ErrorHelper, never construct TGocciaError directly" rule; the array method guards are engine assertions, not user-facing JS errors. Do not recommend delegating `TGocciaArrayValue.ThrowError` to `ErrorHelper` helpers.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/interpreter.md:42-42
Timestamp: 2026-04-14T13:26:33.825Z
Learning: In frostney/GocciaScript (units/Goccia.Scope.pas, units/Goccia.Evaluator.pas): `OnError: TGocciaThrowErrorCallback` exists on BOTH `TGocciaScope` (property backed by `FOnError` at line 70, copied from parent via `FOnError := AParent.FOnError` in the constructor at line 153) AND `TGocciaEvaluationContext` (record field). `TGocciaScope.CreateChild` propagates `OnError` to child scopes because it calls `TGocciaScope.Create(Self, ...)` and the constructor copies `FOnError` from the parent. The evaluator additionally syncs `AContext.OnError` to `AContext.Scope.OnError` as a bridge. Do NOT flag the doc claim that "OnError is stored on TGocciaScope and propagated to child scopes" as incorrect — it is accurate.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:0-0
Timestamp: 2026-04-15T15:41:55.194Z
Learning: In this repo’s Pascal code, when handling “assigning to a read-only” errors, use the exact suggestion type `SSuggestReadOnlyProperty`, paired with `SErrorCannotAssignReadOnly`. Keep this mapping consistent everywhere it applies (e.g., for `ObjectValue`, `ClassValue`, and similar value/descriptor assignments), so the same read-only assignment diagnostic always yields the same suggestion/supported error combination.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 51
File: units/Goccia.AST.Expressions.pas:1213-1216
Timestamp: 2026-03-08T04:34:31.685Z
Learning: In `units/Goccia.AST.Expressions.pas`, `TGocciaThisExpression.Evaluate` must use `AContext.Scope.ThisValue` directly (not `FindThisValue` or `ResolveIdentifier('this')`). `AContext.Scope` is always the scope with `this` properly bound at call time (method call scopes set `ThisValue` on construction). `FindThisValue` is for scope-chain-walking scenarios (e.g., nested contexts), not for evaluating the `this` expression itself. `TGocciaScope.ResolveIdentifier('this')` also accesses `FThisValue` directly rather than calling `FindThisValue`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:3436-3437
Timestamp: 2026-04-15T15:41:47.700Z
Learning: Repo frostney/GocciaScript — units/Goccia.VM.pas:
For TGocciaVM.HasPropertyValue’s “in” operator error path (primitives on the RHS), the project intentionally uses SSuggestCheckNullBeforeAccess for all primitive cases (undefined, null, boolean, number, string). Branching the suggestion by value kind is deferred as low priority; do not re-flag this as an issue unless a dedicated follow-up changes that policy.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Values held only by Pascal code (not in any GocciaScript scope) must be protected with `AddTempRoot`/`RemoveTempRoot` for the duration they are needed.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 278
File: units/Goccia.Parser.pas:1071-1111
Timestamp: 2026-04-12T07:39:20.695Z
Learning: Do not free the result of `TGocciaLexer.ScanTokens` (e.g., in `ParseInterpolationExpression` and any other caller). `ScanTokens` returns a reference to the lexer's internal `FTokens: TObjectList<TGocciaToken>` created with `OwnsObjects=True` and freed by `TGocciaLexer`’s destructor. Adding `Tokens.Free` (or otherwise freeing the returned list/object) will cause double-free/access violations; token lifetime is managed entirely by the `TGocciaLexer` instance.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:00.984Z
Learning: Do not re-introduce TScopeMap. In frostney/GocciaScript, TScopeMap<TValue> was removed due to performance regressions from its linear-scan Add-as-upsert behavior. Reviews should flag any use of TScopeMap in Units/ScopeMap.pas or elsewhere and ensure bindings use TOrderedStringMap<TLexicalBinding> instead. If reviewing similar Pascal files, prefer avoiding upsert-like add semantics that overwrite existing entries, as they correlated with bytecode performance drops. Ensure changes adhere to the established replacement: TOrderedStringMap<TLexicalBinding>.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:02.668Z
Learning: Repo: frostney/GocciaScript — ScopeMap.pas (units/ScopeMap.pas) was deleted (commit d99cf96) after profiling showed TScopeMap's linear-scan IndexOf was ~2.7× slower than hash-based lookup (CreateBridgedContext samples rose from 24% to 51%). Scope bindings now use TOrderedStringMap<TLexicalBinding> (DJB2 hash, O(1) per level); scope chain walking is done via recursive TGocciaScope-level methods (Contains/GetLexicalBinding) rather than a parent-pointer chain inside the map. Do not flag TScopeMap or its Parent property in future reviews — the type no longer exists.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.695Z
Learning: Applies to **/*.pas : All runtime errors must use error helper functions (`ThrowTypeError`, `ThrowRangeError`, `ThrowReferenceError`, `ThrowSyntaxError`, `ThrowError`, `ThrowDataCloneError`) from `Goccia.Values.ErrorHelper.pas`. Never create `TGocciaError` directly. This ensures errors are proper JS Error objects with correct prototype chains, catchable from JavaScript `try...catch`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Builtins.TestAssertions.pas:541-550
Timestamp: 2026-04-15T15:41:15.873Z
Learning: In the GocciaScript test framework (notably in units/Goccia.Builtins.TestAssertions.pas and units/Goccia.Error.Detail.pas), `ErrorHelper.ThrowTypeError(msg, suggestion)` should populate `TGocciaThrowValue.FSuggestion`. `FormatThrowDetail` forwards this suggestion to `FormatErrorWithSourceContext` only when source context (stack trace) is available. For native test-framework errors thrown without source context (e.g., from `TGocciaParameterizedRegistrationFunction.Call` or validation helpers inside `TGocciaTestAssertions`), the error formatting is intentionally the stack-trace-only path that omits the suggestion. When reviewing `runTests()` output, do not flag missing suggestion text for these internal, source-context-less test-framework paths, since this behavior is by design.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: TestRunner.dpr:634-636
Timestamp: 2026-04-15T06:27:53.198Z
Learning: In frostney/GocciaScript (PR `#306`, TestRunner.dpr), the `WriteLn` calls inside the error-handling paths of `RunGocciaScriptInterpreted` and `RunGocciaScriptBytecode` (e.g. on EStreamError, TGocciaError, TGocciaThrowValue, and generic Exception) execute on worker threads during parallel directory runs, causing potentially interleaved stdout. This is an accepted trade-off: these paths are rare (script-load failures / uncaught exceptions only), messages include filenames for identification, and a proper output-capture refactor (routing error text through AConsoleOutput/AErrorMessage and printing on the main thread) is deferred to a follow-up. Do NOT re-flag these WriteLn calls in TestRunner.dpr parallel worker context until the output-capture refactor lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:46.617Z
Learning: GocciaScript’s Promise statics are intentionally not species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics. This applies to units/Goccia.Runtime.Operations.pas and related Promise implementations; ensure consistency across related Pas files in the frostney/GocciaScript repository.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/value-system.md:0-0
Timestamp: 2026-04-14T13:25:45.598Z
Learning: In GocciaScript (docs/value-system.md), `TGocciaError` extends Pascal's `Exception`, not `TGocciaValue`. It should not appear in the `TGocciaValue` type hierarchy diagram.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 226
File: units/Goccia.Values.ProxyValue.pas:758-765
Timestamp: 2026-04-08T17:16:34.934Z
Learning: In frostney/GocciaScript, `TGocciaNativeFunctionValue` is a subclass of `TGocciaFunctionBase` (units/Goccia.Values.NativeFunction.pas). An `is TGocciaFunctionBase` check therefore already covers native function targets — do not flag missing separate `TGocciaNativeFunctionValue` branches in fallback call paths (e.g., `ApplyTrap` in `Goccia.Values.ProxyValue.pas`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 228
File: units/Goccia.Builtins.GlobalReflect.pas:0-0
Timestamp: 2026-04-08T16:50:29.388Z
Learning: In frostney/GocciaScript, `RequireObjectTarget` in `units/Goccia.Builtins.GlobalReflect.pas` accepts only `TGocciaObjectValue` as a valid Reflect target. `TGocciaClassValue` is intentionally rejected because it inherits from `TGocciaValue`, not `TGocciaObjectValue`, in GocciaScript's type hierarchy. There is no `AsObjectValue` helper — class-as-object semantics would require a deeper type hierarchy refactoring and are deferred. Do not re-flag the rejection of `TGocciaClassValue` in `RequireObjectTarget`.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Arrow functions (`(x) => x + 1`) are AST: `TGocciaArrowFunctionExpression`, Runtime: `TGocciaArrowFunctionValue`. They always inherit `this` from their lexical (closure) scope via `BindThis` override. Shorthand methods (`method() { ... }`) are AST: `TGocciaMethodExpression`, Runtime: `TGocciaFunctionValue`. They receive call-site `this` from the receiver. `this` binding is resolved via virtual dispatch on `TGocciaFunctionValue.BindThis` — no boolean flags or runtime branches.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/errors.md:0-0
Timestamp: 2026-04-14T15:13:09.996Z
Learning: In frostney/GocciaScript, `SuppressedError` intentionally supports a `cause` property via a 4th constructor argument: `new SuppressedError(error, suppressed, message, options)`. This is a GocciaScript-specific extension beyond the standard ECMAScript spec (MDN states SuppressedError does not accept an options/cause argument). The implementation is in `Goccia.Builtins.Globals.pas` via `AArgs.GetElement(3)`. Do NOT flag GocciaScript's SuppressedError cause support as non-standard or incorrect.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : `type`/`interface` declarations and `import type`/`export type` must produce `TGocciaEmptyStatement` (no-op at runtime). Access modifiers (`public`, `protected`, `private`, `readonly`, `override`, `abstract`) in class bodies must be consumed and discarded.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.694Z
Learning: Applies to **/*.pas : Use `Goccia.Keywords.Reserved` and `Goccia.Keywords.Contextual` constants instead of hardcoded string literals when referencing JavaScript keywords (e.g., `KEYWORD_THIS`, `KEYWORD_SUPER`, `KEYWORD_GET`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T13:53:18.321Z
Learning: In frostney/GocciaScript CLI tool sources, the duplicated `EscapeJSONString` helper (escaping only `\`, `"`, `\n`, `\r`, `\t`, leaving other C0 control characters U+0000–U+001F unescaped) is tracked for consolidation into a shared helper and full fix in issue `#303`. During code review, do not re-flag missing control-character escaping in individual per-tool `EscapeJSONString` copies (e.g., TestRunner.dpr, BenchmarkRunner.dpr) until the consolidation PR landing issue `#303` is resolved.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:28:05.526Z
Learning: Repo: frostney/GocciaScript — Follow-up tracking
Learning: Splitting the blueprint bridge caches (value bridge vs construct bridge) is tracked in issue `#99`. Until that lands, sharing FBlueprintBridgeCache between UnwrapToGocciaValue (BlueprintToClassValue) and Construct (ConvertBlueprintToClassValue) should not block PRs; reference `#99` instead of re-flagging.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:7514-7544
Timestamp: 2026-03-11T08:22:20.482Z
Learning: When bridging a native namespace object to a TSouffleRecord (e.g., performance) in units/Goccia.Runtime.Operations.pas, always set Result.Delegate := ARuntime.VM.RecordDelegate after TSouffleRecord.Create(...). This ensures Object.prototype methods (toString/hasOwnProperty/valueOf/toLocaleString) work in bytecode mode and NativeRecordToString honors Symbol.toStringTag. Apply this pattern to BuildPerformanceRecord and any future bridged records.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/HashMap.pas:266-309
Timestamp: 2026-03-17T22:17:37.410Z
Learning: Do not flag enumeration in THashMap<TKey, TValue> as concurrent-modification unsafe. The no-mutation contract is maintained during enumeration: GetEnumerator shares the internal FSlots array (no per-call allocation) and only mutation occurs via destructor or Clear, not during active iteration. This pattern should be recognized across similar Pascal units/pascal collection implementations; when reviewing code, ensure no concurrent-modification warnings are raised for shared internal structures during enumeration if the no-mutation contract is preserved.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/OrderedMap.pas:0-0
Timestamp: 2026-03-17T22:17:55.588Z
Learning: Enforce the constraint on TOrderedMap<TKey, TValue> in units/OrderedMap.pas: TKey must be an unmanaged, fixed-size value type (no strings, dynamic arrays, or records with managed/pointer fields). HashKey should use fast paths for SizeOf(TKey)=8 (QWord) and SizeOf(TKey)=4 (Cardinal); KeysEqual should compare via direct PQWord/PCardinal for those sizes. The raw-byte DJB2/CompareMem fallback is dead code for current uses. Document this constraint in a unit-header comment as the enforcement mechanism. Do NOT re-flag the byte-wise hash/equality in future reviews as long as all TOrderedMap specializations use pointer-sized unmanaged keys.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:4349-4353
Timestamp: 2026-03-23T12:38:54.412Z
Learning: In Pascal source under units/, when decoding JSON string escape sequences that represent Unicode code points and then converting them to UTF-8, prefer using the existing helper (implemented via CodePointToUTF8) that correctly combines UTF-16 surrogate pairs (\uD800–\uDBFF + \uDC00–\uDFFF) into a single Unicode code point before UTF-8 encoding. Avoid ad-hoc conversions like Char(StrToInt(...)) for code-point→UTF-8 conversion logic; use the helper instead to ensure surrogate pairs are handled correctly.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:6337-6344
Timestamp: 2026-03-23T15:57:55.802Z
Learning: In Pascal unit files under units/, any iterator result record objects produced by native iterators (e.g., in routines analogous to NativeMapIteratorNext / NativeSetIteratorNext) must: (1) set `Rec.Delegate := FVM.RecordDelegate`; and (2) set properties using `PROP_VALUE` / `PROP_DONE` constants from `Goccia.Constants.PropertyNames` rather than hard-coded string literals. Apply the same pattern to any future native iterator result objects so they interoperate correctly in bytecode mode (e.g., for `Object.prototype` method behavior) and keep property-name usage consistent via the shared constants.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 136
File: docs/adding-built-in-types.md:105-133
Timestamp: 2026-03-27T14:09:25.371Z
Learning: In frostney/GocciaScript Pascal code, do not flag or suggest refactoring the early-exit singleton initialization guard `if Assigned(FShared) then Exit` when it appears at the top of `InitializePrototype`. The project documentation treats this early-exit form as the canonical/shared-prototype singleton initialization template (i.e., avoid recommending an `if not Assigned(FShared) then ...` wrapping alternative).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 189
File: units/Goccia.JSON.pas:499-526
Timestamp: 2026-04-05T17:33:32.709Z
Learning: In frostney/GocciaScript, when using `TStringBuffer` from the `StringBuffer` unit, treat it as an advanced record (stack-allocated) with automatic cleanup by the compiler. For `TStringBuffer` locals, do not require or flag missing `SB.Free` calls in `try/finally` blocks—no destructor/free call is needed or appropriate. Only look for `Free` when the variable is actually a heap-allocated class instance.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 189
File: units/Goccia.JSON.pas:535-597
Timestamp: 2026-04-05T17:33:35.381Z
Learning: When reviewing Free Pascal code in this repo, treat `TStringBuffer` (from the `StringBuffer` unit) as an advanced record (value type), meaning instances are stack-allocated. Do not report or require `SB.Free` / `SB.FreeAndNil` (or any `Free` call) for variables typed as `TStringBuffer`, and do not treat an empty or no-op `finally` block after `TStringBuffer` usage as an issue. Only expect heap-management calls for actual `class`/reference types; calling `Free` on an advanced record is incorrect.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.AST.Expressions.pas:1339-1346
Timestamp: 2026-04-07T21:00:57.707Z
Learning: When reviewing GocciaScript coverage/branch tracking, do not require call-sites to explicitly pre-register binary (two-arm) branches before recording hits. In units/Goccia.Coverage.pas, TGocciaFileCoverage.RecordBranchHit automatically ensures the opposite arm exists via EnsureBranchExists(ALine, AColumn, 1 - ABranchIndex), establishing a zero-hit baseline for the paired arm whenever either arm is first recorded. Therefore, call-sites such as TGocciaConditionalExpression.Evaluate (Goccia.AST.Expressions.pas), EvaluateIf/EvaluateBinary/EvaluateSwitch (Goccia.Evaluator.pas), and OP_JUMP_IF_* handlers (Goccia.VM.pas) should not be flagged for missing a separate RegisterBranch call for binary branches—pairing is handled inside RecordBranchHit.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.VM.pas:3547-3561
Timestamp: 2026-04-07T21:01:16.129Z
Learning: When reviewing calls that record branch coverage via `TGocciaFileCoverage.RecordBranchHit(ALine, AColumn, ABranchIndex)` for binary branches (branch indices 0 and 1), do not require an explicit prior `RegisterBranch(...)` at the call site. `RecordBranchHit` internally calls `EnsureBranchExists(ALine, AColumn, 1 - ABranchIndex)` for binary branches, which guarantees both arms exist in the branch list (the untaken arm is pre-registered with zero hits). Therefore VM/evaluator/AST branch recording sites in `Goccia.VM.pas`, `Goccia.Evaluator.pas`, and `Goccia.AST.Expressions.pas` should not be flagged for missing explicit pre-registration before `RecordBranchHit(...)`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 273
File: units/Goccia.Values.URLValue.pas:465-470
Timestamp: 2026-04-11T17:42:27.330Z
Learning: In this codebase, when implementing or updating a `TGCManagedObject.MarkReferences` override for a held reference, treat `MarkReferences` as the act of marking *and* tracing children. The expected pattern is: after a nil check, call the held reference’s `MarkReferences` implementation directly (e.g., `TGocciaSomeValue(FRef).MarkReferences`) rather than invoking any non-existent separate `Mark(obj)`/`Mark()` helper. Do not suggest “call `Mark(obj)`”; this project’s marking flow starts by setting `FGCMark := FCurrentMark` and then tracing child references from within `MarkReferences` (consistent with `TGocciaMapValue`, `TGocciaSetValue`, `TGocciaArrayValue`, etc.).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 271
File: units/Goccia.Builtins.GlobalObject.pas:104-129
Timestamp: 2026-04-11T19:43:06.627Z
Learning: In frostney/GocciaScript, `TGarbageCollector.AddTempRoot`/`RemoveTempRoot` are set-membership (not reference-counted). In built-in method implementations, ONLY call `AddTempRoot`/`RemoveTempRoot` if the value was freshly boxed by `ToObject` (i.e., the argument was not already a `TGocciaObjectValue`). Do not unconditionally add/remove temp roots for values passed in as method arguments, since a single `RemoveTempRoot(Obj)` will remove the root for all holders and can cause use-after-free on the next GC cycle. Pattern: `if Assigned(TGarbageCollector.Instance) and not (AArgs.GetElement(0) is TGocciaObjectValue) then AddTempRoot(Obj); try ... finally if Assigned(TGarbageCollector.Instance) and not (AArgs.GetElement(0) is TGocciaObjectValue) then RemoveTempRoot(Obj); end;`
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:31-37
Timestamp: 2026-04-14T13:36:54.107Z
Learning: When reviewing Free Pascal (FPC) sources in this project, do not flag the compiler directive `{H+}` as a syntax error or as an inconsistency caused by a missing `$`. In FPC, `{H+}` (short-form) is valid and equivalent to `{$H+}` for enabling long/AnsiStrings. This project’s own `Goccia.inc` and documentation intentionally use `{H+}`, so the reviewer should only consider it an issue if it’s not valid FPC syntax for the surrounding context.
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
frostney
force-pushed
the
t3code/align-error-suggestions
branch
from
7532357 to
838db14
Compare
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 33 improved · 🔴 178 regressed · 153 unchanged · avg -2.6% arraybuffer.js — Interp: 🔴 10, 4 unch. · avg -2.7% · Bytecode: 🔴 12, 2 unch. · avg -4.9%
arrays.js — Interp: 🔴 8, 11 unch. · avg -1.4% · Bytecode: 🔴 15, 4 unch. · avg -3.9%
async-await.js — Interp: 🔴 2, 4 unch. · avg -1.1% · Bytecode: 🔴 4, 2 unch. · avg -5.6%
base64.js — Interp: 🟢 1, 🔴 1, 8 unch. · avg -0.8% · Bytecode: 🟢 1, 🔴 4, 5 unch. · avg -1.6%
classes.js — Interp: 🟢 3, 🔴 2, 26 unch. · avg +0.4% · Bytecode: 🔴 13, 18 unch. · avg -3.1%
closures.js — Interp: 🔴 10, 1 unch. · avg -3.9% · Bytecode: 🔴 10, 1 unch. · avg -4.5%
collections.js — Interp: 🔴 10, 2 unch. · avg -6.4% · Bytecode: 🔴 9, 3 unch. · avg -5.9%
destructuring.js — Interp: 🔴 18, 4 unch. · avg -4.7% · Bytecode: 🔴 22 · avg -7.5%
fibonacci.js — Interp: 🔴 5, 3 unch. · avg -3.4% · Bytecode: 🔴 5, 3 unch. · avg -3.2%
float16array.js — Interp: 🟢 1, 🔴 19, 12 unch. · avg -2.3% · Bytecode: 🟢 2, 🔴 18, 12 unch. · avg -3.1%
for-of.js — Interp: 🔴 5, 2 unch. · avg -3.2% · Bytecode: 🔴 7 · avg -6.1%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 22, 20 unch. · avg +2.8% · Bytecode: 🔴 36, 6 unch. · avg -4.6%
json.js — Interp: 🔴 14, 6 unch. · avg -4.7% · Bytecode: 🔴 17, 3 unch. · avg -5.4%
jsx.jsx — Interp: 🟢 1, 🔴 5, 15 unch. · avg -0.8% · Bytecode: 🟢 1, 🔴 7, 13 unch. · avg -1.4%
modules.js — Interp: 🔴 6, 3 unch. · avg -3.6% · Bytecode: 🔴 7, 2 unch. · avg -4.0%
numbers.js — Interp: 🔴 1, 10 unch. · avg -0.5% · Bytecode: 🔴 7, 4 unch. · avg -5.0%
objects.js — Interp: 🔴 6, 1 unch. · avg -4.4% · Bytecode: 🔴 4, 3 unch. · avg -2.5%
promises.js — Interp: 🟢 1, 🔴 2, 9 unch. · avg -0.8% · Bytecode: 🔴 8, 4 unch. · avg -3.0%
regexp.js — Interp: 🔴 7, 4 unch. · avg -2.8% · Bytecode: 🔴 9, 2 unch. · avg -4.5%
strings.js — Interp: 🟢 3, 🔴 14, 2 unch. · avg -4.3% · Bytecode: 🔴 16, 3 unch. · avg -19.6%
typed-arrays.js — Interp: 🟢 1, 🔴 20, 1 unch. · avg -4.4% · Bytecode: 🟢 1, 🔴 19, 2 unch. · avg -2.6%
uint8array-encoding.js — Interp: 🔴 13, 5 unch. · avg -14.8% · Bytecode: 🟢 5, 🔴 12, 1 unch. · avg +3.9%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
units/Goccia.Values.Iterator.Generic.pas (1)
172-172: Consider areturn()-specific suggestion string for this branch.At Line 172, the error message is about
iterator.return(), butSSuggestIteratorResultObjecttext refers tonext(). A dedicated suggestion constant forreturn()would make the hint fully consistent.Possible refinement
- ThrowTypeError(SErrorIteratorReturnObject, SSuggestIteratorResultObject); + ThrowTypeError(SErrorIteratorReturnObject, SSuggestIteratorReturnResultObject);// Add in units/Goccia.Error.Suggestions.pas SSuggestIteratorReturnResultObject = 'iterator return() must return an object with ''done'' and ''value'' properties';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.Iterator.Generic.pas` at line 172, The ThrowTypeError call at ThrowTypeError(SErrorIteratorReturnObject, SSuggestIteratorResultObject) uses SSuggestIteratorResultObject which references next() instead of return(); create a new suggestion constant (e.g., SSuggestIteratorReturnResultObject) in units/Goccia.Error.Suggestions.pas with wording specific to iterator.return() and replace SSuggestIteratorResultObject with SSuggestIteratorReturnResultObject at the ThrowTypeError site so the hint correctly mentions return() and the required 'done' and 'value' properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Builtins.DisposableStack.pas`:
- Line 99: The ThrowReferenceError call at
ThrowReferenceError(SErrorDisposableStackAlreadyDisposed,
SSuggestDisposableStackThisType) uses SSuggestDisposableStackThisType which is
about receiver type, not disposal state; introduce a new suggestion constant
(e.g., SSuggestDisposableStackNotDisposed or similar) with text like "ensure the
DisposableStack is not disposed before calling this method" and replace
SSuggestDisposableStackThisType in the ThrowReferenceError invocation so
SErrorDisposableStackAlreadyDisposed is paired with the disposed-state-specific
suggestion.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 541-550: The error messages currently pass GetFunctionName (which
yields "describe"/"test") into the SErrorFunctionExpects* formats, causing
misuse of describe.each/test.each to show the wrong entrypoint; update the three
ThrowTypeError calls that use GetFunctionName (the ones validating
AArguments.GetElement(0) string, AArguments.GetElement(1) function, and FTable
being TGocciaArrayValue) to pass GetFunctionName + '.each' (or otherwise append
".each" to the function name) as the argument so the formatted messages point to
the correct entrypoint.
In `@units/Goccia.Evaluator.pas`:
- Around line 823-825: The null/undefined property-read fallback throws via
ThrowTypeError(Format(SErrorCannotReadPropertyOf, [PropertyName,
Obj.ToStringLiteral.Value])) but omits the suggestion text present in sibling
branches; update this ThrowTypeError invocation in the same block (the branch
that uses SErrorCannotReadPropertyOf, PropertyName and
Obj.ToStringLiteral.Value) to include the same suggestion/hint string used by
the other branches (mirror their Format usage or append the suggestion variable
they use) so the error message includes the helpful suggestion text just like
the siblings.
In `@units/Goccia.Values.TemporalDuration.pas`:
- Line 94: The TypeError thrown in AsDuration currently uses
SSuggestTemporalFromArg for a bad receiver; update the ThrowTypeError call in
AsDuration to use SSuggestTemporalThisType instead of SSuggestTemporalFromArg so
the diagnostic references the receiver (AThisValue) correctly; locate the
ThrowTypeError invocation inside the AsDuration routine and replace the
suggestion symbol with SSuggestTemporalThisType while keeping the same message
prefix (AMethod + ' called on non-Duration').
In `@units/Goccia.Values.TemporalInstant.pas`:
- Line 64: The AsInstant receiver-type check is using the wrong suggestion
constant: when validating AThisValue and throwing via ThrowTypeError(AMethod + '
called on non-Instant', SSuggestTemporalFromArg) you must use
SSuggestTemporalThisType instead of SSuggestTemporalFromArg; locate the wrong
ThrowTypeError call in AsInstant and swap the suggestion constant so the
wrong-receiver (`this`) diagnostic uses SSuggestTemporalThisType.
In `@units/Goccia.Values.TemporalPlainTime.pas`:
- Line 77: The type-guard for AsPlainTime currently throws
ThrowTypeError(AMethod + ' called on non-PlainTime', SSuggestTemporalFromArg)
while validating the receiver (AThisValue); change the suggestion token to
SSuggestTemporalThisType so the wrong-receiver error uses the receiver-specific
hint. Locate the ThrowTypeError call in the AsPlainTime/type-guard code path and
replace SSuggestTemporalFromArg with SSuggestTemporalThisType, keeping AMethod
and the rest of the call intact.
---
Nitpick comments:
In `@units/Goccia.Values.Iterator.Generic.pas`:
- Line 172: The ThrowTypeError call at
ThrowTypeError(SErrorIteratorReturnObject, SSuggestIteratorResultObject) uses
SSuggestIteratorResultObject which references next() instead of return(); create
a new suggestion constant (e.g., SSuggestIteratorReturnResultObject) in
units/Goccia.Error.Suggestions.pas with wording specific to iterator.return()
and replace SSuggestIteratorResultObject with SSuggestIteratorReturnResultObject
at the ThrowTypeError site so the hint correctly mentions return() and the
required 'done' and 'value' properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00e5d81c-0fdf-4610-a8a2-16997e821912
📒 Files selected for processing (81)
.gitignoreBenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprunits/Goccia.Application.pasunits/Goccia.Builtins.DisposableStack.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalArrayBuffer.pasunits/Goccia.Builtins.GlobalFFI.pasunits/Goccia.Builtins.GlobalMap.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.Builtins.GlobalPromise.pasunits/Goccia.Builtins.GlobalProxy.pasunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Builtins.GlobalRegExp.pasunits/Goccia.Builtins.GlobalString.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.Globals.pasunits/Goccia.Builtins.JSON.pasunits/Goccia.Builtins.JSON5.pasunits/Goccia.Builtins.JSONL.pasunits/Goccia.Builtins.Math.pasunits/Goccia.Builtins.Performance.pasunits/Goccia.Builtins.Semver.pasunits/Goccia.Builtins.TOML.pasunits/Goccia.Builtins.Temporal.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Builtins.YAML.pasunits/Goccia.DisposalTracker.pasunits/Goccia.Error.Detail.pasunits/Goccia.Error.Messages.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.Evaluator.Arithmetic.pasunits/Goccia.Evaluator.Assignment.pasunits/Goccia.Evaluator.TypeOperations.pasunits/Goccia.Evaluator.pasunits/Goccia.ImportMeta.pasunits/Goccia.Scope.pasunits/Goccia.Temporal.Options.pasunits/Goccia.URI.pasunits/Goccia.Utils.Arrays.pasunits/Goccia.VM.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.ClassValue.pasunits/Goccia.Values.Error.pasunits/Goccia.Values.ErrorHelper.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.FunctionValue.pasunits/Goccia.Values.Iterator.Concat.pasunits/Goccia.Values.Iterator.Generic.pasunits/Goccia.Values.Iterator.Lazy.pasunits/Goccia.Values.Iterator.Zip.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectPropertyDescriptor.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.ProxyValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pasunits/Goccia.Values.SymbolValue.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TemporalPlainYearMonth.pasunits/Goccia.Values.TemporalZonedDateTime.pasunits/Goccia.Values.TextDecoderValue.pasunits/Goccia.Values.TextEncoderValue.pasunits/Goccia.Values.ToPrimitive.pasunits/Goccia.Values.TypedArrayValue.pasunits/Goccia.Values.URLSearchParamsValue.pasunits/Goccia.Values.URLValue.pasunits/Goccia.Values.Uint8ArrayEncoding.pas
✅ Files skipped from review due to trivial changes (18)
- .gitignore
- units/Goccia.Values.FunctionValue.pas
- REPL.dpr
- ScriptLoader.dpr
- units/Goccia.Values.FunctionBase.pas
- units/Goccia.Builtins.Math.pas
- units/Goccia.Values.ToPrimitive.pas
- units/Goccia.Values.Error.pas
- units/Goccia.Values.URLValue.pas
- units/Goccia.Values.TextEncoderValue.pas
- units/Goccia.Evaluator.Arithmetic.pas
- units/Goccia.Builtins.GlobalRegExp.pas
- TestRunner.dpr
- units/Goccia.VM.pas
- units/Goccia.Builtins.Performance.pas
- units/Goccia.Builtins.YAML.pas
- units/Goccia.Values.TemporalZonedDateTime.pas
- units/Goccia.Values.TemporalPlainDate.pas
🚧 Files skipped from review as they are similar to previous changes (42)
- units/Goccia.Application.pas
- units/Goccia.Evaluator.TypeOperations.pas
- units/Goccia.ImportMeta.pas
- units/Goccia.Values.FFIPointer.pas
- units/Goccia.Values.Iterator.Zip.pas
- units/Goccia.Builtins.GlobalSymbol.pas
- units/Goccia.Values.Iterator.Concat.pas
- units/Goccia.URI.pas
- units/Goccia.Values.Iterator.Lazy.pas
- units/Goccia.Values.MapValue.pas
- units/Goccia.Values.ObjectPropertyDescriptor.pas
- units/Goccia.Values.NumberObjectValue.pas
- units/Goccia.Builtins.GlobalArray.pas
- units/Goccia.Builtins.TOML.pas
- units/Goccia.Builtins.GlobalFFI.pas
- units/Goccia.Error.Detail.pas
- units/Goccia.Values.SharedArrayBufferValue.pas
- units/Goccia.Values.ArrayValue.pas
- units/Goccia.DisposalTracker.pas
- units/Goccia.Values.TemporalPlainMonthDay.pas
- units/Goccia.Values.Uint8ArrayEncoding.pas
- units/Goccia.Values.TextDecoderValue.pas
- units/Goccia.Builtins.Semver.pas
- units/Goccia.Builtins.GlobalObject.pas
- units/Goccia.Values.PromiseValue.pas
- BenchmarkRunner.dpr
- units/Goccia.Values.TemporalPlainDateTime.pas
- units/Goccia.Builtins.GlobalPromise.pas
- units/Goccia.Values.ClassValue.pas
- units/Goccia.Values.ProxyValue.pas
- units/Goccia.Builtins.JSONL.pas
- units/Goccia.Values.IteratorValue.pas
- units/Goccia.Values.URLSearchParamsValue.pas
- units/Goccia.Values.TemporalPlainYearMonth.pas
- units/Goccia.Temporal.Options.pas
- units/Goccia.Values.ObjectValue.pas
- units/Goccia.Builtins.JSON5.pas
- units/Goccia.Values.TypedArrayValue.pas
- units/Goccia.Builtins.Globals.pas
- units/Goccia.Values.ErrorHelper.pas
- units/Goccia.Error.Messages.pas
- units/Goccia.Error.Suggestions.pas
frostney
force-pushed
the
t3code/align-error-suggestions
branch
from
838db14 to
7581e1e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Builtins.Temporal.pas`:
- Around line 352-353: The code currently calls ThrowTypeError when ISO
parsing/validation fails (e.g., the TryParseISODateTime failure handling using
TGocciaStringLiteralValue(Arg).Value with SErrorInvalidISOInstant /
SSuggestTemporalISOFormat); per the Temporal spec these are value-domain errors
and must raise a RangeError. Replace the ThrowTypeError calls with
ThrowRangeError at this site and the other listed parsing/validation failure
sites (the analogous failure points in Instant.from, PlainDate.from,
PlainTime.from, PlainDateTime.from, PlainMonthDay.from and the other locations
you noted) so that parsing/field-validation failures use ThrowRangeError instead
of ThrowTypeError. Ensure the same error message arguments (e.g.,
SErrorInvalidISOInstant, SSuggestTemporalISOFormat) are preserved when switching
to ThrowRangeError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4b83321-adee-4438-8522-e62b950e7267
📒 Files selected for processing (81)
.gitignoreBenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprunits/Goccia.Application.pasunits/Goccia.Builtins.DisposableStack.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalArrayBuffer.pasunits/Goccia.Builtins.GlobalFFI.pasunits/Goccia.Builtins.GlobalMap.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.Builtins.GlobalPromise.pasunits/Goccia.Builtins.GlobalProxy.pasunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Builtins.GlobalRegExp.pasunits/Goccia.Builtins.GlobalString.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.Globals.pasunits/Goccia.Builtins.JSON.pasunits/Goccia.Builtins.JSON5.pasunits/Goccia.Builtins.JSONL.pasunits/Goccia.Builtins.Math.pasunits/Goccia.Builtins.Performance.pasunits/Goccia.Builtins.Semver.pasunits/Goccia.Builtins.TOML.pasunits/Goccia.Builtins.Temporal.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Builtins.YAML.pasunits/Goccia.DisposalTracker.pasunits/Goccia.Error.Detail.pasunits/Goccia.Error.Messages.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.Evaluator.Arithmetic.pasunits/Goccia.Evaluator.Assignment.pasunits/Goccia.Evaluator.TypeOperations.pasunits/Goccia.Evaluator.pasunits/Goccia.ImportMeta.pasunits/Goccia.Scope.pasunits/Goccia.Temporal.Options.pasunits/Goccia.URI.pasunits/Goccia.Utils.Arrays.pasunits/Goccia.VM.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.ClassValue.pasunits/Goccia.Values.Error.pasunits/Goccia.Values.ErrorHelper.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.FunctionValue.pasunits/Goccia.Values.Iterator.Concat.pasunits/Goccia.Values.Iterator.Generic.pasunits/Goccia.Values.Iterator.Lazy.pasunits/Goccia.Values.Iterator.Zip.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectPropertyDescriptor.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.ProxyValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pasunits/Goccia.Values.SymbolValue.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TemporalPlainYearMonth.pasunits/Goccia.Values.TemporalZonedDateTime.pasunits/Goccia.Values.TextDecoderValue.pasunits/Goccia.Values.TextEncoderValue.pasunits/Goccia.Values.ToPrimitive.pasunits/Goccia.Values.TypedArrayValue.pasunits/Goccia.Values.URLSearchParamsValue.pasunits/Goccia.Values.URLValue.pasunits/Goccia.Values.Uint8ArrayEncoding.pas
✅ Files skipped from review due to trivial changes (21)
- .gitignore
- ScriptLoader.dpr
- units/Goccia.Application.pas
- units/Goccia.Values.FunctionValue.pas
- units/Goccia.ImportMeta.pas
- REPL.dpr
- units/Goccia.Values.FFIPointer.pas
- units/Goccia.DisposalTracker.pas
- TestRunner.dpr
- units/Goccia.Values.ToPrimitive.pas
- units/Goccia.Builtins.Math.pas
- units/Goccia.Builtins.GlobalString.pas
- units/Goccia.Values.TemporalPlainYearMonth.pas
- units/Goccia.Builtins.GlobalRegExp.pas
- units/Goccia.Evaluator.pas
- units/Goccia.Values.TemporalPlainDateTime.pas
- units/Goccia.Values.ClassValue.pas
- units/Goccia.Values.URLValue.pas
- units/Goccia.Values.TemporalZonedDateTime.pas
- units/Goccia.Error.Messages.pas
- units/Goccia.Builtins.GlobalPromise.pas
🚧 Files skipped from review as they are similar to previous changes (41)
- units/Goccia.Evaluator.Assignment.pas
- units/Goccia.Values.Iterator.Lazy.pas
- units/Goccia.Values.Iterator.Concat.pas
- units/Goccia.Evaluator.TypeOperations.pas
- units/Goccia.Builtins.GlobalMap.pas
- units/Goccia.Utils.Arrays.pas
- units/Goccia.Values.Iterator.Generic.pas
- BenchmarkRunner.dpr
- units/Goccia.Values.FunctionBase.pas
- units/Goccia.Values.ObjectPropertyDescriptor.pas
- units/Goccia.Values.NumberObjectValue.pas
- units/Goccia.Evaluator.Arithmetic.pas
- units/Goccia.Builtins.JSONL.pas
- units/Goccia.Builtins.Performance.pas
- units/Goccia.Values.Error.pas
- units/Goccia.Builtins.GlobalFFI.pas
- units/Goccia.Values.SharedArrayBufferValue.pas
- units/Goccia.Values.TemporalInstant.pas
- units/Goccia.Values.ProxyValue.pas
- units/Goccia.Values.TemporalPlainTime.pas
- units/Goccia.Builtins.GlobalProxy.pas
- units/Goccia.Builtins.TOML.pas
- units/Goccia.Temporal.Options.pas
- units/Goccia.Builtins.GlobalReflect.pas
- units/Goccia.Builtins.GlobalObject.pas
- units/Goccia.Values.ObjectValue.pas
- units/Goccia.Values.ArrayBufferValue.pas
- units/Goccia.Values.ArrayValue.pas
- units/Goccia.Values.TypedArrayValue.pas
- units/Goccia.Builtins.JSON5.pas
- units/Goccia.VM.pas
- units/Goccia.Builtins.GlobalSymbol.pas
- units/Goccia.Builtins.YAML.pas
- units/Goccia.Values.TemporalDuration.pas
- units/Goccia.Values.TemporalPlainMonthDay.pas
- units/Goccia.Builtins.TestAssertions.pas
- units/Goccia.Builtins.JSON.pas
- units/Goccia.Values.ErrorHelper.pas
- units/Goccia.Values.TemporalPlainDate.pas
- units/Goccia.Builtins.Semver.pas
- units/Goccia.Error.Suggestions.pas
frostney
force-pushed
the
t3code/align-error-suggestions
branch
from
7581e1e to
6f3e3b6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Evaluator.pas (1)
3833-3846:⚠️ Potential issue | 🟠 MajorBox non-null primitives before object destructuring.
This branch rejects every non-
TGocciaObjectValue, which breaks valid patterns likeconst { length } = 'abc'or{ toFixed } = 1. Per JavaScript semantics, object-pattern destructuring must only rejectnull/undefined; other primitives require coercion to their wrapper objects before property lookup (e.g.,String('abc')for strings,Number(1)for numbers). Array destructuring already handles string primitives correctly (lines ~3751–3774); object destructuring should apply the same coercion strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 3833 - 3846, The current check in the object-destructuring path rejects any non-TGocciaObjectValue (using TGocciaNullLiteralValue/TGocciaUndefinedLiteralValue checks and ThrowTypeError with SErrorCannotDestructure/SErrorCannotDestructureType), which is wrong for primitives; change it to only throw for TGocciaNullLiteralValue or TGocciaUndefinedLiteralValue and for other primitive values box/coerce them into their object wrappers before continuing (i.e., replace the unconditional error branch so that when AValue is not TGocciaObjectValue but also not null/undefined you create the appropriate boxed object value and assign it to ObjectValue, leaving ThrowTypeError only for null/undefined cases), preserving existing error messages and the subsequent use of ObjectValue.
♻️ Duplicate comments (1)
units/Goccia.Builtins.Temporal.pas (1)
352-353:⚠️ Potential issue | 🟠 MajorUse
RangeErrorfor these Temporal value-domain failures.These branches already accepted the argument type and only fail during ISO parsing or numeric-domain validation, so callers currently observe the wrong error class.
Suggested fix pattern
- ThrowTypeError(SErrorInvalidISOInstant, SSuggestTemporalISOFormat); + ThrowRangeError(SErrorInvalidISOInstant, SSuggestTemporalISOFormat);Apply the same swap at the other listed value-domain failure sites.
For the JavaScript Temporal API/proposal, should invalid ISO string/value-domain failures in Temporal.Instant.from/compare, Temporal.PlainDate.from/compare, Temporal.PlainTime.from/compare, Temporal.PlainDateTime.from/compare, and month-out-of-range errors in Temporal.PlainMonthDay.from throw RangeError or TypeError?Also applies to: 406-407, 496-496, 565-565, 643-643, 687-687, 775-775, 819-819, 1063-1064
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Temporal.pas` around lines 352 - 353, The branch currently calls ThrowTypeError when ISO parsing fails (using TryParseISODateTime with TGocciaStringLiteralValue(Arg).Value) but should raise a RangeError; replace the ThrowTypeError(SErrorInvalidISOInstant, SSuggestTemporalISOFormat) call with ThrowRangeError(...) preserving the same message arguments, and apply the same swap (TypeError -> RangeError) at the other value-domain failure sites referenced (lines around 406-407, 496, 565, 643, 687, 775, 819, 1063-1064) so all TryParse* / numeric-domain validation failures use ThrowRangeError instead of ThrowTypeError.
🧹 Nitpick comments (3)
units/Goccia.Values.TemporalPlainYearMonth.pas (1)
159-164: UsePROP_*constants in the touched Temporal lookup paths.This file still hardcodes
year,month, anddayin the updated property-access flow, andCoercePlainYearMonthstill spells out theyear/monthproperty names directly in its error text. Since this unit is already being touched for diagnostic cleanup, it would be better to switch these lookups/messages toPROP_YEAR,PROP_MONTH, andPROP_DAYhere as well.♻️ Suggested cleanup
+ Goccia.Constants.PropertyNames, Goccia.Error.Messages, Goccia.Error.Suggestions, @@ - V := Obj.GetProperty('year'); + V := Obj.GetProperty(PROP_YEAR); if (V = nil) or (V is TGocciaUndefinedLiteralValue) then - ThrowTypeError(AMethod + ' requires year and month properties', SSuggestTemporalFromArg); - VMonth := Obj.GetProperty('month'); + ThrowTypeError( + Format('%s requires %s and %s properties', [AMethod, PROP_YEAR, PROP_MONTH]), + SSuggestTemporalFromArg); + VMonth := Obj.GetProperty(PROP_MONTH); if (VMonth = nil) or (VMonth is TGocciaUndefinedLiteralValue) then - ThrowTypeError(AMethod + ' requires year and month properties', SSuggestTemporalFromArg); + ThrowTypeError( + Format('%s requires %s and %s properties', [AMethod, PROP_YEAR, PROP_MONTH]), + SSuggestTemporalFromArg); @@ - NewYear := GetFieldOr('year', YM.FYear); - NewMonth := GetFieldOr('month', YM.FMonth); + NewYear := GetFieldOr(PROP_YEAR, YM.FYear); + NewMonth := GetFieldOr(PROP_MONTH, YM.FMonth); @@ - V := Obj.GetProperty('day'); + V := Obj.GetProperty(PROP_DAY);Based on learnings: In frostney/GocciaScript, all Temporal value unit implementations must use
Goccia.Constants.PropertyNames::{PROP_YEAR, PROP_MONTH, PROP_MONTH_CODE, PROP_DAY, PROP_CALENDAR_ID}for Temporal property access and related error messages.Also applies to: 333-337, 566-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainYearMonth.pas` around lines 159 - 164, The code hardcodes 'year'/'month'/'day' property names and messages in CoercePlainYearMonth and related lookup paths; replace direct string literals used in Obj.GetProperty and in the ThrowTypeError messages with the constants PROP_YEAR, PROP_MONTH, and PROP_DAY from Goccia.Constants.PropertyNames (and use PROP_MONTH_CODE / PROP_CALENDAR_ID where applicable) so property access and diagnostic text use the canonical constants; update all occurrences in the unit (including the shown GetProperty calls and the other locations around the referenced ranges) to reference the constants instead of raw strings while preserving AMethod and SSuggestTemporalFromArg in the ThrowTypeError calls.units/Goccia.Evaluator.pas (1)
3519-3524: Split the Symbol+shortcut by string-vs-number coercion.This fast path always throws
SErrorSymbolToString, but+only takes the string-coercion route when one operand is already a string. Cases likesym + 1should stay aligned with the normal numeric path and report the Symbol-to-number failure instead of the string one.💡 Proposed adjustment
- if (LeftVal is TGocciaSymbolValue) or (RightVal is TGocciaSymbolValue) then - ThrowTypeError(SErrorSymbolToString, SSuggestSymbolNoImplicitConversion); - if (LeftVal is TGocciaStringLiteralValue) or (RightVal is TGocciaStringLiteralValue) then - Result := TGocciaStringLiteralValue.Create(LeftVal.ToStringLiteral.Value + RightVal.ToStringLiteral.Value) - else - Result := TGocciaNumberLiteralValue.Create(LeftVal.ToNumberLiteral.Value + RightVal.ToNumberLiteral.Value); + if (LeftVal is TGocciaStringLiteralValue) or + (RightVal is TGocciaStringLiteralValue) then + begin + if (LeftVal is TGocciaSymbolValue) or (RightVal is TGocciaSymbolValue) then + ThrowTypeError(SErrorSymbolToString, SSuggestSymbolNoImplicitConversion); + Result := TGocciaStringLiteralValue.Create( + LeftVal.ToStringLiteral.Value + RightVal.ToStringLiteral.Value); + end + else + begin + if (LeftVal is TGocciaSymbolValue) or (RightVal is TGocciaSymbolValue) then + ThrowTypeError(SErrorSymbolToNumber, SSuggestSymbolNoImplicitConversion); + Result := TGocciaNumberLiteralValue.Create( + LeftVal.ToNumberLiteral.Value + RightVal.ToNumberLiteral.Value); + end;Based on learnings,
TGocciaSymbolValue.ToNumberLiteralmust throwTypeError, while implicit string coercion in the+operator must handle symbols at the operator level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 3519 - 3524, The current fast-path always throws SErrorSymbolToString when either operand is TGocciaSymbolValue; instead, remove that early throw and only raise the symbol-to-string TypeError when taking the string-concatenation branch. Concretely: in the '+' handling code (where TGocciaStringLiteralValue, TGocciaNumberLiteralValue, TGocciaSymbolValue are inspected), don't call ThrowTypeError when a symbol is present up-front; first test whether either operand is a TGocciaStringLiteralValue — if so, perform ToStringLiteral on both operands and if any ToStringLiteral detects a symbol then call ThrowTypeError(SErrorSymbolToString, SSuggestSymbolNoImplicitConversion); otherwise take the numeric path and call ToNumberLiteral on both operands so TGocciaSymbolValue.ToNumberLiteral can raise the proper symbol-to-number TypeError.units/Goccia.Values.ErrorHelper.pas (1)
34-39: Consider addingThrowInvalidCharacterErrorsuggestion overload for API symmetry.Most throw helpers now have
(AMessage, ASuggestion)overloads, but InvalidCharacterError remains message-only. Optional, but adding it would keep the helper surface consistent.♻️ Suggested consistency patch
-{ Raises a TGocciaThrowValue with an InvalidCharacterError (DOMException with code 5) } -procedure ThrowInvalidCharacterError(const AMessage: string); +{ Raises a TGocciaThrowValue with an InvalidCharacterError (DOMException with code 5) } +procedure ThrowInvalidCharacterError(const AMessage: string); overload; +procedure ThrowInvalidCharacterError(const AMessage, ASuggestion: string); overload;procedure ThrowInvalidCharacterError(const AMessage: string); var ErrorObj: TGocciaObjectValue; begin ErrorObj := CreateErrorObject(INVALID_CHARACTER_ERROR_NAME, AMessage); ErrorObj.HasErrorData := False; ErrorObj.AssignProperty(PROP_CODE, TGocciaNumberLiteralValue.Create(5)); if Assigned(GDOMExceptionProto) then ErrorObj.Prototype := GDOMExceptionProto; raise TGocciaThrowValue.Create(ErrorObj); end; + +procedure ThrowInvalidCharacterError(const AMessage, ASuggestion: string); +var + ErrorObj: TGocciaObjectValue; +begin + ErrorObj := CreateErrorObject(INVALID_CHARACTER_ERROR_NAME, AMessage); + ErrorObj.HasErrorData := False; + ErrorObj.AssignProperty(PROP_CODE, TGocciaNumberLiteralValue.Create(5)); + if Assigned(GDOMExceptionProto) then + ErrorObj.Prototype := GDOMExceptionProto; + raise TGocciaThrowValue.Create(ErrorObj, ASuggestion); +end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ErrorHelper.pas` around lines 34 - 39, Add a second overload for ThrowInvalidCharacterError that accepts (const AMessage, ASuggestion: string) to mirror the other throw helpers; declare the new overload alongside the existing procedure and implement it to construct/raise the same TGocciaThrowValue/DOMException (code 5) as the message-only variant but include ASuggestion in the thrown value (same pattern used by ThrowURIError(const AMessage, ASuggestion) and other helpers) so the API surface is symmetric.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Values.SharedArrayBufferValue.pas`:
- Around line 149-157: The code currently calls
ThrowRangeError(SErrorInvalidSharedArrayBufferLength,
SSuggestSharedArrayBufferThisType) when IntegerIndex is out of range (and at the
other noted site), which incorrectly uses the “wrong receiver” hint; change
these calls to either omit the suggestion parameter or pass a new dedicated hint
constant (e.g. SSuggestSharedArrayBufferLength or
SSuggestSharedArrayBufferSpecies) instead of SSuggestSharedArrayBufferThisType
so that ThrowRangeError is invoked with a correct suggestion for length/species
failures; update both occurrences around IntegerIndex and the other referenced
location (253-254) to use the new or no-suggestion identifier consistently.
In `@units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 144-145: Replace the incorrect ThrowTypeError calls with
ThrowRangeError in the Temporal string-parsing failure branches: specifically
change the ThrowTypeError used when
TryParseISODateTime(TGocciaStringLiteralValue(AValue).Value, DateRec, TimeRec)
fails (the call that uses SErrorTemporalInvalidISOStringFor and
SSuggestTemporalISOFormat) and the three other ThrowTypeError occurrences that
are raised for invalid ISO/time/duration string parsing in this unit; keep the
existing error message formatting and arguments but use ThrowRangeError instead
of ThrowTypeError.
---
Outside diff comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 3833-3846: The current check in the object-destructuring path
rejects any non-TGocciaObjectValue (using
TGocciaNullLiteralValue/TGocciaUndefinedLiteralValue checks and ThrowTypeError
with SErrorCannotDestructure/SErrorCannotDestructureType), which is wrong for
primitives; change it to only throw for TGocciaNullLiteralValue or
TGocciaUndefinedLiteralValue and for other primitive values box/coerce them into
their object wrappers before continuing (i.e., replace the unconditional error
branch so that when AValue is not TGocciaObjectValue but also not null/undefined
you create the appropriate boxed object value and assign it to ObjectValue,
leaving ThrowTypeError only for null/undefined cases), preserving existing error
messages and the subsequent use of ObjectValue.
---
Duplicate comments:
In `@units/Goccia.Builtins.Temporal.pas`:
- Around line 352-353: The branch currently calls ThrowTypeError when ISO
parsing fails (using TryParseISODateTime with
TGocciaStringLiteralValue(Arg).Value) but should raise a RangeError; replace the
ThrowTypeError(SErrorInvalidISOInstant, SSuggestTemporalISOFormat) call with
ThrowRangeError(...) preserving the same message arguments, and apply the same
swap (TypeError -> RangeError) at the other value-domain failure sites
referenced (lines around 406-407, 496, 565, 643, 687, 775, 819, 1063-1064) so
all TryParse* / numeric-domain validation failures use ThrowRangeError instead
of ThrowTypeError.
---
Nitpick comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 3519-3524: The current fast-path always throws
SErrorSymbolToString when either operand is TGocciaSymbolValue; instead, remove
that early throw and only raise the symbol-to-string TypeError when taking the
string-concatenation branch. Concretely: in the '+' handling code (where
TGocciaStringLiteralValue, TGocciaNumberLiteralValue, TGocciaSymbolValue are
inspected), don't call ThrowTypeError when a symbol is present up-front; first
test whether either operand is a TGocciaStringLiteralValue — if so, perform
ToStringLiteral on both operands and if any ToStringLiteral detects a symbol
then call ThrowTypeError(SErrorSymbolToString,
SSuggestSymbolNoImplicitConversion); otherwise take the numeric path and call
ToNumberLiteral on both operands so TGocciaSymbolValue.ToNumberLiteral can raise
the proper symbol-to-number TypeError.
In `@units/Goccia.Values.ErrorHelper.pas`:
- Around line 34-39: Add a second overload for ThrowInvalidCharacterError that
accepts (const AMessage, ASuggestion: string) to mirror the other throw helpers;
declare the new overload alongside the existing procedure and implement it to
construct/raise the same TGocciaThrowValue/DOMException (code 5) as the
message-only variant but include ASuggestion in the thrown value (same pattern
used by ThrowURIError(const AMessage, ASuggestion) and other helpers) so the API
surface is symmetric.
In `@units/Goccia.Values.TemporalPlainYearMonth.pas`:
- Around line 159-164: The code hardcodes 'year'/'month'/'day' property names
and messages in CoercePlainYearMonth and related lookup paths; replace direct
string literals used in Obj.GetProperty and in the ThrowTypeError messages with
the constants PROP_YEAR, PROP_MONTH, and PROP_DAY from
Goccia.Constants.PropertyNames (and use PROP_MONTH_CODE / PROP_CALENDAR_ID where
applicable) so property access and diagnostic text use the canonical constants;
update all occurrences in the unit (including the shown GetProperty calls and
the other locations around the referenced ranges) to reference the constants
instead of raw strings while preserving AMethod and SSuggestTemporalFromArg in
the ThrowTypeError calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2792b78-519e-46c5-9488-b00a2fd43fcc
📒 Files selected for processing (81)
.gitignoreBenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprunits/Goccia.Application.pasunits/Goccia.Builtins.DisposableStack.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalArrayBuffer.pasunits/Goccia.Builtins.GlobalFFI.pasunits/Goccia.Builtins.GlobalMap.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.Builtins.GlobalPromise.pasunits/Goccia.Builtins.GlobalProxy.pasunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Builtins.GlobalRegExp.pasunits/Goccia.Builtins.GlobalString.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.Globals.pasunits/Goccia.Builtins.JSON.pasunits/Goccia.Builtins.JSON5.pasunits/Goccia.Builtins.JSONL.pasunits/Goccia.Builtins.Math.pasunits/Goccia.Builtins.Performance.pasunits/Goccia.Builtins.Semver.pasunits/Goccia.Builtins.TOML.pasunits/Goccia.Builtins.Temporal.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Builtins.YAML.pasunits/Goccia.DisposalTracker.pasunits/Goccia.Error.Detail.pasunits/Goccia.Error.Messages.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.Evaluator.Arithmetic.pasunits/Goccia.Evaluator.Assignment.pasunits/Goccia.Evaluator.TypeOperations.pasunits/Goccia.Evaluator.pasunits/Goccia.ImportMeta.pasunits/Goccia.Scope.pasunits/Goccia.Temporal.Options.pasunits/Goccia.URI.pasunits/Goccia.Utils.Arrays.pasunits/Goccia.VM.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.ClassValue.pasunits/Goccia.Values.Error.pasunits/Goccia.Values.ErrorHelper.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.FunctionValue.pasunits/Goccia.Values.Iterator.Concat.pasunits/Goccia.Values.Iterator.Generic.pasunits/Goccia.Values.Iterator.Lazy.pasunits/Goccia.Values.Iterator.Zip.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectPropertyDescriptor.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.ProxyValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pasunits/Goccia.Values.SymbolValue.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TemporalPlainYearMonth.pasunits/Goccia.Values.TemporalZonedDateTime.pasunits/Goccia.Values.TextDecoderValue.pasunits/Goccia.Values.TextEncoderValue.pasunits/Goccia.Values.ToPrimitive.pasunits/Goccia.Values.TypedArrayValue.pasunits/Goccia.Values.URLSearchParamsValue.pasunits/Goccia.Values.URLValue.pasunits/Goccia.Values.Uint8ArrayEncoding.pas
✅ Files skipped from review due to trivial changes (10)
- units/Goccia.Builtins.Performance.pas
- TestRunner.dpr
- units/Goccia.Values.FunctionBase.pas
- .gitignore
- units/Goccia.DisposalTracker.pas
- units/Goccia.Values.Uint8ArrayEncoding.pas
- units/Goccia.Values.URLValue.pas
- units/Goccia.Builtins.GlobalRegExp.pas
- units/Goccia.Builtins.GlobalSymbol.pas
- units/Goccia.Error.Messages.pas
🚧 Files skipped from review as they are similar to previous changes (44)
- units/Goccia.Values.FunctionValue.pas
- units/Goccia.Utils.Arrays.pas
- units/Goccia.Application.pas
- units/Goccia.Values.Iterator.Concat.pas
- units/Goccia.Values.Iterator.Lazy.pas
- units/Goccia.Evaluator.Assignment.pas
- units/Goccia.Values.ObjectPropertyDescriptor.pas
- units/Goccia.Evaluator.TypeOperations.pas
- units/Goccia.Values.FFIPointer.pas
- units/Goccia.Values.MapValue.pas
- units/Goccia.Builtins.GlobalMap.pas
- units/Goccia.Builtins.JSONL.pas
- units/Goccia.Builtins.DisposableStack.pas
- units/Goccia.Values.ToPrimitive.pas
- units/Goccia.Builtins.GlobalFFI.pas
- units/Goccia.Values.NumberObjectValue.pas
- units/Goccia.Values.Iterator.Generic.pas
- units/Goccia.Builtins.GlobalObject.pas
- units/Goccia.Builtins.GlobalString.pas
- units/Goccia.Values.Error.pas
- BenchmarkRunner.dpr
- units/Goccia.Values.TemporalInstant.pas
- units/Goccia.Builtins.GlobalArray.pas
- units/Goccia.Builtins.Globals.pas
- units/Goccia.Values.PromiseValue.pas
- units/Goccia.Builtins.GlobalReflect.pas
- units/Goccia.Builtins.GlobalProxy.pas
- units/Goccia.Temporal.Options.pas
- units/Goccia.Builtins.JSON.pas
- units/Goccia.Builtins.Semver.pas
- units/Goccia.Values.TextEncoderValue.pas
- units/Goccia.Values.StringObjectValue.pas
- units/Goccia.Values.ObjectValue.pas
- units/Goccia.Values.ClassValue.pas
- units/Goccia.Builtins.GlobalPromise.pas
- units/Goccia.Scope.pas
- units/Goccia.Evaluator.Arithmetic.pas
- units/Goccia.Values.TemporalPlainMonthDay.pas
- units/Goccia.Values.TemporalZonedDateTime.pas
- units/Goccia.Values.ArrayValue.pas
- units/Goccia.Builtins.TestAssertions.pas
- units/Goccia.Values.TemporalDuration.pas
- units/Goccia.VM.pas
- units/Goccia.Error.Suggestions.pas
- Thread suggestion hints through throw formatting - Replace ad hoc error strings with shared message constants - Add shell helper for Pascal assembly build step
frostney
force-pushed
the
t3code/align-error-suggestions
branch
from
6f3e3b6 to
9c6134d
Compare
This was referenced Apr 16, 2026
This was referenced May 5, 2026
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
1 participant
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.
Summary
build/ppas.shto support assembling and linking the generated Pascal artifacts on macOS.Testing
./build.pas testrunner && ./build.pas loader && ./build.pas repl && ./build.pas benchmarkrunner../build/TestRunner testsand confirm suggestion text appears in thrown-value error output where expected.