feat(lint): new rule: noImportCycles
#4948
Conversation
github-actions
bot
added
A-CLI
arendjr
force-pushed
the
no-import-cycles
branch
from
39ec848 to
d59fb9a
Compare
github-actions
bot
added
A-Project
CodSpeed Performance ReportMerging #4948 will not alter performanceComparing Summary
|
|
|
||
| impl Phase for DependencyGraphService { | ||
| fn phase() -> Phases { | ||
| Phases::Syntax |
There was a problem hiding this comment.
I believe this service should run for at least during semantic phase, or - even better IMHO - during a new phase after the semantic model.
I believe, in the future, we would want to pull the exported bindings from an imported module
E.g.
import { foo } from "../foo.js" // pull semantic module of `foo.js` so we can know if exports a `foo` bindingThere was a problem hiding this comment.
Yeah, early on I had a similar idea, but I'm starting to wonder if that's really the best way to go. After all, the analyzer phases are still based on the passes they make on an individual file. That's fine, but it doesn't really apply to data that gets populated by the scanner, because the scanner is responsible for making sure data of other files is there before the analyzer is even invoked.
So while I do agree we probably want to pull exported bindings as well, I think we'd do it in a similar "phase" as we currently use for discovering the import statement. But that's not actually an analyzer phase.
In the end I'm not really opinionated about which analyzer phase we should use. I chose Syntax because the data is already available at this point, but either is fine by me 😅
There was a problem hiding this comment.
I tried changing to Phases::Semantic since it made more semantic sense (ghe-ghe), but it actually caused the rule to not work anymore. Didn't dive too deep, so I left it for now.
| let result = ctx.imports_for_path(&resolved_path).and_then(|imports| { | ||
| let mut stack = stack.clone(); | ||
| stack.push(resolved_path.to_string()); | ||
| find_cycle(ctx, imports, seen, stack) |
There was a problem hiding this comment.
Any chance we can remove the recursion? I am a bit worried that this could cause a lot of slowdown and memory pressure in big projects.
There was a problem hiding this comment.
Yeah, I'll give it a shot.
| @@ -0,0 +1,31 @@ | |||
| --- | |||
There was a problem hiding this comment.
Can we add a test where we have a cycle up to 4/5 order of depths? It would be nice to see how the diagnostic is rendered.
| std::env::current_dir() | ||
| .map(|cwd| cwd.to_string_lossy().to_string()) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
I'm not sure this is correct, because the current working directory is now where the biome.json is found. Plus, current_dir might differ between CLI and LSP.
The use of ctx.file_path() might help here.
There was a problem hiding this comment.
Yeah, I had some doubts myself what would be best here, but I think using something relative to the biome.json actually makes sense. After all, we want to strip a common base from the paths, in such a way that it is easy to recognize where they are. The only motivation is that using the full absolute path is just unnecessarily noisy for human readers.
ctx.file_path() doesn't really help with that, since the paths that get imported may be higher up in the hierarchy than the one from where we start, so what would the common base be then?
There was a problem hiding this comment.
Anyway, we could actually create an ad-hoc diagnostic/type that leverage the Location type, and it will resolve the paths once it's rendered at console (via biome_console::fmt::Display) by passing the working directory.
I can look into it if you want.
arendjr
force-pushed
the
no-import-cycles
branch
from
412ad0b to
b33f6ed
Compare
Summary
This PR adds a new rule:
noImportCycles. Similar to the ESLintno-cyclerule: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.mdAlso closes #4930 , because this required exposing the
DependencyGraphto lint rules.Test Plan
Tests added.