-
Notifications
You must be signed in to change notification settings - Fork 42
feat: add circular dependency detection #213
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
feat: add circular dependency detection #213
Conversation
8ee244d to
f0512c0
Compare
| desiredStart := "a.py" | ||
| cycles := graph.DetectCyclesWithStart(desiredStart) | ||
|
|
||
| if len(cycles) != 1 { |
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.
I think this test (TestDetectCycles_NoCycle) should expect no circular dependencies but the code expect one cycles. len(cycles) != 0 should be correct. Could you check the behavour of this test?
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.
I will check it soon, thank you for the response
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.
I modified as len(cycles) != 0, and got the result as the following:
=== RUN TestDetectCycles_NoCycle
--- PASS: TestDetectCycles_NoCycle (0.00s)
cmd/pyscn/check.go
Outdated
|
|
||
| // Dynamically generate the prefix using the first path as the base, ensuring it ends with a slash | ||
| if len(args) > 0 { | ||
| c.prefix = strings.TrimSuffix(args[0], "/") + "/" |
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.
args[0]="." creates prefix="./", but parsed imports like "testdata/python/circular/player.py" don't start with "./". The strings.HasPrefix check always fails, leaving the graph empty.
I suggest that when args[0]=".", set prefix="" instead of "./".
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.
I think the modification has matched what you were mentioned, and these are the result as I tested with . and testdata/python/circular
yang@Mac pyscn % ./pyscn check .
🔍 Running quality check (complexity, deadcode, clones)...
testdata/python/complex/comprehensions.py:0:1: __main__ is too complex (48 > 10)
...
testdata/python/edge_cases/nested_structures.py:0:1: __main__ is too complex (23 > 10)
testdata/python/complex/dead_code_complex.py:30:1: unreachable_after_return (critical)
testdata/python/complex/dead_code_complex.py:74:1: unreachable_after_raise (critical)
...
testdata/python/simple/dead_code_simple.py:60:1: unreachable_after_return (critical)
testdata/python/simple/dead_code_simple.py:12:1: unreachable_after_return (critical)
testdata/python/simple/dead_code_simple.py:34:1: unreachable_after_break (critical)
python/src/pyscn/mcp_main.py:14:1: clone of python/src/pyscn_mcp/__main__.py:14:1 (similarity: 100.0%)
testdata/python/edge_cases/python310_features.py:116:1: clone of testdata/python/simple/control_flow.py:133:1 (similarity: 99.3%)
testdata/python/edge_cases/python310_features.py:49:1: clone of testdata/python/edge_cases/python310_features.py:143:1 (similarity: 99.2%)
testdata/python/edge_cases/python310_features.py:32:1: clone of testdata/python/edge_cases/python310_features.py:94:1 (similarity: 99.0%)
...
testdata/python/edge_cases/dead_code_examples.py:110:1: clone of testdata/python/edge_cases/nested_structures.py:141:1 (similarity: 70.7%)
testdata/python/edge_cases/python310_features.py:94:1: clone of testdata/python/simple/classes.py:48:1 (similarity: 70.0%)
testdata/python/edge_cases/python310_features.py:32:1: clone of testdata/python/simple/classes.py:48:1 (similarity: 70.0%)
⚠️ Found 245 code clone(s) (informational)
testdata/python/circular/player.py:6:1: circular dependency detected: testdata/python/circular/player.py -> testdata/python/circular/physics.py -> testdata/python/circular/player.py
❌ Circular dependency check failed: too many circular dependencies
Error: too many circular dependencies
Usage:
pyscn check [files...] [flags]
Flags:
--allow-circular-deps Allow circular dependencies (warnings only)
--allow-dead-code Allow dead code (don't fail)
-c, --config string Configuration file path
-h, --help help for check
--max-complexity int Maximum allowed complexity (default 10)
--max-cycles int Maximum allowed circular dependency cycles before failing
-q, --quiet Suppress output unless issues found
-s, --select strings Comma-separated list of analyses to run: complexity, deadcode, clones
--skip-clones Skip clone detection
Global Flags:
-v, --verbose Enable verbose output
yang@Mac pyscn %
yang@Mac pyscn % ./pyscn check testdata/python/circular
🔍 Running quality check (complexity, deadcode, clones)...
player.py:6:1: circular dependency detected: player.py -> physics.py -> player.py
❌ Circular dependency check failed: too many circular dependencies
Error: too many circular dependencies
Usage:
pyscn check [files...] [flags]
Flags:
--allow-circular-deps Allow circular dependencies (warnings only)
--allow-dead-code Allow dead code (don't fail)
-c, --config string Configuration file path
-h, --help help for check
--max-complexity int Maximum allowed complexity (default 10)
--max-cycles int Maximum allowed circular dependency cycles before failing
-q, --quiet Suppress output unless issues found
-s, --select strings Comma-separated list of analyses to run: complexity, deadcode, clones
--skip-clones Skip clone detection
Global Flags:
-v, --verbose Enable verbose output
yang@Mac pyscn %
| imported := matches[1] | ||
| // Convert dotted module path to relative file path, e.g., 'player.module' => 'player/module.py' | ||
| importedRelPath := strings.ReplaceAll(imported, ".", "/") + ".py" | ||
| fmt.Printf("File: %s has dependency on %s\n", relPath, importedRelPath) |
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.
service/circular_dependency_service.go:199 prints every import statement unconditionally. This causes thousands of lines of output on large projects. Maybe better to remove this print statement.
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.
Sure, I will remove this print function to get clear result.
But does not need to print anything like A dependency on B message on the result?
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.
A plain “A imports B” dependency is just noise for the user, and dumping thousands of lines buries the circular-dependency warnings they actually care about. What we need to show are only the warnings when a cycle is detected, so printing the full dependency list isn’t the right behavior.
|
@slyang08 Hello thank you for working on this PR. Could you check the comments that I found during my review? Thanks! |
f0512c0 to
f18bd44
Compare
|
I found that our test fails because pyscn check doesn’t recognize the deps/circular selector. Could you amend cmd/pyscn/check.go so those options are accepted and the test passes? You can check the tests with |
|
Also there is a lint issue. |
f18bd44 to
8b3d1c4
Compare
|
I am trying to make sure it will accept When I tested with |
Yes, please make the CLI accept both aliases. The README already advertises --select deps, and earlier revisions used --select circular, so supporting both keeps the UX consistent. Regarding the warning: Failed to read, it comes from existing service-layer tests that deliberately exercise a missing-file code path. It isn’t new and it isn’t fatal. Don't worry. |
8b3d1c4 to
9ba4e97
Compare
Hi DaisukeYoda, I made two optionals available to improve UX The following were the testing results I did: Use circularUse deps |
|
I made some changes after I saw the recommendation by Claude Code. |
9ba4e97 to
b3e1b12
Compare
|
Hello,
@slyang08 |
|
@slyang08 Thanks! Yes, your screenshot looks good. Please push your latest version. |
b3e1b12 to
3776875
Compare
|
@slyang08 |
3776875 to
b8f9329
Compare
|
Appreciate all the revisions you’ve made so far. Now what I’m seeing:
Could we rework this PR to reuse the existing SystemAnalysisService / CircularDependencyDetector instead of maintaining a parallel implementation? If check calls into AnalyzeDependencies, we get the same If you find any difficulites, I am pleased to share my sample codes. |
|
Hi @DaisukeYoda , If any possible, can you share your code, please? New info, I can track the circular dependency by the files and functions that you mentioned. However, I stuck at showing line and column right now. |
|
Hi @slyang08, I've created a reference implementation in PR #220 that you can use! It reuses the existing Key Implementation DetailsLine/Column TrackingFor the func (c *CheckCommand) checkCircularDependencies(cmd *cobra.Command, args []string) (int, error) {
// ... module analyzer setup ...
graph, err := moduleAnalyzer.AnalyzeProject()
result := analyzer.DetectCircularDependencies(graph)
// Output in linter format
for _, cycle := range result.CircularDependencies {
firstModule := cycle.Modules[0]
node := graph.Nodes[firstModule]
// Simple approach: use line 1 for now
cyclePath := strings.Join(cycle.Modules, " -> ")
fmt.Fprintf(cmd.ErrOrStderr(), "%s:1:1: circular dependency detected: %s\n",
node.FilePath, cyclePath)
}
return result.TotalCycles, nil
}Why Line 1?The existing
If You Need Precise LinesTo get actual import line numbers, you'd need to:
type DependencyEdge struct {
From string
To string
EdgeType DependencyEdgeType
ImportInfo *ImportInfo // This already has Line field!
}
// In checkCircularDependencies
for _, cycle := range result.CircularDependencies {
if len(cycle.Dependencies) > 0 {
firstDep := cycle.Dependencies[0]
// cycle.Dependencies already contains the import info
// but you need to map it back to the edge in the graph
}
}However, this adds complexity for minimal benefit in the Full CodeYou can see the complete implementation here:
The implementation is only 58 lines and reuses all existing components. No need for regex or custom graph implementations! Let me know if you have any questions about the approach. |
|
Sorry for late response! |
|
Hi @DaisukeYoda, Sorry to modify the code late. But I cannot be sure is the column showing accurately in every Python files; therefore, I did a test case for Node Columns in If you think the test case is unnecessary, I will delete them. |
|
@slyang08 |
Hi @DaisukeYoda, |
b8f9329 to
54ea9bc
Compare
|
Yes, I'd like to confirm the former option. |
54ea9bc to
f757b81
Compare
|
Hi, If you have any concern that please tell me to modify once again. |
|
@slyang08 Thanks for sticking with all the review feedback. I really appreciate your contribution! I'll try to maintain the repo more actively, so please keep contributing if you're interested. Thanks! 🙏 |







Summary
The
checkcommand supports three more optional features,--allow-circular-deps: Allow or not allow circular dependencies--select deps: Make warning if any circular dependencies in files--max-cycles N: Allow N cycles of circular dependenciesThese for better control code quality in Python project.
Type of Change
Related Issues
Closes #175 for new feature made
Changes
Add files to implement the feature and Adjust the code to match the requirements in issue #175
Testing
make testpasses locallymake lintpasses locallyDocumentation
Additional Context
Result of usage
pyscn check --select deps
pyscn check --max-cycles 3
pyscn check --allow-circular-deps