Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

langium generate: fixed a hidden bug in 'node-processor.ts' of Langium's 'generate' facility #1814

Merged
merged 1 commit into from
Feb 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -158,19 +158,25 @@
"skipFiles": ["<node_internals>/**", "**/node_modules/**"],
"program": "${workspaceRoot}/node_modules/vitest/vitest.mjs",
"args": ["run", "--no-color", "--no-coverage", "--no-watch"],
"smartStep": true,
"console": "integratedTerminal",
"smartStep": true,
},
{
"name": "Vitest: Run Selected File",
"type": "node",
"request": "launch",
"autoAttachChildProcesses": true,
"skipFiles": ["<node_internals>/**", "**/node_modules/**"],
"skipFiles": ["<node_internals>/**", "**/node_modules/**", "!**/node_modules/vscode-*/**"],
"program": "${workspaceRoot}/node_modules/vitest/vitest.mjs",
"args": ["run", "${relativeFile}"],
"smartStep": true,
"console": "integratedTerminal",
"smartStep": true,
"sourceMaps": true,
"outFiles": [
// cs: surprisingly, it makes a significant difference whether the "outFiles" property is not mentioned at all or an empty array is given here;
// this setup seems to work best here, cross check with 'uri-utils.test.ts', for example
// my assumption is that vitest now relies on it's on the fly generated source maps plus those being available in the folder of the particular js modules, like in case of external libraries
]
}
]
}
5 changes: 4 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
"javascript",
"typescript"
],
"vitest.enable": true,
"vitest.configSearchPatternExclude": "{**/node_modules/**,**/dist/**,**/generated/**,**/templates/**,**/examples/hello*/**,**/.*/**,**/*.d.ts}",
"vitest.debugExclude": [
"<node_internals>/**", "**/node_modules/**", "!**/node_modules/vscode-uri/**"
],
"[json]": {
"editor.defaultFormatter": "vscode.json-language-features"
},
Expand Down
23 changes: 11 additions & 12 deletions packages/langium/src/generate/node-processor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,12 @@ class Context {
this.length -= this.lines[this.currentLineNumber].join('').length;
this.lines[this.currentLineNumber] = [];
this.pendingIndent = true;
this.recentNonImmediateIndents.length = 0;
}

addNewLine() {
this.pendingIndent = true;
this.lines.push([]);
this.pendingIndent = true;
this.recentNonImmediateIndents.length = 0;
}

Expand Down Expand Up @@ -228,20 +229,20 @@ function hasContent(node: GeneratorNode | string, ctx: Context): boolean {

function processStringNode(node: string, context: Context) {
if (node) {
if (context.pendingIndent) {
handlePendingIndent(context, false);
}
handlePendingIndent(context, false);
context.append(node);
}
}

function handlePendingIndent(ctx: Context, endOfLine: boolean) {
let indent = '';
for (const indentNode of ctx.relevantIndents.filter(e => e.indentEmptyLines || !endOfLine)) {
indent += indentNode.indentation ?? ctx.defaultIndentation;
if (ctx.pendingIndent) {
let indent = '';
for (const indentNode of ctx.relevantIndents.filter(e => e.indentEmptyLines || !endOfLine)) {
indent += indentNode.indentation ?? ctx.defaultIndentation;
}
ctx.append(indent, true);
ctx.pendingIndent = false;
}
ctx.append(indent, true);
ctx.pendingIndent = false;
}

function processCompositeNode(node: CompositeGeneratorNode, context: Context) {
Expand Down Expand Up @@ -288,9 +289,7 @@ function processNewLineNode(node: NewLineNode, context: Context) {
if (node.ifNotEmpty && !hasNonWhitespace(context.currentLineContent)) {
context.resetCurrentLine();
} else {
if (context.pendingIndent) {
handlePendingIndent(context, true);
}
handlePendingIndent(context, true);
context.append(node.lineDelimiter);
context.addNewLine();
}
Expand Down
16 changes: 16 additions & 0 deletions packages/langium/test/generate/node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,22 @@ describe('indentation', () => {
expect(process(comp, '\t')).toBe(`No indent${EOL}\tIndent {${EOL}\t}${EOL}`);
});

test('should indent nested template starting with a new line with \'ifNotEmpty\', with \'indentImmediately: false\'', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case targets the critical bug, thanks!

const comp = new CompositeGeneratorNode();
comp.append('No indent', NL);
comp.indent({
indentImmediately: false,
indentedChildren: [
'\t',
NLEmpty,
'Indented',
NL,
'Indented',
NL
]
});
expect(process(comp, '\t')).toBe(`No indent${EOL}\tIndented${EOL}\tIndented${EOL}`);
});
});

describe('composite', () => {
Expand Down
52 changes: 51 additions & 1 deletion packages/langium/test/generate/template-node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -832,9 +832,47 @@ describe('Multiple nested substitution templates', () => {
generated text!
`);
});

test('Nested substitution of with indented nested template starting with an _undefined_ line', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case reflects the critical case, nice!

expect(
toString(n`
begin:
${n`
${undefined}
${nestedNode}
`}
`)
).toBe(
s`
begin:
More
generated text!
`
);
});

test('Nested substitution of with indented nested template starting with an _empty string_ line', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case already worked before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. It's good to have it still, especially in contrast to the above one.

expect(
toString(n`
begin:
${n`
${''}
${nestedNode}
`}
`)
).toBe(
s`
begin:
${/* 's' automatically trims the empty lines completely, so insert */'<DUMMY>'}
More
generated text!
`.replace('<DUMMY>', '')
);
});

});

describe('Embedded forEach loops', () => {
describe('Joining lists', () => {
test('ForEach loop with empty iterable', () => {
const node = n`
Data:
Expand Down Expand Up @@ -1083,6 +1121,18 @@ describe('Embedded forEach loops', () => {
b
`);
});

test('Nested ForEach loop with empty iterable followed by an indented line', () => {
const node = n`
${joinToNode([], { appendNewLineIfNotEmpty: true})}
a
`;
const text = toString(node);
expect(text).toBe(s`
a
`);
});

});

describe('Appending templates to existing nodes', () => {
Expand Down