Repository: FHIR/sushi (ts)

Issue:
Rule Sets with Value Set Rules Generates errors
Testing `insert` rules with value sets. SUSHI generates errors that indicate the RuleSet is being interpreted as a CodeSystem:
```
RuleSet: Hands
* $SCT#87683000  "Left handed (finding)"
```
Results in:
`error Do not include the system when listing concepts for a code system.`

https://fshschool.org/FSHOnline/#/share/3i4Mc5w

Pull request:
Adjust error logging strategy for RuleSets with ConceptRules
Fixes #877 and completes task [CIMPL-796](https://standardhealthrecord.atlassian.net/browse/CIMPL-796).

A RuleSet can contain ConceptRules and ValueSetConceptComponentRules. Additionally, it is possible for the same FSH to parse using the concept or vsConceptComponent parser rules. Therefore, it can be ambiguous when parsing the RuleSet which rule should actually be made. When one of these rules is applied, it is handled in different ways depending on whether the target entity is a ValueSet or a CodeSystem.

If a ConceptRule with a system is part of a RuleSet applied to a ValueSet, turn it into a ValueSetConceptComponent and apply it. If a ConceptRule with a system is part of a RuleSet applied to a CodeSystem, apply it, but log an error, because concepts listed on a CodeSystem should not include their system. These special cases only apply when the ConceptRule is part of a RuleSet.

When using the full list of regression FSH repos, there are no differences in the resulting IGs or log counts when making these changes.

