Skip to content

feat(Tactic): add inferInstanceAs% to fix type leakage#35950

Open
kim-em wants to merge 8 commits intomasterfrom
inferInstancePercent
Open

feat(Tactic): add inferInstanceAs% to fix type leakage#35950
kim-em wants to merge 8 commits intomasterfrom
inferInstancePercent

Conversation

@kim-em
Copy link
Contributor

@kim-em kim-em commented Mar 2, 2026

This PR adds inferInstanceAs%, a drop-in replacement for inferInstanceAs that prevents "type leakage" in synthesized instances.

When inferInstanceAs (SomeClass A) is used to define SomeClass B (where B is a non-reducible alias for A), the synthesized instance may contain lambda binder domains (and other sub-expressions) referring to A or deeper unfoldings instead of B. This is invisible at default transparency but causes isDefEq failures at reducibleAndInstances transparency — which is the level used by grind's checkInst.

inferInstanceAs% fixes this by recursively normalizing the constructor tree: it WHNFs to expose the constructor, patches carrier type parameters via isDefEq matching against the unfolding chain, recursively processes instance-implicit fields, and replaces lambda binder domains in function fields.

As a demonstration, this fixes the grind failure in FiniteResidueField that was worked around with #adaptation_note and a manual proof on nightly-testing.

🤖 Prepared with Claude Code

@github-actions
Copy link

github-actions bot commented Mar 2, 2026

PR summary ba37a5c6ee

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference
3 files Mathlib.Algebra.Order.Ring.StandardPart Mathlib.Analysis.Real.Hyperreal Mathlib.Tactic
1
Mathlib.Tactic.InferInstanceAsPercent (new file) 50

Declarations diff

+ B
+ MyInv
+ MyNat
+ TestDivInvMonoid
+ TestField
+ TestInv
+ TestNat
+ addUnfoldings
+ buildReplacements
+ getCtorApp?
+ getFieldInfo
+ instance : Field (FiniteResidueField K)
+ instance : MyInv Nat
+ instance : SomeClass B := inferInstanceAs (SomeClass A)
+ instance : TestField Nat
+ matchesAnyDefeq
+ myNatInv_fixed
+ myNatInv_leaky
+ replaceCarriersInType
+ testField_direct
+ testField_fixed
+ testField_fixed'
+ testField_leaky
+ unfoldChain

You can run this locally as follows
## summary with just the declaration names:
./scripts/pr_summary/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/pr_summary/declarations_diff.sh long <optional_commit>

The doc-module for scripts/pr_summary/declarations_diff.sh contains some details about this script.


Decrease in tech debt: (relative, absolute) = (1.00, 0.01)
Current number Change Type
132 -1 adaptation notes

Current commit 4c91a86a72
Reference commit ba37a5c6ee

You can run this locally as

./scripts/reporting/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@astrainfinita
Copy link
Collaborator

Should we make an elaborator for normalizeInstance? This would also be a better version of #13795.

Copy link
Contributor

@JovanGerb JovanGerb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some small comments, but I do have one high level criticism. This function will unfold the instance all the way down to its constructors when possible. For some instances this will give some very big terms. It is typically better to use pre-existing instances of sub-structures in place of those sub-structures. This is also what fast_instance% does. So I can imagine that we will see some slowdowns as a result of this.

@astrainfinita
Copy link
Collaborator

So I can imagine that we will see some slowdowns as a result of this.

Using fast_instance% and normalizeInstance appropriately can provide further acceleration (see my PR above) rather than a slowdown. You can always use fast_instance% after normalizeInstance.

@JovanGerb
Copy link
Contributor

You're right, and we can use fast_instance% after inferInstanceAs%

@kim-em
Copy link
Contributor Author

kim-em commented Mar 3, 2026

I've left some small comments, but I do have one high level criticism. This function will unfold the instance all the way down to its constructors when possible. For some instances this will give some very big terms. It is typically better to use pre-existing instances of sub-structures in place of those sub-structures. This is also what fast_instance% does. So I can imagine that we will see some slowdowns as a result of this.

I've changed behaviour: when we hit sub-structures, we try to decide if they are "leaky". If so, we emit a (suppresible) warning, hopefully encouraging the user to go fix those first.

@kim-em kim-em marked this pull request as ready for review March 3, 2026 04:52
kim-em and others added 8 commits March 3, 2026 05:13
…ed instances

`inferInstanceAs%` is a drop-in replacement for `inferInstanceAs` that
prevents "type leakage" in synthesized instances. When `inferInstanceAs`
synthesizes an instance for a type alias, the resulting expression may
contain lambda binder domains referring to unfolded forms of the carrier
type rather than the declared alias. This is invisible at `default`
transparency but causes `isDefEq` failures at `reducibleAndInstances`
transparency — which is the level used by `grind`'s `checkInst`.

The fix recursively normalizes the constructor tree: for each
class-valued structure, it WHNFs to expose the constructor, patches the
carrier type parameter via `isDefEq` matching, recursively processes
instance-implicit fields, and replaces lambda binder domains in function
fields.

This fixes the `grind` failure in `FiniteResidueField` that was
previously worked around with an `#adaptation_note` and manual proof.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactor the implementation:
- Make `unfoldChain` iterative with built-in dedup and `skipHead` param
- Extract `addUnfoldings` and `buildReplacements` from inline logic
- Split `normalizeInstance` into `getCtorApp?`, `getFieldInfo`, and
  `normalizeCtorArgs` helpers (mutual block for recursion)

Add deeper test hierarchy (TestInv → TestDivInvMonoid → TestField)
reproducing the real grind failure pattern, with declarative examples
showing defeq at default transparency but not at `instances` transparency.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use parallel `for sArg in sourceArgs, eArg in expectedArgs` (#2)
- Use `repeat do` instead of fuel-bounded loop (#3)
- Use modern range syntax `*...n` and `n...m` (#9, #10)
- Use `fieldInfo[i]?` instead of bounds check (#11)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use `.getD ty` instead of matching on `matchesAnyDefeq` result (#5)
- Remove unnecessary `withDefault` on `isProp` (#6)
- Move `mkAppN` into `normalizeCtorArgs`, returning `Expr` directly (#12)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove extra leading spaces on continuation lines of docstrings
to match mathlib convention (#8).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ard getAppFn

Throw an error when source and expected types fail to unify, and
check that the class heads match before comparing arguments pairwise (#1).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All code is `meta`, so `partial` is fine for termination. Remove the
unnecessary fuel parameter from `normalizeCtorArgs` and `normalizeInstance`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…add option

When `inferInstanceAs%` patches an instance, it now tries to synthesize
sub-instances for the target carrier type. If a synthesized sub-instance
is not defeq to the patched version at `reducibleAndInstances` transparency,
a warning is emitted suggesting the user define it separately.

The warning can be disabled with
`set_option inferInstanceAsPercent.leakySubInstWarning false`.

Also regenerates import files via `lake exe mk_all`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kim-em kim-em force-pushed the inferInstancePercent branch from 77c1035 to 4c91a86 Compare March 3, 2026 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants