-
Notifications
You must be signed in to change notification settings - Fork 210
Fix intrinsic resolution; fix effect param address space propagation; more mir cleanup #1217
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
base: master
Are you sure you want to change the base?
Conversation
a263fda to
41c0f4a
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.
💡 Codex Review
fe/crates/hir/src/analysis/ty/ty_check/expr.rs
Lines 1268 to 1272 in 41c0f4a
| PathRes::Trait(trait_inst) => { | |
| let mut instantiation_args = Vec::with_capacity(1 + callable.generic_args().len()); | |
| instantiation_args.push(provided_ty); | |
| instantiation_args.extend_from_slice(callable.generic_args()); | |
In resolve_effect_requirement, trait effects are instantiated with instantiation_args = [provided_ty] + callable.generic_args(). After this change, callable.generic_args() includes the implicit effect-provider parameters that were inserted ahead of user generics, so a key like uses (ctx: Ctx<T>) will be instantiated as Ctx<__effprov0> instead of Ctx<T>. This misbinds trait requirements and can cause valid calls to fail (or invalid ones to pass) whenever the effect key depends on function generics. You likely need to exclude the effect-provider params (or otherwise realign indices) when building instantiation_args for trait effects.
ℹ️ 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".
8daf1ca to
bc7a5a3
Compare
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc7a5a3fc5
ℹ️ 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".
bdf51b8 to
296e5c5
Compare
- Monomorphize by-ref trait effect providers by call-site address space, overriding default mem when the provider comes from storage.\n- Fix trait method receiver-space inference to use the trait instantiation’s Self type so storage-backed by-ref receivers generate sload-based code.\n- Update MIR + codegen snapshots.
- Treat receiver address space = memory as the default (no specialization) so method calls don’t force separate *_mem monomorphizations.\n- Canonicalize receiver space in monomorphization/mangling for consistency.\n- Update MIR + Yul snapshots.
296e5c5 to
edef8a0
Compare
edef8a0 to
39039fe
Compare
MIR was resolving any function in core/std named "mload", "mstore", etc as an intrinsic (including trait methods). Fixing this by tracking and checking
func.is_extern(this should be tightened up more, to check a specific path) revealed that these methods weren't resolvable to concrete impls when used in free functions called from contracts, because the concrete effect provider type from the contract wasn't being propagated to the functions called within the contract.So, some reworking of how we do things to make this tidy(er):
__effprov0: EffectRef<T>. This has been the mental model for a while, now we're just fully leaning into it.