Add AnnotationEmitCollection instruction and format standard output annotations - #1014
Add AnnotationEmitCollection instruction and format standard output annotations#1014HarshPopat23 wants to merge 2 commits into
Conversation
…nnotations Signed-off-by: HarshPopat23 <musichk61@gmail.com>
d93eb41 to
2a161cf
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/output/output_standard_basic.json">
<violation number="1" location="test/output/output_standard_basic.json:42">
P2: The new `success_properties_multiple` test expects the `properties` collection annotation in alphabetical order `["bar", "foo"]`, but this file is the shared suite run by both the C++ and JavaScript ports via `output.test.mjs`. C++ produces alphabetical order (its `prettify_annotations_multiple` test matches), yet the JS `LoopProperties` handler in `ports/javascript/index.mjs` iterates instance keys with `for (const key in target)`, which for instance `{"foo":"a","bar":"b"}` yields `["foo","bar"]`. The two ports will serialize the same annotation in different orders, so this expectation cannot hold for both ports and the shared JS suite will diverge from the C++ expectation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "keywordLocation": "/properties", | ||
| "absoluteKeywordLocation": "#/properties", | ||
| "instanceLocation": "", | ||
| "annotation": [ "bar", "foo" ] |
There was a problem hiding this comment.
P2: The new success_properties_multiple test expects the properties collection annotation in alphabetical order ["bar", "foo"], but this file is the shared suite run by both the C++ and JavaScript ports via output.test.mjs. C++ produces alphabetical order (its prettify_annotations_multiple test matches), yet the JS LoopProperties handler in ports/javascript/index.mjs iterates instance keys with for (const key in target), which for instance {"foo":"a","bar":"b"} yields ["foo","bar"]. The two ports will serialize the same annotation in different orders, so this expectation cannot hold for both ports and the shared JS suite will diverge from the C++ expectation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/output/output_standard_basic.json, line 42:
<comment>The new `success_properties_multiple` test expects the `properties` collection annotation in alphabetical order `["bar", "foo"]`, but this file is the shared suite run by both the C++ and JavaScript ports via `output.test.mjs`. C++ produces alphabetical order (its `prettify_annotations_multiple` test matches), yet the JS `LoopProperties` handler in `ports/javascript/index.mjs` iterates instance keys with `for (const key in target)`, which for instance `{"foo":"a","bar":"b"}` yields `["foo","bar"]`. The two ports will serialize the same annotation in different orders, so this expectation cannot hold for both ports and the shared JS suite will diverge from the C++ expectation.</comment>
<file context>
@@ -20,6 +20,30 @@
+ "keywordLocation": "/properties",
+ "absoluteKeywordLocation": "#/properties",
+ "instanceLocation": "",
+ "annotation": [ "bar", "foo" ]
+ }
+ ]
</file context>
| "annotation": [ "bar", "foo" ] | |
| "annotation": [ "foo", "bar" ] |
|
cc: @jviotti |
| ControlDynamicAnchorJump, | ||
| ControlJump}; | ||
| ControlJump, | ||
| AnnotationEmitCollection}; |
There was a problem hiding this comment.
Can you put this one alongside the other annotation instructions and bump the template serialisation version instead?
There was a problem hiding this comment.
Done! I've placed AnnotationEmitWrapped alongside the other annotation instructions (index 51) and bumped JSON_VERSION from 6 to 7.
| value); | ||
| } | ||
|
|
||
| INSTRUCTION_HANDLER(AnnotationEmitCollection) { |
There was a problem hiding this comment.
The whole code looks good but I wonder if EmitCollection is the right name here. I think the idea of a "collection" here is a bit confusing to me. What about AnnotationEmitWrapped? I think that seems be more obvious to me? and you can rename the little helper in the standard output format and the variables accordingly?
Just a minor thing really, but I find "collection" confusing
There was a problem hiding this comment.
Done! Renamed AnnotationEmitCollection to AnnotationEmitWrapped, is_collection_annotation to is_wrapped_annotation, and is_collection to is_wrapped across the C++ and JavaScript codebases.
|
Some minor comments, but looks very solid so far |
1bf7745 to
79d2f53
Compare
There was a problem hiding this comment.
2 issues found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/evaluator/include/sourcemeta/blaze/evaluator.h">
<violation number="1" location="src/evaluator/include/sourcemeta/blaze/evaluator.h:49">
P2: The JSON_VERSION bump to 7 breaks the JS port's version tests in ports/javascript/official.test.mjs, which still expect the `'Only version 6 ...'` error message. With version 7, the `[7, false, false, [[]], [], []]` template is accepted as a valid version so `assert.throws` gets no error, and the `{}` case now throws `'Only version 7 ...'`, mismatching the asserted string. Update those two assertions (and the test template to an unsupported version such as 6) in ports/javascript/official.test.mjs to match the new version.</violation>
</file>
<file name="ports/javascript/opcodes.mjs">
<violation number="1" location="ports/javascript/opcodes.mjs:52">
P1: The rename leaves stale `ANNOTATION_EMIT_COLLECTION` references in the JavaScript callback and description paths. A `properties` annotation now uses opcode 51, so trace and standard-output validation throw `ReferenceError` instead of reporting the annotation; update those comparisons to `ANNOTATION_EMIT_WRAPPED`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| export const CONTROL_EVALUATE = 97; | ||
| export const CONTROL_DYNAMIC_ANCHOR_JUMP = 98; | ||
| export const CONTROL_JUMP = 99; | ||
| export const ANNOTATION_EMIT_WRAPPED = 51; |
There was a problem hiding this comment.
P1: The rename leaves stale ANNOTATION_EMIT_COLLECTION references in the JavaScript callback and description paths. A properties annotation now uses opcode 51, so trace and standard-output validation throw ReferenceError instead of reporting the annotation; update those comparisons to ANNOTATION_EMIT_WRAPPED.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ports/javascript/opcodes.mjs, line 52:
<comment>The rename leaves stale `ANNOTATION_EMIT_COLLECTION` references in the JavaScript callback and description paths. A `properties` annotation now uses opcode 51, so trace and standard-output validation throw `ReferenceError` instead of reporting the annotation; update those comparisons to `ANNOTATION_EMIT_WRAPPED`.</comment>
<file context>
@@ -49,56 +49,56 @@ export const ASSERTION_ARRAY_PREFIX = 47;
-export const CONTROL_DYNAMIC_ANCHOR_JUMP = 98;
-export const CONTROL_JUMP = 99;
-export const ANNOTATION_EMIT_COLLECTION = 100;
+export const ANNOTATION_EMIT_WRAPPED = 51;
+export const ANNOTATION_TO_PARENT = 52;
+export const ANNOTATION_BASENAME_TO_PARENT = 53;
</file context>
|
|
||
| /// @ingroup evaluator | ||
| constexpr std::size_t JSON_VERSION{6}; | ||
| constexpr std::size_t JSON_VERSION{7}; |
There was a problem hiding this comment.
P2: The JSON_VERSION bump to 7 breaks the JS port's version tests in ports/javascript/official.test.mjs, which still expect the 'Only version 6 ...' error message. With version 7, the [7, false, false, [[]], [], []] template is accepted as a valid version so assert.throws gets no error, and the {} case now throws 'Only version 7 ...', mismatching the asserted string. Update those two assertions (and the test template to an unsupported version such as 6) in ports/javascript/official.test.mjs to match the new version.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/evaluator/include/sourcemeta/blaze/evaluator.h, line 49:
<comment>The JSON_VERSION bump to 7 breaks the JS port's version tests in ports/javascript/official.test.mjs, which still expect the `'Only version 6 ...'` error message. With version 7, the `[7, false, false, [[]], [], []]` template is accepted as a valid version so `assert.throws` gets no error, and the `{}` case now throws `'Only version 7 ...'`, mismatching the asserted string. Update those two assertions (and the test template to an unsupported version such as 6) in ports/javascript/official.test.mjs to match the new version.</comment>
<file context>
@@ -46,7 +46,7 @@ struct Template {
/// @ingroup evaluator
-constexpr std::size_t JSON_VERSION{6};
+constexpr std::size_t JSON_VERSION{7};
/// @ingroup evaluator
</file context>
79d2f53 to
cce674e
Compare
…plate version to 7 Rename instruction to AnnotationEmitWrapped and place alongside other annotation instructions. Rename is_collection_annotation to is_wrapped_annotation and is_collection to is_wrapped. Bump template serialization version JSON_VERSION from 6 to 7. Update ports/javascript opcodes, handlers, and fastHandlers dispatch table. Update evaluator describe, compiler, output formatters, and unit tests. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
cce674e to
53e20e9
Compare
2 participants
Description
This PR introduces the \AnnotationEmitCollection\ instruction to distinguish collection-shaped annotations (such as property names from the \properties\ keyword) from scalar/singleton annotations (such as metadata keywords \ itle, \description,
eadOnly, \default, and unknown keyword annotations under custom vocabularies).
This is the prerequisite PR requested in #978 to separate instruction design and standard output value wrapping from the official output test suite integration.
Changes
Ref: #978