Address Copilot review feedback on the libfyaml backend

Reject canonical output with NotImplementedError instead of silently
ignoring the request, and honor the implicit flag in start_sequence and
start_mapping so an implicit tag is not printed as a redundant verbose
tag. Relax the unquoted-boolean dump test to allow an optional document
end marker. Also correct the version docstrings and a stale comment
about how the parser collects diagnostics.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: hsbt

ext/psych/psych.c

@@ -2,7 +2,9 @@
 
 /* call-seq: Psych.libyaml_version
  *
- * Returns the version of libyaml being used
+ * Returns the version of the underlying YAML library as a three-element
+ * array. This is libyaml by default. On the experimental libfyaml backend,
+ * where libyaml is not linked, it reports the libfyaml version instead.
  */
 static VALUE libyaml_version(VALUE module)
 {
@@ -30,7 +32,9 @@ static VALUE libyaml_version(VALUE module)
 #ifdef PSYCH_USE_LIBFYAML
 /* call-seq: Psych.libfyaml_version
  *
- * Returns the libfyaml version string, or nil when not built with libfyaml.
+ * Returns the libfyaml version string. This method is only defined when
+ * psych was built with the experimental libfyaml backend
+ * (+--enable-libfyaml+).
  */
 static VALUE libfyaml_version(VALUE module)
 {

ext/psych/psych_emitter_fy.c

@@ -122,7 +122,12 @@ static VALUE initialize(int argc, VALUE *argv, VALUE self)
     if (rb_scan_args(argc, argv, "11", &io, &options) == 2) {
         e->width     = NUM2INT(rb_funcall(options, id_line_width, 0));
         e->indent    = NUM2INT(rb_funcall(options, id_indentation, 0));
-        e->canonical = (Qtrue == rb_funcall(options, id_canonical, 0)) ? 1 : 0;
+        /* libfyaml has no canonical emit mode, so fail fast instead of
+         * silently producing non-canonical output. */
+        if (RTEST(rb_funcall(options, id_canonical, 0))) {
+            rb_raise(rb_eNotImpError,
+                     "canonical output is not supported by the libfyaml backend");
+        }
     }
 
     rb_ivar_set(self, id_io, io);
@@ -329,10 +334,14 @@ static VALUE start_sequence(VALUE self, VALUE anchor, VALUE tag,
     if (!NIL_P(anchor)) { Check_Type(anchor, T_STRING); anchor = rb_str_export_to_enc(anchor, encoding); }
     if (!NIL_P(tag))    { Check_Type(tag, T_STRING);    tag    = rb_str_export_to_enc(tag, encoding); }
 
+    /* An implicit tag can be omitted, matching libyaml; emitting it anyway
+     * would print a redundant (often verbose) tag. */
+    int emit_tag = !NIL_P(tag) && !RTEST(implicit);
+
     struct fy_event *event = fy_emit_event_create(e->emit, FYET_SEQUENCE_START,
             psych_to_fyns(NUM2INT(style)),
             NIL_P(anchor) ? NULL : StringValueCStr(anchor),
-            NIL_P(tag) ? NULL : StringValueCStr(tag));
+            emit_tag ? StringValueCStr(tag) : NULL);
 
     do_emit(e, event);
     RB_GC_GUARD(anchor);
@@ -360,10 +369,14 @@ static VALUE start_mapping(VALUE self, VALUE anchor, VALUE tag,
     if (!NIL_P(anchor)) { Check_Type(anchor, T_STRING); anchor = rb_str_export_to_enc(anchor, encoding); }
     if (!NIL_P(tag))    { Check_Type(tag, T_STRING);    tag    = rb_str_export_to_enc(tag, encoding); }
 
+    /* An implicit tag can be omitted, matching libyaml; emitting it anyway
+     * would print a redundant (often verbose) tag. */
+    int emit_tag = !NIL_P(tag) && !RTEST(implicit);
+
     struct fy_event *event = fy_emit_event_create(e->emit, FYET_MAPPING_START,
             psych_to_fyns(NUM2INT(style)),
             NIL_P(anchor) ? NULL : StringValueCStr(anchor),
-            NIL_P(tag) ? NULL : StringValueCStr(tag));
+            emit_tag ? StringValueCStr(tag) : NULL);
 
     do_emit(e, event);
     RB_GC_GUARD(anchor);
@@ -397,8 +410,12 @@ static VALUE set_canonical(VALUE self, VALUE style)
 {
     psych_fy_emitter_t *e;
     TypedData_Get_Struct(self, psych_fy_emitter_t, &psych_emitter_type, e);
-    e->canonical = (Qtrue == style) ? 1 : 0;
-    rebuild_emitter(self, e);
+    /* libfyaml has no canonical emit mode, so reject enabling it rather than
+     * pretending to honor the request. */
+    if (RTEST(style)) {
+        rb_raise(rb_eNotImpError,
+                 "canonical output is not supported by the libfyaml backend");
+    }
     return style;
 }
 

ext/psych/psych_parser_fy.c

@@ -98,8 +98,8 @@ static VALUE allocate(VALUE klass)
 }
 
 /* Reconstruct a Psych::SyntaxError from libfyaml's collected diagnostics.  The
- * parser is created with FYPCF_COLLECT_DIAG, so the first collected error gives
- * us the message and position. */
+ * parser's diag was switched to collect mode with fy_diag_set_collect_errors()
+ * in parse(), so the first collected error gives us the message and position. */
 static VALUE make_exception(psych_fy_parser_t *parser, VALUE path)
 {
     VALUE ePsychSyntaxError = rb_const_get(mPsych, rb_intern("SyntaxError"));

test/psych/test_emitter.rb

@@ -17,6 +17,14 @@ def test_line_width
     end
 
     def test_set_canonical
+      if libfyaml?
+        # The libfyaml backend has no canonical mode and rejects enabling it.
+        assert_raise(NotImplementedError) { @emitter.canonical = true }
+        @emitter.canonical = false
+        assert_equal false, @emitter.canonical
+        return
+      end
+
       @emitter.canonical = true
       assert_equal true, @emitter.canonical
 

test/psych/test_string.rb

@@ -36,7 +36,8 @@ def test_all_yaml_1_1_booleans_are_quoted
     def test_yaml_1_1_booleans_are_not_quoted_on_libfyaml
       omit 'YAML 1.1 booleans are plain strings on the libfyaml backend' unless libfyaml?
       %w[yes no on off].each do |boolean|
-        assert_equal "--- #{boolean}\n", Psych.dump(boolean)
+        # Unquoted plain scalar, allowing an optional document end marker.
+        assert_match(/\A--- #{boolean}\n(?:\.\.\.\n)?\z/, Psych.dump(boolean))
         assert_equal boolean, Psych.load(Psych.dump(boolean))
       end
     end