Bugfix/atr 973 dev fix exception on broken solution#675
Open
SENya1990 wants to merge 4 commits into
Open
Conversation
…pping for scenarios when the solution is broken
… candidate based on args count
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Acuminator’s Roslyn symbol resolution and argument-to-parameter mapping against partially broken/non-compiling solutions, preventing an unhandled IndexOutOfRangeException caused by mapping a call site to an incorrect candidate symbol.
Changes:
- Replaced
GetSymbolOrFirstCandidatewithGetSymbolOrBestCandidate, adding a candidate-selection heuristic based on argument count. - Made
ArgumentsToParametersMapping.GetMappedParameter(...)resilient to mismatched method symbols by returningnullinstead of indexing out of bounds, and updated call sites accordingly. - Added an
ISymbol.Parameters()helper to support generic parameter-count inspection across symbol kinds.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Acuminator/Acuminator.Vsix/Coloriser/Roslyn/PXRoslynColorizerTagger.PXColorizerSyntaxWalker.cs | Switches to best-candidate symbol resolution for VSIX colorizer type lookup. |
| src/Acuminator/Acuminator.Utilities/Roslyn/Walkers/ParametersReassignedFinder.cs | Uses best-candidate symbol resolution when analyzing invocations in reassignment finder. |
| src/Acuminator/Acuminator.Utilities/Roslyn/Syntax/PXGraph/GraphSyntaxUtils.cs | Uses best-candidate symbol resolution for PXGraph instantiation detection. |
| src/Acuminator/Acuminator.Utilities/Roslyn/Syntax/MultipleParametersUsagesRewriter.cs | Uses best-candidate symbol resolution for parameter identifier rewrites. |
| src/Acuminator/Acuminator.Utilities/Roslyn/Semantic/SemanticModelUtils.cs | Introduces GetSymbolOrBestCandidate and arg-count heuristic candidate selection. |
| src/Acuminator/Acuminator.Utilities/Roslyn/Semantic/ISymbolGenericUtils.cs | Adds ISymbol.Parameters() helper for method/property parameter access. |
| src/Acuminator/Acuminator.Utilities/Roslyn/Semantic/ArgumentsToParametersMapping/ArgumentsToParametersMapping.cs | Makes mapped-parameter retrieval nullable to avoid out-of-bounds on wrong symbols. |
| src/Acuminator/Acuminator.Tests/Tests/Utilities/LocalFunction/LocalFunctionTests.cs | Updates test to use best-candidate symbol resolution. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/PXOverride/Helpers/BaseMethodReferenceInXmlDocComment.cs | Uses best-candidate symbol resolution for cref symbol lookup. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/PXGraphCreationForBqlQueries/Walker.cs | Uses best-candidate symbol resolution in graph-creation walker. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/PXGraphCreationForBqlQueries/PXGraphCreationForBqlQueriesAnalyzer.cs | Uses best-candidate symbol resolution when tracking graph argument symbols/usages. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/PXGraphCreateInstance/PXGraphCreateInstanceFix.cs | Uses best-candidate symbol resolution in code fix rewriter for graph creation. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/PXGraphCreateInstance/PXGraphCreateInstanceAnalyzer.Walker.cs | Uses best-candidate symbol resolution while analyzing object creation. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/NoPrimaryViewForPrimaryDac/NoPrimaryViewForPrimaryDacAnalyzer.cs | Uses best-candidate symbol resolution when resolving base class type nodes. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/LongOperationDelegateClosures/LongOperationDelegateTypeClassifier.cs | Uses best-candidate symbol resolution for long-operation delegate classification. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/LongOperationDelegateClosures/LongOperationDelegateClosuresAnalyzer.LongOperationsChecker.cs | Handles nullable mapped parameters to avoid crashes during closure analysis. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/Localization/LocalizationPXExceptionAndPXexceptionInfoAnalyzer.cs | Uses best-candidate symbol resolution and guards against null mapped parameters. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/IncorrectTaskUsageInAsyncCode/IncorrectTaskUsageInAsyncCodeAnalyzer.TaskUsageCheckingWalker.cs | Uses best-candidate symbol resolution for type/symbol lookups in async task checks. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/DatabaseQueries/DatabaseQueriesInRowSelectingAnalyzer.PXConnectionScopeVisitor.cs | Uses best-candidate symbol resolution when identifying PXConnectionScope usage. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/BannedApi/BannedApiAnalyzer.ApiNodesWalker.cs | Uses best-candidate symbol resolution for banned API symbol detection. |
| src/Acuminator/Acuminator.Analyzers/StaticAnalysis/AsyncVoidMethodsAndLambdas/AsyncVoidMethodsAndLambdasAnalyzer.cs | Uses best-candidate symbol resolution for async lambda symbol retrieval. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+99
to
+103
| if (argsCount > parametersCount) | ||
| continue; | ||
| else if (argsCount == parametersCount) | ||
| return candidate; // perfect match | ||
| else if (minSuitableParametersCount > parametersCount) |
| methodSymbol.ThrowOnNull(); | ||
| int parameterIndex = GetMappedParameterPosition(argIndex); | ||
|
|
||
| // In case of a broken solution we can get index out of bound because of a method node being mapped to a wrong symbol |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes Overview
During the investigation of ATR-973 it was discovered that Acuminator may throw unhandled IndexOutOfBounds exception on a broken solution. By broken solution I mean a solution that does not compile and is only partially correct.
In case of such solution, the IOOB exception can be thrown by the Argument to Parameter matching utility, This happens when semantic model's
GetSymbolInfodoesn't return a correctly resolvedSymbolInfo(solution is broken). So, there is no Symbol, just candidate symbols. In this case Acuminator just returns the first of the candidate symbols which is random and almost always incorrect.This leads to the argument to parameter matching try to match arguments to parameters of an incorrect method symbol. Sometimes, it is a symbol that has less parameters then there are arguments. This leads to an attempt to access the mapped parameters array out of its bounds.
The fix does two things:
The heuristic will pick the suitable candidate with the lowest possible number of parameters.
This is not always correct (for example, if there are overrides with parameters with different types, or optional parameters).
However, we don't want to re-implement complex C# overload analysis in Acuminator, this is only a heuristic that should not run in normal situations at all. It will work only on broken solutions.