Fixing compilation after changes in platform. Localizing and refactoring

[unv-unv]

No description provided.

[unv-unv]

@claude review pr with inline comments

[unv-unv]

@claude review pr with inline comments

[github-actions]

Claude finished @unv-unv's task in 6m 42s —— View job

---

PR Review — Localization & API migration

• Gather context (read changed files & diff)
• Review ExecutionHandler.java
• Review OutputParser.java
• Review RunCanceledException.java
• Review OutputBuilder.java
• Post inline review comments

I left 5 inline comments on the specific lines (with one-click suggestion blocks where applicable). Summary below.

Overview

This is overwhelmingly a mechanical pass: reindentation (2→4 spaces, IJ→IJ4 code style), localization (AntBundle.message(...) → ApacheAntImplLocalize / LocalizeValue), and platform-API migration (ApplicationManager → Application, the old OutputParser.Callback.message(...) → the new newMessage(...) builder). The structure and control flow are preserved well, and the localization migration looks consistent. Nice cleanup overall. 👍

Findings (see inline comments)

🔶 Lost null guard on the URL — possible NPE (both parsers)
In both OutputParser and OutputBuilder, the new url(String url) builder method calls VirtualFileManager.getInstance().findFileByUrl(url) unconditionally. The old message(...) implementations guarded it (url == null ? null : ...). If the platform ever calls .url(null), this NPEs. Also, the VFS lookup previously ran inside runReadAction; it now runs in url(), outside any read action — findFileByUrl is generally expected to run under a read lock. Suggestions provided inline.

🔶 switch over CompilerMessageCategory has no default (OutputBuilder)
OutputBuilder.newMessage uses an exhaustive-looking switch with no default, whereas OutputParser keeps a default. A null category will NPE, and a future enum constant would cause a MatchException. The old convertCategory fell back to INFO. Defensive default suggested inline.

🔹 FilePosition row depends on position() being called (OutputBuilder)
myRow defaults to 0, so new FilePosition(file, myRow - 1, myColumn) yields row -1 if position(...) is never invoked. The old message(...) always received lineNum directly. Worth confirming the platform always calls position(...) before add().

🔹 Nit — LocalizeValue.ofNullable(buildFile.getName()) (ExecutionHandler)
getName() is effectively non-null; LocalizeValue.of(...) matches the actual contract better.

Minor observations (not blocking, no inline comment)

• OutputParser message.get().split("\n") replaces the old StringTokenizer (which skipped all empty tokens). String.split keeps interior empty lines and drops only trailing ones — a tiny behavioral nuance, almost certainly fine.
• Pre-existing (not introduced here): in ExecutionHandler.processTerminated, the parser.getErrorsCount() > 0 branch reports AntBuildListener.FINISHED_SUCCESSFULLY rather than a failure status. Carried over unchanged, just flagging.

None of these are blockers — the two 🔶 items are worth a look before merge since they're the only places behavior could actually differ from the pre-migration code.
· branch compilation-fix

[github-actions]

Default position values depend on position() being called. myRow defaults to 0, so if the parser never calls position(...) this builds a FilePosition with myRow - 1 == -1. The old message(...) always received lineNum/columnNum directly, so it never produced a negative row. Worth confirming the platform always calls position(...) before add() for javac messages — otherwise guard against the negative row here.

[github-actions]

Same null-guard regression as in OutputParser. The old message() did url == null ? null : findFileByUrl(url). Here url() calls it unconditionally, risking an NPE if .url(null) is ever called, and the lookup now runs outside the runReadAction that wraps add():

Suggested change

	public CompileContext.MessageBuilder url(String url) {
	myFile = VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}
	public CompileContext.MessageBuilder url(String url) {
	myFile = url == null ? null : VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}

[github-actions]

Possible NPE / lost null-guard. The previous message() guarded the URL with url == null ? null : findFileByUrl(url). The new builder calls findFileByUrl(url) unconditionally, so if the platform ever invokes .url(null) this will NPE. Restoring the guard keeps the old contract:

Suggested change

	public CompileContext.MessageBuilder url(String url) {
	myUrl = url;
	myFile = VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}
	public CompileContext.MessageBuilder url(String url) {
	myUrl = url;
	myFile = url == null ? null : VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}

Separately: this VFS lookup used to run inside runReadAction (now moved into add()). findFileByUrl is generally expected to run under a read lock — consider deferring the lookup into the add() read action instead of doing it here.

[github-actions]

This switch has no default, unlike the one in OutputParser. The old convertCategory fell back to INFO for any unrecognized category, and switch on a null category throws NPE. A defensive default restores the old behavior and avoids a MatchException if a new CompilerMessageCategory constant is ever added:

Suggested change

	MessageEvent.Kind kind = switch (category) {
	case CompilerMessageCategory.ERROR -> MessageEvent.Kind.ERROR;
	case CompilerMessageCategory.WARNING -> MessageEvent.Kind.WARNING;
	case CompilerMessageCategory.INFORMATION -> MessageEvent.Kind.INFO;
	case CompilerMessageCategory.STATISTICS -> MessageEvent.Kind.STATISTICS;
	};
	MessageEvent.Kind kind = switch (category) {
	case CompilerMessageCategory.ERROR -> MessageEvent.Kind.ERROR;
	case CompilerMessageCategory.WARNING -> MessageEvent.Kind.WARNING;
	case CompilerMessageCategory.INFORMATION -> MessageEvent.Kind.INFO;
	case CompilerMessageCategory.STATISTICS -> MessageEvent.Kind.STATISTICS;
	default -> MessageEvent.Kind.INFO;
	};

[github-actions]

Nit: buildFile.getName() is effectively non-null, so LocalizeValue.of(...) reads more accurately than ofNullable(...) here. Not a correctness issue — ofNullable is just a slightly weaker contract than what the value actually guarantees.

[unv-unv]

@claude review pr with inline comments

[github-actions]

Claude finished @unv-unv's task in 5m 47s —— View job

---

PR Review — Localization & API migration (round 2)

• Gather context (read changed files & latest diff)
• Review OutputParser.java
• Review OutputBuilder.java
• Review ExecutionHandler.java
• Review RunCanceledException.java
• Post inline review comments

I re-reviewed against the current head (fa4920f) and left 5 inline comments, each with a one-click suggestion block.

What changed since round 1

Commit fa4920f ("Fixing initial values of row/column") switched the myRow/myColumn defaults from 0 to -1. That's the right sentinel for OutputParser (which passes myRow through directly), but it interacts with OutputBuilder's myRow - 1 conversion — see below. The four other round-1 findings are still open.

Findings (all have inline suggestions)

🔶 OutputBuilder switch over CompilerMessageCategory has no default — OutputBuilder.java:286. A null category NPEs and a future enum constant throws MatchException. OutputParser keeps a default -> INFO; mirror it.

🔶 Lost null-guard on the URL (both parsers) — OutputBuilder.java:299 and OutputParser.java:297. The old message() did url == null ? null : findFileByUrl(url); the new url() calls it unconditionally → NPE on .url(null). Side note: the VFS lookup used to run inside the runReadAction that now only wraps add(), so findFileByUrl is no longer under a read lock.

🔹 OutputBuilder FilePosition row can be -2 — OutputBuilder.java:324. With the new -1 default and the myRow - 1 conversion, a file-bearing message that never gets position() produces row -2 (the old 0 default produced -1, the usual "unknown line" value). Worth confirming javac messages always call position() before add(); otherwise guard with myRow < 0 ? -1 : myRow - 1.

🔹 Nit — LocalizeValue.ofNullable(buildFile.getName()) — ExecutionHandler.java:148. getName() is effectively non-null, so LocalizeValue.of(...) reads more accurately.

Looks good

• RunCanceledException.java — clean LocalizeValue migration, title.get() at the Messages boundary is correct. No issues.
• The localization (AntBundle → ApacheAntImplLocalize) and ApplicationManager → Application.get() migrations are consistent throughout.
• Reindentation / code-style pass preserves control flow well.

Minor (non-blocking, no inline comment)

• OutputParser's message.get().split("\n") replaces the old StringTokenizer — String.split keeps interior empty lines (drops only trailing), a tiny behavioral nuance, almost certainly fine.
• Pre-existing (carried over unchanged): in ExecutionHandler.processTerminated, the parser.getErrorsCount() > 0 branch still reports FINISHED_SUCCESSFULLY. Just flagging — not introduced here.

The two 🔶 items are the only spots where behavior can actually differ from the pre-migration code, so they're worth addressing before merge.
· branch compilation-fix

[github-actions]

switch over CompilerMessageCategory still has no default. (Round 1 finding, not yet addressed.) A null category throws an NPE, and adding a new enum constant later would throw MatchException. The old convertCategory fell back to INFO. OutputParser.newMessage already keeps a default, so mirror it here:

Suggested change

	MessageEvent.Kind kind = switch (category) {
	case CompilerMessageCategory.ERROR -> MessageEvent.Kind.ERROR;
	case CompilerMessageCategory.WARNING -> MessageEvent.Kind.WARNING;
	case CompilerMessageCategory.INFORMATION -> MessageEvent.Kind.INFO;
	case CompilerMessageCategory.STATISTICS -> MessageEvent.Kind.STATISTICS;
	};
	MessageEvent.Kind kind = switch (category) {
	case CompilerMessageCategory.ERROR -> MessageEvent.Kind.ERROR;
	case CompilerMessageCategory.WARNING -> MessageEvent.Kind.WARNING;
	case CompilerMessageCategory.INFORMATION -> MessageEvent.Kind.INFO;
	case CompilerMessageCategory.STATISTICS -> MessageEvent.Kind.STATISTICS;
	default -> MessageEvent.Kind.INFO;
	};

[github-actions]

Lost null-guard on the URL (still open). The old message() did url == null ? null : findFileByUrl(url); the new url() calls it unconditionally, so .url(null) would NPE. Note too that the VFS lookup used to run inside the runReadAction that now wraps add(), and findFileByUrl is expected under a read lock. Restoring the guard at minimum:

Suggested change

	public CompileContext.MessageBuilder url(String url) {
	myFile = VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}
	public CompileContext.MessageBuilder url(String url) {
	myFile = url == null ? null : VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}

[github-actions]

myRow - 1 now yields -2 when position() isn't called. Commit fa4920f changed the default from 0 to -1. That is correct for OutputParser, which passes myRow straight through (-1 = unknown line). But here FilePosition receives myRow - 1, so the un-positioned case went from -1 (the conventional unknown-line sentinel under the old 0 default) to -2. If a file-bearing message can ever reach add() without position(), guard the conversion:

Suggested change

	new FilePosition(new File(Objects.requireNonNull(myFile.getCanonicalPath())), myRow - 1, myColumn);
	new FilePosition(new File(Objects.requireNonNull(myFile.getCanonicalPath())), myRow < 0 ? -1 : myRow - 1, myColumn);

[github-actions]

Lost null-guard on the URL (still open). Same as the previous round: the old message() guarded with url == null ? null : findFileByUrl(url). The new url() calls findFileByUrl(url) unconditionally, so .url(null) NPEs. Also, this VFS lookup previously ran inside runReadAction (now only add() is wrapped), and findFileByUrl generally wants a read lock. Restore the guard:

Suggested change

	public CompileContext.MessageBuilder url(String url) {
	myUrl = url;
	myFile = VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}
	public CompileContext.MessageBuilder url(String url) {
	myUrl = url;
	myFile = url == null ? null : VirtualFileManager.getInstance().findFileByUrl(url);
	return this;
	}

[github-actions]

Nit (still open): buildFile.getName() is effectively non-null, so LocalizeValue.of(...) matches the actual contract more accurately than ofNullable(...).

Suggested change

	LocalizeValue buildFileName = LocalizeValue.ofNullable(buildFile.getName());
	LocalizeValue buildFileName = LocalizeValue.of(buildFile.getName());