Repository: FHIR/sushi (ts)

Issue:
Avoid type slicing when a choice is constrained to a single type
**For SUSHI 3.0**: do not create a type slice for elements when the choice has already been restricted to a single type.  For example, consider this FSH:
```
Profile: MyObservation
Parent: Observation
* value[x] only CodeableConcept
* valueCodeableConcept from http://example.org/foo
```

*Currently*, whenever SUSHI sees a type-specific path (like `valueCodeableConcept`), it uses the standard type slicing mechanism to represent it.  It does this even after `value[x]` has been constrained to a single type (as above).  So you end up w/ a differential something like this:
```json
"differential": {
    "element": [
      {
        "id": "Observation.value[x]",
        "path": "Observation.value[x]",
        "slicing": {
          "discriminator": [
            {
              "type": "type",
              "path": "$this"
            }
          ],
          "ordered": false,
          "rules": "open"
        },
        "type": [
          {
            "code": "CodeableConcept"
          }
        ]
      },
      {
        "id": "Observation.value[x]:CodeableConcept",
        "path": "Observation.value[x]",
        "sliceName": "CodeableConcept",
        "min": 0,
        "max": "1",
        "binding": {
          "strength": "required",
          "valueSet": "http://example.org/foo"
        }
      }
    ]
  }
```

Technically, this type slicing is totally unnecessary since `value[x]` can only be a single type -- therefore you don't need slices to disambiguate.  Since slicing adds complexity, we like to avoid it when we can.  To that end, SUSHI should detect when this happens (type slice on a choice that has already been constrained to a single type) and apply any constraints directly to the choice element instead.  This would effectively be the same as:
```
Profile: MyObservation
Parent: Observation
* value[x] only CodeableConcept
* value[x] from http://example.org/foo
```

Resulting in this much simpler differential:
```json
"differential": {
    "element": [
      {
        "id": "Observation.value[x]",
        "path": "Observation.value[x]",
        "type": [
          {
            "code": "CodeableConcept"
          }
        ],
        "binding": {
          "strength": "required",
          "valueSet": "http://example.org/foo"
        }
      }
    ]
  }
```

This was proposed on Zulip and received positive feedback.  See: https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/Type.20Slices.20on.20Choices.20w.2F.20a.20Single.20Type/near/282241129

Pull request description:
Don't slice single-type choices (unless already sliced)
If a user refers to a specific choice type (e.g., valueQuantity), but the choice (e.g., value[x]) only allows one type anyway, then don't create a type slicing and slice, because they're not necessary.

On the other hand, if the choice has already been sliced, then go ahead and create the slice for consistency.

To test, you can use the example in #1088:
```
Profile: MyObservation
Parent: Observation
* value[x] only CodeableConcept
* valueCodeableConcept from http://example.org/foo
```

On `next`, it will slice `value[x]` and create a `valueCodeableConcept` slice.  On this branch, it will apply the binding directly to `value[x]` instead.

