Project

Profile

Help

Bug #4783

closed

Incorrect results when comment node is created as child of document node

Added by Michael Kay over 3 years ago. Updated about 3 years ago.

Status:
Closed
Priority:
Low
Assignee:
Category:
-
Sprint/Milestone:
-
Start date:
2020-10-05
Due date:
% Done:

100%

Estimated time:
Applies to JS Branch:
2
Fix Committed on JS Branch:
2
Fixed in JS Release:
SEF Generated with:
Platforms:
Company:
-
Contact person:
-
Additional contact persons:
-

Description

Test case function-lookup-008 is failing. If the test is modified by removing the final xsl:comment instruction (which is there only to add diagnostics) then the test passes. Somehow the addition of the final document-level comment node is causing the constructed tree to be incorrect.

Actions #1

Updated by Michael Kay over 3 years ago

The raw result of the transformation consists of a parentless element node (which at the implementation level actually belongs to a DocumentFragment), followed by a parentless Comment node.

It's then up to the fn:transform() function to turn this sequence into a document tree by wrapping in a document node. The code to do this is at CoreFn#2776, and it appears to ignore everything in the sequence except element nodes and text nodes.

For some reason a call to Copy.makeComplexContent() has been commented out here.

I think the correct logic is to create a Builder, issue a startDoc(), append() each item in the raw result sequence, then issue endDoc().

Actions #2

Updated by Michael Kay over 3 years ago

I've considerably simplified the logic in fn:transform() so it now does:

            if (buildTree) {
                Diag.assert(Array.isArray(r));
                const builder = getBuilder(context);
                const out = Push.getComplexContentOutputter(builder);
                out.startDoc();
                r.forEach(it => out.append(it));
                out.endDoc();
                out.close();
                r = builder.sequence;
            }

and this test now passes. But it's a big change, so it needs careful regression testing.

Actions #3

Updated by Michael Kay over 3 years ago

In the XSLT3 test suite, the change has fixed a few outstanding test failures (e.g. in accumulator), but has also resulted in a handful of new test failures, notably in current-output-uri, message, mode, output, position, result-document, try. I will investigate these.

In QT3 we're seeing 13 failures in fn-transform and one failure in xs-numeric (which is probably induced by previous bug fixes since I last ran QT3).

Actions #4

Updated by Michael Kay over 3 years ago

Homing in on the fn:transform failures, they are failures in the line

Diag.assert(Array.isArray(r));

included in the new code above.

The existing code tries to cope with r (the raw result) being either an array or a singleton. I'd prefer to be consistent about it always being an array if possible, which is why I added the assertion.

Actions #5

Updated by Michael Kay over 3 years ago

In fn-transform-14 there is no principal result document (only secondary results), and r is null. Bypassing the tree construction if r===null I now get 11 failures in fn:transform, for example fn-transform-35.

In fn-transform-35, the request is for delivery-format:"serialized": it seems that the serialized form has been wrapped in a new doccument node and serialized again. The problem seems to be that the buildTree variable is true even though what we have is already a serialized result.

CoreFn#2694 sets let buildTree = destinationType !== "raw";. Changing that so we don't set buildTree if the destination is "serialized" means we now pass all the fn:transform tests.

Actions #6

Updated by Michael Kay over 3 years ago

  • Status changed from New to Resolved
  • Applies to JS Branch 2.0, Trunk added
  • Fix Committed on JS Branch 2.0, Trunk added

This change fixes the remaining regressions in the QT3 and XSLT3 tests; marking as resolved.

Actions #7

Updated by Community Admin about 3 years ago

  • Applies to JS Branch 2 added
  • Applies to JS Branch deleted (2.0, Trunk)
Actions #8

Updated by Community Admin about 3 years ago

  • Fix Committed on JS Branch 2 added
  • Fix Committed on JS Branch deleted (2.0, Trunk)
Actions #9

Updated by Debbie Lockett about 3 years ago

  • Status changed from Resolved to Closed
  • % Done changed from 0 to 100
  • Fixed in JS Release set to Saxon-JS 2.1

Bug fix applied in the Saxon-JS 2.1 maintenance release.

Please register to edit this issue

Also available in: Atom PDF Tracking page