This comprehensive document consolidates learnings from the failed first
attempt (Oct 21-23) and provides a clear path forward.

Key additions:
- Clear vision for context-rich HIR traversal API
- Correct impl block pattern vs wrong standalone function pattern
- API naming principle: 'Name for what it IS, not the PROCESS' (Gemini)
- Complete accounting of 6 major mistakes from first attempt
- Detailed elimination strategy starting from clean baseline
- Wrapper inventory with complexity assessments

CLAUDE.md updated to reference IMPORTANT_READ_THIS_FIRST_BRIEFING.md

Also includes meeting transcript for reference.

- Restore removal of TraitDef and lower_trait imports
- Change trait_ parameter types from TraitDef to Trait
- Fix .super_traits() calls to use .super_trait_insts()
- Remove redundant .trait_() calls
- Update available_traits() to return Trait instead of TraitDef

All these changes were accidentally reverted by Gemini but have now
been restored. The refactoring is complete with 65/66 tests passing.

One remaining issue: type_error test shows error message regression
where 'type mismatch' is reported instead of 'trait not implemented'.
This will be fixed in next commit based on Gemini's analysis.

When checking binary operators like `i32 + bool`, method selection was
finding `impl Add<i32> for i32`, which passed trait satisfiability
checks, but then failed during RHS type unification with a generic
"type mismatch" error instead of the more specific "trait not
implemented" error.

Fixed by checking RHS type compatibility first in check_ops_trait and
emitting the trait error when unification fails, before calling
check_expr which would emit the generic type mismatch.

Also accept snapshot updates for ambiguous trait method error ordering
changes (cosmetic differences from IndexSet iteration order).

Captures lessons from the operator trait error regression:
- Run targeted tests during refactoring
- Separate type substitutions from semantic changes
- Document non-obvious code before changing it
- Bottom line: understand first, then refactor

Part of FuncDef elimination. Added the following methods to Func:
- arg_tys() - returns lowered argument types wrapped in Binders
- return_ty() - returns lowered return type wrapped in Binder
- param_set() - returns generic parameter set
- receiver_ty() - returns receiver type for methods

These methods move the type lowering logic from the FuncDef wrapper
directly onto the Func HIR item, following the impl block pattern.

Also made TyId::unit() and TyId::tuple() pub(crate) so they can be
called from hir_def module.

Note: EnumVariant support for variant constructors needs more analysis
before implementation - it involves complex ADT type handling that
requires careful study.

Changed TyCheckEnv.func() to return Option<Func> directly instead of
Option<FuncDef>. All call sites were immediately unwrapping to get
the underlying Func via .hir_func_def(), so this simplifies the API.

Removed lower_func import and FuncDef usage from env.rs and stmt.rs.

Tests passing: All 46 UI tests + all other tests passing.

Better name that describes what it represents: a definition of something
callable (either a Func or an EnumVariant constructor).

This is preparation for using CallableDef directly in TyBase instead
of the FuncDef wrapper.

Adds arg_tys(), return_ty(), and param_set() methods to EnumVariant
to support treating variant constructors as callable functions. These
methods mirror the functionality added to Func and enable EnumVariant
to be used directly in CallableDef without requiring FuncDef wrapper.

- arg_tys() returns argument types for tuple variants (None for Unit)
- return_ty() returns the enum type with type parameters applied
- param_set() returns generic parameter set from parent enum

These methods enable the next step of using CallableDef directly in
the type system instead of the FuncDef wrapper.

Major refactoring that removes FuncDef wrapper from the type system.
TyBase::Func now stores CallableDef directly instead of wrapping it in
FuncDef. This simplifies the type representation and aligns with our
goal of eliminating unnecessary wrapper structs.

Key changes:
- Changed TyBase::Func from storing FuncDef to CallableDef
- Added methods to CallableDef: params(), name(), arg_tys(), ret_ty(),
  offset_to_explicit_params_position(), param_label()
- Updated all pattern matches and method calls to work with CallableDef
- Updated Callable struct to use CallableDef instead of FuncDef
- Fixed pretty_print, HasKind impl, and visitor to use CallableDef
- Changed TyId::func() to take CallableDef parameter
- Updated variant constructor logic to use CallableDef directly
- Fixed path resolver to create CallableDef::Func directly

FuncDef still exists for now (used by method selection and other places)
but is no longer in the core type representation.

Remove the intermediate FuncDef wrapper struct that was causing unnecessary
indirection in the type system. All code now uses CallableDef directly,
which is a simple enum wrapping either Func or EnumVariant.

Key changes:
- Removed FuncDef struct and lower_func function entirely
- Updated CallableDef to provide all necessary methods directly
- Fixed method comparison, type checking, and constraint collection
- Added Enum::variant_arg_tys as salsa-tracked method for variant constructors
- EnumVariant::arg_tys delegates to Enum method for proper salsa caching

This eliminates one layer of wrapper indirection while maintaining clean
APIs and proper salsa query tracking throughout the HIR analysis phase.

All tests pass.

Add key learnings from FuncDef wrapper elimination:
- Pattern for decomposing multi-purpose wrappers into enums
- Salsa workaround for non-tracked structs delegating to parent
- Refactoring checklist for wrapper extraction patterns

Apply decompose-then-migrate strategy to AdtDef planning:
- Replace AdtDef with AdtRef enum separating Struct/Contract/Enum
- Migrate simplest case (Struct) first
- Encapsulate complex cases temporarily in enum variants
- Pattern proven successful with FuncDef → CallableDef elimination

Successfully eliminated the Implementor struct and migrated to using
ImplTrait directly throughout the HIR analysis layer. All semantic
information that was previously precomputed and stored in Implementor
is now computed on-demand via salsa-tracked methods on ImplTrait.

Key changes:
- Added semantic methods to ImplTrait: trait_inst(), self_ty(),
  trait_def(), impl_params(), assoc_types(), impl_constraints()
- Implemented TyFoldable for ImplTrait as no-op (HIR IDs don't fold)
- Updated proof_forest, method_selection, path_resolver, and
  def_analysis to use ImplTrait API directly
- Deleted Implementor struct, lower_impl_trait function, and related
  TyFoldable impl
- Removed redundant conflict checking from def_analysis (conflicts are
  already checked during trait impl collection)

Test Status: 89/90 tests passing
- Failing: constraints_standalone__specialized
- Issue: Constrained trait impl not being selected for method calls
- See commit notes for investigation details

The failing test involves a constrained impl:
  impl<T> Trait for S2<T> where S<T>: Trait

When calling s2.f() where s2: S2<i32>, the method isn't found even
though impl Trait for S<i32> exists. The impl is likely being filtered
out during method selection or failing constraint checks.

Changed self_ty() to return TyId instead of Option<TyId>, returning
TyId::invalid() when the impl's type is malformed. This matches the
old Implementor behavior where self_ty was always accessible.

Also removed the use of assumptions when lowering the self type to
avoid potential Salsa cycles (reverted back in follow-up - old code
used assumptions too).

Test status: Still 1/90 tests failing (constraints_standalone__specialized)
The failure persists, indicating the root cause is elsewhere in the
trait resolution logic, not in self_ty() returning None.

CRITICAL FIX: TyParam cannot unify with concrete types - only with identical
TyParams. The old Implementor struct was TyFoldable and created fresh TyVars
when instantiated. But ImplTrait is just a salsa ID (not foldable), so
instantiate_with_fresh_vars was a no-op.

Solution: Manually create fresh type variables for generic params at call sites
and use Binder::instantiate() to substitute them into self_ty and constraints.

Progress: Test error changed from 'method not found' to 'trait bound not
satisfied', proving the impl is now being found. Constraint checking still
fails - likely needs similar fixes in trait resolution code.

Test status: Still 1/90 tests failing (constraints_standalone__specialized)

…tore conflict checking

This commit completes the elimination of the Implementor struct by addressing
two critical issues discovered during testing:

1. **Fresh Variable Instantiation for ImplTrait**
   - Root cause: ImplTrait is a salsa ID (TyFoldable no-op), so
     `table.instantiate_with_fresh_vars(impl_trait)` doesn't create fresh type
     variables. The returned trait_inst() contains TyParams from HIR.
   - TyParam unification limitation: TyParams can only unify with identical
     TyParams, not with concrete types or TyVars (see unify.rs:136-140).
   - Solution: Manually create fresh TyVars at all call sites by:
     1. Collecting generic params with collect_generic_params
     2. Creating fresh vars with table.new_var_from_param
     3. Instantiating with Binder::instantiate()
   - Fixed in: impls_for_trait, impls_for_ty_with_constraints, ProofForest,
     path_resolver.rs (assoc type resolution), does_impl_trait_conflict

2. **Trait Conflict Diagnostic Generation**
   - Root cause: WIP commit removed analyze_conflict_impl function that
     generated conflict diagnostics, leaving only a comment
   - The does_impl_trait_conflict function in trait_lower.rs prevents
     conflicting impls from being added to the impl table (no diagnostic)
   - Solution: Restore analyze_conflict_impl in def_analysis.rs to check each
     impl against the trait environment and generate diagnostics
   - Critical fix: Skip checking impl against itself to avoid false positives
   - Critical fix: Swap primary/conflict_with roles to match expected format

All 90+ tests now pass, completing the Implementor elimination refactoring.

Document the validated hybrid approach that splits pure (cached) and
stateful (instantiation) logic. This strategy:

- Creates internal TraitImplData (salsa::interned, pub(crate))
- Caches expensive lowering work in trait_impl_data query
- Provides atomic instantiation via instantiated() method
- Eliminates manual fresh var management at 6+ call sites
- Maintains clean public API on ImplTrait

This approach prevents the atomicity bug that caused the first
refactoring attempt to fail while leveraging Salsa caching properly.
Aligns with "Option A (Conservative)" from original briefing.

…tantiation

Add TraitImplData (salsa::interned) to cache expensive lowering work and
enable atomic instantiation. This eliminates manual fresh var management.

Changes:
- Add TraitImplData struct with TyFoldable/TyVisitable implementations
- Add ImplTrait::trait_impl_data() cached query for lowering
- Update existing methods (self_ty, trait_inst, impl_params, impl_constraints)
  to delegate to cached query
- Add ImplTrait::instantiated() method for atomic fresh var instantiation
- Add InstantiatedTraitImpl return struct

Next: Update call sites to use new instantiated() API

…te all call sites

This completes the hybrid approach by making instantiated() work with both
InPlace and Persistent unification tables, and eliminates all manual fresh
variable creation patterns at call sites.

Changes:
- Make instantiated() generic over UnificationStore trait to support both
  InPlace and Persistent table types
- Update proof_forest.rs to use instantiated() API (eliminated 15 lines
  of manual fresh var creation)
- Update does_impl_trait_conflict() to use instantiated() API (reduced
  from ~70 lines to ~40 lines)
- Update impls_for_ty() filter to use instantiated() API (eliminated
  10 lines of manual fresh var creation)

All fresh variable creation for ImplTrait is now centralized in the
instantiated() method, ensuring atomic instantiation across all fields
and eliminating the code smell of duplicated patterns.

All tests passing.

This completes the HIR API refactoring for ImplTrait by making semantic
methods public and following the established naming convention.

API Changes:
- Rename field `ty` → `raw_ty` (pub(crate), syntactic HIR field)
- Rename method `self_ty()` → `ty()` (pub, semantic analysis method)
- Make `ty()` and `trait_inst()` public methods
- External code now calls `impl_trait.ty(db)` to get analyzed type

Call Site Improvements:
- path_resolver.rs: Simplified ImplTrait path resolution to use public
  ty() API, removing manual lowering
- def_analysis.rs:335: Reduced from 9 lines to 1 line using ty() API
- Updated 3 other call sites to use raw_ty() for syntactic access
- Updated 4 call sites to use renamed ty() method

This follows the briefing's pattern: syntactic fields are pub(crate),
semantic methods are pub. External consumers use the clean public API,
which encapsulates the complexity of lowering and analysis internally.

All tests passing.