Skip to content

Adding missing @Override annotations (part 5). Adding RRA/RWA/RUI annotations. Some localization and refactoring - #80

Merged
VISTALL merged 1 commit into
consulo:masterfrom
unv-unv:overrides-5
Jul 29, 2026
Merged

Adding missing @Override annotations (part 5). Adding RRA/RWA/RUI annotations. Some localization and refactoring#80
VISTALL merged 1 commit into
consulo:masterfrom
unv-unv:overrides-5

Conversation

@unv-unv

@unv-unv unv-unv commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@unv-unv

unv-unv commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — completed

  • Read the full diff (50 files, +914/−760)
  • Analyze changes for correctness / style issues
  • Post inline review comment with a one-click suggestion

Summary

This is a large, mostly-mechanical cleanup PR: adding @Override, @RequiredReadAction/@RequiredWriteAction/@RequiredUIAccess annotations, migrating *Bundle.message(...)*Localize calls, and modernizing syntax (pattern-matching instanceof, diamond operators, lambdas). The vast majority of it is correct and a nice readability improvement. 👍

I verified the non-trivial rewrites (the negated ContentIterator lambda in GroovycStubGenerator, the switch in GroovyElementPattern.methodCallParameter, the calculateAnchorToInsertBefore restructuring in GroovyExtractMethodHandler, and the many instanceof conversions) — those are all behavior-preserving.

Findings

🔴 1 bug — GroovyElseUnwrapper.findTopmostIfStatement (posted inline)

The pattern-matching rewrite changed the loop from climbing one ancestor level per iteration to two:

// before — parent moves up ONE level
while (parent.getParent() instanceof GrIfStatement) {
    parent = parent.getParent();
}
// after — ifStmt already *is* parent.getParent(), so this moves up TWO levels
while (parent.getParent() instanceof GrIfStatement ifStmt) {
    parent = ifStmt.getParent();
}

For nested else if chains this skips every other if and returns the wrong topmost element, breaking the "Unwrap else" action. The fix is parent = ifStmt; — see the inline suggestion on the file. (Contrast with GroovyElseUnwrapperBase.collectElementsToIgnore, where the same idiom is correct because the instanceof there matches parent itself.)

Minor nit (not blocking): In GroovyDocMethodHandler.shortenParameterReferences, the reworked if (... instanceof GrDocMethodReference methodRef) { line and its for body are indented inconsistently (6 spaces vs. the surrounding 8/10). Cosmetic only.

Nice work on this batch — once the GroovyElseUnwrapper one-liner is applied, this looks good to merge.
· branch overrides-5

parent = parent.getParent();
private static PsiElement findTopmostIfStatement(PsiElement parent) {
while (parent.getParent() instanceof GrIfStatement ifStmt) {
parent = ifStmt.getParent();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Behavioral regression from the pattern-matching rewrite.

The original loop climbed one level per iteration:

while (parent.getParent() instanceof GrIfStatement) {
    parent = parent.getParent();
}

Here parent = parent.getParent() is exactly the matched GrIfStatement. In the rewrite, the pattern variable ifStmt already is parent.getParent(), so parent = ifStmt.getParent() climbs two levels per iteration — skipping every other if and returning the wrong element for nested else if chains.

Suggested change
parent = ifStmt.getParent();
parent = ifStmt;

…otations. Some localization and refactoring.
@unv-unv

unv-unv commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — completed

  • Read the full diff (50 files, +916/−760)
  • Analyze changes for correctness / behavior-preservation
  • Post inline review comment with a one-click suggestion

Summary

This is a large, mostly-mechanical modernization pass: adding @Override, @RequiredReadAction/@RequiredWriteAction/@RequiredUIAccess, migrating *Bundle.message(...)*Localize calls, pattern-matching instanceof, diamond operators, lambdas, and GROOVY_FILE_TYPEINSTANCE. I reviewed every file and the vast majority is correct and a genuine readability win. 👍

I paid particular attention to the non-trivial rewrites and confirmed they are behavior-preserving:

  • GroovycStubGenerator — the negated ContentIterator → lambda (fileOrDir.isDirectory() || JavaFileType.INSTANCE != fileOrDir.getFileType()) is a correct De Morgan transform of the original !isDirectory() && == JavaFileType → return false. ✅
  • GroovyElementPattern.methodCallParameter — the switch (call) over GrMethodCall/GrNewExpression/default → null matches the original ternary chain, and the collapsed && guard preserves the original ordering/short-circuit. ✅
  • GroovyExtractMethodHandler.calculateAnchorToInsertBefore — restructured getParent() check is equivalent. ✅
  • GroovyConstantExpressionEvaluator / GroovyExpressionPattern / GroovyFindUsagesProvider — the many instanceof binding-pattern conversions are all faithful. ✅

Re: the previously-flagged GroovyElseUnwrapper bug

A prior automated review flagged findTopmostIfStatement as a two-levels-per-iteration regression. That does not apply to the current code — the committed version is already:

while (parent.getParent() instanceof GrIfStatement ifStmt) {
    parent = ifStmt;   // ifStmt == parent.getParent(), so this climbs exactly ONE level
}

Since ifStmt is bound to parent.getParent(), parent = ifStmt is identical to the original parent = parent.getParent(). No behavioral change — this one is correct. ✅ (GroovyElseUnwrapperBase.collectElementsToIgnore uses the same idiom correctly as well.)

Findings

No correctness issues. One cosmetic nit, posted inline as a one-click suggestion:

  • GroovyElementType.java:48public String toString() switched to 4-space indentation while the @Override directly above it (and the rest of the file) uses tabs. Inline suggestion realigns it.

One thing worth a sanity check (not blocking): in GroovyConfigUtils, ModuleUtilCore.findModuleForPsiElement(psiElement) was replaced with psiElement.getModule() in isVersionAtLeast / getSDKVersion(PsiElement). That's fine as long as getModule() resolves the module the same way for these PSI elements — assuming it's the intended platform-API swap, no action needed.

Nice, clean batch — good to merge once the one-line indentation nit is applied.
· branch overrides-5


public String toString()
@Override
public String toString()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit (cosmetic): this line switched to 4-space indentation, but the rest of the file — including the @Override right above it — is tab-indented. Realign to a tab for consistency.

Suggested change
public String toString()
public String toString()

@VISTALL
VISTALL merged commit ad9a3f1 into consulo:master Jul 29, 2026
1 check passed
@unv-unv
unv-unv deleted the overrides-5 branch July 29, 2026 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants