improve errors for circular dependencies #7940
Conversation
There was a problem hiding this comment.
Pull Request Overview
Improve circular dependency error reporting with clearer context and actionable guidance.
- Enhance cycle formatting to include module display names with relative file paths.
- Add "How to fix" guidance to the circular dependency error.
- Append the error message to all involved package logs to surface it via .compiler.log.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rewatch/tests/snapshots/dependency-cycle.txt | Updates snapshot to reflect richer cycle output and guidance text. |
| rewatch/src/build/compile/dependency_cycle.rs | Changes format() to include file paths and requires BuildCommandState; normalizes paths and preserves cycle loop. |
| rewatch/src/build/compile.rs | Integrates new format() signature, adds guidance text, and writes the error to all involved packages' logs. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
rewatch/src/build/compile.rs Outdated
| | ||
| compile_errors.push_str(&format!( | ||
| "\n{}\n{}\n", | ||
| let guidance = "\nHow to fix:\n- Extract shared code into a new module both depend on.\n- If a namespace entry is part of the cycle, import submodules directly instead of the entry.\n"; |
There was a problem hiding this comment.
I'm not sure if what you mean by
If a namespace entry is part of the cycle, import submodules directly instead of the entry
Do you have any example?
Do you mean you should drop the namespace and directly use the name of the module? Submodule feels a bit weird here given it can be a file module, not necessarily a submodule, right?
There was a problem hiding this comment.
Oops, was going to remove that one, I didn't find a good way to word it. Essentially it's about when you access a module through the main namespace file. But I'll just remove this hint.
There was a problem hiding this comment.
I see, for some reason this used to give me error ms using bsb but not with rewatch.
I think indeed that it adds more confusion than it solves, better without it!
| @codex review |
| Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
Closes #7052
Improves the circular dependency error slightly, and makes sure it ends up in the compiler logs, so we can add support on the editor side for picking the errors up and showing them in the editor.