Repository: FHIR/sushi (ts)

Issue:
Don't allow (or require) slicings in slices
Based on [this thread](https://chat.fhir.org/#narrow/stream/179252-IG-creation/topic/Reslicing.20extensions.20causes.20validator.20errors/near/360153471), you cannot introduce new `slicing`s in existing slices. Or expressed as a FHIRPath invariant expression:
```
sliceName.exists() implies slicing.empty()
```

This is a departure from how we thought reslicing worked -- and as a result, SUSHI complains if you try to introduce re-slices into a slice that does not have a `slicing`.  SUSHI needs to be updated to look for a slicing on the base element (not the slice that is being re-sliced). In addition, if SUSHI finds a `slicing` inside a slice, it should log a warning saying that the slicing info should be moved to the base slicing. If possible, we should consider having SUSHI move the discriminators for the author (but still have the warning).

Pull request:
Slicing rules should not be on slices
Fixes #1283 and completes task [CIMPL-1108](https://standardhealthrecord.atlassian.net/browse/CIMPL-1108).

You can add reslices of a slice without having to define slicing rules on the slice. If you want more discriminators, you set those on the base element. Show a warning if the user adds slicing rules to an element that has a slice name.

I decided to somewhat expand the `ValidationError` class and `validate` functions for this implementation, since that let me manage the new condition being checked alongside the existing checks.

The tests on `sliceIt` are added for completeness, but probably don't come up during actual program execution. This is because `sliceIt` is only called in specific circumstances where SUSHI wants to define the slicing rules. If the author defines the slicing using caret rules, this function is not used.

Patch:
diff --git a/src/errors/ValidationError.ts b/src/errors/ValidationError.ts
index c86a4bf..74cde49 100644
--- a/src/errors/ValidationError.ts
+++ b/src/errors/ValidationError.ts
@@ -1,5 +1,11 @@
 export class ValidationError extends Error {
-  constructor(public issue: string, public fshPath: string) {
+  constructor(
+    public issue: string,
+    public fshPath: string,
+    public severity: ErrorSeverity = 'error'
+  ) {
     super(`${fshPath}: ${issue}`);
   }
 }
+
+type ErrorSeverity = 'error' | 'warn' | 'info';
diff --git a/src/export/StructureDefinitionExporter.ts b/src/export/StructureDefinitionExporter.ts
index 265dc43..1284364 100644
--- a/src/export/StructureDefinitionExporter.ts
+++ b/src/export/StructureDefinitionExporter.ts
@@ -1184,7 +1184,7 @@ export class StructureDefinitionExporter implements Fishable {
     structDef.inProgress = false;
 
     structDef.validate().forEach(err => {
-      logger.error(err.message, fshDefinition.sourceInfo);
+      logger.log(err.severity, err.message, fshDefinition.sourceInfo);
     });
 
     // check for another structure definition with the same id
diff --git a/src/fhirtypes/ElementDefinition.ts b/src/fhirtypes/ElementDefinition.ts
index 1c8e138..9c5ae8a 100644
--- a/src/fhirtypes/ElementDefinition.ts
+++ b/src/fhirtypes/ElementDefinition.ts
@@ -319,10 +319,20 @@ export class ElementDefinition {
   }
 
   validate(): ValidationError[] {
+    const errors: ValidationError[] = [];
     if (this.slicing) {
-      return this.validateSlicing(this.slicing);
+      if (this.sliceName) {
+        errors.push(
+          new ValidationError(
+            'An element with a slice name should not define its own slicing. Instead, append additional discriminators to the original slicing on the base element.',
+            '',
+            'warn'
+          )
+        );
+      }
+      errors.push(...this.validateSlicing(this.slicing));
     }
-    return [];
+    return errors;
   }
 
   private validateRequired(value: AssignmentValueType, fshPath: string): ValidationError {
@@ -2562,6 +2572,11 @@ export class ElementDefinition {
     ordered?: boolean,
     rules?: string
   ): ElementDefinitionSlicing {
+    if (this.sliceName) {
+      // slices should not have slicing info added to them
+      // see also https://chat.fhir.org/#narrow/stream/179252-IG-creation/topic/Reslicing.20extensions.20causes.20validator.20errors/near/360153471
+      logger.warn(`${this.id} is a slice. Slices should not have slicing info added to them.`);
+    }
     if (!this.slicing || !this.slicing.discriminator) {
       this.slicing = {
         discriminator: [
@@ -2617,7 +2632,9 @@ export class ElementDefinition {
    * @returns {ElementDefinition} the new element representing the slice
    */
   addSlice(name: string, type?: ElementDefinitionType): ElementDefinition {
-    if (!this.slicing) {
+    // to add a slice, an element must either have slicing information, or must itself be a slice.
+    // see also https://chat.fhir.org/#narrow/stream/179252-IG-creation/topic/Reslicing.20extensions.20causes.20validator.20errors/near/360153471
+    if (!this.slicing && !this.sliceName) {
       throw new SlicingNotDefinedError(this.id, name);
     }
 
diff --git a/src/fhirtypes/StructureDefinition.ts b/src/fhirtypes/StructureDefinition.ts
index cd76704..3d28eac 100644
--- a/src/fhirtypes/StructureDefinition.ts
+++ b/src/fhirtypes/StructureDefinition.ts
@@ -93,12 +93,11 @@ export class StructureDefinition {
     const validationErrors: ValidationError[] = [];
     this.elements.forEach(e => {
       e.validate().forEach(err => {
-        validationErrors.push(
-          new ValidationError(
-            err.issue,
-            `${e.id.replace(/(\S):(\S+)/g, (_, c1, c2) => `${c1}[${c2}]`)} ^${err.fshPath}`
-          )
-        );
+        let transformedPath = e.id.replace(/(\S):(\S+)/g, (_, c1, c2) => `${c1}[${c2}]`);
+        if (err.fshPath.length > 0) {
+          transformedPath += ` ^${err.fshPath}`;
+        }
+        validationErrors.push(new ValidationError(err.issue, transformedPath, err.severity));
       });
     });
     return validationErrors;


Test patch:
diff --git a/test/export/StructureDefinitionExporter.test.ts b/test/export/StructureDefinitionExporter.test.ts
index b4d2ba7..523285b 100644
--- a/test/export/StructureDefinitionExporter.test.ts
+++ b/test/export/StructureDefinitionExporter.test.ts
@@ -5735,23 +5735,19 @@ describe('StructureDefinitionExporter R4', () => {
       expect(diff.max).toBe('*');
     });
 
-    it('should apply a ContainsRule on a slice with defined slicing', () => {
+    it('should apply a ContainsRule on a slice', () => {
       // Profile: Foo
       // Parent: resprate
       // * code.coding contains fooSlice
-      // * code.coding[fooSlice] ^slicing.discriminator.type = #pattern
       // * code.coding[fooSlice] contains barReslice
       const profile = new Profile('Foo');
       profile.parent = 'resprate';
 
       const sliceRule = new ContainsRule('code.coding');
       sliceRule.items = [{ name: 'fooSlice' }];
-      const caretRule = new CaretValueRule('code.coding[fooSlice]');
-      caretRule.caretPath = 'slicing.discriminator.type';
-      caretRule.value = new FshCode('pattern');
       const resliceRule = new ContainsRule('code.coding[fooSlice]');
       resliceRule.items = [{ name: 'barReslice' }];
-      profile.rules.push(sliceRule, caretRule, resliceRule);
+      profile.rules.push(sliceRule, resliceRule);
 
       exporter.exportStructDef(profile);
       const sd = pkg.profiles[0];
@@ -5766,29 +5762,20 @@ describe('StructureDefinitionExporter R4', () => {
       const diff = barReslice.calculateDiff();
       expect(diff.min).toBe(0);
       expect(diff.max).toBe('*');
+      expect(loggerSpy.getAllMessages()).toHaveLength(0);
     });
 
-    it('should apply a ContainsRule on an extension with defined slicing', () => {
+    it('should apply a ContainsRule on an extension slice', () => {
       // Extension: Foo
       // * extension contains bar
-      // * extension[bar] ^slicing.discriminator.type = #pattern
       // * extension[bar] contains barReslice
       const extension = new Extension('Foo');
 
       const sliceRule = new ContainsRule('extension');
       sliceRule.items = [{ name: 'bar' }];
-      const caretRule = new CaretValueRule('extension[bar]');
-      caretRule.caretPath = 'slicing.discriminator.type';
-      caretRule.value = new FshCode('pattern');
-      const rulesRule = new CaretValueRule('extension[bar]');
-      rulesRule.caretPath = 'slicing.rules';
-      rulesRule.value = new FshCode('open');
-      const pathRule = new CaretValueRule('extension[bar]');
-      pathRule.caretPath = 'slicing.discriminator.path';
-      pathRule.value = 'foo';
       const resliceRule = new ContainsRule('extension[bar]');
       resliceRule.items = [{ name: 'barReslice' }];
-      extension.rules.push(sliceRule, caretRule, rulesRule, pathRule, resliceRule);
+      extension.rules.push(sliceRule, resliceRule);
 
       exporter.exportStructDef(extension);
       const sd = pkg.extensions[0];
@@ -5801,7 +5788,54 @@ describe('StructureDefinitionExporter R4', () => {
       const diff = barReslice.calculateDiff();
       expect(diff.min).toBe(0);
       expect(diff.max).toBe('*');
+      expect(loggerSpy.getAllMessages()).toHaveLength(0);
+    });
+
+    it('should log a warning when an element has both a slice name and slicing', () => {
+      // Profile: Foo
+      // Parent: resprate
+      // * code.coding contains fooSlice
+      // * code.coding[fooSlice] ^slicing.discriminator.type = #pattern
+      // * code.coding[fooSlice] ^slicing.discriminator.path = "system"
+      // * code.coding[fooSlice] ^slicing.rules = #open
+      // * code.coding[fooSlice] contains barReslice
+      const profile = new Profile('Foo');
+      profile.parent = 'resprate';
+
+      const sliceRule = new ContainsRule('code.coding');
+      sliceRule.items = [{ name: 'fooSlice' }];
+      const caretRule = new CaretValueRule('code.coding[fooSlice]');
+      caretRule.caretPath = 'slicing.discriminator.type';
+      caretRule.value = new FshCode('pattern');
+      const pathRule = new CaretValueRule('code.coding[fooSlice]');
+      pathRule.caretPath = 'slicing.discriminator.path';
+      pathRule.value = 'system';
+      const rulesRule = new CaretValueRule('code.coding[fooSlice]');
+      rulesRule.caretPath = 'slicing.rules';
+      rulesRule.value = new FshCode('open');
+      const resliceRule = new ContainsRule('code.coding[fooSlice]');
+      resliceRule.items = [{ name: 'barReslice' }];
+      profile.rules.push(sliceRule, caretRule, pathRule, rulesRule, resliceRule);
+
+      exporter.exportStructDef(profile);
+      const sd = pkg.profiles[0];
+      const baseStructDef = fisher.fishForStructureDefinition('resprate');
+
+      const barReslice = sd.elements.find(
+        e => e.id === 'Observation.code.coding:fooSlice/barReslice'
+      );
+
+      expect(sd.elements.length).toBe(baseStructDef.elements.length + 2);
+      expect(barReslice).toBeDefined();
+      const diff = barReslice.calculateDiff();
+      expect(diff.min).toBe(0);
+      expect(diff.max).toBe('*');
       expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      // putting slice info on a slice is a warning-level event
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(
+        'Observation.code.coding[fooSlice]: An element with a slice name should not define its own slicing.'
+      );
     });
 
     it('should apply a ContainsRule of a defined extension on an extension element', () => {
diff --git a/test/fhirtypes/ElementDefinition.slicing.test.ts b/test/fhirtypes/ElementDefinition.slicing.test.ts
index 5021b5c..2b49722 100644
--- a/test/fhirtypes/ElementDefinition.slicing.test.ts
+++ b/test/fhirtypes/ElementDefinition.slicing.test.ts
@@ -2,7 +2,7 @@ import { loadFromPath } from 'fhir-package-loader';
 import { FHIRDefinitions } from '../../src/fhirdefs/FHIRDefinitions';
 import { StructureDefinition } from '../../src/fhirtypes/StructureDefinition';
 import { ElementDefinitionType } from '../../src/fhirtypes';
-import { TestFisher } from '../testhelpers';
+import { loggerSpy, TestFisher } from '../testhelpers';
 import findLastIndex from 'lodash/findLastIndex';
 import path from 'path';
 
@@ -337,7 +337,6 @@ describe('ElementDefinition', () => {
       component.sliceIt('pattern', 'code', false, 'open');
       const systolicBP = component.addSlice('SystolicBP');
       const diastolicBP = component.addSlice('DiastolicBP');
-      systolicBP.sliceIt('pattern', 'interpretation', false, 'open');
       const fooSlice = systolicBP.addSlice('Foo');
       expect(fooSlice.id).toBe('Observation.component:SystolicBP/Foo');
       expect(fooSlice.path).toBe('Observation.component');
@@ -353,6 +352,7 @@ describe('ElementDefinition', () => {
       expect(observation.elements[lastComponentIndex + 1]).toEqual(systolicBP);
       expect(observation.elements[lastComponentIndex + 2]).toEqual(fooSlice);
       expect(observation.elements[lastComponentIndex + 3]).toEqual(diastolicBP);
+      expect(loggerSpy.getAllMessages()).toHaveLength(0);
     });
 
     it('should reslice when slicing a reslice', () => {
@@ -363,9 +363,9 @@ describe('ElementDefinition', () => {
       component.sliceIt('pattern', 'code', false, 'open');
       const systolicBP = component.addSlice('SystolicBP');
       const diastolicBP = component.addSlice('DiastolicBP');
-      systolicBP.sliceIt('pattern', 'interpretation', false, 'open');
+      component.sliceIt('pattern', 'interpretation', false, 'open');
       const fooSlice = systolicBP.addSlice('Foo');
-      fooSlice.sliceIt('pattern', 'dataAbsentReason', false, 'open');
+      component.sliceIt('pattern', 'dataAbsentReason', false, 'open');
       const barSlice = fooSlice.addSlice('Bar');
       expect(barSlice.id).toBe('Observation.component:SystolicBP/Foo/Bar');
       expect(barSlice.path).toBe('Observation.component');
@@ -382,6 +382,26 @@ describe('ElementDefinition', () => {
       expect(observation.elements[lastComponentIndex + 2]).toEqual(fooSlice);
       expect(observation.elements[lastComponentIndex + 3]).toEqual(barSlice);
       expect(observation.elements[lastComponentIndex + 4]).toEqual(diastolicBP);
+      expect(loggerSpy.getAllMessages()).toHaveLength(0);
+    });
+
+    it('should log a warning when trying to slice a slice', () => {
+      // see also https://chat.fhir.org/#narrow/stream/179252-IG-creation/topic/Reslicing.20extensions.20causes.20validator.20errors/near/360153471
+      const component = observation.elements.find(e => e.id === 'Observation.component');
+      component.sliceIt('pattern', 'code', false, 'open');
+      const systolicBP = component.addSlice('SystolicBP');
+      expect(systolicBP.slicing).toBeUndefined();
+      const slicing = systolicBP.sliceIt('pattern', 'interpretation', false, 'open');
+      expect(slicing.discriminator).toHaveLength(1);
+      expect(slicing.discriminator[0].type).toBe('pattern');
+      expect(slicing.discriminator[0].path).toBe('interpretation');
+      expect(slicing.ordered).toBe(false);
+      expect(slicing.rules).toBe('open');
+      expect(systolicBP.slicing).toEqual(slicing);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(
+        'Observation.component:SystolicBP is a slice. Slices should not have slicing info added to them.'
+      );
     });
 
     it('should throw when no slicing is present', () => {