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(widget): pretty printing restricted quantifiers #5440

Closed
wants to merge 18 commits into from

Conversation

EdAyers
Copy link
Member

@EdAyers EdAyers commented Dec 19, 2020

Eg converts ∀ (x : ℕ), x > 3 → ∃ (N : set ℕ), ∀ y, y ∈ N → x > 2
to ∀ (x > 3), ∃ (N : set ℕ), ∀ (y ∈ N), x > 2
in the widget view.

image


Eg converts `∀ (x : ℕ), x > 3 → ∃ (N : set ℕ), ∀ y, y ∈ N → x > 2`
to `∀ (x > 3), ∃ (N : set ℕ), ∀ (y ∈ N), x > 2`
in the widget view.

Things to do:
- testing
- include support for other binders, not just ∀
- docstrings
@EdAyers
Copy link
Member Author

EdAyers commented Dec 19, 2020

I probably won't have time to work on this for a while so if anyone wants to pick it up; go nuts.

@robertylewis
Copy link
Member

I'm unsure about this for two reasons. One, it suppresses information (the type of x) that's not necessarily obvious. Two, if this is how we want to display these quantifiers, it should probably be changed in the pretty printer, right? With an option to disable it?

@EdAyers
Copy link
Member Author

EdAyers commented Dec 20, 2020

Yeah @robertylewis I think these are valid concerns. The context of this is that @PatrickMassot noted that beginners find the non-compacted notation that the pretty printer generates to be confusing and its a sticking point for learning Lean. Perhaps you can weigh in Patrick?

In terms of not knowing the type, you can still inspect the expression in the widget view and see the type, although it is not immediate from a glance.

I agree that this should really belong in Lean's pretty printer C++ code. But this was just way simpler to implement in Lean as a post-processing step. There are already a few pretty-printer post-processing steps in use in the file: eg the lines above 303 in interactive_expr.lean. We can add an option to disable.

@PatrickMassot
Copy link
Member

Yes, I think this would be a huge improvement for teaching, especially when existentials quantifiers will be handled, getting rid of the infamous ∃ (δ : ℝ) (H : δ > 0), .... I'm not worried about hiding information since the information is still accessible in the widget in the very rare cases where it could be needed. Of course it would be even nicer if there was a way to opt out, and even nicer if there was a way to do this more efficiently from C++. However it would also be very nice to have a clearer path for people who want to use widgets to implement custom pretty-printing. There could be many more tricks, although the issue tackled in this PR is, from my experience, the most pressing one.

@PatrickMassot
Copy link
Member

Any news here? @EdAyers

@EdAyers EdAyers marked this pull request as ready for review January 16, 2021 21:51
@bryangingechen bryangingechen added the awaiting-review The author would like community review of the PR label Jan 17, 2021
src/tactic/interactive_expr.lean Outdated Show resolved Hide resolved
src/tactic/interactive_expr.lean Show resolved Hide resolved
meta def sf.replace {m} [monad m] [alternative m] (f : sf → m sf) : sf → m sf
| x := (f x >>= sf.traverse sf.replace) <|> pure x

/-- The test for whether the proposition is a valid target for restricted quantifier collapsing.
Copy link
Member

Choose a reason for hiding this comment

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

Aren't the valid targets "everything with infix notation"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a programmatic way to test whether a given relation has an infix notation?

Copy link
Member

Choose a reason for hiding this comment

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

Not that I'm aware of. You could build a dummy application and see if it pretty prints differently than you expect, which is very hackish.

src/tactic/interactive_expr.lean Outdated Show resolved Hide resolved
@robertylewis robertylewis added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Jan 17, 2021
For @PatrickMassot 👁‿👁
I do this by refactoring the sf handling code in to its own method that
returns the sf tree with the new look.
@EdAyers EdAyers added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jan 24, 2021
@PatrickMassot
Copy link
Member

Ed, could you update the TODO list in the PR description?
Is there a test somewhere that ensures we won't forget to update this code when the addressing bug is fixed in Lean?

bors bot pushed a commit to leanprover-community/lean that referenced this pull request Jan 25, 2021
The pretty printer was getting the addresses for a special notation binder wrong because of a bug in pp_binders.

See also leanprover-community/mathlib#5440
@github-actions github-actions bot added blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Jan 26, 2021
@EdAyers
Copy link
Member Author

EdAyers commented Jan 28, 2021

Ready for review

@robertylewis
Copy link
Member

I'm still not really thrilled about this change.

My first worry is debatable, I know, but I feel like bundling this in the goal view really does obscure things in the logic and type theory. Maybe it helps for exists, but:

example : ∀ x : ℤ, x > 0 → true :=
begin 
  -- ∀ (x > 0), true
end 

In this example I don't see the type of x or the fact that I need to do two intros. It makes you totally dependent on clicking on widgets and "try intro, see what happens."

Worse, because this is implemented as widget post-processing, it doesn't interact well with pp options. This is ugly and doesn't round-trip:

set_option pp.notation false 

example : ∀ x : ℤ, x > 0 → true :=
begin 
  -- ∀ (gt x 0), true
end 

and pp.all makes it worse.

At a bare minimum, I think this feature needs an option to disable it. I'd prefer it disabled by default. Even better, I'd prefer it implemented as part of the pretty-printer so it interacted right with pp options.

@robertylewis robertylewis added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Feb 2, 2021
@PatrickMassot
Copy link
Member

I still think this is a great contribution, but of course it would be better to have an option controlling this. I think the pp.notation false issue is definitely not an argument for turning this PR off by default, since the issue shows off only when opting out of notations. The fact that some types are hidden is slightly more serious, but I think the PR still makes most situations less confusing and you have a clear path to investigate types if needed. So my favorite solution would be to provide a way to turn off this PR, but still have it on by default.

Note that in my course I ship everything with this PR included (not in mathlib, in my project).

@EdAyers
Copy link
Member Author

EdAyers commented Feb 14, 2021

To address some of Rob's concerns;

There are other existing cases where the pp system makes the types of expressions ambiguous. I agree that this one is particularly bad because the information it is hiding is something that the user could really do with knowing. There is not going to be a non-clunky way to reveal this information without being worse than the original uncollapsed notation. The community are going to have to make an aesthetic judgement call.

I agree that turning off pp.notation makes this ugly, hopefully it is possible for the tactic state to know whether pp.notation is set and turn off this feature in that case.

I agree it needs a disabling option.

In the small amount of Lean teaching that I have done I can concur with Patrick that students stumble on these non-collapsed quantifiers. So I do think that this addition has some merit for teaching purposes even at the expense of being bloat for the power users. Perhaps the tradeoff is not worth it, I am genuinely not sure.

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 6, 2021
@github-actions
Copy link

This PR/issue depends on:

@YaelDillies
Copy link
Collaborator

Lean 4 handles nested binders completely differently, so this will have to be redone mostly from scratch. To be revisited after the port.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes merge-conflict Please `git merge origin/master` then a bot will remove this label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants