Adding missing @Override annotations (part 2). Adding RRA/RWA/RUI annotations. Some refactoring

[unv-unv]

No description provided.

[unv-unv]

@claude review pr with inline comments

[github-actions]

Claude finished @unv-unv's task in 3m 30s —— View job

---

PR Review — @OverRide / RRA / RWA / RUI annotations + refactoring

I reviewed all 50 changed files. This is a large, mostly-mechanical cleanup (adding @Override, @RequiredReadAction/@RequiredWriteAction, pattern-matching instanceof, diamond operators, lambdas, and AopBundle → AopLocalize). It reads well and the vast majority is a safe, welcome improvement. I left 5 inline comments on the specific spots worth a look:

• Read the full diff
• Analyze changes for correctness & style
• Post inline review comments with suggestions
• Summarize findings

Findings (inline)

1. AopPointcutExpressionFileType#getId() — the @Override got wedged into the modifiers (public @Override String getId()). Compiles, but it's a formatting slip. One-click fix suggested. (discussion)
2. AopParameterList — PairFunction import is now unused after the BiFunction migration. (discussion)
3. AopPsiTypePatternsUtil — same unused PairFunction import. (discussion)
4. AopPointcutExpressionFile#toString() — marking toString() as @RequiredReadAction is risky, since toString() is often called outside a read action (logging/debugger). Consider dropping it. (discussion)
5. AopPointcutConverter#getVariants() — ModuleUtilCore.findModuleForPsiElement(element) → element.getModule() is null-safe here, but the two don't always resolve the module identically for library/injected contexts. Worth a sanity check. (discussion)

Minor (non-blocking, not left inline)

• AopReferenceExpression — the return inside the new else if (resolve() instanceof PsiClass psiClass) block is over-indented (10 spaces); harmless.
• Several @NonNls annotations were dropped alongside the refactors. No behavior impact, but if the intent was to keep marking non-localizable strings, some of those could stay.

Nothing here is a correctness blocker — #1 is the only clear "should fix", and it's cosmetic. Nice work on the read/write-action annotations; they add real value for threading correctness.

Note: I can't submit a formal GitHub review or approve — the notes above are posted as regular inline review comments.
· branch overrides-2

[github-actions]

The @Override annotation ended up wedged into the middle of the modifiers here, producing public @Override String getId(). It compiles, but it's clearly a formatting slip — the annotation should sit above the public modifier like every other method in this class.

Suggested change

	@Nonnull
	@NonNls
	public String getId() {
	public @Override
	String getId() {
	@Nonnull
	@Override
	public String getId() {

[github-actions]

With matches(...) now taking a java.util.function.BiFunction (and AopAbstractList.accepts migrated too), the import consulo.util.lang.function.PairFunction; at the top of this file is no longer used and can be removed.

[github-actions]

All PairFunction usages in this file were replaced with BiFunction, so the import consulo.util.lang.function.PairFunction; near the top is now unused and can be removed.

[github-actions]

Marking toString() as @RequiredReadAction is a bit risky: toString() is frequently invoked outside of a read action (logging, the debugger's variable view, assertion messages), and the contract here would now assert a read lock is held on every such call. Since the only thing needing the read action is getName(), consider dropping @RequiredReadAction from toString() to avoid spurious threading assertions.

[github-actions]

This swaps ModuleUtilCore.findModuleForPsiElement(element) for element.getModule(). getAopModel(null) is handled safely (returns the empty model), so there's no NPE risk, but findModuleForPsiElement and PsiElement.getModule() don't always resolve the module the same way for elements sitting in libraries / injected contexts. Worth a quick check that pointcut resolution in <aop:*> tags still behaves the same after this change.

[unv-unv]

@claude review pr with inline comments

[github-actions]

Claude finished @unv-unv's task in 3m 9s —— View job

---

PR Review — @OverRide / RRA / RWA / RUI annotations + refactoring

• Read the full diff (all 50 files)
• Analyze changes for correctness & style
• Post inline review comments with suggestions
• Summarize findings

Re-reviewed the current head (a15a941). This is a large, well-executed, mostly-mechanical cleanup — @Override / @RequiredReadAction / @RequiredWriteAction annotations, pattern-matching instanceof, diamond operators, lambdas, and AopBundle → AopLocalize. I read every change and it's a genuine, safe improvement.

✅ Already fixed since the last review

The Fixes. commit addressed three of the five previously-flagged items:

1. AopPointcutExpressionFileType#getId() — the wedged public @Override formatting is now correct.
2. AopParameterList — unused PairFunction import removed.
3. AopPsiTypePatternsUtil — unused PairFunction import removed.

🔎 Still open — 2 fresh inline comments

4. AopPointcutExpressionFile#toString() — @RequiredReadAction on toString() is risky (it's called from logging / debugger / assertions, outside any read action). One-click suggestion to drop it. (comment)
5. AopPointcutConverter#getVariants() — ModuleUtilCore.findModuleForPsiElement(element) → element.getModule(); null-safe, but the two don't always resolve identically for library/injected contexts. Worth a sanity check. (comment)

Verified equivalences (no action needed)

• ArgNamesWarningsInspection#findParametersUsedInPointcuts — the nested Processor/Condition → lambda rewrite (!(x instanceof … ) || !designatorClass.isInstance(…)) is logically identical to the original if (A) { if (B) return false; } return true;.
• ArgNamesErrorsInspection, AopHighlightErrorFilter#value, AopMemberReferenceExpression#getReferenceExpression, and the AopReferenceExpression#getPatterns / getTypePattern else if (resolve() instanceof PsiClass …) collapses all preserve behavior.
• AopPsiTypePatternsUtil — Pair<Pair<Class,Class>,PairFunction> → Pair<Couple<Class>,BiFunction> with .apply(...) is a clean, correct migration.

Minor (non-blocking)

• AopPointcutImpl#isValid() gained @RequiredReadAction but no @Override (it's isValid() — likely overrides a supertype); harmless inconsistency with the sibling AopIntroductionImpl#isValid(), which got both.
• Several @NonNls annotations were dropped alongside refactors. No behavior impact, but if the intent was to keep marking non-localizable strings, some could stay.

Nothing here is a correctness blocker. Nice work — the read/write-action annotations add real threading-correctness value.

Note: I can't submit a formal GitHub review or approve — the notes above are posted as regular inline review comments.
· branch overrides-2

[github-actions]

(still open from previous review) Marking toString() as @RequiredReadAction is risky. toString() is frequently invoked outside a read action — logging, the debugger's variable view, assertion messages — and this contract would now assert a read lock is held on every such call. The only member needing the read action is getName(). Consider dropping @RequiredReadAction here.

Suggested change

	@Override
	@RequiredReadAction
	public String toString() {
	@Override
	public String toString() {

[github-actions]

(still open from previous review) This swaps ModuleUtilCore.findModuleForPsiElement(element) for element.getModule(). getAopModel(null) returns the empty model, so there is no NPE risk, but findModuleForPsiElement and PsiElement.getModule() don't always resolve to the same module for elements sitting in libraries / injected contexts. Worth a quick check that pointcut resolution in <aop:*> tags still behaves the same after this change.