Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: change apply_rfl tactic so that it does not operate on = #3784

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

kim-em
Copy link
Collaborator

@kim-em kim-em commented Mar 27, 2024

Previously:

If the rfl macro was going to fail, it would:

  1. expand to eq_refl, which is implemented by Lean.Elab.Tactic.evalRefl, and call Lean.MVarId.refl which would:
  • either try kernel defeq (if in .default or .all transparency mode)
  • otherwise try IsDefEq
  • then fail.
  1. Next expand to the apply_rfl tactic, which is implemented by Lean.Elab.Tactic.Rfl.evalApplyRfl, and call Lean.MVarId.applyRefl which would look for lemmas labelled @[refl], and unfortunately in Mathlib find Eq.refl, so try applying that (resulting in another IsDefEq)
  2. Because of an accidental duplication, if Lean.Elab.Tactic.Rfl was imported, it would again expand to apply_rfl.

Now:

  1. Same behaviour in eq_refl.
  2. The @[refl] attribute will reject Eq.refl, and MVarId.applyRefl will fail when applied to equality goals.
  3. The duplication has been removed.

@kim-em kim-em requested a review from leodemoura as a code owner March 27, 2024 03:08
@kim-em kim-em requested a review from nomeata March 27, 2024 03:10
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Mar 27, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Mar 27, 2024

Mathlib CI status (docs):

  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase b17c47d8526208db27105eafdf96a0668621c740 --onto 4c0106d757d4d6d3b7f3903611ce22100d539d2a. (2024-03-27 03:25:36)
  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 16fdca1cbd603a05bd173584045adc9c13b38591 --onto b4caee80a3dfc5c9619d88b16c40cc3db90da4e2. (2024-03-27 11:57:04)

@kim-em
Copy link
Collaborator Author

kim-em commented Mar 27, 2024

I guess this should be considered as quicker / hackier alternative to #3714.

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 27, 2024 03:47 Inactive
-/
def _root_.Lean.MVarId.applyRfl (goal : MVarId) : MetaM Unit := do
def _root_.Lean.MVarId.applyRfl (goal : MVarId) (failOnEq : Bool := false) : MetaM Unit := do
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a use case for failOnEq := false?

It seems we have

  • End users calling rfl. They don’t need it.
  • Tactics or Meta use that wants to work on Eq only. They can use eq_refl or with_reducible eq_refl resp. MVarId.refl or withReducible MVarId.refl as suitable.

Who wants to use MVarId.applyRfl, wants to handle more than Eq, but also wants to handle Eq, and can’t go via the rfl tactic?

(Also note that .applyRfl still behaves different than the rfl tactic. All very confusing.)

Preventing users from registering Eq.refl is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I can grep mathlib myself…

There is

def rflTac : TacticM Unit :=
  withMainContext do liftMetaFinishingTactic (·.applyRfl)

which is corresponds to

src/Lean/Elab/Tactic/Rfl.lean:  | `(tactic| apply_rfl) => withMainContext do liftMetaFinishingTactic (·.applyRfl)

in core.

It is used in

attribute [aesop safe tactic (rule_sets := [CategoryTheory])] Mathlib.Tactic.rflTac

Then there is

/-- See if the term is `a = b` and the goal is `a ∼ b` or `b ∼ a`, with `∼` reflexive. -/
@[gcongr_forward] def exactRefl : ForwardExt where
  eval h goal := do
    let m ← mkFreshExprMVar none
    goal.assignIfDefeq (← mkAppOptM ``Eq.subst #[h, m])
    goal.applyRfl

in Mathlib/Tactic/GCongr/Core.lean. I’m not entirely sure, but it seems that here you don’t want to handle also Eq.

There is also

withReducible (← mkFreshExprMVar r.result.eNew).mvarId!.applyRfl

in Mathlib/Tactic/Rewrites.lean. Upstreamed and fixed with #3783.

And finally in rw_search we have

/-- Check whether a goal can be solved by `rfl`, and fill in the `SearchNode.rfl?` field. -/
def compute_rfl? (n : SearchNode) : MetaM SearchNode := withMCtx n.mctx do
  if (← try? n.goal.applyRfl).isSome then

and I wonder if that really wants to use applyRfl, and not .refl instead.

Hyrum’s law is strong again :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this last one does want to use applyRfl, or at least there are test cases that expect it, so they can discharge Iff goals...

So when this PR lands, it will again break rw_search, and I'll patch it up to try .refl and then .applyRfl....

nomeata
nomeata approved these changes Mar 27, 2024
@kim-em kim-em enabled auto-merge March 27, 2024 11:21
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 27, 2024 12:03 Inactive
@kim-em kim-em added this pull request to the merge queue Mar 27, 2024
Merged via the queue into master with commit 02c5700 Mar 27, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants