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

doc: revise show tactic documentation to more accurately describe its behavior #7389

Closed
wants to merge 1 commit into from

Conversation

euprunin
Copy link
Contributor

@euprunin euprunin commented Mar 7, 2025

No description provided.

@euprunin euprunin changed the title chore: revise show tactic documentation to more accurately describe its behavior doc: revise show tactic documentation to more accurately describe its behavior Mar 7, 2025
@euprunin euprunin force-pushed the fix-show-documentation branch from a71e805 to 7a31bdc Compare March 7, 2025 22:07
@@ -791,10 +791,11 @@ The `let` tactic is for adding definitions to the local context of the main goal
-/
macro "let " d:letDecl : tactic => `(tactic| refine_lift let $d:letDecl; ?_)
/--
`show t` finds the first goal whose target unifies with `t`. It makes that the main goal,
performs the unification, and replaces the target with the unified version of `t`.
`show t` declares that the current goal is `t`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be expanded a bit; it's unclear from this description if this is just a check with no effect if successful, or if it actually changes the goal (as it does). And maybe somewhere uses the verb “unify” to hint that show _ < _ for example works.

Copy link
Contributor

@Rob23oba Rob23oba Mar 8, 2025

Choose a reason for hiding this comment

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

One question: In its current state, show is basically a weaker version of change, right? I feel like instead of changing the documentation and making it officially just a weaker change, we should rather change show to what it's supposed to do from its documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually don't know, so looking forward to reading about it here :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation of change implies this: "The tactic show e is interchangeable with change e, where the pattern e is applied to the main goal." I'd be happy though to make a PR that fixes show (that would imply adding a new @[builtin_tactic] and thus a stage0 update though)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm a bit lost. What would you fix beyond the docstring?

Copy link
Contributor

Choose a reason for hiding this comment

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

Change show such that it finds the first goal whose target unifies with t, makes that the main goal, performs the unification, and replaces the target with the unified version of t?

Copy link
Contributor

Choose a reason for hiding this comment

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

And in such a way that if it applies to the main goal, it behaves exactly like change

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see, make it behave like in lean3. I'm sympathetic, but given that users didn't mind that much so far, unsure. I'll let those with more expertise in the tactic language design weigh in.

Copy link
Contributor Author

@euprunin euprunin Mar 16, 2025

Choose a reason for hiding this comment

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

I’m not familiar enough with the show tactic to offer a detailed, accurate description beyond what’s already suggested in the original PR. If anyone is interested in taking ownership of this PR or can provide a snippet to include, I would appreciate the help.

@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 7, 2025
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 45806017e570fc17d3182c907a6ffe6c146025c6 --onto ca0d8226192e7c0cdcc71d6322c3226ad4f73f30. You can force Mathlib CI using the force-mathlib-ci label. (2025-03-07 22:33:27)

@nomeata
Copy link
Collaborator

nomeata commented Mar 18, 2025

After discussing this, it seems rather better to adjust the behavior to match the documentation. There is already a draft at #7395.

@nomeata nomeata closed this Mar 18, 2025
@euprunin
Copy link
Contributor Author

Even better! Thanks!

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.

4 participants