Merge pull request #8892 from keymanapp/chore/developer/8886-kvk-compiler-checks

chore(developer): verify kvks files and report errors

common/web/types/src/kvk/kvks-file-reader.ts

@@ -7,12 +7,6 @@ import { VisualKeyboard, VisualKeyboardHeaderFlags, VisualKeyboardKey, VisualKey
 import { USVirtualKeyCodes } from '../consts/virtual-key-constants.js';
 import { BUILDER_KVK_HEADER_VERSION } from './kvk-file.js';
 
-export enum KVKSParseErrorType { invalidVkey };
-export class KVKSParseError extends Error {
-  public type: KVKSParseErrorType;
-  public vkey: string;
-};
-
 export default class KVKSFileReader {
   public read(file: Uint8Array): KVKSourceFile {
     let source: KVKSourceFile;
@@ -33,10 +27,12 @@ export default class KVKSFileReader {
       // rather than using the version tagged on npmjs.com.
     });
 
-    parser.parseString(file, (e: unknown, r: unknown) => { source = r as KVKSourceFile });
-    source = this.boxArrays(source);
-    this.cleanupFlags(source);
-    this.cleanupUnderscore('visualkeyboard', source.visualkeyboard);
+    parser.parseString(file, (e: unknown, r: unknown) => { if(e) { throw e }; source = r as KVKSourceFile });
+    if(source) {
+      source = this.boxArrays(source);
+      this.cleanupFlags(source);
+      this.cleanupUnderscore('visualkeyboard', source.visualkeyboard);
+    }
     return source;
   }
 
@@ -80,7 +76,7 @@ export default class KVKSFileReader {
     }
   }
 
-  public transform(source: KVKSourceFile, errors?: KVKSParseError[]): VisualKeyboard {
+  public transform(source: KVKSourceFile, invalidVkeys?: string[]): VisualKeyboard {
     // NOTE: at this point, the xml should have been validated
     // and matched the schema result so we can assume properties exist
     let result: VisualKeyboard = {
@@ -118,11 +114,8 @@ export default class KVKSFileReader {
         for(let sourceKey of layer.key) {
           let vkey = (USVirtualKeyCodes as any)[sourceKey.$?.vkey];
           if(!vkey) {
-            if(errors) {
-              let e = new KVKSParseError();
-              e.type = KVKSParseErrorType.invalidVkey;
-              e.vkey = sourceKey.$?.vkey;
-              errors.push(e);
+            if(typeof invalidVkeys !== 'undefined') {
+              invalidVkeys.push(sourceKey.$?.vkey);
             }
             continue;
           }

common/web/types/test/kvk/test-kvk-round-trip.ts

@@ -5,7 +5,7 @@ import Hexy from 'hexy';
 import gitDiff from 'git-diff';
 const { hexy } = Hexy;
 import { loadSchema, makePathToFixture } from '../helpers/index.js';
-import KvksFileReader, { KVKSParseError } from "../../src/kvk/kvks-file-reader.js";
+import KvksFileReader from "../../src/kvk/kvks-file-reader.js";
 import KvkFileReader from "../../src/kvk/kvk-file-reader.js";
 import KvkFileWriter from "../../src/kvk/kvk-file-writer.js";
 import KvksFileWriter from "../../src/kvk/kvks-file-writer.js";
@@ -53,9 +53,9 @@ describe('kvks-file-reader', function () {
     assert.doesNotThrow(() => {
       reader.validate(kvks, loadSchema('kvks'));
     });
-    const errors: KVKSParseError[] = [];
-    const vk = reader.transform(kvks, errors);
-    assert.isEmpty(errors);
+    const invalidVkeys: string[] = [];
+    const vk = reader.transform(kvks, invalidVkeys);
+    assert.isEmpty(invalidVkeys);
     const writer = new KvkFileWriter();
     const output = writer.write(vk);
     assertBufferMatch(Buffer.from(output), compiled);
@@ -76,9 +76,9 @@ describe('kvks-file-reader', function () {
       assert.doesNotThrow(() => {
         kvksReader.validate(kvks, loadSchema('kvks'));
       });
-      const errors: KVKSParseError[] = [];
-      const vk2 = kvksReader.transform(kvks, errors);
-      assert.isEmpty(errors);
+      const invalidVkeys: string[] = [];
+      const vk2 = kvksReader.transform(kvks, invalidVkeys);
+      assert.isEmpty(invalidVkeys);
 
       // make sure the binary is the same
       assert.deepEqual(vk2, vk);

common/web/types/test/kvk/test-kvks-file.ts

@@ -1,7 +1,7 @@
 import * as fs from 'fs';
 import 'mocha';
 import { loadSchema, makePathToFixture } from '../helpers/index.js';
-import KvksFileReader, { KVKSParseError } from "../../src/kvk/kvks-file-reader.js";
+import KvksFileReader from "../../src/kvk/kvks-file-reader.js";
 import KvksFileWriter from "../../src/kvk/kvks-file-writer.js";
 import { verify_khmer_angkor } from './test-kvk-utils.js';
 import { assert } from 'chai';
@@ -16,9 +16,9 @@ describe('kvks-file-reader', function() {
     assert.doesNotThrow(() => {
       reader.validate(kvks, loadSchema('kvks'));
     });
-    const errors: KVKSParseError[] = [];
-    const vk = reader.transform(kvks, errors);
-    assert.isEmpty(errors);
+    const invalidVkeys: string[] = [];
+    const vk = reader.transform(kvks, invalidVkeys);
+    assert.isEmpty(invalidVkeys);
     verify_khmer_angkor(vk);
   });
 });
@@ -30,9 +30,9 @@ describe('kvks-file-writer', function() {
 
     const reader = new KvksFileReader();
     const kvksExpected = reader.read(input);
-    const errors: KVKSParseError[] = [];
-    const vk = reader.transform(kvksExpected, errors);
-    assert.isEmpty(errors);
+    const invalidVkeys: string[] = [];
+    const vk = reader.transform(kvksExpected, invalidVkeys);
+    assert.isEmpty(invalidVkeys);
 
     const writer = new KvksFileWriter();
     const output = writer.write(vk);

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

@@ -88,18 +88,7 @@ export class KmnCompiler {
   }
 
   public run(infile: string, outfile: string, options?: CompilerOptions): boolean {
-    if(!this.verifyInitialized()) {
-      /* c8 ignore next 2 */
-      return false;
-    }
-
-    options = {...baseOptions, ...options};
-    (globalThis as any)[this.callbackID] = {
-      message: this.compilerMessageCallback,
-      loadFile: this.loadFileCallback
-    };
     let result = this.runCompiler(infile, outfile, options);
-    delete (globalThis as any)[this.callbackID];
     if(result) {
       if(result.kmx) {
         this.callbacks.fs.writeFileSync(result.kmx.filename, result.kmx.data);
@@ -133,9 +122,8 @@ export class KmnCompiler {
     }
 
     if(bufferSize != data.byteLength) {
-      // TODO: consider chucking a wobbly because this is a bug #8885
       /* c8 ignore next 2 */
-      return 0;
+      throw new Error(`Second call, expected file size ${bufferSize} == ${data.byteLength}`);
     }
 
     this.Module.HEAP8.set(data, buffer);
@@ -143,7 +131,19 @@ export class KmnCompiler {
     return 1;
   }
 
-  private runCompiler(infile: string, outfile: string, options: CompilerOptions): CompilerResult {
+  public runCompiler(infile: string, outfile: string, options: CompilerOptions): CompilerResult {
+    if(!this.verifyInitialized()) {
+      /* c8 ignore next 2 */
+      return null;
+    }
+
+    options = {...baseOptions, ...options};
+
+    (globalThis as any)[this.callbackID] = {
+      message: this.compilerMessageCallback,
+      loadFile: this.loadFileCallback
+    };
+
     let result: CompilerResult = {};
     let wasm_interface = new this.Module.CompilerInterface();
     let wasm_options = new this.Module.CompilerOptions();
@@ -183,6 +183,7 @@ export class KmnCompiler {
       }
       wasm_interface.delete();
       wasm_options.delete();
+      delete (globalThis as any)[this.callbackID];
     }
   }
 
@@ -190,22 +191,19 @@ export class KmnCompiler {
     // The compiler detected a .kvks file, which needs to be captured
     let reader = new KvksFileReader();
     kvksFilename = this.callbacks.resolveFilename(kmnFilename, kvksFilename);
-    let kvks = reader.read(this.callbacks.loadFile(kvksFilename));
+    let filename = this.callbacks.path.basename(kvksFilename);
+    let kvks = null;
     try {
+      kvks = reader.read(this.callbacks.loadFile(kvksFilename));
       reader.validate(kvks, this.callbacks.loadSchema('kvks'));
     } catch(e) {
-      console.log(e);
-      // TODO: also unit test #8886
-      // TODO: this.callbacks.reportMessage(CompilerMessages.Error_InvalidKvksFile({e}));
+      this.callbacks.reportMessage(CompilerMessages.Error_InvalidKvksFile({filename, e}));
       return null;
     }
-    let errors: any = []; //TODO: KVKSParseError[];
-    let vk = reader.transform(kvks, errors);
-    if(!vk || errors.length) {
-      console.dir(errors);
-      // TODO: also unit test #8886
-      // TODO: this.callbacks.reportMessage(CompilerMessages.Error_InvalidKvksFile({e}));
-      return null;
+    let invalidVkeys: string[] = [];
+    let vk = reader.transform(kvks, invalidVkeys);
+    for(let invalidVkey of invalidVkeys) {
+      this.callbacks.reportMessage(CompilerMessages.Warn_InvalidVkeyInKvksFile({filename, invalidVkey}));
     }
     let writer = new KvkFileWriter();
     return {

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

@@ -67,6 +67,14 @@ export class CompilerMessages {
 
   static Error_UnicodeSetSyntaxError = () => m(this.ERROR_UnicodeSetSyntaxError, `UnicodeSet had a Syntax Error while parsing`);
   static ERROR_UnicodeSetSyntaxError = SevError | 0x1007;
+
+  static Error_InvalidKvksFile = (o:{filename: string, e: any}) => m(this.ERROR_InvalidKvksFile,
+    `Error encountered parsing ${o.filename}: ${o.e}`);
+  static ERROR_InvalidKvksFile = SevError | 0x1008;
+
+  static Warn_InvalidVkeyInKvksFile = (o:{filename: string, invalidVkey: string}) => m(this.WARN_InvalidVkeyInKvksFile,
+    `Invalid virtual key ${o.invalidVkey} found in ${o.filename}`);
+  static WARN_InvalidVkeyInKvksFile = SevWarn | 0x1009;
 }
 
 export function mapErrorFromKmcmplib(line: number, code: number, msg: string): CompilerEvent {

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/error_invalid_kvks_file.kmn

@@ -0,0 +1,12 @@
+c Description: Verifies that kmc throws an error with an invalid .kvks file
+
+store(&NAME) 'error_invalid_kvks_file'
+store(&VERSION) '9.0'
+store(&VISUALKEYBOARD) 'error_invalid_kvks_file.kvks'
+
+begin unicode > use(main)
+
+group(main) using keys
+
++ [K_A] > 'a'
+'a' + [K_B] > 'ខ្មែរ'

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/error_invalid_kvks_file.kvks

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="utf-8"?>
+<visualkeyboard>
+  <header>
+    <version>10.0</version>
+    <kbdname>caps_lock_layer_3620</kbdname>
+    <!-- error_invalid_kvks_file: missing closing tag below -->
+    <flags>
+  </header>
+</visualkeyboard>

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/warn_invalid_vkey_in_kvks_file.kmn

@@ -0,0 +1,12 @@
+c Description: Verifies that kmc reports a warning with an invalid vkey in the kvks
+
+store(&NAME) 'warn_invalid_vkey_in_kvks_file'
+store(&VERSION) '9.0'
+store(&VISUALKEYBOARD) 'warn_invalid_vkey_in_kvks_file.kvks'
+
+begin unicode > use(main)
+
+group(main) using keys
+
++ [K_A] > 'a'
+'a' + [K_B] > 'ខ្មែរ'

developer/src/kmc-kmn/test/fixtures/invalid-keyboards/warn_invalid_vkey_in_kvks_file.kvks

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8"?>
+<visualkeyboard>
+  <header>
+    <version>10.0</version>
+    <kbdname>caps_lock_layer_3620</kbdname>
+    <flags />
+  </header>
+  <encoding name="unicode" fontname="Khmer Busra Kbd" fontsize="16">
+    <layer shift="RA">
+      <!-- warn_invalid_vkey_in_kvks_file, K_FOO -->
+      <key vkey="K_FOO">ឞ</key>
+    </layer>
+  </encoding>
+</visualkeyboard>

developer/src/kmc-kmn/test/helpers/index.ts

@@ -0,0 +1,17 @@
+/**
+ * Helpers and utilities for the Mocha tests.
+ */
+import * as path from 'path';
+import { fileURLToPath } from 'url';
+
+/**
+ * Builds a path to the fixture with the given path components.
+ *
+ * e.g., makePathToFixture('example.qaa.trivial')
+ * e.g., makePathToFixture('example.qaa.trivial', 'model.ts')
+ *
+ * @param components One or more path components.
+ */
+ export function makePathToFixture(...components: string[]): string {
+  return fileURLToPath(new URL(path.join('..', '..', '..', 'test', 'fixtures', ...components), import.meta.url));
+}

developer/src/kmc-kmn/test/test-messages.ts

@@ -1,9 +1,54 @@
 import 'mocha';
+import { assert } from 'chai';
 import { CompilerMessages } from '../src/compiler/messages.js';
-import { verifyCompilerMessagesObject } from '@keymanapp/developer-test-helpers';
+import { TestCompilerCallbacks, verifyCompilerMessagesObject } from '@keymanapp/developer-test-helpers';
+import { makePathToFixture } from './helpers/index.js';
+import { KmnCompiler } from '../src/main.js';
 
 describe('CompilerMessages', function () {
+  const callbacks = new TestCompilerCallbacks();
+
   it('should have a valid CompilerMessages object', function() {
     return verifyCompilerMessagesObject(CompilerMessages);
   });
+
+  //
+  // Message tests
+  //
+
+  async function testForMessage(context: Mocha.Context, fixture: string[], messageId?: number) {
+    context.timeout(10000);
+
+    callbacks.clear();
+
+    const compiler = new KmnCompiler();
+    assert(await compiler.init(callbacks));
+    assert(compiler.verifyInitialized());
+
+    const kmnPath = makePathToFixture(...fixture);
+    const outfile = callbacks.path.basename(kmnPath, '.kmn') + '.kmx';
+
+    // Note: throwing away compile results (just to memory)
+    compiler.runCompiler(kmnPath, outfile, {saveDebug: true, shouldAddCompilerVersion: false});
+
+    if(messageId) {
+      assert.isTrue(callbacks.hasMessage(messageId), `messageId ${messageId.toString(16)} not generated, instead got: `+JSON.stringify(callbacks.messages,null,2));
+      assert.lengthOf(callbacks.messages, 1);
+    } else {
+      assert.lengthOf(callbacks.messages, 0, `messages should be empty, but instead got: `+JSON.stringify(callbacks.messages,null,2));
+    }
+  }
+
+  // ERROR_InvalidKvksFile
+
+  it('should generate ERROR_InvalidKvksFile if the kvks is not valid XML', async function() {
+    await testForMessage(this, ['invalid-keyboards', 'error_invalid_kvks_file.kmn'], CompilerMessages.ERROR_InvalidKvksFile);
+  });
+
+  // WARN_InvalidVkeyInKvksFile
+
+  it('should generate WARN_InvalidVkeyInKvksFile if the kvks contains an invalid virtual key', async function() {
+    await testForMessage(this, ['invalid-keyboards', 'warn_invalid_vkey_in_kvks_file.kmn'], CompilerMessages.WARN_InvalidVkeyInKvksFile);
+  });
+
 });

developer/src/kmc-kmn/test/tsconfig.json

@@ -13,7 +13,8 @@
       },
     },
   "include": [
-      "**/test-*.ts"
+      "**/test-*.ts",
+      "./helpers/index.ts"
   ],
   "references": [
       { "path": "../../../../common/web/keyman-version/tsconfig.esm.json" },