Fix missing unnecessary_non_null_assertion (#1270) - #1348
Conversation
📝 WalkthroughWalkthroughThe pull request expands the default list of suppressed Dart analyzer lints in generated copyWith templates from ChangesLint Suppression Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/freezed/lib/src/templates/copy_with.dart (1)
257-266:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
unnecessary_non_null_assertionsuppress comment is placed on the wrong line — the lint will still fire.Dart's
// ignore:comment "suppress[es] a specific non-error diagnostic on a specific line" and must be placed above (i.e., immediately before) the offending line.The
_ignoreLintshelper wraps theas ${p.typeDisplayString}expression.parameterToValuetherefore emits:Line N: null == value ? _self.value! : value Line N+1: // ignore: cast_nullable_to_non_nullable, unnecessary_non_null_assertion Line N+2: as String,The comment on Line N+1 suppresses lints on Line N+2 (
as String,). Theunnecessary_non_null_assertiondiagnostic fires on_self.value!— which is on Line N, one line before the ignore comment. Adding the lint name to this comment has no effect on the actual offending token.The root cause is in
thisPropertyFor(line 296):cast = '!'is unconditionally set whenever the property type differs from the parameter type, even when the property is already non-nullable (String). The!is only semantically needed when the property itself is nullable. The correct fix is:🐛 Proposed root-cause fix in `thisPropertyFor`
var cast = ''; - if (propertyGetterForCopyWithParameter.type != to.type) cast = '!'; + if (propertyGetterForCopyWithParameter.type != to.type && + propertyGetterForCopyWithParameter.type.isNullable) cast = '!'; return '$accessor.$propertyName$cast';Alternatively, if a suppress-only approach is preferred, the ignore comment must be placed on the line that contains the ternary (i.e., wrapping the result of
parameterToValue, not just theascast insideparameterAssignmentFor).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/freezed/lib/src/templates/copy_with.dart` around lines 257 - 266, The suppress comment is placed after the cast so it doesn't prevent the unnecessary_non_null_assertion on the preceding ternary; fix by adjusting thisPropertyFor to only append the non-null assertion ('!') when the property type is nullable (i.e., detect property nullability and set cast = '!' only if propertyTypeIsNullable), so parameterToValue no longer emits an unnecessary '!' for already non-nullable properties; alternatively (if you prefer a suppress-only change) move the ignore comment generation in _ignoreLints/parameterAssignmentFor to be emitted immediately above the entire ternary expression produced by parameterToValue instead of above just the `as` cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/freezed/lib/src/templates/copy_with.dart`:
- Around line 257-266: The suppress comment is placed after the cast so it
doesn't prevent the unnecessary_non_null_assertion on the preceding ternary; fix
by adjusting thisPropertyFor to only append the non-null assertion ('!') when
the property type is nullable (i.e., detect property nullability and set cast =
'!' only if propertyTypeIsNullable), so parameterToValue no longer emits an
unnecessary '!' for already non-nullable properties; alternatively (if you
prefer a suppress-only change) move the ignore comment generation in
_ignoreLints/parameterAssignmentFor to be emitted immediately above the entire
ternary expression produced by parameterToValue instead of above just the `as`
cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a0b74a3-100e-4813-8d90-0f1bcaf4f2cf
📒 Files selected for processing (1)
packages/freezed/lib/src/templates/copy_with.dart
1 participant
Fixes #1270
Summary by CodeRabbit