Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17ac59eacb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3afd41a74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ingot | ||
| .all_modules(self) | ||
| .iter() | ||
| .flat_map(|top_mod| self.mir_diagnostics_for_top_mod(*top_mod, mode)) |
There was a problem hiding this comment.
Iterating ingot.all_modules(self) here and calling mir_diagnostics_for_top_mod on each module can emit the same semantic-borrow diagnostic multiple times for the same code, because the per-module collector walks each module’s full item tree (including nested items) rather than only file-local items. In multi-file ingots this causes duplicated diagnostics to flow to callers like the language server (which sorts but does not deduplicate), and also does redundant analysis work. Running once from a single root module (or deduplicating before return) avoids this.
Useful? React with 👍 / 👎.
1 participant
Note: I haven't fully vetted this on the external test cases since rebasing, will do so in the morning.
The big change here is to split MIR into phases. SMIR -> NSMIR -> MIR -> Sonatina/Yul
SMIR ("semantic mir") - this is basically hir in cfg form. Used for const time function evaluation (and thus needs to live in the hir crate to avoid a dependency cycle between hir and mir). This allows us to cleanly expand const fn capabilities, and replaces the previous ugly hir-based implementation.
NSMIR (normalized) - includes concrete borrowing and place/origin information. Used for borrow checking and other diagnostics.
MIR - low-level, but backend neutral representation that's lowered into sonatina/yul. Includes concrete layouts, const/code regions, etc
(Aside: SMIR and NSMIR could maybe be collapsed together. NSMIR just derives a bunch of information that isn't relevant for ctfe)
This fixes a lot of tricky bugs we had previously due to sidecar info alongside the insufficient representation, and information being deduced/inferred in late stages because it wasn't propagated forward by earlier stages.
Unfortunately basically every snapshot has been updated. Name mangling that was previously done in the old MIR is now deferred to codegen, and yul currently uses a simple incremented suffix for conflicting names, which should be changed.
(Aside: I think we should consider replacing the big codegen snapshots with small targeted snapshots.)
TODO, here or in quick follow-up prs