Fixing compilation after platform update. Some refactoring#612
Fixing compilation after platform update. Some refactoring#612VISTALL merged 1 commit intoconsulo:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates C# live template macros to compile against the updated platform APIs, mainly by modernizing formatting and adopting the newer LocalizeValue-based presentable names.
Changes:
- Migrate macro
getPresentableName()implementations fromStringtoLocalizeValue. - Refactor/modernize code style (lambdas, pattern matching
instanceof, generics diamond). - Add/adjust read/UI access annotations in several macros and helper utilities.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/VariableTypeMacroBase.java |
Refactor base macro implementation; adds pattern matching and annotates some methods. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/TypeMacro.java |
Updates getPresentableName() to return LocalizeValue. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/SuggestVariableNameMacro.java |
Switches to Application.get() and updates presentable name localization. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/SuggestIndexVariableNameMacro.java |
Updates getPresentableName() to LocalizeValue and reformats code. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/IListVariableMacro.java |
Adds read-action annotation and updates presentable name localization. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/ForeachVariableMacro.java |
Adds read-action annotation and updates presentable name localization. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/ForeachComponentTypeMacro.java |
Updates getPresentableName() to LocalizeValue and reformats logic. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/CSharpLiveTemplateMacroUtil.java |
Minor refactor/modernization of variable resolution helper. |
csharp-impl/src/main/java/consulo/csharp/impl/ide/liveTemplates/macro/ArrayVariableMacro.java |
Adds read-action annotation and updates presentable name localization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PsiFile file = PsiDocumentManager.getInstance(project).getPsiFile(context.getEditor().getDocument()); | ||
| assert file != null; | ||
| PsiElement element = file.findElementAt(offset); | ||
|
|
||
| assert element != null; | ||
| if(PsiUtilCore.getElementType(element) == CSharpTokens.IDENTIFIER) | ||
| { | ||
| DotNetVariable variable = CSharpLineMarkerUtil.getNameIdentifierAs(element, DotNetVariable.class); | ||
| if(variable != null) | ||
| { | ||
| return CSharpNameSuggesterUtil.getSuggestedVariableNames(variable); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| final PsiFile fileCopy = (PsiFile) file.copy(); | ||
| ApplicationManager.getApplication().runWriteAction(new Runnable() | ||
| { | ||
| @Override | ||
| public void run() | ||
| { | ||
| try | ||
| { | ||
| ReparseRangeUtil.reparseRange(fileCopy, offset, offset, "xxx"); | ||
| } | ||
| catch(IncorrectOperationException e) | ||
| { | ||
| LOGGER.error(e); | ||
| } | ||
| assert element != null; | ||
| if (PsiUtilCore.getElementType(element) == CSharpTokens.IDENTIFIER) { |
There was a problem hiding this comment.
Using assert for file / element nullability here is unsafe in production because assertions may be disabled, which would turn these into potential NPEs (file.findElementAt(offset) and subsequent PSI access). Prefer explicit null checks with an early return (e.g., Collections.emptyList()), so the macro fails gracefully when the editor/document isn't backed by a PSI file or no element exists at the offset.
| @Override | ||
| public Result calculateResult(Expression[] params, ExpressionContext context) { | ||
| final PsiElement[] vars = getVariables(params, context); | ||
| if (vars == null || vars.length == 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
calculateResult() calls getVariables() and constructs a PsiElementResult based on PSI, but the method itself is not annotated with @RequiredReadAction (only calculateLookupItems() is). Several implementations of getVariables() were updated to require a read action, so calculateResult() should also be marked @RequiredReadAction (or otherwise ensure it is always invoked under a read lock) to avoid read/write access violations at runtime.
| PsiFile file = PsiDocumentManager.getInstance(project).getPsiFile(context.getEditor().getDocument()); | ||
| PsiElement place = file.findElementAt(offset); |
There was a problem hiding this comment.
getPsiFile() and findElementAt() can return null. As written, file.findElementAt(offset) and then resolveAllVariables(place) will throw if either file or place is null. Please add null-handling (e.g., return null/default value when file or place is null) before dereferencing and before calling resolveAllVariables().
| PsiFile file = PsiDocumentManager.getInstance(project).getPsiFile(context.getEditor().getDocument()); | |
| PsiElement place = file.findElementAt(offset); | |
| PsiFile file = PsiDocumentManager.getInstance(project).getPsiFile(context.getEditor().getDocument()); | |
| if (file == null) { | |
| return new TextResult(getDefaultValue()); | |
| } | |
| PsiElement place = file.findElementAt(offset); | |
| if (place == null) { | |
| return new TextResult(getDefaultValue()); | |
| } |
No description provided.