Project

Profile

Help

Bug #5855

closed

map:merge with duplicates=combine gives wrong answer

Added by Mary Holstege about 1 year ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Category:
XPath Conformance
Sprint/Milestone:
-
Start date:
2023-01-25
Due date:
% Done:

100%

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

Description

See attached files for specifics. The maps being merged are computed in a for loop with computed keys and values.

Run as XSLT, the results are correct. Running compiled to JS using Saxon-JS, the results are incorrect: all the keys pick up stray values they shouldn't have.


Files

README.txt (1.36 KB) README.txt Output and more details Mary Holstege, 2023-01-25 21:20
mapmerge.sef.json (9.86 KB) mapmerge.sef.json Compiled stylesheet Mary Holstege, 2023-01-25 21:20
mapmerge.html (640 Bytes) mapmerge.html HTML file (local paths) Mary Holstege, 2023-01-25 21:20
mapmerge.xsl (2.51 KB) mapmerge.xsl Stylesheet Mary Holstege, 2023-01-25 21:20
Actions #1

Updated by Martin Honnen about 1 year ago

Interesting, if xslt3 from SaxonJS 2.5 runs the original XSLT stylesheet it gets it right:

C:\Users\marti\OneDrive\Documents\xslt\blog-xslt-3-by-example\map-merge>xslt3 -t -it:test -xsl:test1.xsl
SaxonJS 2.5 from Saxonica
Node.js version v16.17.1
Compiling stylesheet C:\Users\marti\OneDrive\Documents\xslt\blog-xslt-3-by-example\map-merge\test1.xsl
Stylesheet compilation time: 0.289s
Initial template: Q{}test
Asynchronous transform with options: stylesheetText={"N":"package","version":"30",(string), stylesheetBaseURI=file://C:/Users/marti/OneDrive(string), stylesheetParams=[object Object](string), outputProperties=[object Object](string), extraOptions=[object Object](string), destination=stdout(string), baseOutputURI=file://C:/Users/marti/OneDrive(string), logLevel=2(string), initialTemplate=Q{}test(string),
SEF generated by SaxonJS 2.5 at 2023-01-26T00:09:17.156+01:00
fn:trace: submap: HashTrie map{xs:integer('1'): xs:string('a')}
fn:trace: submap: HashTrie map{xs:integer('2'): xs:string('a')}
fn:trace: submap: HashTrie map{xs:integer('2'): xs:string('b')}
fn:trace: submap: HashTrie map{xs:integer('1'): xs:string('c')}
fn:trace: submap: HashTrie map{xs:integer('3'): xs:string('d')}
fn:trace: merged: HashTrie map{xs:integer('1'): (xs:string('a'),xs:string('c')), xs:integer('2'): (xs:string('a'),xs:string('b')), xs:integer('3'): xs:string('d')}
<?xml version="1.0" encoding="UTF-8"?><div>true true true true</div>

So is the Saxon EE compiler to blame?

Actions #2

Updated by Debbie Lockett about 1 year ago

  • Priority changed from Low to Normal
  • Applies to JS Branch 2, Trunk added

Thanks for reporting the bug, and supplying the repro.

I have added a browser unit test, browser/iss5855 (committed on the main and saxonjs2 branches) and confirmed that the test passes when the SEF is XX-compiled using SaxonJS (./gradlew unit_test_browser_iss5855 -PtestCompiler=xx), but fails when the SEF is XJ-compiled (-PtestCompiler=xj).

I'm afraid I have not yet got further than that in diagnosing the problem (and I'm about to go on holiday for a week); but will return to this in due course!

Actions #3

Updated by Debbie Lockett about 1 year ago

I've added some further browser tests: iss5855a uses modified syntax (e.g. more use of XSLT rather than XPath); and iss5855b has further simplifications to avoid the use of locally defined functions.

I've also added test map-merge-027 in the QT3 test suite (based on iss5855b). This is useful as we run the test suites with SaxonJS on Node.js as well as in the browser.

Actions #4

Updated by Debbie Lockett about 1 year ago

The fact that the original test failed with an XJ-compiled SEF but not an XX-compiled SEF suggested that the problem is with the compiler. However further tests and debugging show that the issue is actually in the implementation of map:merge (in ExtraFn.js).

The bug is that when constructing a new merged map (represented internally with a HashTrie), for duplicate keys, we were simply using val.push() to update the existing array value val in the HashTrie, and then replacing the entry using inSituPut:

                        if (dup === "combine") {
                            const val = m.get(pair.k);
                            pair.v.forEach(function (x) {
                                val.push(x);
                            });
                            m.inSituPut(pair.k, val);
                        }

However, in some circumstances, as manifested in this bug, the value in the map entry might be a shallow copy of the array object; so it is not safe to do such a direct update. This can be fixed by ensuring we create a new array for the new value, rather than amending the old one.

The difference in the test results with XX and XJ-compiling is something to do with the difference in the compilation, and somehow the XX-compiled version avoids the bug manifesting. For instance, the use of the map constructor in the this:map-invert function compiles differently: with XJ we get an ifCall to map:entry, but with XX we simply get a map expression. I can't quite see how this makes a difference to avoid/cause the bug, but it at least explains how the results are different.

Actions #5

Updated by Debbie Lockett about 1 year ago

  • Category set to XPath Conformance
  • Status changed from New to Resolved
  • Assignee set to Debbie Lockett
  • Fix Committed on JS Branch 2, Trunk added

Bug fix in ExtraFn.js:

                        if (dup === "combine") {
                            const val = m.get(pair.k);
                            // Not safe to modify val itself; must use a new array object
                            let newVal = [];
                            val.forEach(function (x) {
                                newVal.push(x);
                            });
                            pair.v.forEach(function (x) {
                                newVal.push(x);
                            });
                            m.inSituPut(pair.k, newVal);
                        }

Committed on the main and saxonjs2 branches.

Actions #6

Updated by Debbie Lockett 6 months ago

  • Status changed from Resolved to Closed
  • % Done changed from 0 to 100
  • Fixed in JS Release set to SaxonJS 2.6

Bug fix applied in the SaxonJS 2.6 maintenance release.

Please register to edit this issue

Also available in: Atom PDF Tracking page