Skip to content

Commit 0e75182

Browse files
CopilotDaanV2
andauthored
fix: MoLang optimization quick-fix range resolution to prevent malformed rewrites (#497)
MoLang optimization diagnostics were generating correct replacement text (`math.pi` no longer `math.pi()`), but quick-fix application still used an incorrect edit range in some cases. This caused partial-token replacement and corrupted output such as `5*3(math.pi() * 15).pi`. - **Root cause** - Optimization code actions relied on `diag.range`, which could point to an inner/misaligned span instead of the full expression intended for rewrite. - **Diagnostic payload now carries source offsets** - `packages/bedrock-diagnoser/src/diagnostics/molang/optimizations/registry.ts` - Added `startOffset` and `endOffset` to optimization diagnostic `data` when a `replacement` is present. - Keeps existing replacement behavior while giving code actions an unambiguous source span. - **Code action now prefers offset-derived range** - `ide/base/server/src/lsp/code-action/minecraft/molang/optimization.ts` - Quick fix computes `editRange` from `startOffset/endOffset` when available; falls back to `diag.range` for compatibility. - Ensures rewrite applies to the full expression (`5*3*math.pi`) rather than a partial segment. - **Regression coverage** - Added server-side code-action regression test: - `ide/base/server/src/lsp/code-action/minecraft/molang/optimization.test.ts` - Verifies malformed `diag.range` does not affect replacement when offsets are present. - Updated optimization tests to allow expanded diagnostic data shape and assert offset fields for the `5*3*math.pi` regression path: - `packages/bedrock-diagnoser/test/lib/diagnostics/molang/optimizations.test.ts` ```ts const hasOffsetRange = typeof startOffset === 'number' && typeof endOffset === 'number' && endOffset >= startOffset; const editRange = hasOffsetRange ? { start: builder.context.document.positionAt(startOffset), end: builder.context.document.positionAt(endOffset), } : diag.range; ``` --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: DaanV2 <2393905+DaanV2@users.noreply.github.com>
1 parent d2dc29e commit 0e75182

4 files changed

Lines changed: 86 additions & 9 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { CodeActionParams, Diagnostic, DiagnosticSeverity, Range } from 'vscode-languageserver';
2+
import { CodeActionBuilder } from '../../builder';
3+
import { onCodeAction } from './optimization';
4+
5+
function positionAt(text: string, offset: number) {
6+
const lines = text.slice(0, offset).split('\n');
7+
return { line: lines.length - 1, character: lines[lines.length - 1].length };
8+
}
9+
10+
function rangeFromOffsets(text: string, startOffset: number, endOffset: number): Range {
11+
return {
12+
start: positionAt(text, startOffset),
13+
end: positionAt(text, endOffset),
14+
};
15+
}
16+
17+
describe('Molang optimization code action', () => {
18+
it('uses explicit offsets from diagnostic data for replacement range', () => {
19+
const text = 'v.test=5*3*math.pi;';
20+
const uri = 'file:///test.json';
21+
22+
const correctStart = text.indexOf('5*3*math.pi');
23+
const correctEnd = correctStart + '5*3*math.pi'.length;
24+
const badStart = text.indexOf('*math');
25+
const badEnd = badStart + '*math'.length;
26+
27+
const diag: Diagnostic = {
28+
range: rangeFromOffsets(text, badStart, badEnd),
29+
message: 'Can rewrite the operation to...',
30+
severity: DiagnosticSeverity.Information,
31+
code: 'molang.optimization.constant-folding',
32+
data: {
33+
replacement: '15*math.pi',
34+
startOffset: correctStart,
35+
endOffset: correctEnd,
36+
},
37+
source: 'mc',
38+
};
39+
40+
const params: CodeActionParams = {
41+
textDocument: { uri },
42+
range: diag.range,
43+
context: { diagnostics: [diag] },
44+
};
45+
46+
const context = {
47+
document: {
48+
uri,
49+
positionAt: (offset: number) => positionAt(text, offset),
50+
},
51+
} as any;
52+
const builder = new CodeActionBuilder(params, context);
53+
54+
onCodeAction(builder, diag);
55+
56+
expect(builder.out).toHaveLength(1);
57+
const action = builder.out[0] as any;
58+
expect(action.edit.changes[uri][0].range).toEqual(rangeFromOffsets(text, correctStart, correctEnd));
59+
expect(action.edit.changes[uri][0].newText).toBe('15*math.pi');
60+
});
61+
});

ide/base/server/src/lsp/code-action/minecraft/molang/optimization.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import { CodeActionBuilder } from '../../builder';
44
/** The shape of data attached to molang optimization diagnostics */
55
interface OptimizationData {
66
replacement: string;
7+
startOffset?: number;
8+
endOffset?: number;
79
}
810

911
function isOptimizationData(data: unknown): data is OptimizationData {
@@ -13,7 +15,14 @@ function isOptimizationData(data: unknown): data is OptimizationData {
1315
export function onCodeAction(builder: CodeActionBuilder, diag: Diagnostic): void {
1416
if (!isOptimizationData(diag.data)) return;
1517

16-
const replacement = diag.data.replacement;
18+
const { replacement, startOffset, endOffset } = diag.data;
19+
const hasOffsetRange = typeof startOffset === 'number' && typeof endOffset === 'number' && endOffset >= startOffset;
20+
const editRange = hasOffsetRange
21+
? {
22+
start: builder.context.document.positionAt(startOffset),
23+
end: builder.context.document.positionAt(endOffset),
24+
}
25+
: diag.range;
1726

1827
builder.push({
1928
title: `Rewrite to: ${replacement}`,
@@ -22,7 +31,7 @@ export function onCodeAction(builder: CodeActionBuilder, diag: Diagnostic): void
2231
isPreferred: true,
2332
edit: {
2433
changes: {
25-
[builder.context.document.uri]: [TextEdit.replace(diag.range, replacement)],
34+
[builder.context.document.uri]: [TextEdit.replace(editRange, replacement)],
2635
},
2736
},
2837
});

packages/bedrock-diagnoser/src/diagnostics/molang/optimizations/registry.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,10 @@ export class OptimizationRegistry {
7272
const nodeRange = OffsetWord.create('x'.repeat(length), startOffset);
7373

7474
for (const optimizedNode of result) {
75-
const data = optimizedNode.replacement !== undefined ? { replacement: optimizedNode.replacement } : undefined;
75+
const data =
76+
optimizedNode.replacement !== undefined
77+
? { replacement: optimizedNode.replacement, startOffset, endOffset }
78+
: undefined;
7679
diagnoser.add(
7780
nodeRange,
7881
optimizedNode.message,

packages/bedrock-diagnoser/test/lib/diagnostics/molang/optimizations.test.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ describe('Molang Optimization Diagnostics', () => {
284284

285285
const diag = diagnoser.items.find((d) => d.code === 'molang.optimization.self-cancellation');
286286
expect(diag).toBeDefined();
287-
expect(diag?.data).toEqual({ replacement: '0' });
287+
expect(diag?.data).toEqual(expect.objectContaining({ replacement: '0' }));
288288
});
289289
});
290290

@@ -320,7 +320,7 @@ describe('Molang Optimization Diagnostics', () => {
320320

321321
const diag = diagnoser.items.find((d) => d.code === 'molang.optimization.self-division');
322322
expect(diag).toBeDefined();
323-
expect(diag?.data).toEqual({ replacement: '1' });
323+
expect(diag?.data).toEqual(expect.objectContaining({ replacement: '1' }));
324324
});
325325
});
326326

@@ -462,7 +462,7 @@ describe('Molang Optimization Diagnostics', () => {
462462

463463
const diag = diagnoser.items.find((d) => d.code === 'molang.optimization.identity-operation');
464464
expect(diag).toBeDefined();
465-
expect(diag?.data).toEqual({ replacement: 'v.smooth_turn' });
465+
expect(diag?.data).toEqual(expect.objectContaining({ replacement: 'v.smooth_turn' }));
466466
});
467467

468468
it('should include replacement data for identity operation: addition with 0', () => {
@@ -471,7 +471,7 @@ describe('Molang Optimization Diagnostics', () => {
471471

472472
const diag = diagnoser.items.find((d) => d.code === 'molang.optimization.identity-operation');
473473
expect(diag).toBeDefined();
474-
expect(diag?.data).toEqual({ replacement: 'v.speed' });
474+
expect(diag?.data).toEqual(expect.objectContaining({ replacement: 'v.speed' }));
475475
});
476476

477477
it('should include replacement data for constant folding', () => {
@@ -490,7 +490,7 @@ describe('Molang Optimization Diagnostics', () => {
490490

491491
const diag = diagnoser.items.find((d) => d.code === 'molang.optimization.constant-result');
492492
expect(diag).toBeDefined();
493-
expect(diag?.data).toEqual({ replacement: '6' });
493+
expect(diag?.data).toEqual(expect.objectContaining({ replacement: '6' }));
494494
});
495495

496496
it('should not add parentheses to math constants like math.pi in replacement (regression)', () => {
@@ -499,10 +499,14 @@ describe('Molang Optimization Diagnostics', () => {
499499

500500
const diag = diagnoser.items.find((d) => d.code === 'molang.optimization.constant-folding');
501501
expect(diag).toBeDefined();
502-
const replacement = (diag?.data as { replacement: string })?.replacement;
502+
const data = diag?.data as { replacement: string; startOffset?: number; endOffset?: number };
503+
const replacement = data?.replacement;
503504
expect(replacement).toBeDefined();
504505
expect(replacement).toContain('math.pi');
505506
expect(replacement).not.toContain('math.pi()');
507+
expect(typeof data?.startOffset).toBe('number');
508+
expect(typeof data?.endOffset).toBe('number');
509+
expect((data.endOffset ?? 0) - (data.startOffset ?? 0)).toBe('5*3*math.pi'.length);
506510
});
507511
});
508512
});

0 commit comments

Comments
 (0)