If you're curious, I'm attaching the regression reports here (I filtered it to just html and log files): 
[regression-output.zip](https://github.com/FHIR/sushi/files/9049900/regression-output.zip)

Fixes #1088

Patch:
diff --git a/src/export/InstanceExporter.ts b/src/export/InstanceExporter.ts
index b59d496..e2483e7 100644
--- a/src/export/InstanceExporter.ts
+++ b/src/export/InstanceExporter.ts
@@ -16,7 +16,7 @@ import {
 import { InstanceOfNotDefinedError } from '../errors/InstanceOfNotDefinedError';
 import { InstanceOfLogicalProfileError } from '../errors/InstanceOfLogicalProfileError';
 import { Package } from '.';
-import { cloneDeep, merge, uniq } from 'lodash';
+import { cloneDeep, merge, uniq, upperFirst } from 'lodash';
 import { AssignmentRule } from '../fshtypes/rules';
 
 export class InstanceExporter implements Fishable {
@@ -280,6 +280,17 @@ export class InstanceExporter implements Fishable {
             }
           }
         }
+        // If we still haven't found it, it's possible that a type slice just wasn't created. In that case, there would
+        // be a type in the choice element's type array that would be a match if it were type-sliced.
+        if (instanceChild == null) {
+          for (const type of child.type) {
+            const name = childPathEnd.replace(/\[x\]$/, upperFirst(type.code));
+            instanceChild = instance[`_${name}`] ?? instance[name];
+            if (instanceChild != null) {
+              break;
+            }
+          }
+        }
       }
       // Recursively validate children of the current element
       if (Array.isArray(instanceChild)) {
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index 8bd10ba..2086565 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -716,17 +716,29 @@ export class StructureDefinition {
    */
   private sliceMatchingValueX(fhirPath: string, elements: ElementDefinition[]): ElementDefinition {
     let matchingType: ElementDefinitionType;
-    const matchingXElements = elements.filter(e => {
-      if (e.path.endsWith('[x]')) {
-        for (const t of e.type ?? []) {
-          if (`${e.path.slice(0, -3)}${upperFirst(t.code)}` === fhirPath) {
-            matchingType = t;
-            return true;
-          }
+    const xElements = elements.filter(e => e.path.endsWith('[x]'));
+    const matchingXElements = xElements.filter(e => {
+      for (const t of e.type ?? []) {
+        if (`${e.path.slice(0, -3)}${upperFirst(t.code)}` === fhirPath) {
+          matchingType = t;
+          return true;
         }
       }
     });
-    if (matchingXElements.length > 0) {
+    // If the only match is the choice[x] element itself, and it's already been restricted
+    // to just a single type, and there are no existing slices (for this type or otherwise),
+    // just return that instead of creating an unnecessary slice.
+    // See: https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/Type.20Slices.20on.20Choices.20w.2F.20a.20Single.20Type/near/282241129
+    if (
+      matchingXElements.length === 1 &&
+      matchingXElements[0].sliceName == null &&
+      matchingXElements[0].type?.length === 1 &&
+      xElements.filter(e => e.path === matchingXElements[0].path).length === 1
+    ) {
+      return matchingXElements[0];
+    }
+    // Otherwise we want a slice representing the specific type
+    else if (matchingXElements.length > 0) {
       const sliceName = fhirPath.slice(fhirPath.lastIndexOf('.') + 1);
       const matchingSlice = matchingXElements.find(c => c.sliceName === sliceName);
       // if we have already have a matching slice, we want to return it


Test patch:
diff --git a/test/fhirtypes/StructureDefinition.test.ts b/test/fhirtypes/StructureDefinition.test.ts
index 54f6764..fbafa9e 100644
--- a/test/fhirtypes/StructureDefinition.test.ts
+++ b/test/fhirtypes/StructureDefinition.test.ts
@@ -1009,12 +1009,12 @@ describe('StructureDefinition', () => {
     it('should find a child of a sliced element by path with URL', () => {
       const originalLength = clinicalDocument.elements.length;
       const valueString = clinicalDocument.findElementByPath(
-        'extension[http://hl7.org/fhir/StructureDefinition/composition-clinicaldocument-versionNumber].valueString',
+        'extension[http://hl7.org/fhir/StructureDefinition/composition-clinicaldocument-versionNumber].value[x]',
         fisher
       );
       expect(valueString).toBeDefined();
-      expect(valueString.id).toBe('Composition.extension:versionNumber.value[x]:valueString');
-      expect(clinicalDocument.elements.length).toBe(originalLength + 5);
+      expect(valueString.id).toBe('Composition.extension:versionNumber.value[x]');
+      expect(clinicalDocument.elements.length).toBe(originalLength + 4);
     });
 
     // Choices
@@ -1088,6 +1088,142 @@ describe('StructureDefinition', () => {
       expect(respRate.elements.length).toBe(originalLength);
     });
 
+    it('should not create a slicing or slice for single-choice elements when finding it by a type-specific path ', () => {
+      const originalLength = observation.elements.length;
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.type = [new ElementDefinitionType('Quantity')];
+      // Since there is only one possible choice, it should return the choice element directly
+      const valueQuantity = observation.findElementByPath('valueQuantity', fisher);
+      expect(valueX.slicing).toBeUndefined();
+      expect(valueQuantity).toBe(valueX);
+      expect(valueQuantity.id).toBe('Observation.value[x]');
+      expect(observation.elements.length).toBe(originalLength);
+    });
+
+    it('should not create a slicing or slice for single-choice elements when finding it by a type-specific path to a child element ', () => {
+      const originalLength = observation.elements.length;
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.type = [new ElementDefinitionType('Quantity')];
+      // Since there is only one possible choice, it should return the choice element directly
+      const valueQuantityUnit = observation.findElementByPath('valueQuantity.unit', fisher);
+      expect(valueX.slicing).toBeUndefined();
+      expect(valueQuantityUnit).toBeDefined();
+      expect(valueQuantityUnit.parent()).toBe(valueX);
+      expect(valueQuantityUnit.id).toBe('Observation.value[x].unit');
+      expect(observation.elements.length).toBe(originalLength + 7); // Plus 7 for unfolded Quantity elements
+    });
+
+    it('should create a slicing and slice for two-choice elements when finding it by a type-specific path', () => {
+      const originalLength = observation.elements.length;
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.type = [
+        new ElementDefinitionType('CodeableConcept'),
+        new ElementDefinitionType('Quantity')
+      ];
+      // Since there is more than one choice, it should slice to distinguish between them
+      const valueQuantity = observation.findElementByPath('valueQuantity', fisher);
+      expect(valueX.slicing).toBeDefined();
+      expect(valueX.slicing.discriminator[0]).toEqual({ type: 'type', path: '$this' });
+      expect(valueX.slicing.ordered).toBe(false);
+      expect(valueX.slicing.rules).toBe('open');
+      expect(valueQuantity).toBeDefined();
+      expect(valueQuantity.id).toBe('Observation.value[x]:valueQuantity');
+      expect(valueQuantity.slicing).toBeUndefined();
+      expect(valueQuantity.sliceName).toBe('valueQuantity');
+      expect(valueQuantity.path).toBe('Observation.value[x]');
+      expect(valueQuantity.min).toBe(0);
+      expect(observation.elements.length).toBe(originalLength + 1);
+    });
+
+    it('should create a slicing and slice for two-choice elements when finding it by a type-specific path to a child element', () => {
+      const originalLength = observation.elements.length;
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.type = [
+        new ElementDefinitionType('CodeableConcept'),
+        new ElementDefinitionType('Quantity')
+      ];
+      // Since there is more than one choice, it should slice to distinguish between them
+      const valueQuantityUnit = observation.findElementByPath('valueQuantity.unit', fisher);
+      expect(valueX.slicing).toBeDefined();
+      expect(valueX.slicing.discriminator[0]).toEqual({ type: 'type', path: '$this' });
+      expect(valueX.slicing.ordered).toBe(false);
+      expect(valueX.slicing.rules).toBe('open');
+      expect(valueQuantityUnit).toBeDefined();
+      expect(valueQuantityUnit.id).toBe('Observation.value[x]:valueQuantity.unit');
+      const valueQuantity = valueQuantityUnit.parent();
+      expect(valueQuantity.slicing).toBeUndefined();
+      expect(valueQuantity.sliceName).toBe('valueQuantity');
+      expect(valueQuantity.path).toBe('Observation.value[x]');
+      expect(valueQuantity.min).toBe(0);
+      expect(observation.elements.length).toBe(originalLength + 8); // Plus 8 for slice and unfolded Quantity elements
+    });
+
+    it('should create a slice for single-choice elements with other existing slices when finding it by a type-specific path', () => {
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.sliceIt('type', '$this', false, 'open');
+      valueX.addSlice('valueCodeableConcept', new ElementDefinitionType('valueCodeableConcept'));
+      valueX.type = [new ElementDefinitionType('Quantity')];
+      const originalLength = observation.elements.length;
+      // Since there are already other slices, even though this is now a single-choice,
+      // it should create a new slice to be consistent with what's already there.
+      // (This was a subjective decision on our part, not necessarily driven be spec).
+      const valueQuantity = observation.findElementByPath('valueQuantity', fisher);
+      expect(valueQuantity).toBeDefined();
+      expect(valueQuantity.id).toBe('Observation.value[x]:valueQuantity');
+      expect(valueQuantity.slicing).toBeUndefined();
+      expect(valueQuantity.sliceName).toBe('valueQuantity');
+      expect(valueQuantity.path).toBe('Observation.value[x]');
+      expect(valueQuantity.min).toBe(0);
+      expect(observation.elements.length).toBe(originalLength + 1);
+    });
+
+    it('should create a slice for single-choice elements with other existing slices when finding it by a type-specific path to a child element ', () => {
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.sliceIt('type', '$this', false, 'open');
+      valueX.addSlice('valueCodeableConcept', new ElementDefinitionType('valueCodeableConcept'));
+      valueX.type = [new ElementDefinitionType('Quantity')];
+      const originalLength = observation.elements.length;
+      // Since there are already other slices, even though this is now a single-choice,
+      // it should create a new slice to be consistent with what's already there.
+      // (This was a subjective decision on our part, not necessarily driven be spec).
+      const valueQuantityUnit = observation.findElementByPath('valueQuantity.unit', fisher);
+      expect(valueQuantityUnit).toBeDefined();
+      expect(valueQuantityUnit.id).toBe('Observation.value[x]:valueQuantity.unit');
+      const valueQuantity = valueQuantityUnit.parent();
+      expect(valueQuantity.slicing).toBeUndefined();
+      expect(valueQuantity.sliceName).toBe('valueQuantity');
+      expect(valueQuantity.path).toBe('Observation.value[x]');
+      expect(valueQuantity.min).toBe(0);
+      expect(observation.elements.length).toBe(originalLength + 8); // Plus 8 for slice and unfolded Quantity elements
+    });
+
+    it('should return a pre-existing slice for single-choice elements when finding it by a type-specific path', () => {
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.sliceIt('type', '$this', false, 'open');
+      valueX.type = [new ElementDefinitionType('Quantity')];
+      const slice = valueX.addSlice('valueQuantity', new ElementDefinitionType('Quantity'));
+      const originalLength = observation.elements.length;
+      // Since a slice for Quantity already exists, it should return that slice directly
+      const valueQuantity = observation.findElementByPath('valueQuantity', fisher);
+      expect(valueQuantity).toBe(slice);
+      expect(valueQuantity.id).toBe('Observation.value[x]:valueQuantity');
+      expect(observation.elements.length).toBe(originalLength);
+    });
+
+    it('should return a pre-existing slice for single-choice elements when finding it by a type-specific path to a child element', () => {
+      const valueX = observation.findElementByPath('value[x]', fisher);
+      valueX.sliceIt('type', '$this', false, 'open');
+      valueX.type = [new ElementDefinitionType('Quantity')];
+      const slice = valueX.addSlice('valueQuantity', new ElementDefinitionType('Quantity'));
+      const originalLength = observation.elements.length;
+      // Since a slice for Quantity already exists, it should return that slice directly
+      const valueQuantityUnit = observation.findElementByPath('valueQuantity.unit', fisher);
+      expect(valueQuantityUnit).toBeDefined();
+      expect(valueQuantityUnit.parent()).toBe(slice);
+      expect(valueQuantityUnit.id).toBe('Observation.value[x]:valueQuantity.unit');
+      expect(observation.elements.length).toBe(originalLength + 7); // Plus 7 for unfolded Quantity elements
+    });
+
     // Unfolding
     it('should find an element that must be unfolded by path', () => {
       const originalLength = observation.elements.length;
@@ -1614,7 +1750,7 @@ describe('StructureDefinition', () => {
         brackets: ['maiden-name', '0'],
         slices: ['maiden-name']
       });
-      expect(respRate.elements.length).toBe(originalLength + 6);
+      expect(respRate.elements.length).toBe(originalLength + 5);
     });
 
     it('should rename modifierExtensions when they are referred to by name instead of sliceName', () => {
@@ -1636,7 +1772,7 @@ describe('StructureDefinition', () => {
         brackets: ['maiden-name', '0'],
         slices: ['maiden-name']
       });
-      expect(respRate.elements.length).toBe(originalLength + 6);
+      expect(respRate.elements.length).toBe(originalLength + 5);
     });
 
     describe('#Inline Instances', () => {