Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
💤 Files with no reviewable changes (27)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughType aliases, associated types, projection types, generic defaults, recursive-type checks, and related diagnostics now span parsing, AST validation, HIR lowering, resolution, type checking, THIR lowering, and integration tests. The backend and LLVM code-generation modules were also removed. ChangesType alias and projection compiler flow
Priority: ⬇️ Low Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant AST
participant Resolver
participant Typeck
participant THIR
Parser->>AST: Build type alias or projection node
AST->>Resolver: Assign IDs and collect type definitions
Resolver->>Typeck: Provide definitions, modules, and paths
Typeck->>Typeck: Convert, normalize, and unify types
Typeck->>THIR: Provide resolved node types
THIR->>THIR: Lower resolved aliases and associated items
Merge Risk: ⚪ Minimal · up to No actionable regression is established for the reviewed head. The compiler’s default command did not produce a program executable before this change, and the remaining comment concerns wording only. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new language features include recursion and invalid-type checks. No introduced privilege escalation or broader security exposure was demonstrated, but visibility behavior and some combined failure paths remain uncertain. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit reads each line, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/resolve/late.rs (1)
206-251: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
defer_type_relative_pathbails out on the first failed prefix candidate instead of trying shorter ones.The "Case 2" loop (Lines 221-248) is designed to try progressively shorter module prefixes to find the
module::...::Typeboundary, but.ok()?(Line 228) and?(Line 232) useOption-chaining semantics: any failure on the first (longest) candidate causes the entire function to returnNone, never reaching shorter prefixes later in the loop. Since the loop starts atsegments.len() - 1and shrinks, the longest candidate will almost always include the type segment inside the "module" prefix and fail to resolve as a module — meaning the correct (shorter) split is never tried for any 3+ segment type-relative path (e.g.a::b::Type::method).🐛 Proposed fix
- let module = self - .resolver - .resolve_module_path(self.resolver.module_idx, module_prefix) - .ok()?; - - let resolution = self.resolver.modules[module] - .resolutions - .get(&type_seg.ident.value)?; + let Ok(module) = self + .resolver + .resolve_module_path(self.resolver.module_idx, module_prefix) + else { + continue; + }; + + let Some(resolution) = self.resolver.modules[module] + .resolutions + .get(&type_seg.ident.value) + else { + continue; + };Also applies to: 221-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolve/late.rs` around lines 206 - 251, Update the Case 2 loop in defer_type_relative_path to skip a prefix when resolve_module_path fails or when the module resolution lacks the candidate type segment, rather than returning None. Preserve the existing type-definition and trailing-segment checks, allowing all progressively shorter prefixes to be tried before returning None.src/parser/stmt.rs (1)
96-176: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStruct associated types can be declared without a body, unlike struct methods.
parse_struct_decl_item'sFnbranch emitsStructMethodMissingBodywhen a method has no body, but the newTypebranch (Lines 147-151) never checks whethertype_isSome. Since structs are concrete (no later "impl" phase fills in a struct's own inline associated type, unlike trait declarations), a bodylesstype Foo;inside astruct { ... }silently producesAssocItemKind::Type { type_: None }with no diagnostic, similar to howparse_impl_itemguards this withImplTypeMissingBody.🐛 Proposed fix
TokenKind::Type => { let mut assoc = parse_type_assoc_item(parser)?; + let has_body = matches!(&assoc.kind, AssocItemKind::Type { type_: Some(_), .. }); + if !has_body { + builders::emit_at( + parser.ctx, + assoc.span, + parser.current_token().module_id, + diag::StructTypeMissingBody, + diag_params! {}, + ); + } assoc.visibility = visibility; items.push(assoc); }Note: this requires adding a
StructTypeMissingBodyentry tosrc/parser/diagnostics.toml, which is not part of the files provided for this review.Also applies to: 147-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parser/stmt.rs` around lines 96 - 176, Update the TokenKind::Type branch in parse_struct_decl_item to detect when the parsed associated type has no body (type_ is None) and emit the new StructTypeMissingBody diagnostic at the associated type name/span, while still adding the item. Add the corresponding StructTypeMissingBody entry to parser diagnostics, following the existing ImplTypeMissingBody and StructMethodMissingBody patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/grammar.txt`:
- Line 45: Remove the optional [pub]? from the Assoc production so the grammar
matches parse_trait_decl_item and parse_impl_item, which dispatch associated
items only from Type or Fn and reject a leading Pub.
In `@src/parser/diagnostics.toml`:
- Around line 37-43: Remove the duplicate TOML table definitions for
impl_type_missing_body and const_item_without_value in the diagnostics
configuration, keeping exactly one header and its associated level/message
entries for each diagnostic so the file parses successfully.
In `@src/resolve/early.rs`:
- Around line 428-469: Extract a shared associated-item symbol helper for both
AssocItemKind::Fn and AssocItemKind::Type, then use it to collapse the
duplicated match arms in src/resolve/early.rs lines 428-469 and 546-570;
preserve each site’s existing allocation or insertion behavior. Reuse the helper
in src/resolve/late.rs lines 315-350 to remove the duplicated branching while
retaining its def_id_for_node mechanism.
In `@src/typeck/passes/collect.rs`:
- Around line 123-125: Update the associated-item collection logic around
AssocItemKind::Fn to handle associated type aliases instead of continuing past
all non-function items. Create and store a Scheme for each alias in
item_schemes, including the parent generic variables when applicable, so
Ty::normalize_aliases can resolve its associated-type DefId.
In `@src/typeck/types.rs`:
- Around line 125-136: Validate and complete generic arguments for the alias
before the substitution in the `item_schemes` branch, using the same
arity/default handling as `instantiate_with_explicit_args`. Ensure extra or
invalid arguments are rejected and omitted parameters receive valid mappings,
rather than allowing `zip` or an unmapped collector-owned `TyVarId` to normalize
the alias incorrectly.
---
Outside diff comments:
In `@src/parser/stmt.rs`:
- Around line 96-176: Update the TokenKind::Type branch in
parse_struct_decl_item to detect when the parsed associated type has no body
(type_ is None) and emit the new StructTypeMissingBody diagnostic at the
associated type name/span, while still adding the item. Add the corresponding
StructTypeMissingBody entry to parser diagnostics, following the existing
ImplTypeMissingBody and StructMethodMissingBody patterns.
In `@src/resolve/late.rs`:
- Around line 206-251: Update the Case 2 loop in defer_type_relative_path to
skip a prefix when resolve_module_path fails or when the module resolution lacks
the candidate type segment, rather than returning None. Preserve the existing
type-definition and trailing-segment checks, allowing all progressively shorter
prefixes to be tried before returning None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e83ec0cc-ec35-422c-9688-f6c509fee93d
📒 Files selected for processing (29)
AGENTS.mddocs/grammar.txtlib/oxi/std/ops.oxisrc/ast/mod.rssrc/ast/tests.rssrc/ast/validate.rssrc/ast/visit.rssrc/hir/def.rssrc/hir/lower/item.rssrc/hir/mod.rssrc/hir/types.rssrc/lexer/token.rssrc/parser/diagnostics.tomlsrc/parser/lookups.rssrc/parser/stmt.rssrc/resolve/early.rssrc/resolve/late.rssrc/resolve/mod.rssrc/thir/lower.rssrc/thir/mod.rssrc/thir/scope/builder.rssrc/typeck/mod.rssrc/typeck/passes/check/mod.rssrc/typeck/passes/collect.rssrc/typeck/types.rstests/integration/type_alias/chain.oxitests/integration/type_alias/generic.oxitests/integration/type_alias/invalid_rhs.oxitests/integration/type_alias/type_alias1.oxi
TODO: Report an error without panicking
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hir/lower/item.rs (1)
79-100: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftImplement associated-type type checking before enabling this lowering.
A valid
type Bar = i32;associated item reachestodo!branches insrc/typeck/passes/collect.rsandsrc/typeck/passes/check/mod.rs, causing a compiler panic. Collect and check associated-type schemes, then add a positive integration test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hir/lower/item.rs` around lines 79 - 100, Before enabling the associated-type lowering in the `AssocItemKind::Type` branch, implement associated-type scheme collection in `collect.rs` and associated-type checking in `check/mod.rs`, replacing the reachable `todo!` paths with the expected type-checking behavior. Ensure valid declarations such as `type Bar = i32;` complete without panicking, and add a positive integration test covering this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/parser/diagnostics.toml`:
- Around line 21-23: Remove the duplicate [struct_assoc_type_missing_body] table
header from the diagnostics configuration, keeping a single definition with its
existing level and message so the TOML parses successfully.
In `@src/typeck/passes/check/mod.rs`:
- Around line 242-262: Update the generic-argument handling in the resolved-type
construction to return Ty::Error immediately after emitting
UnexpectedGenericArgs when args.len() differs from scheme.vars.len(). Only build
the mapping and call substitute_ty_vars for matching arity, preserving the
existing behavior for valid generic arguments.
- Around line 236-273: Update normalize_aliases to track TypeAlias def_ids
currently being expanded, detect self- and mutually-recursive re-entry before
recursively calling self.normalize_aliases, and emit the existing appropriate
cycle diagnostic instead of recursing indefinitely. Remove each alias from the
active-expansion set after expansion, preserving normal alias substitution, and
add a recursive alias case to the type-alias integration tests near invalid_rhs.
In `@src/typeck/passes/collect.rs`:
- Around line 168-170: The AssocItemKind::Type branches currently panic via
todo! instead of reporting unsupported associated types. In
src/typeck/passes/collect.rs lines 168-170, replace the todo! in
collect_signatures with associated-type scheme collection or a builders::emit_at
diagnostic; apply the same graceful handling in src/typeck/passes/check/mod.rs
lines 120-122 so body checking also avoids panics.
---
Outside diff comments:
In `@src/hir/lower/item.rs`:
- Around line 79-100: Before enabling the associated-type lowering in the
`AssocItemKind::Type` branch, implement associated-type scheme collection in
`collect.rs` and associated-type checking in `check/mod.rs`, replacing the
reachable `todo!` paths with the expected type-checking behavior. Ensure valid
declarations such as `type Bar = i32;` complete without panicking, and add a
positive integration test covering this case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82c36d8e-f3e4-4517-abb1-3d868c93d065
📒 Files selected for processing (22)
crates/oxic_test/src/lib.rssrc/hir/lower/item.rssrc/hir/lower/mod.rssrc/hir/mod.rssrc/hir/owner.rssrc/hir/types.rssrc/parser/diagnostics.tomlsrc/parser/stmt.rssrc/parser/utils.rssrc/resolve/early.rssrc/resolve/late.rssrc/resolve/mod.rssrc/typeck/mod.rssrc/typeck/passes/check/mod.rssrc/typeck/passes/coherence.rssrc/typeck/passes/collect.rssrc/typeck/passes/method.rssrc/typeck/passes/rewrite.rstests/integration/deeply_nested_struct_method.oxitests/integration/generics/type_alias1.oxitests/integration/generics/type_alias2.oxitests/integration/struct_assoc_type_no_body.oxi
💤 Files with no reviewable changes (1)
- src/hir/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/typeck/passes/check/mod.rs (1)
270-285: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftInstantiate associated-type schemes in the active inference context.
Line 285 caches
scheme.body.clone(). That body can contain parent genericTyVarIdvalues created during signature collection.check_bodiescreates a newInferCtxand registers new variables beforeSelf::Ais checked.For a generic associated type such as
type A = T,Self::Acan therefore resolve to stale or unrelated inference variables. This can mis-typecheck a generic associated function.Keep the associated-type
Schemein the cache. Resolve its parent variables from the projection self type or the activeBodyCheckerinference context. Do not cache a context-specific instantiatedTy.Add an integration case where a generic associated type directly aliases its parent parameter, such as
type A = T, and useSelf::Ain an associated function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 270 - 285, Update compute_assoc_types to cache each associated-type Scheme rather than scheme.body.clone(), then instantiate or resolve its parent variables using the projection self type or active BodyChecker inference context when Self::A is checked. Ensure cached data is not tied to the signature-collection inference context, and add an integration case where a generic associated type aliases its parent parameter (type A = T) and an associated function uses Self::A.src/typeck/types.rs (2)
479-507: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject or process generic arguments on intermediate path segments.
ty_hir_generic_argsreads only the final segment. Its own example,Foo::<u8>::Bar::<u16>, shows thatFoo::<u8>is silently discarded. This can resolve an associated type with incorrect outer generic arguments.Walk every segment, or emit a diagnostic for non-final segment arguments until full support exists.
I can help implement the segment traversal and add an integration fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 479 - 507, Update ty_hir_generic_args to inspect every path segment instead of silently ignoring generic_args on intermediate segments; either process those arguments correctly or emit a diagnostic for any non-final segment arguments while preserving final-segment handling.
411-477: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSubstitute earlier defaults into later associated-type defaults.
When no explicit generic arguments exist, each default is converted independently with
ty_from_hir. Forstruct S<T = i32, U = T>, the second default is not substituted withT = i32. It becomes the current inference variable or an error value forT.Reuse the substitution approach from
fill_generic_defaults: resolve each default with the accumulated mapping, then add the resolved value to that mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 411 - 477, Update resolve_inherent_assoc_type’s default-resolution branch to accumulate a TyVarId-to-Ty mapping while processing defaults in declaration order. Before calling ty_from_hir for each default, substitute the mapping accumulated from earlier defaults into that default, then normalize and add the resolved value under the corresponding scheme variable; preserve the existing recursion guard and final substitute_ty_vars result.src/typeck/mod.rs (1)
283-296: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReuse canonicalized generic arguments in coherence checks.
Explicit generic args do not reuse
hir_id_to_ty_var, so two non-default impls of the same trait get freshTy::Varids and bypasshas_conflicting_implthrough raw equality. Normalize or canonicalizetrait_generic_argsandself_type_generic_argsbefore inserting or comparing them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/mod.rs` around lines 283 - 296, The coherence check in has_conflicting_impl currently compares raw generic argument and self-type values, allowing equivalent explicit non-default impls with different Ty::Var IDs to bypass conflicts. Canonicalize or normalize trait_generic_args and self_type_generic_args consistently before storing them in impl_resolved_generic_args and impl_resolved_self_type, and compare the canonical forms in has_conflicting_impl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/typeck/types.rs`:
- Around line 283-285: Update projection resolution and normalization to use
Ty::Projection.generic_args alongside self_ty when matching implementation
candidates, including CoherenceTable lookups, consistency checks, and
ambiguity/in-progress keys. Ensure candidates with differing resolved trait
arguments are not conflated; report the existing overload error when multiple
matches differ, while preserving single-candidate behavior.
---
Outside diff comments:
In `@src/typeck/mod.rs`:
- Around line 283-296: The coherence check in has_conflicting_impl currently
compares raw generic argument and self-type values, allowing equivalent explicit
non-default impls with different Ty::Var IDs to bypass conflicts. Canonicalize
or normalize trait_generic_args and self_type_generic_args consistently before
storing them in impl_resolved_generic_args and impl_resolved_self_type, and
compare the canonical forms in has_conflicting_impl.
In `@src/typeck/passes/check/mod.rs`:
- Around line 270-285: Update compute_assoc_types to cache each associated-type
Scheme rather than scheme.body.clone(), then instantiate or resolve its parent
variables using the projection self type or active BodyChecker inference context
when Self::A is checked. Ensure cached data is not tied to the
signature-collection inference context, and add an integration case where a
generic associated type aliases its parent parameter (type A = T) and an
associated function uses Self::A.
In `@src/typeck/types.rs`:
- Around line 479-507: Update ty_hir_generic_args to inspect every path segment
instead of silently ignoring generic_args on intermediate segments; either
process those arguments correctly or emit a diagnostic for any non-final segment
arguments while preserving final-segment handling.
- Around line 411-477: Update resolve_inherent_assoc_type’s default-resolution
branch to accumulate a TyVarId-to-Ty mapping while processing defaults in
declaration order. Before calling ty_from_hir for each default, substitute the
mapping accumulated from earlier defaults into that default, then normalize and
add the resolved value under the corresponding scheme variable; preserve the
existing recursion guard and final substitute_ty_vars result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1016898f-fa0e-4d0f-a506-a125cabdd2c6
📒 Files selected for processing (7)
src/typeck/mod.rssrc/typeck/passes/check/mod.rssrc/typeck/passes/collect.rssrc/typeck/types.rstests/integration/generics/projection_trait_partial_args.oxitests/integration/generics/projection_trait_wrong_generic_count.oxitests/integration/generics/self_type_generic_struct.oxi
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/typeck/types.rs (2)
164-171: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject non-trailing generic defaults before counting required arguments.
position(|d| d.is_some())sets the required count to the first default. A default before another non-defaulted generic parameter lets absent required arguments passcheck_generic_arity, thenfill_generic_defaultsdoes not fill parameters after theNone. Add AST validation that rejects a default before a later non-defaulted parameter, thencheck_generic_arityandfill_generic_defaultswill work for trailing defaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 164 - 171, Validate generic parameter defaults in the AST to reject any defaulted parameter followed by a non-defaulted parameter. Add this validation before the arity logic in check_generic_arity, and ensure fill_generic_defaults relies on the resulting trailing-default invariant while preserving normal handling of valid trailing defaults.
341-356: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftStore trait args and assoc-type args separately in
Ty::Projection.
Ty::Projection.generic_argscan contain<S as Trait<i32>>::Assoc'sTraitargumenti32, whileS::Assocleaves itNone.Ty::Projection generic_argsunification rejectsSome/Noneas a mismatch, and normalization ignores it, so the same associated type can fail to unify. Store the two argument lists in separate fields, or use the same trait arguments for both spellings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 341 - 356, Update the projection construction in the surrounding type conversion method so trait-path generic arguments and associated-type segment arguments are represented separately in Ty::Projection, or consistently reuse the trait arguments for both spellings. Ensure S::Assoc and <S as Trait<i32>>::Assoc produce compatible projection types during unification and normalization, rather than combining them with segment_args.or(trait_path_args).src/typeck/passes/check/mod.rs (1)
206-238: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winResolve default generic arguments before writing
node_types.
resolve_default_generic_argcallsty_from_hir, andTyKind::Inferallocates a freshTy::Var. This post-pass runs after the generic-default fallback loop, so such variables are not defaulted later. If any default argument still contains a variable, don’t insert theTy::Adtsubstitution here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 206 - 238, Update the `resolved` map construction around `resolve_default_generic_arg` so generic defaults containing unresolved inference variables are not written back as a `Ty::Adt` substitution. Before assigning `ty = Ty::Adt(*def_id, Some(args))`, detect whether any resolved default argument still contains a variable and only apply the substitution when all defaults are fully resolved and non-error.src/typeck/passes/coherence.rs (3)
116-131: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
idxadvances twice per iteration and can panic.
full_argsgrows by one element on each iteration (line 131), andidxre-readsfull_args.len()on each iteration (line 119). The sliceinfo.defaults[full_args.len()..]is bound once, soialready advances by one per iteration. The result isidx = base + 2*iinstead ofbase + i.Two consequences:
- The substitution registers the default under the wrong generic parameter for the second and later defaults.
info.hir_ids[idx]panics with an out-of-bounds index. For a trait with three generic parameters and one supplied argument,base = 1, and the second iteration computesidx = 3against a three-elementhir_ids.Capture the base offset before the loop.
🐛 Proposed fix
- for (i, default) in - info.defaults[full_args.len()..].iter().enumerate() - { - let idx = full_args.len() + i; + let base = full_args.len(); + for (i, default) in info.defaults[base..].iter().enumerate() { + let idx = base + i; let ty = this.resolve_default_generic_arg( default.as_ref().expect("default exists"), module_id, &subst, Some((trait_def_id, struct_def_id)), ); if let Some(&var) = this.icx.hir_id_to_ty_var.get(&info.hir_ids[idx]) { subst.insert(var, ty.clone()); } full_args.push(ty); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/coherence.rs` around lines 116 - 131, Capture the initial full_args.len() before iterating over the sliced defaults, then compute each hir_ids index from that fixed base plus i. Update the loop around resolve_default_generic_arg so growing full_args does not affect idx, preserving correct substitution for every default and avoiding out-of-bounds access.
219-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis registration block is unreachable work.
collect_signaturesalready pushes every impl intocoherence.implsand setscoherence.impl_to_trait(seesrc/typeck/passes/collect.rslines 131-145).run()callscollect_signaturesbeforecheck_coherence, so thecontains(&def_id)guard is always true here and the body never runs.Remove this block, or move impl registration out of
collect_signaturesso that only one pass owns it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/coherence.rs` around lines 219 - 227, Remove the unreachable impl-registration block in check_coherence’s surrounding flow, since collect_signatures already populates coherence.impls and coherence.impl_to_trait before it runs. Keep registration owned by only one pass and preserve the existing coherence checking behavior.
173-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEmit conflicting impl diagnostics once per conflict.
has_conflicting_implalready ignoresdef_id, so both impls in a duplicate pair can emitConflictingImplementations. Emit either only whendef_idis later than the matched impl, or accumulate a conflict set before reporting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/coherence.rs` around lines 173 - 218, Update the conflicting-implementation reporting around has_conflicting_impl so each duplicate impl pair emits ConflictingImplementations only once. Use the impl IDs collected in others to report only when def_id is later than the matched conflicting impl, or otherwise accumulate and deduplicate conflicts before calling builders::emit_at; preserve the existing diagnostic details and early return behavior.src/typeck/mod.rs (1)
149-180: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTwo passes populate the same coherence tables.
collect_signaturesalready fillscoherence.generic_params,coherence.impls, andcoherence.impl_to_trait, andrun()calls it before bothbuild_generic_paramsconsumers andcheck_coherence. The other writers therefore either duplicate the work or are unreachable, and the duplicatedgeneric_paramswriters already disagree about whether to insert an entry for an item with no generic parameters.
src/typeck/mod.rs#L149-L180: decide whetherbuild_generic_paramsorcollect_signaturesownscoherence.generic_params, then remove the other writer. Ifbuild_generic_paramsstays, makecollect_signaturesstop re-insertingGenericParamInfo, and keep the unconditional-insert behavior socheck_generic_aritysees a consistent map.src/typeck/passes/coherence.rs#L219-L227: remove this block, becausecollect_signaturesalready pusheddef_idintocoherence.implsand setcoherence.impl_to_trait, so thecontains(&def_id)guard is always true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/mod.rs` around lines 149 - 180, Remove the duplicate coherence-table population: in src/typeck/mod.rs:149-180, keep build_generic_params as the owner of coherence.generic_params, remove collect_signatures’ re-insertion of GenericParamInfo, and preserve unconditional insertion so check_generic_arity sees entries for non-generic items; in src/typeck/passes/coherence.rs:219-227, remove the redundant impls/impl_to_trait block guarded by contains(&def_id), since collect_signatures already populates those tables.src/typeck/passes/collect.rs (2)
439-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup the threaded parameters into a context struct.
visit_hir_assoc_type_alias,walk_hir_ty_for_cycles, andwalk_hir_qpath_for_cycleseach take the same five context values plus two mutable sets, and each needs#[allow(clippy::too_many_arguments)]. Every recursive call repeats all eight arguments, which makes the call sites long and easy to get wrong.Introduce a small struct that holds
krate,assoc_type_index,assoc_to_parent,defs,visited, andin_progress, then pass&mut ctxplusstart. The three#[allow]attributes can then be removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/collect.rs` around lines 439 - 449, Introduce a context struct containing krate, assoc_type_index, assoc_to_parent, defs, visited, and in_progress, then update visit_hir_assoc_type_alias, walk_hir_ty_for_cycles, and walk_hir_qpath_for_cycles to accept &mut context plus start instead of the repeated parameters. Propagate the context through all recursive calls and remove the three #[allow(clippy::too_many_arguments)] attributes while preserving existing cycle-detection behavior.
298-311: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an impl-aware key for the shared associated type lookup.
full_assoc_type_indexis rebuilt intoself.coherence.assoc_type_index, whose key is(struct_def_id, name). When one struct implements multiple traits with the same associated type name, later inserts replace earlier ones, so recursive associated types in the replaced impl may be missed. Key the lookup by the impl, or keep a separate composite key per impl, instead of reusing the trait-index key.
self.coherence.implsis anFxHashMap, so which entry remains is non-deterministic across runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/collect.rs` around lines 298 - 311, The rebuilt full_assoc_type_index must distinguish associated types by impl as well as struct and name, preventing same-named associated types from different trait implementations from overwriting one another. Update the index key and all corresponding lookup/use sites in the coherence collection flow, while preserving coverage for recursive associated types across every impl in self.coherence.impls.src/typeck/passes/check/call.rs (1)
426-449: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the shared auto-ref receiver decision.
Lines 426-449 in
try_match_method_argsand lines 504-527 inapply_auto_ref_adjustmentimplement the same rule: resolve the receiver and the first parameter, and if the receiver is neither a pointer nor a variable while the parameter is a pointer, unify against the pointer's inner type. The two copies differ only in what they do with the result, and both were touched here.The probe uses the first copy to accept a candidate, and the finalization uses the second copy to record
Adjustment::AutoRef. If the copies drift, the recorded adjustment stops matching the accepted candidate.Extract a helper that returns the effective parameter type and whether auto-ref applies, then call it from both sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/call.rs` around lines 426 - 449, Extract a shared helper for the receiver/first-parameter auto-ref decision used by try_match_method_args and apply_auto_ref_adjustment. Have it resolve both types, detect the non-pointer/non-variable receiver with pointer parameter case, and return the pointer’s inner type plus whether auto-ref applies; otherwise return the original parameter type and false. Replace both duplicated branches with this helper, preserving each caller’s existing result handling and Adjustment::AutoRef recording.
♻️ Duplicate comments (2)
src/typeck/unify.rs (1)
158-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ty::Aliasstill reaches the catch-all and reports a mismatch.The projection arms normalize one side and retry.
Ty::Aliashas no arm, so(Ty::Alias { .. }, Ty::Adt(..))falls to line 174 and produces a type mismatch. TheTy::Vararm at lines 47-52 also normalizes onlyTy::Projection, so an alias bound to a variable stays unexpanded.normalize_assoc_projectionsalready expandsTy::Alias, so symmetric alias arms would resolve these pairs.This repeats an earlier review comment that is not marked as addressed.
🐛 Proposed alias arms
+ (Ty::Alias { .. }, _) => { + let a = self.normalize_assoc_projections(&a); + if matches!(&a, Ty::Alias { .. }) { + Err(mismatch(a, b, span, module_id)) + } else { + self.unify(&a, &b, span, module_id) + } + } + (_, Ty::Alias { .. }) => { + let b = self.normalize_assoc_projections(&b); + if matches!(&b, Ty::Alias { .. }) { + Err(mismatch(a, b, span, module_id)) + } else { + self.unify(&a, &b, span, module_id) + } + } _ => Err(mismatch(a, b, span, module_id)),Run the following script to check whether a
Ty::Aliascan reachunifyunexpanded:#!/bin/bash set -euo pipefail echo "=== unify call sites outside unify.rs ===" rg -nP --type=rust -C 6 '\.unify\s*\(' -g '!src/typeck/unify.rs' echo echo "=== paths that store Ty::Alias into schemes or node types without normalization ===" rg -nP --type=rust -C 6 'Ty::Alias\s*\{' -g '!**/tests/**' echo echo "=== normalize_assoc_projections definition ===" ast-grep run --pattern 'fn normalize_assoc_projections($$$) { $$$ }' --lang rust src/typeck🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/unify.rs` around lines 158 - 174, Update the unify matching logic to add symmetric Ty::Alias arms alongside the existing Ty::Projection arms, normalizing the alias-containing operand with normalize_assoc_projections and retrying self.unify when expansion succeeds, otherwise returning mismatch. Also update the Ty::Var normalization path so aliases bound to variables are normalized, preserving the existing projection behavior.src/typeck/types.rs (1)
108-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe catch-all maps non-type definitions to
Ty::Adt.
DefKind::TypeAliasbecomesTy::Alias. Every other kind becomesTy::Adt(def_id, generic_args). A path in type position that resolves to a function, a constant, or an associated type therefore becomes an ADT whoseDefIdnames a non-type. The failure surfaces later during unification or field lookup with a message that does not name the real problem.An earlier review raised this and marked it addressed, but the arm is unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 108 - 114, The catch-all arm in the type-resolution match must reject non-type definitions instead of constructing Ty::Adt. Update the match around resolver.def(def_id).kind to handle only valid ADT definitions as Ty::Adt, preserve the TypeAlias-to-Ty::Alias path, and return the established type-resolution error for functions, constants, associated items, and other invalid kinds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/typeck/mod.rs`:
- Around line 149-180: Remove the duplicate coherence-table population: in
src/typeck/mod.rs:149-180, keep build_generic_params as the owner of
coherence.generic_params, remove collect_signatures’ re-insertion of
GenericParamInfo, and preserve unconditional insertion so check_generic_arity
sees entries for non-generic items; in src/typeck/passes/coherence.rs:219-227,
remove the redundant impls/impl_to_trait block guarded by contains(&def_id),
since collect_signatures already populates those tables.
In `@src/typeck/passes/check/call.rs`:
- Around line 426-449: Extract a shared helper for the receiver/first-parameter
auto-ref decision used by try_match_method_args and apply_auto_ref_adjustment.
Have it resolve both types, detect the non-pointer/non-variable receiver with
pointer parameter case, and return the pointer’s inner type plus whether
auto-ref applies; otherwise return the original parameter type and false.
Replace both duplicated branches with this helper, preserving each caller’s
existing result handling and Adjustment::AutoRef recording.
In `@src/typeck/passes/check/mod.rs`:
- Around line 206-238: Update the `resolved` map construction around
`resolve_default_generic_arg` so generic defaults containing unresolved
inference variables are not written back as a `Ty::Adt` substitution. Before
assigning `ty = Ty::Adt(*def_id, Some(args))`, detect whether any resolved
default argument still contains a variable and only apply the substitution when
all defaults are fully resolved and non-error.
In `@src/typeck/passes/coherence.rs`:
- Around line 116-131: Capture the initial full_args.len() before iterating over
the sliced defaults, then compute each hir_ids index from that fixed base plus
i. Update the loop around resolve_default_generic_arg so growing full_args does
not affect idx, preserving correct substitution for every default and avoiding
out-of-bounds access.
- Around line 219-227: Remove the unreachable impl-registration block in
check_coherence’s surrounding flow, since collect_signatures already populates
coherence.impls and coherence.impl_to_trait before it runs. Keep registration
owned by only one pass and preserve the existing coherence checking behavior.
- Around line 173-218: Update the conflicting-implementation reporting around
has_conflicting_impl so each duplicate impl pair emits
ConflictingImplementations only once. Use the impl IDs collected in others to
report only when def_id is later than the matched conflicting impl, or otherwise
accumulate and deduplicate conflicts before calling builders::emit_at; preserve
the existing diagnostic details and early return behavior.
In `@src/typeck/passes/collect.rs`:
- Around line 439-449: Introduce a context struct containing krate,
assoc_type_index, assoc_to_parent, defs, visited, and in_progress, then update
visit_hir_assoc_type_alias, walk_hir_ty_for_cycles, and
walk_hir_qpath_for_cycles to accept &mut context plus start instead of the
repeated parameters. Propagate the context through all recursive calls and
remove the three #[allow(clippy::too_many_arguments)] attributes while
preserving existing cycle-detection behavior.
- Around line 298-311: The rebuilt full_assoc_type_index must distinguish
associated types by impl as well as struct and name, preventing same-named
associated types from different trait implementations from overwriting one
another. Update the index key and all corresponding lookup/use sites in the
coherence collection flow, while preserving coverage for recursive associated
types across every impl in self.coherence.impls.
In `@src/typeck/types.rs`:
- Around line 164-171: Validate generic parameter defaults in the AST to reject
any defaulted parameter followed by a non-defaulted parameter. Add this
validation before the arity logic in check_generic_arity, and ensure
fill_generic_defaults relies on the resulting trailing-default invariant while
preserving normal handling of valid trailing defaults.
- Around line 341-356: Update the projection construction in the surrounding
type conversion method so trait-path generic arguments and associated-type
segment arguments are represented separately in Ty::Projection, or consistently
reuse the trait arguments for both spellings. Ensure S::Assoc and <S as
Trait<i32>>::Assoc produce compatible projection types during unification and
normalization, rather than combining them with segment_args.or(trait_path_args).
---
Duplicate comments:
In `@src/typeck/types.rs`:
- Around line 108-114: The catch-all arm in the type-resolution match must
reject non-type definitions instead of constructing Ty::Adt. Update the match
around resolver.def(def_id).kind to handle only valid ADT definitions as
Ty::Adt, preserve the TypeAlias-to-Ty::Alias path, and return the established
type-resolution error for functions, constants, associated items, and other
invalid kinds.
In `@src/typeck/unify.rs`:
- Around line 158-174: Update the unify matching logic to add symmetric
Ty::Alias arms alongside the existing Ty::Projection arms, normalizing the
alias-containing operand with normalize_assoc_projections and retrying
self.unify when expansion succeeds, otherwise returning mismatch. Also update
the Ty::Var normalization path so aliases bound to variables are normalized,
preserving the existing projection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72319e86-3d30-46f0-a7ad-c7a32065ff9c
📒 Files selected for processing (9)
src/typeck/mod.rssrc/typeck/passes/check/call.rssrc/typeck/passes/check/mod.rssrc/typeck/passes/coherence.rssrc/typeck/passes/collect.rssrc/typeck/types.rssrc/typeck/unify.rstests/integration/generics/assoc_type_inherent_default_chain.oxitests/integration/generics/projection_ambiguous_impl_args.oxi
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/typeck/passes/check/call.rs (1)
352-361: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse auto-reference matching in the fallback path.
If a method takes
&Self, this path unifies&Selfdirectly withSelf. A failed call can then emit a false receiver mismatch. Usereceiver_auto_refbefore this unification, astry_match_method_argsdoes.Proposed fix
let first = param_tys.first().expect("method has at least 1 param"); +self + .typeck + .unify(&self.receiver_auto_ref(&recv_ty, first).0, &recv_ty, call_span, self.module_id) - self.typeck - .unify(first, &recv_ty, call_span, self.module_id) .or_push_err(&mut self.typeck.icx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/call.rs` around lines 352 - 361, Update the method receiver unification in the fallback path around the `is_method_call` block to apply `receiver_auto_ref` to the receiver type before unifying it with the first parameter, matching the behavior in `try_match_method_args`. Preserve the existing argument unification and error propagation.src/typeck/passes/check/mod.rs (3)
872-876: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResolve dependent defaults against earlier arguments.
try_complete_generic_argscopies the default HIR type without substituting prior arguments. ForU = T,Foo::<i8>can lowerUwith the declaration-timeTvariable instead ofi8. Resolve defaults sequentially with a substitution map from preceding parameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 872 - 876, Update try_complete_generic_args so each default type is resolved sequentially using substitutions from previously supplied and completed arguments before being appended to args. Build or reuse a substitution map keyed by preceding generic parameters, apply it to default_ty, and update the map after each argument so dependent defaults such as U = T resolve to the concrete earlier argument.
2005-2028: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude trait generic arguments in projection diagnostics.
Ty::Projectiondiscardstrait_generic_argshere. Diagnostics render both<S as A::<i8>>::Assocand<S as A::<u8>>::Assocas<S as A>::Assoc. Append the trait generic arguments aftername.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 2005 - 2028, Update the Ty::Projection formatting branch to include its trait_generic_args after the resolved trait name, while preserving the existing associated-type generic arguments and fallback naming behavior. Ensure projections with different trait arguments render distinctly, such as A::<i8> versus A::<u8>.
343-346: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftInclude generic arguments in projection recursion keys.
Both normalizers treat projections with different self type arguments or trait generic arguments as the same recursion entry. A valid chain from
<S as A::<i8>>::Assocto<S as A::<u8>>::Assoccan therefore stop as a false cycle.
src/typeck/passes/check/mod.rs#L343-L346: include the full projection identity inprojections_in_progress.src/typeck/passes/check/mod.rs#L541-L545: use the same full projection identity in body-checking normalization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 343 - 346, The projection recursion keys used by both normalization paths omit generic arguments, causing distinct projections to be treated as the same cycle: update `src/typeck/passes/check/mod.rs` lines 343-346 and 541-545 to use the full projection identity, including self type arguments and trait generic arguments, in `projections_in_progress`; keep both paths consistent.src/typeck/types.rs (2)
361-367: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ty_displayignorestrait_generic_args, so trait-argument mismatches print identical types.
unify_alias_argsinsrc/typeck/unify.rsLines 197-206 now reports a mismatch when two projections differ only intrait_generic_args.ty_displayinsrc/typeck/passes/check/mod.rsLines 2005-2027 destructuresTy::Projectionwith..and never printstrait_generic_args. The mismatch diagnostic then shows the same text on both sides, for example<Foo as Multi>::Outversus<Foo as Multi>::Out.Include
trait_generic_argsafter the trait name inty_display.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 361 - 367, Update ty_display’s Ty::Projection formatting to include trait_generic_args after the displayed trait name, while preserving the existing self type, trait, and associated type formatting. Ensure projections differing only in trait arguments render distinct diagnostic text.
109-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn an error for invalid type-position path resolutions.
is_type_def()acceptsAssocType, butty_from_hir()’s catch-all emits no diagnostic before returningTy::Error. Sinceunify()acceptsTy::Error, this lets invalid type bindings continue without a reported error. Add aTyFromHirErrorvariant or emit a diagnostic before returningTy::Error. Trait paths already getExpectedTypeFoundTraitduring lowering, so this does not need to handleDefKind::Trait.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 109 - 116, Update the type-position resolution match in ty_from_hir() so invalid definitions such as DefKind::AssocType produce a TyFromHirError or emit the appropriate diagnostic before returning Ty::Error; preserve the existing TypeAlias and Struct handling, and do not add separate Trait handling because lowering already reports it.src/typeck/passes/coherence.rs (1)
305-316: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNormalize default projections after substitutions.
In
resolve_default_generic_arg,normalize_type_alias(&ty)can only resolve aTy::Projectionwhenself_tyisTy::Adt. When a default projects over an earlier generic arg, that arg is substituted after normalization, so the projection stays unresolved in the returned type. If the projection can be resolved later,SubstituteTy, unify it again; otherwiseTy::Projectionleaks into generics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/coherence.rs` around lines 305 - 316, Update resolve_default_generic_arg to perform normalize_type_alias after applying subst and self_subst, so projections over substituted generic arguments can resolve once self_ty becomes a Ty::Adt. Re-run substitution or unification as needed to resolve any newly normalized result, and ensure unresolved projections do not leak into the returned generic type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ast/diagnostics.toml`:
- Around line 37-39: Remove the duplicate [generic_default_before] table
declaration in diagnostics.toml, keeping a single header while preserving its
existing level and message entries so diagnostic loading succeeds.
In `@src/typeck/types.rs`:
- Around line 370-385: Update shorthand_trait_args and the related projection
argument flow so traits without defaults use the same normalized
trait_generic_args representation as fully qualified paths, rather than
returning None; alternatively, infer the missing arguments from the selected
impl before unify_alias_args and impl_matching_trait_args compare or select
candidates. Preserve default filling for traits whose parameters all have
defaults and ensure ambiguous impls are resolved using their impl arguments.
In `@src/typeck/unify.rs`:
- Around line 167-182: Add unit tests covering both Ty::Alias arms in the typeck
test module: verify unexpandable aliases produce mismatches on either side, and
aliases with a Scheme entry in item_schemes expand and unify with their concrete
type. Also update the Ty::Projection handling near the existing projection arm
to normalize both operands when the left operand is an alias, while preserving
the current mismatch result.
---
Outside diff comments:
In `@src/typeck/passes/check/call.rs`:
- Around line 352-361: Update the method receiver unification in the fallback
path around the `is_method_call` block to apply `receiver_auto_ref` to the
receiver type before unifying it with the first parameter, matching the behavior
in `try_match_method_args`. Preserve the existing argument unification and error
propagation.
In `@src/typeck/passes/check/mod.rs`:
- Around line 872-876: Update try_complete_generic_args so each default type is
resolved sequentially using substitutions from previously supplied and completed
arguments before being appended to args. Build or reuse a substitution map keyed
by preceding generic parameters, apply it to default_ty, and update the map
after each argument so dependent defaults such as U = T resolve to the concrete
earlier argument.
- Around line 2005-2028: Update the Ty::Projection formatting branch to include
its trait_generic_args after the resolved trait name, while preserving the
existing associated-type generic arguments and fallback naming behavior. Ensure
projections with different trait arguments render distinctly, such as A::<i8>
versus A::<u8>.
- Around line 343-346: The projection recursion keys used by both normalization
paths omit generic arguments, causing distinct projections to be treated as the
same cycle: update `src/typeck/passes/check/mod.rs` lines 343-346 and 541-545 to
use the full projection identity, including self type arguments and trait
generic arguments, in `projections_in_progress`; keep both paths consistent.
In `@src/typeck/passes/coherence.rs`:
- Around line 305-316: Update resolve_default_generic_arg to perform
normalize_type_alias after applying subst and self_subst, so projections over
substituted generic arguments can resolve once self_ty becomes a Ty::Adt. Re-run
substitution or unification as needed to resolve any newly normalized result,
and ensure unresolved projections do not leak into the returned generic type.
In `@src/typeck/types.rs`:
- Around line 361-367: Update ty_display’s Ty::Projection formatting to include
trait_generic_args after the displayed trait name, while preserving the existing
self type, trait, and associated type formatting. Ensure projections differing
only in trait arguments render distinct diagnostic text.
- Around line 109-116: Update the type-position resolution match in
ty_from_hir() so invalid definitions such as DefKind::AssocType produce a
TyFromHirError or emit the appropriate diagnostic before returning Ty::Error;
preserve the existing TypeAlias and Struct handling, and do not add separate
Trait handling because lowering already reports it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ea3a207-9985-40cf-b61a-15dec2dd5035
📒 Files selected for processing (16)
src/ast/diagnostics.tomlsrc/ast/validate.rssrc/typeck/fold.rssrc/typeck/passes/check/call.rssrc/typeck/passes/check/mod.rssrc/typeck/passes/coherence.rssrc/typeck/passes/collect.rssrc/typeck/types.rssrc/typeck/unify.rstests/integration/generics/partial_explicit_args_trait_defaults.oxitests/integration/generics/partial_explicit_args_trait_defaults_mismatch.oxitests/integration/generics/struct_generic_default_before.oxitests/integration/generics/trait_generic_default_before.oxitests/integration/traits/assoc_type_trait_generic_arg_select.oxitests/integration/traits/assoc_type_trait_generic_arg_wrong.oxitests/integration/traits/trait_default_assoc_shorthand.oxi
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/typeck/passes/check/call.rs (1)
213-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate only method generic arguments.
scheme.varsincludes parent impl or trait variables. Explicit call arguments map only tomethod_vars. Forimpl<T> Foo<T> { fn f<U>(...) },foo.f::<i32>()is rejected because probing expects two arguments. Conversely, extra arguments can pass validation and then be dropped byzip(method_vars).
src/typeck/passes/check/call.rs#L213-L218: Complete and validate explicit arguments against the method-variable count.src/typeck/passes/check/mod.rs#L926-L945: Derivemethod_varsbefore completion and map exactly that list.Add a regression for an explicit generic method inside a generic impl.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/call.rs` around lines 213 - 218, Validate explicit call generic arguments against method_vars rather than scheme.vars, so parent impl or trait variables are excluded and extra arguments are rejected instead of truncated. In src/typeck/passes/check/call.rs lines 213-218, update try_complete_generic_args to use the method-variable count; in src/typeck/passes/check/mod.rs lines 926-945, derive method_vars before completion and map arguments exactly to that list. Add a regression covering an explicit generic method declared inside a generic impl.
♻️ Duplicate comments (1)
src/typeck/types.rs (1)
312-320: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftInstantiate associated-type generic arguments during resolution.
segment.generic_argsis not available when inherent associated types resolve. The projection normalizers also discardTy::Projection.generic_args, whilecompute_assoc_typesretains only an uninstantiated body. A declaration such astype Item<T> = Tcan therefore resolveFoo::Item::<i32>to a collector-owned type variable instead ofi32.
src/typeck/types.rs#L312-L320: Convert and passsegment.generic_argsinto inherent associated-type resolution.src/typeck/passes/check/mod.rs#L261-L284: Preserve sufficient associated-type scheme data to instantiate its generic parameters.src/typeck/passes/check/mod.rs#L322-L360: Apply projection generic arguments before normalizing the selected associated type.src/typeck/passes/check/mod.rs#L527-L609: Apply the same substitution in fallible body-check normalization and include these arguments in the recursion key.Add regressions for inherent and trait associated types with generic arguments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/types.rs` around lines 312 - 320, Instantiate associated-type generic arguments throughout resolution and normalization. In src/typeck/types.rs:312-320, convert segment.generic_args and pass them to resolve_inherent_assoc_type. In src/typeck/passes/check/mod.rs:261-284, retain the associated-type scheme needed to instantiate generic parameters; in 322-360 and 527-609, apply projection arguments before normalization and include them in the fallible normalization recursion key. Add regressions covering generic inherent and trait associated types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/typeck/passes/check/call.rs`:
- Around line 213-218: Validate explicit call generic arguments against
method_vars rather than scheme.vars, so parent impl or trait variables are
excluded and extra arguments are rejected instead of truncated. In
src/typeck/passes/check/call.rs lines 213-218, update try_complete_generic_args
to use the method-variable count; in src/typeck/passes/check/mod.rs lines
926-945, derive method_vars before completion and map arguments exactly to that
list. Add a regression covering an explicit generic method declared inside a
generic impl.
---
Duplicate comments:
In `@src/typeck/types.rs`:
- Around line 312-320: Instantiate associated-type generic arguments throughout
resolution and normalization. In src/typeck/types.rs:312-320, convert
segment.generic_args and pass them to resolve_inherent_assoc_type. In
src/typeck/passes/check/mod.rs:261-284, retain the associated-type scheme needed
to instantiate generic parameters; in 322-360 and 527-609, apply projection
arguments before normalization and include them in the fallible normalization
recursion key. Add regressions covering generic inherent and trait associated
types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65e013cd-e04c-4c9c-91ba-cd12f074e2ac
📒 Files selected for processing (5)
src/typeck/passes/check/call.rssrc/typeck/passes/check/mod.rssrc/typeck/passes/coherence.rssrc/typeck/types.rssrc/typeck/unify.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/typeck/passes/check/mod.rs`:
- Around line 48-67: Change ProjectionRecKey to store the identifying projection
values directly—trait_def_id, assoc_def_id, self_ty, trait_generic_args, and
generic_args—instead of a u64 hash, preserving suitable Clone, PartialEq, and Eq
derives. Update its constructor to build that tuple-backed key without hashing,
and adjust guard-vector contains and retain usages in normalize_aliases_inner
and normalize_assoc_projections_inner to borrow the non-Copy key.
🪄 Autofix
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a4a9ee7e-cb11-4feb-bf46-cb022761fdb0
📒 Files selected for processing (12)
src/ast/mod.rssrc/hir/mod.rssrc/typeck/passes/check/call.rssrc/typeck/passes/check/mod.rssrc/typeck/passes/coherence.rssrc/typeck/types.rssrc/typeck/unify.rstests/integration/generics/assoc_type_inherent_generic_args.oxitests/integration/generics/method_explicit_type_args_in_generic_impl.oxitests/integration/generics/method_explicit_type_args_in_generic_impl_wrong_count.oxitests/integration/traits/assoc_type_generic_struct.oxitests/integration/traits/assoc_type_proj_generic_args.oxi
|
An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/typeck/passes/check/mod.rs (1)
1210-1239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInstantiate dependent generic defaults before use.
The completed defaults are converted without substituting earlier resolved arguments. A declaration such as
struct Pair<T, U = T>can therefore retain the declaration-level variable forUwhen callers provide onlyT.
src/typeck/passes/check/mod.rs#L1210-L1239: Convert completed struct arguments in order. Substitute prior resolved arguments into each default.src/typeck/passes/check/mod.rs#L1276-L1296: Build an alias substitution map from provided arguments. Apply it before appending each alias default.src/typeck/passes/check/mod.rs#L1352-L1360: Apply the target struct substitution map before appending omitted target arguments.src/typeck/passes/check/mod.rs#L1372-L1380: Map target generic variables to fresh variables before resolving and storing defaults.Add coverage for direct and alias-expanded initialization with
U = T.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 1210 - 1239, Update the generic argument handling in src/typeck/passes/check/mod.rs at lines 1210-1239, 1276-1296, 1352-1360, and 1372-1380: resolve completed struct defaults in order by substituting previously resolved arguments, build alias substitutions from provided arguments before appending alias defaults, apply target struct substitutions before omitted target arguments, and map target generic variables to fresh variables before resolving stored defaults. Preserve direct and alias-expanded initialization behavior and add coverage for defaults such as U = T.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/typeck/passes/check/mod.rs`:
- Around line 1210-1239: Update the generic argument handling in
src/typeck/passes/check/mod.rs at lines 1210-1239, 1276-1296, 1352-1360, and
1372-1380: resolve completed struct defaults in order by substituting previously
resolved arguments, build alias substitutions from provided arguments before
appending alias defaults, apply target struct substitutions before omitted
target arguments, and map target generic variables to fresh variables before
resolving stored defaults. Preserve direct and alias-expanded initialization
behavior and add coverage for defaults such as U = T.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9d2f82e-85b8-4eda-b779-7fa377fbf5ad
📒 Files selected for processing (2)
src/typeck/passes/check/mod.rssrc/typeck/types.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/typeck/passes/check/mod.rs`:
- Around line 1284-1295: In the type-checking flow containing the
resolved-argument loop, validate whether args.len() exceeds scheme.vars.len()
before iterating and indexing alias_info.hir_ids[i]. Reuse the existing
UnexpectedGenericArgs diagnostic and return StructInitDef::Error immediately,
while preserving normal substitution and resolution for valid argument counts.
- Around line 1378-1387: Update the omitted-argument loop in the type-checking
pass to insert each newly created target generic variable into subst using its
corresponding declared variable before resolving that parameter’s default type.
Ensure later defaults resolve through these fresh variables, including chained
defaults such as U = T and V = U, and add a regression test covering a partial
target-generic alias.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 37410330-e50c-4081-98fa-3328db6ff986
📒 Files selected for processing (6)
src/typeck/passes/check/mod.rstests/integration/generics/default_complete_chain.oxitests/integration/generics/default_complete_chain_error.oxitests/integration/type_alias/struct_init_bare_alias_chain.oxitests/integration/type_alias/struct_init_default_chain.oxitests/integration/type_alias/struct_init_default_chain_error.oxi
|
@coderabbitai review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/typeck/passes/check/mod.rs (1)
1374-1394: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate target generic arity before indexing
info.hir_ids.An alias RHS can contain more generic arguments than
struct_defdeclares. The loop at Line 1383 indexesinfo.hir_ids[i]before any target-arity validation. This can panic the compiler instead of emittingUnexpectedGenericArgs.Add an
args.len() > info.hir_ids.len()check before the first loop. Emit the existing diagnostic and returnStructInitDef::Error.Proposed fix
if let Some(info) = &struct_info { + if args.len() > info.hir_ids.len() { + emit_unexpected_generic_args( + self.typeck.ctx, + span, + self.module_id, + info.hir_ids.len(), + args.len(), + ); + return StructInitDef::Error; + } let mut subst: FxHashMap<TyVarId, Ty> = FxHashMap::default();The conversion path in
src/typeck/types.rs:548-550converts each HIR generic argument before this branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 1374 - 1394, Validate target generic arity in the Ty::Adt branch before the loop that indexes info.hir_ids: when args.len() exceeds info.hir_ids.len(), emit the existing UnexpectedGenericArgs diagnostic and return StructInitDef::Error. Keep normal substitution and inference unchanged for valid arities.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/typeck/passes/check/mod.rs`:
- Around line 1374-1394: Validate target generic arity in the Ty::Adt branch
before the loop that indexes info.hir_ids: when args.len() exceeds
info.hir_ids.len(), emit the existing UnexpectedGenericArgs diagnostic and
return StructInitDef::Error. Keep normal substitution and inference unchanged
for valid arities.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a67be62-bfae-444e-8099-0bb22d7805f9
📒 Files selected for processing (3)
src/typeck/passes/check/mod.rstests/integration/type_alias/struct_init_partial_target_chain.oxitests/integration/type_alias/struct_init_partial_target_chain_error.oxi
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/typeck/passes/check/mod.rs (3)
595-601: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMatch trait generic arguments before using
current_assoc_types.This fast path checks
current_self_tybut ignorestrait_generic_args. For an implementation such asimpl Trait<u32> for Foo, a projection such as<Self as Trait<bool>>::Assochas the same receiver and can resolve to theTrait<u32>associated type. The fallback beginning at Line 608 performs the trait-argument match, but this branch bypasses it. Store or recover the current implementation's trait arguments before usingcurrent_assoc_types, or restrict this fast path to projections whose trait arguments are known to match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 595 - 601, Update the fast path constructing resolved in the current_self_ty/current_assoc_types branch to also verify that the projection’s trait_generic_args match the current implementation’s trait arguments before retrieving the associated type. Recover or store those implementation arguments as needed, and otherwise fall through to the existing argument-aware resolution beginning at the fallback path.
1013-1051: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize type aliases before accepting a qpath receiver.
After
icx.resolve,basecan still beTy::Alias. The function then returnsNonebecause it does not callnormalize_type_alias, so receiver lookup through a type alias cannot find methods or associated items.check_member_accessalready normalizes the analogous receiver at Line 1824.Proposed fix
- let base = self.typeck.icx.resolve(&base); + let base = self.typeck.icx.resolve(&base); + let base = self.typeck.normalize_type_alias(&base); if matches!(base, Ty::Adt(_, _)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 1013 - 1051, After resolving base in the qpath receiver flow, normalize it with the existing type-alias normalization helper before continuing receiver lookup. Update the logic around the base computation in the enclosing method, matching check_member_access’s existing normalization behavior so Ty::Alias receivers can resolve methods and associated items.
1363-1368: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReport alias-normalization errors before returning
StructInitDef::Error.
normalize_aliasesreturnsTyFromHirError, but thelet Ok(body) = ... elsebranch discards it.check_struct_initthen returnsTy::Errorwithout emitting a diagnostic. Invalid projections or aliases used in a struct initializer can therefore fail silently.Proposed fix
- let Ok(body) = self.normalize_aliases(body, span) else { - return StructInitDef::Error; - }; + let body = match self.normalize_aliases(body, span) { + Ok(body) => body, + Err(err) => { + self.report_ty_from_hir_error(err); + return StructInitDef::Error; + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typeck/passes/check/mod.rs` around lines 1363 - 1368, Update the normalize_aliases handling in check_struct_init to retain the Err(TyFromHirError) value and report it through the existing type-checking diagnostic path before returning StructInitDef::Error. Preserve the current successful normalization flow and error return behavior after emitting the diagnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/typeck/passes/check/mod.rs`:
- Around line 595-601: Update the fast path constructing resolved in the
current_self_ty/current_assoc_types branch to also verify that the projection’s
trait_generic_args match the current implementation’s trait arguments before
retrieving the associated type. Recover or store those implementation arguments
as needed, and otherwise fall through to the existing argument-aware resolution
beginning at the fallback path.
- Around line 1013-1051: After resolving base in the qpath receiver flow,
normalize it with the existing type-alias normalization helper before continuing
receiver lookup. Update the logic around the base computation in the enclosing
method, matching check_member_access’s existing normalization behavior so
Ty::Alias receivers can resolve methods and associated items.
- Around line 1363-1368: Update the normalize_aliases handling in
check_struct_init to retain the Err(TyFromHirError) value and report it through
the existing type-checking diagnostic path before returning
StructInitDef::Error. Preserve the current successful normalization flow and
error return behavior after emitting the diagnostic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7c5c10ab-6e80-4675-a628-a881c33454d6
📒 Files selected for processing (1)
src/typeck/passes/check/mod.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Documentation
Tests