Fix Feature Filter#64
Conversation
📝 WalkthroughWalkthroughThe PR adds deterministic tie-breaking to feature locale resolution when multiple entries share the same highest version, selecting the lexicographically greater feature by ChangesFeature Locale Deterministic Resolution & Package Release
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Maple2.File.Generator/Resource/FeatureLocaleFilter.cs`:
- Around line 68-70: The tie-breaker condition in FeatureLocaleFilter that reads
(version == maxVersion && string.Compare(entry.Feature, result.Feature,
StringComparison.Ordinal) > 0) can dereference result (default(T) / null) when
the first seen entry has version == maxVersion (e.g. Features[entry.Feature] ==
-1); modify the condition to first check result == null before accessing
result.Feature so that any entry at the same version always replaces a null
result. In practice update the if in the method using variables version,
maxVersion, entry and result to: if (version > maxVersion || (version ==
maxVersion && (result == null || string.Compare(entry.Feature, result.Feature,
StringComparison.Ordinal) > 0))) so result is not dereferenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b3b8d8e-3443-4655-9ccb-956bfccb1f8f
📒 Files selected for processing (2)
Maple2.File.Generator/Resource/FeatureLocaleFilter.csMaple2.File.Parser/Maple2.File.Parser.csproj
| if (version > maxVersion || | ||
| // This is how the client resolves same version matches. Dumb as bricks. | ||
| (version == maxVersion && string.Compare(entry.Feature, result.Feature, StringComparison.Ordinal) > 0)) { |
There was a problem hiding this comment.
Latent NullReferenceException when the first feature entry has version −1
result starts as default(T) — null for any reference-type T. The tie-breaker arm version == maxVersion fires the moment a feature entry's version matches the initial sentinel value of −1 (i.e. Features[entry.Feature] == -1). If that happens before any prior entry has set result, the expression result.Feature dereferences a null object and throws.
Note that string.Compare itself is null-safe for its string arguments (one or both comparands can be null — by definition, any string compares greater than a null reference), so the problem is specifically the member access on result, not the comparison call.
The correct guard also covers the semantics correctly: a null result should always be replaced by any feature entry at the same version (just as any feature beats no-feature).
🛡️ Proposed fix
- if (version > maxVersion ||
- // This is how the client resolves same version matches. Dumb as bricks.
- (version == maxVersion && string.Compare(entry.Feature, result.Feature, StringComparison.Ordinal) > 0)) {
+ if (version > maxVersion ||
+ // This is how the client resolves same version matches. Dumb as bricks.
+ (version == maxVersion && (result == null || string.Compare(entry.Feature, result.Feature, StringComparison.Ordinal) > 0))) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (version > maxVersion || | |
| // This is how the client resolves same version matches. Dumb as bricks. | |
| (version == maxVersion && string.Compare(entry.Feature, result.Feature, StringComparison.Ordinal) > 0)) { | |
| if (version > maxVersion || | |
| // This is how the client resolves same version matches. Dumb as bricks. | |
| (version == maxVersion && (result == null || string.Compare(entry.Feature, result.Feature, StringComparison.Ordinal) > 0))) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Maple2.File.Generator/Resource/FeatureLocaleFilter.cs` around lines 68 - 70,
The tie-breaker condition in FeatureLocaleFilter that reads (version ==
maxVersion && string.Compare(entry.Feature, result.Feature,
StringComparison.Ordinal) > 0) can dereference result (default(T) / null) when
the first seen entry has version == maxVersion (e.g. Features[entry.Feature] ==
-1); modify the condition to first check result == null before accessing
result.Feature so that any entry at the same version always replaces a null
result. In practice update the if in the method using variables version,
maxVersion, entry and result to: if (version > maxVersion || (version ==
maxVersion && (result == null || string.Compare(entry.Feature, result.Feature,
StringComparison.Ordinal) > 0))) so result is not dereferenced.
2 participants
Summary by CodeRabbit