Patch:
diff --git a/src/fhirtypes/common.ts b/src/fhirtypes/common.ts
index b7efda3..619e0fa 100644
--- a/src/fhirtypes/common.ts
+++ b/src/fhirtypes/common.ts
@@ -451,26 +451,30 @@ export function applyInsertRules(
       let context = rule.path;
       let firstRule = true;
       ruleSet.rules.forEach(ruleSetRule => {
-        // On the import side, a rule that is intended to be a ValueSetConceptComponent can
-        // be imported as a ConceptRule because the syntax is identical. If this is the case,
-        // create a ValueSetConceptComponent that corresponds to the ConceptRule, and use that
-        if (fshDefinition instanceof FshValueSet && ruleSetRule instanceof ConceptRule) {
-          if (ruleSetRule.definition != null) {
-            logger.warn(
-              'ValueSet concepts should not include a definition, only system, code, and display are supported. The definition will be ignored.',
+        ruleSetRule.sourceInfo.appliedFile = rule.sourceInfo.file;
+        ruleSetRule.sourceInfo.appliedLocation = rule.sourceInfo.location;
+        // On the import side, there are some rules that syntactically match both ConceptRule and
+        // ValueSetConceptComponentRule. When this happens, a ConceptRule is created with a value
+        // set on its system. If we are applying rules to a ValueSet, and the ConceptRule has a
+        // system, create a ValueSetConceptComponent that corresponds to the ConceptRule, and use that.
+        // BUT! If we have a ConceptRule with a system, and we are applying rules to a CodeSystem,
+        // log an error to let the author know to not do that.
+        if (ruleSetRule instanceof ConceptRule && ruleSetRule.system) {
+          if (fshDefinition instanceof FshValueSet) {
+            const relatedCode = new FshCode(
+              ruleSetRule.code,
+              ruleSetRule.system,
+              ruleSetRule.display
+            );
+            ruleSetRule = new ValueSetConceptComponentRule(true);
+            (ruleSetRule as ValueSetConceptComponentRule).concepts = [relatedCode];
+          } else if (fshDefinition instanceof FshCodeSystem) {
+            logger.error(
+              'Do not include the system when listing concepts for a code system.',
               ruleSetRule.sourceInfo
             );
           }
-          const relatedCode = new FshCode(
-            ruleSetRule.code,
-            ruleSetRule.system,
-            ruleSetRule.display
-          );
-          ruleSetRule = new ValueSetConceptComponentRule(true);
-          (ruleSetRule as ValueSetConceptComponentRule).concepts = [relatedCode];
         }
-        ruleSetRule.sourceInfo.appliedFile = rule.sourceInfo.file;
-        ruleSetRule.sourceInfo.appliedLocation = rule.sourceInfo.location;
         if (isAllowedRule(fshDefinition, ruleSetRule)) {
           const ruleSetRuleClone = cloneDeep(ruleSetRule);
           if (context) {
diff --git a/src/import/FSHImporter.ts b/src/import/FSHImporter.ts
index 079071a..8fbad3b 100644
--- a/src/import/FSHImporter.ts
+++ b/src/import/FSHImporter.ts
@@ -1372,15 +1372,21 @@ export class FSHImporter extends FSHVisitor {
       concept.definition = this.extractMultilineString(ctx.MULTILINE_STRING());
     }
     if (localCodePath.some(listedConcept => listedConcept.system)) {
-      logger.error(
-        'Do not include the system when listing concepts for a code system.',
-        concept.sourceInfo
-      );
-      // If this is on a ruleset, and if this rule is then used on a ValueSet, this could actually
-      // be a ValueSetConceptComponent, and not a ConceptRule, in which case we should carry through
-      // the system
-      if (codePart.system) {
+      // If this concept is part of a RuleSet and has a system, no definition, and no hierarchy,
+      // it might actually represent a ValueSetConceptComponent. We can't know for sure until the RuleSet
+      // is inserted somewhere. For now, assume it will be okay, so keep the system for later.
+      if (
+        ctx.parentCtx instanceof FSHParser.RuleSetRuleContext &&
+        codePart.system &&
+        !concept.definition &&
+        concept.hierarchy.length === 0
+      ) {
         concept.system = codePart.system;
+      } else {
+        logger.error(
+          'Do not include the system when listing concepts for a code system.',
+          concept.sourceInfo
+        );
       }
     }
     return concept;


Test patch:
diff --git a/test/fhirtypes/applyInsertRules.test.ts b/test/fhirtypes/applyInsertRules.test.ts
index afcbc98..48823a8 100644
--- a/test/fhirtypes/applyInsertRules.test.ts
+++ b/test/fhirtypes/applyInsertRules.test.ts
@@ -1,5 +1,13 @@
 import { FSHTank, FSHDocument } from '../../src/import';
-import { Profile, Logical, RuleSet, FshCode, FshValueSet, Instance } from '../../src/fshtypes';
+import {
+  Profile,
+  Logical,
+  RuleSet,
+  FshCode,
+  FshValueSet,
+  Instance,
+  FshCodeSystem
+} from '../../src/fshtypes';
 import {
   InsertRule,
   CardRule,
@@ -14,7 +22,8 @@ import {
   assertValueSetConceptComponent,
   assertAssignmentRule,
   assertAddElementRule,
-  assertCaretValueRule
+  assertCaretValueRule,
+  assertConceptRule
 } from '../testhelpers';
 import { minimalConfig } from '../utils/minimalConfig';
 import { applyInsertRules } from '../../src/fhirtypes/common';
@@ -359,7 +368,7 @@ describe('applyInsertRules', () => {
     assertCardRule(profile.rules[3], 'focus', 1, '1');
   });
 
-  it('should convert a ConceptRule to a ValueSetConceptComponent when applying to a FshValueSet', () => {
+  it('should convert a ConceptRule with a system to a ValueSetConceptComponent when applying to a FshValueSet', () => {
     // RuleSet: Bar
     // * ZOO#bear "brown bear"
     //
@@ -388,37 +397,57 @@ describe('applyInsertRules', () => {
     );
   });
 
-  it('should convert a ConceptRule with a definition to a ValueSetConceptComponent and log a warning when applying to a FshValueSet', () => {
+  it('should not convert a ConceptRule without a system to a ValueSetConceptComponent when applying to a FshValueSet', () => {
     // RuleSet: Bar
-    // * ZOO#bear "brown bear" "brown bears are kind of scary"
+    // * #bear "brown bear"
     //
     // ValueSet: Foo
     // * insert Bar
-    const vs = new FshValueSet('Foo').withFile('VS.fsh').withLocation([5, 6, 7, 16]);
+    const vs = new FshValueSet('Foo');
     doc.valueSets.set(vs.name, vs);
 
-    const concept = new ConceptRule('bear', 'brown bear', 'brown bears are kind of scary')
-      .withFile('Concept.fsh')
-      .withLocation([1, 2, 3, 4]);
-    concept.system = 'ZOO';
+    const concept = new ConceptRule('bear', 'brown bear')
+      .withFile('RuleSets.fsh')
+      .withLocation([8, 5, 8, 16]);
     ruleSet1.rules.push(concept);
 
-    const insertRule = new InsertRule('').withFile('Insert.fsh').withLocation([1, 2, 3, 4]);
+    const insertRule = new InsertRule('').withFile('VS.fsh').withLocation([7, 6, 7, 16]);
     insertRule.ruleSet = 'Bar';
     vs.rules.push(insertRule);
 
     applyInsertRules(vs, tank);
 
-    expect(vs.rules).toHaveLength(1);
-    assertValueSetConceptComponent(
-      vs.rules[0],
-      undefined,
-      undefined,
-      [new FshCode('bear', 'ZOO', 'brown bear')],
-      true
+    expect(vs.rules).toHaveLength(0);
+    expect(loggerSpy.getLastMessage('error')).toMatch(
+      /ConceptRule.*ValueSet.*File: RuleSets\.fsh.*Line: 8.*Applied in File: VS\.fsh.*Line: 7\D*/s
     );
-    expect(loggerSpy.getLastMessage('warn')).toMatch(
-      /ValueSet concepts should not include a definition, only system, code, and display are supported.*File: Concept\.fsh.*Line: 1 - 3\D*/s
+  });
+
+  it('should log an error when a ConceptRule with a system is applied to a FshCodeSystem', () => {
+    // RuleSet: Bar
+    // * ZOO#bear "brown bear"
+    //
+    // CodeSystem: Foo
+    // * insert Bar
+    const cs = new FshCodeSystem('Foo');
+    doc.codeSystems.set(cs.name, cs);
+
+    const concept = new ConceptRule('bear', 'brown bear')
+      .withFile('RuleSets.fsh')
+      .withLocation([7, 5, 7, 12]);
+    concept.system = 'ZOO';
+    ruleSet1.rules.push(concept);
+
+    const insertRule = new InsertRule('').withFile('CodeSystems.fsh').withLocation([3, 5, 3, 19]);
+    insertRule.ruleSet = 'Bar';
+    cs.rules.push(insertRule);
+
+    applyInsertRules(cs, tank);
+
+    expect(cs.rules).toHaveLength(1);
+    assertConceptRule(cs.rules[0], 'bear', 'brown bear', undefined, []);
+    expect(loggerSpy.getLastMessage('error')).toMatch(
+      /Do not include the system when listing concepts for a code system\..*File: RuleSets\.fsh.*Line: 7.*Applied in File: CodeSystems\.fsh.*Line: 3\D*/s
     );
   });
 
diff --git a/test/import/FSHImporter.RuleSet.test.ts b/test/import/FSHImporter.RuleSet.test.ts
index 59da785..1edd556 100644
--- a/test/import/FSHImporter.RuleSet.test.ts
+++ b/test/import/FSHImporter.RuleSet.test.ts
@@ -6,7 +6,8 @@ import {
   assertInsertRule,
   assertValueSetConceptComponent,
   assertAddElementRule,
-  assertCaretValueRule
+  assertCaretValueRule,
+  assertConceptRule
 } from '../testhelpers/asserts';
 import { loggerSpy } from '../testhelpers/loggerSpy';
 import { Rule, ConceptRule } from '../../src/fshtypes/rules';
@@ -216,5 +217,20 @@ describe('FSHImporter', () => {
       );
       expect(loggerSpy.getLastMessage('error')).toMatch(/File: SameName\.fsh.*Line: 5 - 6\D*/s);
     });
+
+    it('should not log an error when the ConceptRule has one code with a system, no definition, and no hierarchy', () => {
+      const input = `
+      RuleSet: VSRuleSet
+      * ZOO#bear
+      * ZOO#gator "Alligator"
+      `;
+      const result = importSingleText(input, 'VSRules.fsh');
+      expect(result.ruleSets.size).toBe(1);
+      const ruleSet = result.ruleSets.get('VSRuleSet');
+      expect(ruleSet.rules.length).toBe(2);
+      assertConceptRule(ruleSet.rules[0], 'bear', undefined, undefined, []);
+      assertConceptRule(ruleSet.rules[1], 'gator', 'Alligator', undefined, []);
+      expect(loggerSpy.getAllMessages('error')).toBeEmpty();
+    });
   });
 });