Skip to content

Commit

Permalink
fix(developer): check vars string usage before definition
Browse files Browse the repository at this point in the history
Validation of vars was not properly checking for forward references to
string variables. This, coupled with a null vs undefined bug in
subsequent use, meant that forward reference variables were ending up
with a literal string value of 'undefined'.

This also fixes the test for visual-keyboard-compiler, where the fixture
was actually buggy and was the trigger for investigating this problem.

Fixes: #12403
Relates-to: #12395
  • Loading branch information
mcdurdin committed Sep 12, 2024
1 parent 91cd736 commit 44e28e2
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 7 deletions.
2 changes: 1 addition & 1 deletion common/web/types/src/kmx/kmx-plus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ export class Vars extends Section {
});
}
findStringVariableValue(id: string): string {
return Vars.findVariable(this.strings, id)?.value?.value; // Unwrap: Variable, StrsItem
return Vars.findVariable(this.strings, id)?.value?.value ?? null; // Unwrap: Variable, StrsItem
}
substituteSetRegex(str: string, sections: DependencySections): string {
return str.replaceAll(VariableParser.SET_REFERENCE, (_entire, id) => {
Expand Down
9 changes: 8 additions & 1 deletion developer/src/kmc-ldml/src/compiler/vars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,16 @@ export class VarsCompiler extends SectionCompiler {
// Strings
for (const { id, value } of variables.string) {
addId(id);
allStrings.add(id);
const stringrefs = VariableParser.allStringReferences(value);
for(const ref of stringrefs) {
if(!allStrings.has(ref)) {
valid = false;
this.callbacks.reportMessage(LdmlCompilerMessages.Error_MissingStringVariable({id: ref}));
allStrings.add(ref); // avoids multiple reports of same missing variable
}
}
st.string.add(SubstitutionUse.variable, stringrefs);
allStrings.add(id);
}
// Sets
for (const { id, value } of variables.set) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="UTF-8"?>

<keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
<info name="vars-fail"/>

<keys />

<!-- from spec -->
<variables>
<string id="y" value="${usedBeforeDefinition}" /> <!-- FAIL: reference before definition -->
<string id="usedBeforeDefinition" value="yes" />
</variables>


</keyboard3>
9 changes: 8 additions & 1 deletion developer/src/kmc-ldml/test/test-vars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,14 @@ describe('vars', function () {
CompilerMessages.Error_MissingStringVariable({id: 'missingStringInSet'})
],
},
], varsDependencies);
{
subpath: 'sections/vars/fail-badref-7.xml',
errors: [
LdmlCompilerMessages.Error_MissingStringVariable({id: 'usedBeforeDefinition'})
],
strictErrors: true
},
], varsDependencies);
describe('should match some marker constants', () => {
// neither of these live here, but, common/web/types does not import ldml-keyboard-constants otherwise.

Expand Down
8 changes: 4 additions & 4 deletions developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ describe('visual-keyboard-compiler', function() {
assert.equal(vk.keys[1].text, '\u{0e81}');
});

it.skip('should read string variables in key.output', async function() {
it('should read string variables in key.output', async function() {
const xml = stripIndent`
<?xml version="1.0" encoding="UTF-8"?>
<keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
Expand All @@ -164,8 +164,8 @@ describe('visual-keyboard-compiler', function() {
<layer modifiers="none"><row keys="x" /></layer>
</layers>
<variables>
<string id="one" value="\${two}" />
<string id="two" value="2" />
<string id="one" value="\${two}" />
</variables>
</keyboard3>
`;
Expand All @@ -176,7 +176,7 @@ describe('visual-keyboard-compiler', function() {
assert.equal(vk.keys[0].text, '2');
});

it.skip('should read string variables in display.display', async function() {
it('should read string variables in display.display', async function() {
const xml = stripIndent`
<?xml version="1.0" encoding="UTF-8"?>
<keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
Expand All @@ -191,8 +191,8 @@ describe('visual-keyboard-compiler', function() {
<layer modifiers="none"><row keys="x" /></layer>
</layers>
<variables>
<string id="one" value="\${two}" />
<string id="two" value="2" />
<string id="one" value="\${two}" />
</variables>
</keyboard3>
`;
Expand Down

0 comments on commit 44e28e2

Please sign in to comment.