Fix analyzer false-positive regressions - #148
Conversation
|
@coderabbitai autofix |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis pull request updates 39 analyzer and code-fix source files plus 18 test files. Changes include: shifting AL0138 midpoint rounding guidance from |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f54a11a183
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (IsSymbolEqualityComparerEquals(invocation, cache)) { | ||
| return true; | ||
| } | ||
| return IsSupportedComparisonInvocation(invocation, iteratorName, cache); |
There was a problem hiding this comment.
In ContainsSymbolEqualityComparison, the loop now returns on the first IInvocationOperation it encounters, even when that invocation is unrelated to symbol equality. This causes false negatives whenever the first invocation in the loop body/condition is something else (e.g., logging/helper call) and the actual SymbolEqualityComparer.Equals/IsEqualTo check appears later, so AL0030 can silently stop reporting valid AllInterfaces/BaseType patterns.
Useful? React with 👍 / 👎.
| case "CreateCommand": | ||
| if (receiver is not null | ||
| && TryGetAssignedSymbol(context.SemanticModel, invocation) is { } commandSymbol) { | ||
| commandConnections[commandSymbol] = receiver; | ||
| } |
There was a problem hiding this comment.
The new correlation logic only records command-to-connection relationships for CreateCommand assignments, and IsProtectedByTransaction relies exclusively on that map (or transactions invoked on the command symbol itself). This means a read/modify/write command created via constructor/factory patterns (or later Connection assignment) can still be transaction-protected but will be flagged as unprotected, introducing false positives in common ADO.NET usage outside the CreateCommand path.
Useful? React with 👍 / 👎.
| return false; | ||
| } | ||
|
|
||
| source = sourceSyntax.ToString(); |
There was a problem hiding this comment.
TryGetConstantValueSource keys HasValue/Value pairing by sourceSyntax.ToString(), so different symbols with identical text can be conflated (for example, shadowed operation names across nested scopes/lambdas inside the same binary expression). In those cases AL0031 can still suggest TryGetConstantValue even though the HasValue and Value accesses are on different operations, which reintroduces the mismatched-source false positive this change was meant to avoid.
Useful? React with 👍 / 👎.
| case var name when s_readMethods.Contains(name): | ||
| if (receiver is null) { | ||
| hasUnknownCorrelation = true; | ||
| } |
There was a problem hiding this comment.
The new hasUnknownCorrelation gate suppresses the entire diagnostic when a read/write method name is seen without a resolvable receiver symbol. Because GetInvokedMethodName matches by name only, an unrelated local/static call like ExecuteReader() now turns off AL0118 even when the method also contains a real untransactional DbCommand read/modify/write sequence, creating a broad false-negative path.
Useful? React with 👍 / 👎.
1 participant
Summary
GetAttributes().Any()without a predicate, AL0031 pairsConstantValue.HasValue/.Valueby source operation, and AL0040 no longer offers unsafe<object>fixes.Validation
dotnet build ANcpLua.Analyzers.slnx -c Release --no-restoredotnet test --project tests/ANcpLua.Analyzers.Tests/ANcpLua.Analyzers.Tests.csproj -c Release --no-buildgit diff --check HEAD~1..HEADSummary by CodeRabbit
Release Notes
Bug Fixes
Math.Roundfix to useMidpointRounding.ToEveninstead ofAwayFromZerofor more accurate default behavior.Documentation
ToEvenrounding behavior.