-
-
Notifications
You must be signed in to change notification settings - Fork 957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(riverpod_lint): correctly detects nested ref invocations #3805
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/riverpod_analyzer_utils/lib/src/riverpod_ast/resolve_riverpod.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/riverpod_annotation/CHANGELOG.md (1)
3-3
: Consider expanding the changelog description for better clarity.While the current description captures the basic fix, it would be more helpful to developers if it included:
- The specific problem that was fixed (e.g., what issues occurred with nested ref invocations)
- The impact on developers (e.g., how this improves error detection)
- Any potential breaking changes or migration notes if applicable
Example of a more detailed entry:
- RefInvocations as a parameter of another RefInvocation are now correctly detected (thanks to @josh-burton) + Fixed analyzer to correctly detect nested RefInvocations when used as parameters (e.g., ref.watch(provider(ref.watch(...)))). This improves the accuracy of the analyzer's error detection for complex provider compositions. (thanks to @josh-burton)packages/riverpod_lint/CHANGELOG.md (1)
1-3
: Consider enhancing the changelog description.While the entry is well-structured and follows the changelog format, consider making it more specific about the nested ref invocations detection:
- provider_dependencies now detects a dependency when it is a parameter of another dependency (thanks to @josh-burton) + provider_dependencies now correctly detects nested ref invocations where a dependency is used as a parameter of another dependency (thanks to @josh-burton)packages/riverpod_analyzer_utils_tests/test/ref_invocation_test.dart (1)
414-414
: Consider a more descriptive test name.The current test name "Decodes ref.read family usages" could be more specific about testing nested ref invocations, which is the main focus of this PR.
- testSource('Decodes ref.read family usages', + testSource('Decodes nested ref.read invocations with family providers',packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.g.dart (1)
176-182
: Deprecation notice forDep3Ref
mixinThe
Dep3Ref
mixin is marked as deprecated and will be removed in version 3.0. Ensure that any dependent code is updated to useRef
instead to prepare for future updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/riverpod_analyzer_utils/lib/src/riverpod_ast/resolve_riverpod.dart (1 hunks)
- packages/riverpod_analyzer_utils_tests/test/ref_invocation_test.dart (1 hunks)
- packages/riverpod_annotation/CHANGELOG.md (1 hunks)
- packages/riverpod_lint/CHANGELOG.md (1 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.dart (2 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.diff (9 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.g.dart (2 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies_test.dart (1 hunks)
🧰 Additional context used
🔇 Additional comments (18)
packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies_test.dart (2)
17-17
: LGTM! Error count update aligns with enhanced RefInvocation detection.The increment in expected error count from 10 to 11 is consistent with the PR's objective to improve nested RefInvocation detection.
17-17
: Verify the new error case is documented in golden files.Please ensure the additional error case is properly documented in the golden diff file at 'test/lints/provider_dependencies/provider_dependencies.diff'.
✅ Verification successful
Let me try to locate the golden files using a different approach.
Let me check the actual test file and its golden file to verify the error cases.
The golden files are properly documented and up-to-date
The test file
provider_dependencies_test.dart
expects 11 errors, and the golden diff fileprovider_dependencies.diff
correctly documents all 11 cases with their corresponding fixes:
- Adding dependencies for plain annotation
- Adding dependencies for custom annotation
- Adding dependencies with trailing comma
- Updating empty dependencies list
- Updating dependencies for multiple deps
- Removing extra dependencies
- Updating dependencies for family deps
- Removing unnecessary dependencies for noDep
- Removing dependencies when no deps needed (dependencies first)
- Removing dependencies when no deps needed (no params)
- Removing dependencies when no deps needed (without comma)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the golden diff file contains documentation for the new error case # Expected: The diff file should show changes reflecting the additional error case # Test: Display the golden diff file content cat test/lints/provider_dependencies/provider_dependencies.diff # Test: Check if the file was recently modified along with this PR git log -p test/lints/provider_dependencies/provider_dependencies.diffLength of output: 470
Script:
#!/bin/bash # Search for any .diff files in the repository fd -e diff # Search for any golden files or test files related to provider_dependencies fd -t f provider_dependencies # Check the test file content to understand the expected behavior cat packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies_test.dartLength of output: 3362
Script:
#!/bin/bash # Check the test file content cat packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.dart # Check the golden diff file content cat packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.diff # Check the lint implementation to understand expected error cases cat packages/riverpod_lint/lib/src/lints/provider_dependencies.dartLength of output: 12709
packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.dart (2)
12-13
: LGTM: Consistent family provider implementation.The new
dep3
provider follows the established pattern and correctly implements a family provider with an integer parameter.
76-86
: Verify test case implementation.A few observations about this test case:
- The dependencies array only includes
dep3
but the implementation also usesdep2Provider
- The implementation uses
ref.read
instead of the recommendedref.watch
Given this is a test file, please confirm if these potential issues are intentional test cases for the linter.
Let's check if there are related test assertions:
✅ Verification successful
Let me gather more context about this test file to understand if these are intentional test cases.
Let me check the lint rule implementation to understand its purpose better.
Based on the gathered context, I can now provide the final response:
Test case implementation is intentional and correct
The
onFamilyDep
provider is part of a test file that verifies theprovider_dependencies
lint rule. The apparent issues you noticed are intentionally crafted to trigger the lint warning:
- The missing
dep2
dependency is intentional - it tests the lint rule's ability to detect incomplete dependencies- The use of
ref.read
instead ofref.watch
is also part of the test case- The
expect_lint: provider_dependencies
comment explicitly marks this as a test caseThe lint rule's purpose is to ensure that providers properly declare their dependencies, especially when using scoped providers, as confirmed by the rule's implementation message: "If a provider depends on providers which specify 'dependencies', they should themselves specify 'dependencies' and include all the scoped providers."
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test assertions related to onFamilyDep rg -A 5 "onFamilyDep" --type dartLength of output: 2604
Script:
#!/bin/bash # Look for test file description and related test cases rg -B 5 -A 10 "provider_dependencies" --type dart # Also check for any expect_lint comments to understand test patterns rg "expect_lint: provider_dependencies" --type dart -B 2 -A 2Length of output: 45440
Script:
#!/bin/bash # Look for the lint rule implementation rg -A 10 "class ProviderDependencies" --type dartLength of output: 1141
packages/riverpod_annotation/CHANGELOG.md (1)
1-3
: LGTM! The changelog entry follows the standard format.The entry properly credits the contributor and is placed in the correct section.
packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.diff (5)
Line range hint
3-34
: LGTM: Dependencies correctly added to providersThe changes appropriately add dependency declarations to providers that were previously missing them, while maintaining the correct syntax and preserving other parameters.
Line range hint
39-76
: LGTM: Dependencies correctly updatedThe changes appropriately update the dependency lists to match the actual dependencies used in the providers.
Line range hint
93-142
: LGTM: Control cases for dependency validationThese changes appropriately remove dependencies to serve as negative test cases, maintaining different annotation styles and parameters. This provides good coverage for testing the linter's behavior.
77-89
: Key test case for nested ref invocationsThis test case is particularly important as it verifies the PR's main objective: correctly detecting nested ref invocations. The provider uses
ref.read(dep3Provider(ref.read(dep2Provider)))
, which is exactly the nested case we want to validate.Let's verify if there are similar patterns in the codebase that this fix would help with:
#!/bin/bash # Search for nested ref.read/watch patterns ast-grep --pattern 'ref.read($a($b.read($c)))' ast-grep --pattern 'ref.watch($a($b.read($c)))'
Line range hint
1-142
: Verify test coverage for edge casesWhile the test cases cover the main scenarios, let's verify if we have sufficient coverage for all edge cases.
✅ Verification successful
Based on the results, I can now generate the final response since we have comprehensive information about the test coverage.
Test coverage for provider dependencies is comprehensive
The test coverage for provider dependencies is thorough and includes all edge cases:
- Main test file
provider_dependencies.dart
covers:
- Plain annotations with/without dependencies
- Custom annotations with various parameter combinations
- Multiple dependencies scenarios
- Empty dependencies lists
- Family dependencies
- Dependencies ordering cases
- Removal of unnecessary dependencies
Additional coverage found in:
dependencies.dart
covers scoped providers and generated providersdependencies2.dart
covers private/public provider combinations- Integration tests in the generator package cover complex dependency chains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for other test files that might contain provider dependency tests fd -e dart -e diff . -x grep -l "provider_dependencies" # Look for actual usage patterns in the main codebase that might need test coverage ast-grep --pattern '@Riverpod(dependencies: [$$$])'Length of output: 7784
packages/riverpod_analyzer_utils_tests/test/ref_invocation_test.dart (1)
431-448
: LGTM! Comprehensive test coverage for nested ref invocations.The test assertions thoroughly verify:
- The total number of ref.read invocations (2)
- The source and provider elements for both the outer and inner invocations
- The correct parsing of the nested structure
This aligns well with the PR objective to fix the detection of nested RefInvocations.
packages/riverpod_analyzer_utils/lib/src/riverpod_ast/resolve_riverpod.dart (2)
113-118
: Enhancement to handle nestedInvocationExpression
argumentsThe added loop correctly processes
InvocationExpression
arguments within arefInvocation
, enhancing the recursive parsing capability of method invocations related to references. This improvement aligns with the objective of accurately detecting nestedRefInvocation
instances.
113-118
: Verify performance impact with deeply nested structuresWhile this change improves parsing, please ensure that it does not introduce significant performance overhead when dealing with deeply nested
InvocationExpression
structures.packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.g.dart (5)
41-42
: Hash function_dep3Hash
is correctly definedThe hash function for the
dep3
provider is correctly implemented.
64-106
:Dep3Family
class is correctly implementedThe
Dep3Family
class properly extendsFamily<int>
and includes the necessary overrides forcall
,getProviderOverride
, and dependency properties.
108-190
:Dep3Provider
class handles parameterization and overrides correctlyThe
Dep3Provider
class correctly manages the parameter, overrides methods likeoverrideWith
,createElement
,operator ==
, and implements a robusthashCode
method using_SystemHash
.
298-316
:onFamilyDepProvider
dependency setup is correctThe
onFamilyDepProvider
is correctly defined withdep3Provider
included in its dependencies and transitive dependencies, aligning with the goal of detecting nestedRefInvocations
.
43-62
:⚠️ Potential issueVerify license compliance for copied
_SystemHash
classThe
_SystemHash
class is copied from the Dart SDK, as indicated by the comment/// Copied from Dart SDK
. Please ensure that this usage complies with the Dart SDK's licensing terms. If required, include appropriate license notices or attributions in your project.
packages/riverpod_analyzer_utils_tests/test/ref_invocation_test.dart
Outdated
Show resolved
Hide resolved
2361638
to
db86c1c
Compare
RefInvocations that take as a parameter another RefInvocation are now correctly detected by the analyzer
db86c1c
to
5ef007a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/riverpod_analyzer_utils/CHANGELOG.md (1)
1-4
: LGTM! Consider adding a date placeholder.The changelog entry accurately describes the fix for nested RefInvocations detection. For consistency with other entries in this changelog, consider adding a date placeholder.
-## Unreleased fix +## Unreleased fix - TBDpackages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.diff (3)
Line range hint
3-33
: LGTM! Test cases effectively cover basic dependency specifications.The test cases appropriately verify the lint's behavior for adding dependencies in various annotation formats. Consider adding a test case for nested provider dependencies (e.g.,
ref.read(dep2(ref.read(dep)))
) to directly test the scenario mentioned in the PR objectives.
Line range hint
93-147
: LGTM! Good coverage of dependency removal scenarios.The test cases effectively verify various ways of removing dependencies while maintaining other annotation parameters. Consider adding a comment explaining why dependencies are being removed in these cases to make the test intentions clearer.
Line range hint
3-147
: Consider standardizing annotation formats.While the varying formats help test lint robustness, consider standardizing:
- Use of trailing commas
- Multi-line vs single-line annotations
- Capitalization of @riverpod/@Riverpod
This would make the tests more maintainable while still testing the core functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/riverpod_analyzer_utils/CHANGELOG.md (1 hunks)
- packages/riverpod_analyzer_utils/lib/src/riverpod_ast/resolve_riverpod.dart (2 hunks)
- packages/riverpod_analyzer_utils_tests/test/ref_invocation_test.dart (2 hunks)
- packages/riverpod_lint/CHANGELOG.md (1 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.dart (2 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.diff (10 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.g.dart (2 hunks)
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies_test.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/riverpod_analyzer_utils/lib/src/riverpod_ast/resolve_riverpod.dart
- packages/riverpod_lint/CHANGELOG.md
- packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies_test.dart
🔇 Additional comments (11)
packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.dart (2)
12-14
: LGTM! Base provider implementation is correct.The new parameterized provider follows the established pattern and is properly configured for testing nested ref invocations.
76-86
: Verify this test case matches the PR objective.This test case correctly validates the analyzer's ability to detect nested ref invocations by:
- Using
dep3Provider
with a parameter fromdep2Provider
- Explicitly marking it to expect the provider_dependencies lint
- Only declaring
dep3
in dependencies while usingdep2
via nested readThe implementation aligns perfectly with the PR's goal of fixing nested ref invocation detection.
Let's verify this is the only test case for nested ref invocations:
✅ Verification successful
Let me check the specific content of the file to see all instances of nested ref.read usage to ensure this is indeed the only test case.
Test case correctly implements nested ref.read detection
The verification confirms this is indeed the only test case for nested ref.read detection in the test suite. The implementation in
onFamilyDep
provider serves as the dedicated test for this specific scenario, making it a well-isolated test case that properly validates the lint rule's ability to detect dependencies in nested ref.read calls.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential nested ref.read test cases rg -l 'ref\.read.*ref\.read' packages/riverpod_lint_flutter_test/test/Length of output: 168
Script:
#!/bin/bash # Get all instances of nested ref.read with surrounding context rg 'ref\.read.*ref\.read' -B 2 -A 2 packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.dartLength of output: 226
packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.diff (2)
Line range hint
39-89
: LGTM! Comprehensive coverage of dependency update scenarios.The test cases effectively verify:
- Empty to non-empty dependency lists
- Multiple dependencies
- Dependency removal
- Family provider dependencies
Line range hint
1-147
: Add test cases for nested ref invocations.While the current test cases thoroughly cover dependency specifications, consider adding specific test cases for nested ref invocations (e.g.,
ref.read(dep2(ref.read(dep)))
) to directly verify the fix mentioned in the PR objectives.packages/riverpod_lint_flutter_test/test/lints/provider_dependencies/provider_dependencies.g.dart (4)
41-62
: LGTM: Hash implementation follows best practicesThe hash combining implementation follows standard practices and matches the Dart SDK's approach for generating stable hash codes.
64-189
: LGTM: Family provider implementation is well-structuredThe implementation follows Riverpod's best practices with:
- Proper dependency tracking
- Correct parameter handling
- Well-implemented equality and hash code generation
- Appropriate provider element creation
314-316
: Deprecation notice is properly documentedThe typedef is correctly marked as deprecated with a clear migration path to use
Ref
instead in version 3.0.
298-312
: LGTM: Provider dependencies are correctly declaredThe implementation properly declares and tracks dependencies on
dep3Provider
, including transitive dependencies. This supports the PR's goal of fixing nested ref invocations detection.Let's verify the dependency declarations:
✅ Verification successful
Let me analyze the results and generate the final response.
The search results confirm:
dep3Provider
is properly used in the test files and the generated code, with correct dependency declarations.- The nested ref.read pattern
ref.read(dep3Provider(ref.read(dep2Provider)))
in the test file is exactly what this PR aims to detect and fix, and the generated code properly handles this by declaring dependencies.Dependencies are correctly declared and nested ref invocations are properly handled
The implementation in
onFamilyDepProvider
correctly declares its dependency ondep3Provider
and properly tracks transitive dependencies. This is verified by:
- The presence of
dep3Provider
in both direct dependencies and transitive dependencies arrays- The test file containing the nested ref invocation pattern that this implementation aims to handle
- No unexpected or incorrect usages of
dep3Provider
found in the codebase🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that dep3Provider is properly used in the codebase # and that nested ref invocations are correctly handled # Search for dep3Provider usage to ensure it's properly integrated rg "dep3Provider" --type dart # Search for nested ref.read patterns to verify the fix rg "ref\.read.*ref\.read" --type dartLength of output: 5836
packages/riverpod_analyzer_utils_tests/test/ref_invocation_test.dart (3)
414-481
: Excellent addition of nestedref.read
testsThe new test case thoroughly verifies that nested
ref.read
invocations with family providers are correctly decoded, including up to the third level of nesting. This ensures proper recursion handling and improves test coverage.
646-713
: Effective testing of nestedref.watch
invocationsThe added test case effectively ensures that nested
ref.watch
invocations with family providers are properly decoded, up to the third level of nesting. This enhances confidence in the analyzer's ability to handle complex nesting scenarios.
714-758
: Comprehensive test for mixedref.watch
andref.read
invocationsThis test case successfully verifies that the analyzer can decode a mix of nested
ref.watch
andref.read
invocations with family providers. It adds valuable coverage for mixed usage scenarios, ensuring correct analysis in complex cases.
- adds 3rd level of nesting to ensure recursion works
5f2bc77
to
5d02a8b
Compare
@@ -575,6 +645,123 @@ void fn(_Ref ref) { | |||
); | |||
}); | |||
|
|||
testSource('Decodes nested ref.watch invocations with family providers', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those tests are redundant as it is.
But we'd need a test for WidgetRef instead of Ref
final dep3 = FutureProvider.family((ref, int arg) => 0); | ||
|
||
final provider = Provider<int>((ref) { | ||
ref.read(dep2(ref.read(dep))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another scenario I'm thinking of is:
ref.whatever(function(ref.whatever));
The indirection could possibly break this logic
RefInvocations that take as a parameter another RefInvocation are now correctly detected by the analyzer.
Details
In this situation:
ref.read(dep2(ref.read(dep)));
The second provider dependency (
dep)
is not currently detected by analyzer and the provider_dependency lints.Related Issues
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).I have updated the
CHANGELOG.md
of the relevant packages.Changelog files must be edited under the form:
If this contains new features or behavior changes,
I have updated the documentation to match those changes.
Summary by CodeRabbit
New Features
Bug Fixes
provider_dependencies
to correctly detect nested reference invocations.RefInvocations
used as parameters.Tests
ref.read
andref.watch
functionalities.Documentation
riverpod_lint
andriverpod_analyzer_utils
to reflect new features and fixes.