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

[Merged by Bors] - chore(data/set/basic): use decidable_pred (∈ s) instead of decidable_pred s. #8211

Closed
wants to merge 8 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 6, 2021

The latter exploits the fact that sets are functions to Prop, and is an annoying form as lemmas are never stated about it.

In future we should consider removing the set.decidable_mem instance which encourages this misuse.

Making this change reveals a collection of pointless decidable arguments requiring that finset membership be decidable; something which is always true anyway.

Two proofs in data/pequiv caused a crash inside the C++ portion of the finish tactic; it was easier to just write the simple proofs manually than to debug the C++ code.


Open in Gitpod

…le_pred s`.

The latter exploits the fact that sets are functions to Prop, but this form is annoying as lemmas are never stated about it.

In future we should consider removing the `set.decidable_mem` instance which encourages this misuse.
@gebner
Copy link
Member

gebner commented Jul 6, 2021

this form is annoying as lemmas are never stated about it.

I've often wondered whether it would be better to make set a a structure instead for this reason.

@eric-wieser
Copy link
Member Author

eric-wieser commented Jul 6, 2021

It looks like there's a bug either in the lean code for finish, or more alarmingly possible inside the c++ code I assume is responsible for smt / ematching.

Edit: yep, the error originates from smt_state.mk:

https://github.com/leanprover-community/lean/blob/65ad4ffdb3abac75be748554e3cbe990fb1c6500/src/library/tactic/smt/smt_state.cpp#L500-L504

@eric-wieser
Copy link
Member Author

eric-wieser commented Jul 6, 2021

I've often wondered whether it would be better to make set a a structure instead for this reason.

The downside of making set a structure is that {x | x ∈ s} = s (aka set.of set.mem) stops being true by rfl. (∈ set_of p) = p would at least still be true by rfl.

@gebner
Copy link
Member

gebner commented Jul 6, 2021

more alarmingly possible inside the c++ code I assume is responsible for smt / ematching.

This is neither alarming nor surprising. The SMT codebase hasn't been tested a lot, and finish is always a good way to make Lean segfault. See e.g. leanprover-community/lean#575

@eric-wieser
Copy link
Member Author

Do you think I should just eliminate the finish tactics here then?

@gebner
Copy link
Member

gebner commented Jul 6, 2021

Do you think I should just eliminate the finish tactics here then?

That's probably the easiest solution.

@eric-wieser eric-wieser marked this pull request as ready for review July 6, 2021 17:35
@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Jul 6, 2021
@eric-wieser
Copy link
Member Author

Do you think I should just eliminate the finish tactics here then?

That's probably the easiest solution.

Done

@sgouezel
Copy link
Collaborator

sgouezel commented Jul 9, 2021

bors r+
Thanks!

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Jul 9, 2021
bors bot pushed a commit that referenced this pull request Jul 9, 2021
…le_pred s`. (#8211)

The latter exploits the fact that sets are functions to Prop, and is an annoying form as lemmas are never stated about it.

In future we should consider removing the `set.decidable_mem` instance which encourages this misuse.

Making this change reveals a collection of pointless decidable arguments requiring that finset membership be decidable; something which is always true anyway.

Two proofs in `data/pequiv` caused a crash inside the C++ portion of the `finish` tactic; it was easier to just write the simple proofs manually than to debug the C++ code.
@bors
Copy link

bors bot commented Jul 9, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(data/set/basic): use decidable_pred (∈ s) instead of decidable_pred s. [Merged by Bors] - chore(data/set/basic): use decidable_pred (∈ s) instead of decidable_pred s. Jul 9, 2021
@bors bors bot closed this Jul 9, 2021
@bors bors bot deleted the eric-wieser/decidable-s branch July 9, 2021 15:14
bors bot pushed a commit that referenced this pull request Jul 11, 2021
The only purpose of this instance was to enable the spelling `[decidable_pred s]` when what is actually needed is `decidable_pred (∈ s)`, which is a bad idea.

This is a follow-up to #8211.
Only two proofs needed this instance, and both were using completely the wrong lemmas anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants