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

fix: display deprecation warning for simp lemmas on the lemma name #4448

Closed
wants to merge 9 commits into from

Conversation

joneugster
Copy link
Contributor

@joneugster joneugster commented Jun 13, 2024

Closes #4452

@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 Jun 13, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Jun 13, 2024

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase e33c32fb00e7b2a01c6a7f186c18a7c97f54a635 --onto bedcbfcfeed429428c3e7f008b6984fc8c2b2b76. (2024-06-13 20:37:40)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase e33c32fb00e7b2a01c6a7f186c18a7c97f54a635 --onto f237fb67ebe344b7e26f67cf7c35b1a69dcefdc9. (2024-06-17 10:27:13)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase e33c32fb00e7b2a01c6a7f186c18a7c97f54a635 --onto 1cf71e54cf268e87cf5c43c830d953f5c88e6c39. (2024-06-17 12:16:03)

@@ -204,7 +204,7 @@ def elabSimpArgs (stx : Syntax) (ctx : Simp.Context) (simprocs : Simp.SimprocsAr
arg[0][0].getKind == ``Parser.Tactic.simpPost
let inv := !arg[1].isNone
let term := arg[2]
match (← resolveSimpIdTheorem? term) with
match (← withRef arg do resolveSimpIdTheorem? term) with
Copy link
Member

@tydeu tydeu Jun 13, 2024

Choose a reason for hiding this comment

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

Suggested change
match (← withRef arg do resolveSimpIdTheorem? term) with
match (← withRef term do resolveSimpIdTheorem? term) with

It seems more reasonable to use the term itself rather than the whole arg (which potentially includes the inversion arrow). Alternatively, the withRef could be integrated into resolveSimpIdTheorem? itself, which might be even better.

Copy link
Contributor Author

@joneugster joneugster Jun 17, 2024

Choose a reason for hiding this comment

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

Overlooked the second suggestion, but just addressed that now, see below.

@semorrison semorrison added missing tests This PR requires the addition of tests before it can be merged. awaiting-author Waiting for PR author to address issues labels Jun 14, 2024
@adomani
Copy link

adomani commented Jun 14, 2024

In the Zulip chat it was suggested to add a test. Would this (updated to the correct message) work?

import Lean.Elab.Command

@[deprecated]
theorem hi : True := .intro

open Lean Elab.Command
elab "msgs_lines " cmd:command : command => do
  elabCommand cmd
  let msgs := (← get).messages.unreported.toList
  if let [m] := msgs then logInfo m!"{(m.pos, m.endPos)}"

/--
warning: `hi` has been deprecated
---
info: (⟨20, 2⟩, some (⟨22, 3⟩))
-/
#guard_msgs in
msgs_lines
example : True := by
  simp [
    hi
  ]

@semorrison
Copy link
Collaborator

In the Zulip chat it was suggested to add a test. Would this (updated to the correct message) work?

Haha, I like it. The traditional way is to do it like the tests in tests/lean/interactive/.

@joneugster
Copy link
Contributor Author

joneugster commented Jun 14, 2024

The traditional way is to do it like the tests in tests/lean/interactive/.

@semorrison I did add a test to the best of my knowledge (as 4448.lean and 4448.lean.expected.out), not following tests/lean/interactive/ but other tests in tests/lean/, is this the correct way?

EDIT: I guess the failing CI in af33b4a suggests it does indeed work :D

@joneugster
Copy link
Contributor Author

@semorrison I managed to get the tests correctly :) I don't think I can remove the awaiting-author label, so I assume the right thing is to ping you instead?

@nomeata
Copy link
Contributor

nomeata commented Jun 18, 2024

It seems that this was now fixed by #4484, please re-open if I am confused. Thanks for helping, though!

@nomeata nomeata closed this Jun 18, 2024
@joneugster
Copy link
Contributor Author

Absolutely, thanks for the fix!

@adomani
Copy link

adomani commented Jun 18, 2024

I also tried the web-server and I can still see the old simp-underlining. Would you mind also adding dsimp to the test, just to make sure that the same modifications fix both? Thanks!

@joneugster
Copy link
Contributor Author

I also tried the web-server and I can still see the old simp-underlining.

Did you try the "Nightly" version of the webeditor? (go to settings and change the project) It seems to be as intended:
MWE on web editor nightly

@adomani
Copy link

adomani commented Jun 26, 2024

Ah, no, I did not know that this was possible! I can confirm that on the "nightly", the issue that I had raised is resolved, both for simp and for dsimp!

I am not really able to comment whether this is the "right" solution, but I would be very happy if this behaviour became standard! Thanks!

@adomani
Copy link

adomani commented Jun 26, 2024

Oh, I realize now that you are saying that this already is the behaviour, it just has not trickled down to Mathlib! Anyway, this is great, thanks!

@joneugster
Copy link
Contributor Author

exactly, I assume that means it'll be in v4.10.0 and the corresponding rcs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author Waiting for PR author to address issues missing tests This PR requires the addition of tests before it can be merged. 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.

deprecated produces its message on simp
6 participants