fix(developer): restrict invalid characters in identifiers in kmcmplib by mcdurdin · Pull Request #14746 · keymanapp/keyman

mcdurdin · 2025-09-12T09:02:13Z

The compiler has always been very ambiguous on which characters were accepted in group and store names, even to the point of accepting things like comma in a store name, which would then make it impossible to reference in an index statement!

This commit clarifies the allowable characters in an identifier. While it would have been possible to use UAX#31 for this, that would have extended the requirements for this change substantially, and may have caused us more trouble with legacy keyboards. Given kmcmplib is end-of-life (see epic/ng-compiler), I have chosen a lower friction approach. There are certainly other characters that could be excluded, but in general I have chosen to exclude only those that will definitely be problematic.

The set of allowable characters for deadkeys has actually been expanded in this release to match the store and group name rules.

It is expected that there may be some impacted keyboards, but addressing this change will be relatively straightforward, so I consider this to be an acceptable back-compatibility trade-off, see
https://github.com/keymanapp/keyman/wiki/Principles-of-Keyman-Code-Changes#4-source-backward-compatibility-keyboard-model-and-package-source-file-formats-should-be-backward-compatible

Fixes: #14604
Test-bot: skip
Build-bot: skip build:developer

The compiler has always been very ambiguous on which characters were accepted in group and store names, even to the point of accepting things like comma in a store name, which would then make it impossible to reference in an `index` statement! This commit clarifies the allowable characters in an identifier. While it would have been possible to use UAX#31 for this, that would have extended the requirements for this change substantially, and may have caused us more trouble with legacy keyboards. Given kmcmplib is end-of-life (see epic/ng-compiler), I have chosen a lower friction approach. There are certainly other characters that could be excluded, but in general I have chosen to exclude only those that will definitely be problematic. The set of allowable characters for deadkeys has actually been expanded in this release to match the store and group name rules. It is expected that there may be some impacted keyboards, but addressing this change will be relatively straightforward, so I consider this to be an acceptable back-compatibility trade-off, see https://github.com/keymanapp/keyman/wiki/Principles-of-Keyman-Code-Changes#4-source-backward-compatibility-keyboard-model-and-package-source-file-formats-should-be-backward-compatible Fixes: #14604 Test-bot: skip Build-bot: skip build:developer

keymanapp-test-bot · 2025-09-12T09:02:17Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Developer
- Keyman Developer - build : all tests passed (no artifacts on BuildLevel "build")
- Compiler Regression Tests - build : all tests passed (no artifacts on BuildLevel "build")
- Keyman Developer (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
Keyboards
- Test Keyboards - build : all tests passed (no artifacts on BuildLevel "build")

Document the tightened validity of all identifiers in .kmn, which will make it easier to build the new parser and allow for extension of the language in the future. Relates-to: keymanapp/keyman#14604 Relates-to: keymanapp/keyman#14746 Test-bot: skip

mcdurdin · 2025-09-12T09:03:45Z

@markcsinclair I am aware that you are not currently available to review. Tagging you for your future reference.

mcdurdin · 2025-09-12T09:04:18Z

developer/src/common/include/kmn_compiler_errors.h


 namespace KmnCompilerMessages {
-  enum {
+  enum KmnCompilerMessages {


naming the enum so we can use it elsewhere for cleaner types

mcdurdin · 2025-09-12T09:04:57Z

developer/src/kmcmplib/src/Compiler.cpp

-KMX_WCHAR const * DeadKeyChars =
-u"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_";


Actually expanding deadkey name validity

mcdurdin · 2025-09-12T09:05:25Z

developer/src/kmcmplib/src/CompilerErrors.h

+class CompilerMessage {
+public:
+  virtual void report(enum KmnCompilerMessages::KmnCompilerMessages msg, const std::vector<std::string>& parameters) = 0;
+  virtual void report(enum KmnCompilerMessages::KmnCompilerMessages msg) = 0;
+};
+
+class DefaultCompilerMessage : public CompilerMessage {
+  virtual void report(enum KmnCompilerMessages::KmnCompilerMessages msg, const std::vector<std::string>& parameters) ;
+  virtual void report(enum KmnCompilerMessages::KmnCompilerMessages msg);
+};


This pattern makes CompilerMessage easily mockable

v19 of kmc will disallow spaces in group names. Pre-emptively tweaking this. As this change has no functional impact, am not bumping the version number or history. Relates-to: keymanapp/keyman#14746 Relates-to: keymanapp/keyman#14604

mcdurdin · 2025-09-12T09:29:56Z

I have tested this against the keyboards repository, and only sil_yi is impacted. sil_yi was the keyboard that triggered this investigation. I have opened PR keymanapp/keyboards#3720 to fix this.

…tion classes

sil_yi was impacted by the changes in #14746, as it had the group name 'Unicode Group', which is now illegal, so the compiler fails to build the keyboard at the referenced commit. Easiest workaround currently is to remove it from the set of compared keyboards.

jahorton

Are there any corresponding changes that need to be made within the Web TS compiler?

mcdurdin · 2025-09-12T13:55:45Z

Are there any corresponding changes that need to be made within the Web TS compiler?

No, because that starts with the output from kmcmplib, so all identifiers have already been checked.

.../src/kmc-kmn/test/fixtures/invalid-keyboards/error_name_contains_invalid_character-group.kmn

.../src/kmc-kmn/test/fixtures/invalid-keyboards/error_name_contains_invalid_character-store.kmn

...rc/kmc-kmn/test/fixtures/invalid-keyboards/error_name_contains_invalid_character-deadkey.kmn

ermshiperete · 2025-09-12T13:58:43Z

developer/src/kmcmplib/src/validation.cpp

+KMX_BOOL Uni_IsControlCharacter(KMX_WCHAR ch);
+
+/*
+  Unicode version 16.0; GC=Zs.


I guess these are all the spaces that Unicode defines? A sentence explaining that might help...

Yeah, that was the cryptic GC=Zs 😆 I will elucidate!

We've got Unicode 17.0 now 😄
It's got the same list

developer/src/kmcmplib/src/validation.cpp

… variables

darcywong00 · 2025-09-13T01:50:01Z

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

+  static ERROR_NameMustNotContainSquareBrackets               = SevError | 0x0BA;
+  static Error_NameMustNotContainSquareBrackets               = (o:{name:string}) => m(
+    this.ERROR_NameMustNotContainSquareBrackets,
+    `The referenced name '${def(o.name)}' must not contain opening or closing square brackets`,


I get that the error message has enough detail for the user to clean up on the invalid parts of the name.

I also wonder since the error codes are split up, would it be helpful to have a specific message for each error? Granted it could also be frustrating to have to keep recompiling to find each error...

What about
"The referenced name must (not) contain X...
Names (or identifiers) must not contain spaces, commas, ...."

I could simplify to a single error message... still need all the tests though. But it would make the error message very long.

darcywong00 · 2025-09-13T02:01:15Z

developer/src/kmcmplib/src/validation.cpp

+KMX_BOOL Uni_IsControlCharacter(KMX_WCHAR ch);
+
+/*
+  Unicode version 16.0; GC=Zs.


We've got Unicode 17.0 now 😄
It's got the same list

darcywong00 · 2025-09-13T02:03:50Z

developer/src/common/include/kmn_compiler_errors.h

+    ERROR_NameMustNotContainSpaces =                    SevError | 0x0B7,
+    ERROR_NameMustNotContainComma =                     SevError | 0x0B8,
+    ERROR_NameMustNotContainParentheses =               SevError | 0x0B9,
+    ERROR_NameMustNotContainSquareBrackets =            SevError | 0x0BA,


curly braces allowed though, huh?

Curly braces are not recognized characters in Kmn, whereas square brackets are.

ermshiperete

LGTM now 😄

keyman-server · 2025-09-15T18:01:40Z

Changes in this pull request will be available for download in Keyman version 19.0.119-alpha

sil_yi was impacted by the changes in keymanapp#14746, as it had the group name 'Unicode Group', which is now illegal, so the compiler fails to build the keyboard at the referenced commit. Easiest workaround currently is to remove it from the set of compared keyboards.

github-project-automation bot added this to Keyman Sep 12, 2025

github-project-automation bot moved this to Todo in Keyman Sep 12, 2025

github-actions bot added developer/ developer/compilers/ fix labels Sep 12, 2025

keymanapp-test-bot bot added this to the A19S11 milestone Sep 12, 2025

mcdurdin mentioned this pull request Sep 12, 2025

docs(developer): tightened validity of store, group, and deadkey names keymanapp/help.keyman.com#2255

Merged

mcdurdin requested a review from ermshiperete September 12, 2025 09:03

mcdurdin requested review from darcywong00, jahorton and markcsinclair September 12, 2025 09:03

mcdurdin commented Sep 12, 2025

View reviewed changes

mcdurdin mentioned this pull request Sep 12, 2025

fix(sil_yi): remove space from group name keymanapp/keyboards#3720

Merged

fix(developer): add default destructor for CompilerMessage and Valida…

08c36db

…tion classes

mcdurdin force-pushed the fix/developer/14604-disallow-space-in-group-name branch from b79cc94 to 08c36db Compare September 12, 2025 12:14

mcdurdin added 2 commits September 12, 2025 14:20

chore(developer): rename constructor parameter to avoid shadow

5facfd9

jahorton reviewed Sep 12, 2025

View reviewed changes

ermshiperete requested changes Sep 12, 2025

View reviewed changes

fix(developer): address review comments, adding comments and renaming…

bd3d268

… variables

darcywong00 modified the milestones: A19S11, A19S12 Sep 13, 2025

darcywong00 reviewed Sep 13, 2025

View reviewed changes

ermshiperete approved these changes Sep 15, 2025

View reviewed changes

mcdurdin merged commit 121fb80 into master Sep 15, 2025
8 checks passed

mcdurdin deleted the fix/developer/14604-disallow-space-in-group-name branch September 15, 2025 12:49

github-project-automation bot moved this from Todo to Done in Keyman Sep 15, 2025

		KMX_WCHAR const * DeadKeyChars =
		u"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_";

Uh oh!

Conversation

mcdurdin commented Sep 12, 2025

Uh oh!

keymanapp-test-bot bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

mcdurdin commented Sep 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcdurdin commented Sep 12, 2025

Uh oh!

jahorton left a comment

Choose a reason for hiding this comment

Uh oh!

mcdurdin commented Sep 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ermshiperete left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keyman-server commented Sep 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

keymanapp-test-bot bot commented Sep 12, 2025 •

edited

Loading