Merge pull request #8951 from keymanapp/feat/KmxFileReader-error-hand…

…ling-and-more

chore(common): cleanup error handling for KmxFileReader and CompilerOptions as shared type

common/web/types/package.json

@@ -28,7 +28,7 @@
   "dependencies": {
     "@keymanapp/keyman-version": "*",
     "ajv": "^8.11.0",
-    "restructure": "git+https://github.com/keymanapp/dependency-restructure.git#49d129cf0916d082a7278bb09296fb89cecfcc50",
+    "restructure": "git+https://github.com/keymanapp/dependency-restructure.git#7a188a1e26f8f36a175d95b67ffece8702363dfc",
     "semver": "^7.3.7",
     "xml2js": "git+https://github.com/keymanapp/dependency-node-xml2js#535fe732dc408d697e0f847c944cc45f0baf0829"
   },

common/web/types/src/kmx/kmx-file-reader.ts

@@ -1,5 +1,10 @@
 import { KMXFile, BUILDER_COMP_KEYBOARD, KEYBOARD, STORE, GROUP, KEY } from "./kmx.js";
 import * as r from 'restructure';
+import { KeymanTypesError } from "../util/errors.js";
+
+export class KmxFileReaderError extends KeymanTypesError {
+
+}
 
 export class KmxFileReader {
   private readonly rString = new r.String(null, 'utf16le');
@@ -13,7 +18,17 @@ export class KmxFileReader {
     return this.rString.fromBuffer(source.slice(offset));
   }
 
-  private processSystemStore(store: STORE, result: KEYBOARD) {
+  private isValidCodeUse(s: string, keyboard: KEYBOARD): boolean {
+    return (
+      s.length == 3 &&
+      s.charCodeAt(0) == KMXFile.UC_SENTINEL &&
+      s.charCodeAt(1) == KMXFile.CODE_USE &&
+      s.charCodeAt(2) > 0 &&
+      s.charCodeAt(2) <= keyboard.groups.length
+    );
+  }
+
+  private processSystemStore(store: STORE, result: KEYBOARD): void {
     switch(store.dwSystemID) {
       case KMXFile.TSS_MNEMONIC:
         result.isMnemonic = store.dpString == '1';
@@ -22,34 +37,26 @@ export class KmxFileReader {
         result.keyboardVersion = store.dpString;
         break;
       case KMXFile.TSS_BEGIN_NEWCONTEXT:
-        if(store.dpString.length == 3 && store.dpString.charCodeAt(0) == 0xFFFF && store.dpString.charCodeAt(1) == KMXFile.CODE_USE) {
-          result.startGroup.newContext = store.dpString.charCodeAt(2) - 1;
-        }
-        else {
-          // TODO: error
-          return false;
+        if(!this.isValidCodeUse(store.dpString, result)) {
+          throw new KmxFileReaderError(`Invalid TSS_BEGIN_NEWCONTEXT system store`);
         }
+        result.startGroup.newContext = store.dpString.charCodeAt(2) - 1;
         break;
       case KMXFile.TSS_BEGIN_POSTKEYSTROKE:
-        if(store.dpString.length == 3 && store.dpString.charCodeAt(0) == 0xFFFF && store.dpString.charCodeAt(1) == KMXFile.CODE_USE) {
-          result.startGroup.postKeystroke = store.dpString.charCodeAt(2) - 1;
-        }
-        else {
-          // TODO: error
-          return false;
+        if(!this.isValidCodeUse(store.dpString, result)) {
+          throw new KmxFileReaderError(`Invalid TSS_BEGIN_POSTKEYSTROKE system store`);
         }
+        result.startGroup.postKeystroke = store.dpString.charCodeAt(2) - 1;
         break;
     }
-    return true;
   }
 
   public read(source: Uint8Array): KEYBOARD {
     let binaryKeyboard: BUILDER_COMP_KEYBOARD;
     let kmx = new KMXFile();
     binaryKeyboard = kmx.COMP_KEYBOARD.fromBuffer(source);
     if(binaryKeyboard.dwIdentifier != KMXFile.FILEID_COMPILED) {
-      // TODO: error
-      return null;
+      throw new KmxFileReaderError(`Not a .kmx file: header does not contain FILEID_COMPILED`);
     }
 
     let result = new KEYBOARD();
@@ -59,32 +66,39 @@ export class KmxFileReader {
     result.startGroup = {
       ansi: binaryKeyboard.StartGroup_ANSI == 0xFFFFFFFF ? -1 : binaryKeyboard.StartGroup_ANSI,
       unicode: binaryKeyboard.StartGroup_Unicode == 0xFFFFFFFF ? -1 : binaryKeyboard.StartGroup_Unicode,
-      newContext: -1, //TODO
-      postKeystroke: -1 // TODO
+      newContext: -1,
+      postKeystroke: -1
     }
 
     // Informative data
+
     result.keyboardVersion = '';
     result.isMnemonic = false;
 
-    let offset = binaryKeyboard.dpStoreArray;
-    for(let i = 0; i < binaryKeyboard.cxStoreArray; i++) {
-      let binaryStore = kmx.COMP_STORE.fromBuffer(source.slice(offset));
-      let store = new STORE();
-      store.dwSystemID = binaryStore.dwSystemID;
-      store.dpName = this.readString(source, binaryStore.dpName);
-      store.dpString = this.readString(source, binaryStore.dpString);
-      result.stores.push(store);
+    this.readStores(binaryKeyboard, kmx, source, result);
+    this.readGroupsAndRules(binaryKeyboard, kmx, source, result);
 
-      if(!this.processSystemStore(store, result)) {
-        return null;
-      }
+    // Process system stores once we have all stores and groups loaded
+    for(let store of result.stores) {
+      this.processSystemStore(store, result);
+    }
 
-      offset += KMXFile.COMP_STORE_SIZE;
+    // TODO: KMXPlusFile
+
+    // Validate startGroup offsets
+    let gp: keyof KEYBOARD['startGroup'];
+    for(gp in result.startGroup) {
+      if(result.startGroup[gp] < -1 || result.startGroup[gp] >= result.groups.length) {
+        throw new KmxFileReaderError(`Invalid begin group reference`);
+      }
     }
 
-    offset = binaryKeyboard.dpGroupArray;
-    for(let i = 0; i < binaryKeyboard.cxGroupArray; i++) {
+    return result;
+  }
+
+  private readGroupsAndRules(binaryKeyboard: BUILDER_COMP_KEYBOARD, kmx: KMXFile, source: Uint8Array, result: KEYBOARD) {
+    let offset = binaryKeyboard.dpGroupArray;
+    for (let i = 0; i < binaryKeyboard.cxGroupArray; i++) {
       let binaryGroup = kmx.COMP_GROUP.fromBuffer(source.slice(offset));
       let group = new GROUP();
       group.dpMatch = this.readString(source, binaryGroup.dpMatch);
@@ -94,7 +108,7 @@ export class KmxFileReader {
       group.keys = [];
 
       let keyOffset = binaryGroup.dpKeyArray;
-      for(let j = 0; j < binaryGroup.cxKeyArray; j++) {
+      for (let j = 0; j < binaryGroup.cxKeyArray; j++) {
         let binaryKey = kmx.COMP_KEY.fromBuffer(source.slice(keyOffset));
         let key = new KEY();
         key.Key = binaryKey.Key;
@@ -109,18 +123,18 @@ export class KmxFileReader {
       result.groups.push(group);
       offset += KMXFile.COMP_GROUP_SIZE;
     }
+  }
 
-    // TODO: KMXPlusFile
-
-    // Validate startGroup offsets
-    let gp: keyof KEYBOARD['startGroup'];
-    for(gp in result.startGroup) {
-      if(result.startGroup[gp] < -1 || result.startGroup[gp] >= result.groups.length) {
-        // TODO: error
-        return null;
-      }
+  private readStores(binaryKeyboard: BUILDER_COMP_KEYBOARD, kmx: KMXFile, source: Uint8Array, result: KEYBOARD): void {
+    let offset = binaryKeyboard.dpStoreArray;
+    for (let i = 0; i < binaryKeyboard.cxStoreArray; i++) {
+      let binaryStore = kmx.COMP_STORE.fromBuffer(source.slice(offset));
+      let store = new STORE();
+      store.dwSystemID = binaryStore.dwSystemID;
+      store.dpName = this.readString(source, binaryStore.dpName);
+      store.dpString = this.readString(source, binaryStore.dpString);
+      result.stores.push(store);
+      offset += KMXFile.COMP_STORE_SIZE;
     }
-
-    return result;
   }
 };

common/web/types/src/main.ts

@@ -2,7 +2,7 @@
 export * as KMX from './kmx/kmx.js';
 export * as KMXPlus from './kmx/kmx-plus.js';
 export { default as KMXBuilder } from './kmx/kmx-builder.js';
-export { KmxFileReader } from './kmx/kmx-file-reader.js';
+export { KmxFileReader, KmxFileReaderError } from './kmx/kmx-file-reader.js';
 
 export * as VisualKeyboard from './kvk/visual-keyboard.js';
 export { default as KMXPlusBuilder}  from './kmx/kmx-plus-builder/kmx-plus-builder.js';
@@ -18,7 +18,7 @@ export { LDMLKeyboardXMLSourceFileReader, LDMLKeyboardXMLSourceFileReaderOptions
 
 export * as Constants from './consts/virtual-key-constants.js';
 
-export { CompilerCallbacks, CompilerSchema, CompilerEvent, CompilerErrorNamespace, CompilerErrorSeverity, CompilerPathCallbacks, CompilerFileSystemCallbacks, CompilerMessageSpec, compilerErrorSeverityName, compilerExceptionToString, compilerErrorFormatCode } from './util/compiler-interfaces.js';
+export { CompilerCallbacks, CompilerOptions, CompilerSchema, CompilerEvent, CompilerErrorNamespace, CompilerErrorSeverity, CompilerPathCallbacks, CompilerFileSystemCallbacks, CompilerMessageSpec, compilerErrorSeverityName, compilerExceptionToString, compilerErrorFormatCode } from './util/compiler-interfaces.js';
 export { CommonTypesMessages } from './util/common-events.js';
 
 export * as TouchLayout from './keyman-touch-layout/keyman-touch-layout-file.js';

common/web/types/src/util/compiler-interfaces.ts

@@ -141,6 +141,16 @@ export interface CompilerCallbacks {
   debug(msg: string): void;
 };
 
+/**
+ * Abstract interface for compiler options
+ */
+export interface CompilerOptions {
+  shouldAddCompilerVersion?: boolean;
+  saveDebug?: boolean;
+  compilerWarningsAsErrors?: boolean;
+	warnDeprecatedCode?: boolean;
+};
+
 /**
  * Convenience function for constructing CompilerEvents
  * @param code

common/web/types/src/util/errors.ts

@@ -0,0 +1,6 @@
+/**
+ * Base class for all common/web/types errors thrown
+ */
+export class KeymanTypesError extends Error {
+
+}

developer/src/kmc-kmn/src/compiler/compiler.ts

@@ -6,7 +6,7 @@ TODO: implement additional interfaces:
 */
 
 // TODO: rename wasm-host?
-import { CompilerCallbacks, CompilerEvent, KvkFileWriter, KvksFileReader } from '@keymanapp/common-types';
+import { CompilerCallbacks, CompilerEvent, CompilerOptions, KvkFileWriter, KvksFileReader } from '@keymanapp/common-types';
 import loadWasmHost from '../import/kmcmplib/wasm-host.js';
 import { CompilerMessages, mapErrorFromKmcmplib } from './messages.js';
 
@@ -21,15 +21,11 @@ export interface CompilerResult {
   js?: CompilerResultFile;
 };
 
-export interface CompilerOptions {
-  shouldAddCompilerVersion?: boolean;
-  saveDebug?: boolean;
-  compilerWarningsAsErrors?: boolean;
-	warnDeprecatedCode?: boolean;
+export interface KmnCompilerOptions extends CompilerOptions {
   target?: 'kmx' | 'js';
 };
 
-const baseOptions: CompilerOptions = {
+const baseOptions: KmnCompilerOptions = {
   shouldAddCompilerVersion: true,
   saveDebug: true,
   compilerWarningsAsErrors: false,
@@ -90,7 +86,7 @@ export class KmnCompiler {
     return true;
   }
 
-  public run(infile: string, outfile: string, options?: CompilerOptions): boolean {
+  public run(infile: string, outfile: string, options?: KmnCompilerOptions): boolean {
     let result = this.runCompiler(infile, outfile, options);
     if(result) {
       if(result.kmx) {
@@ -146,7 +142,7 @@ export class KmnCompiler {
     return 1;
   }
 
-  public runCompiler(infile: string, outfile: string, options: CompilerOptions): CompilerResult {
+  public runCompiler(infile: string, outfile: string, options: KmnCompilerOptions): CompilerResult {
     if(!this.verifyInitialized()) {
       /* c8 ignore next 2 */
       return null;

developer/src/kmc-kmw/package.json

@@ -24,9 +24,10 @@
   },
   "dependencies": {
     "@keymanapp/common-types": "*",
+    "@keymanapp/keyman-version": "*",
     "@keymanapp/kmc-kmn": "*",
     "ajv": "^8.11.0",
-    "restructure": "git+https://github.com/keymanapp/dependency-restructure.git#49d129cf0916d082a7278bb09296fb89cecfcc50",
+    "restructure": "git+https://github.com/keymanapp/dependency-restructure.git#7a188a1e26f8f36a175d95b67ffece8702363dfc",
     "semver": "^7.3.7",
     "xml2js": "git+https://github.com/keymanapp/dependency-node-xml2js#535fe732dc408d697e0f847c944cc45f0baf0829"
   },

developer/src/kmc-kmw/src/compiler/compiler-globals.ts

@@ -1,5 +1,4 @@
-import { KMX, CompilerCallbacks } from "@keymanapp/common-types";
-import CompilerOptions from "./compiler-options.js";
+import { KMX, CompilerCallbacks, CompilerOptions } from "@keymanapp/common-types";
 
 export let FTabStop: string;
 export let nl: string;

developer/src/kmc-kmw/src/compiler/javascript-strings.ts

@@ -15,7 +15,7 @@ export function JavaScript_Name(i: number, pwszName: string, KeepNameForPersiste
   let FChanged: boolean = false;
   let p = pwszName;
 
-  if((pwszName == null || pwszName == undefined || pwszName == '') || (!options.debug && !KeepNameForPersistentStorage)) {   // I3659   // I3681
+  if((pwszName == null || pwszName == undefined || pwszName == '') || (!options.saveDebug && !KeepNameForPersistentStorage)) {   // I3659   // I3681
     return i.toString(10); // for uniqueness
   }
   else {
@@ -135,7 +135,7 @@ function JavaScript_Rule(FTabStops: string, FElse: string, fk: KMX.KEYBOARD, fgp
   let predicate: string = '1', linecomment: string = '', FIndent: string;
   let result = '';
 
-  if(fkp.Line > 0 && options.debug) {   // I4384
+  if(fkp.Line > 0 && options.saveDebug) {   // I4384
     linecomment = '   // Line '+fkp.Line.toString();   // I4373
   }
 
@@ -315,7 +315,7 @@ export function JavaScript_Shift(fkp: KMX.KEY, FMnemonic: boolean): number {
  *         '16656'
  */
 export function JavaScript_ShiftAsString(fkp: KMX.KEY, FMnemonic: boolean): string {
-  if(!options.debug) {
+  if(!options.saveDebug) {
     return JavaScript_Shift(fkp, FMnemonic).toString();
   }
   return ' '+FormatModifierAsBitflags(JavaScript_Shift(fkp, FMnemonic));
@@ -719,7 +719,7 @@ export function JavaScript_Key(fkp: KMX.KEY, FMnemonic: boolean): number {
 /// @return string representation of the key value, e.g. 'keyCodes.K_A /* 0x41 */' or '65'
 ///
 export function JavaScript_KeyAsString(fkp: KMX.KEY, FMnemonic: boolean): string {
-  if(options.debug) {
+  if(options.saveDebug) {
     return ' '+FormatKeyAsString(JavaScript_Key(fkp, FMnemonic));
   } else {
     return JavaScript_Key(fkp, FMnemonic).toString();