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] - fix(tactic/induction): fix multiple cases'/induction' bugs #7717

Closed
wants to merge 1 commit into from

Conversation

JLimperg
Copy link
Collaborator

  • Fix generalisation in the presence of frozen local instances.

    Any time we revert a potentially frozen hypothesis, we now unfreeze local
    hypotheses during the operation. This makes sure that generalisation works
    uniformly whether or not any local instances are frozen.

  • Treat local defs as fixed during auto-generalisation

    induction' gets confused if we generalise over local definitions since they
    turn into lets when reverted. Ideally, we would handle local defs
    transparently, but that would require a lot of new code. So instead, we at
    least stop auto-generalisation from generalising them (and their
    dependencies).

  • Handle infinitely branching types

    induction' and cases' previously did not acknowledge the existence of
    infinitely branching types at all, leading to various internal errors.

New test cases for all these bugs, due to Patrick Massot, were added to the test
suite.


Open in Gitpod

@JLimperg JLimperg added awaiting-review The author would like community review of the PR t-meta Tactics, attributes or user commands labels May 26, 2021
Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Thanks! My only complaint is that it was hard to figure out which changes correspond to which fix. However, the tests make me confident enough that they work, even ignoring my confidence in your coding skills :)

bors d+

src/tactic/induction.lean Outdated Show resolved Hide resolved
@bors
Copy link

bors bot commented Jun 8, 2021

✌️ JLimperg can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Jun 8, 2021
@JLimperg
Copy link
Collaborator Author

JLimperg commented Jun 8, 2021

Thanks! Sorry for being too lazy to disentangle the various changes.

bors r+

@Vierkantor
Copy link
Collaborator

I wonder why Bors didn't notice your comment, maybe the Fastly outage?

bors r+

@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+`.) merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Jun 11, 2021
@sgouezel
Copy link
Collaborator

bors r-
Can you merge master, fix the conflict, and then set it back to "ready to merge"?
bors d+

@bors
Copy link

bors bot commented Jun 12, 2021

✌️ JLimperg can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@bors
Copy link

bors bot commented Jun 12, 2021

Canceled.

@bryangingechen bryangingechen added awaiting-author A reviewer has asked the author a question or requested changes and removed ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) labels Jun 12, 2021
* Fix generalisation in the presence of frozen local instances.

  Any time we revert a potentially frozen hypothesis, we now unfreeze local
  hypotheses during the operation. This makes sure that generalisation works
  uniformly whether or not any local instances are frozen.

* Treat local defs as fixed during auto-generalisation

  induction' gets confused if we generalise over local definitions since they
  turn into lets when reverted. Ideally, we would handle local defs
  transparently, but that would require a lot of new code. So instead, we at
  least stop auto-generalisation from generalising them (and their
  dependencies).

* Handle infinitely branching types

  induction' and cases' previously did not acknowledge the existence of
  infinitely branching types at all, leading to various internal errors.

New test cases for all these bugs, due to Patrick Massot, were added to the test
suite.
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Jun 14, 2021
@JLimperg
Copy link
Collaborator Author

Thanks Anne for prodding Bors, and Sebastien for prodding me.

bors r+

@JLimperg JLimperg removed the awaiting-author A reviewer has asked the author a question or requested changes label Jun 14, 2021
bors bot pushed a commit that referenced this pull request Jun 14, 2021
* Fix generalisation in the presence of frozen local instances.

  Any time we revert a potentially frozen hypothesis, we now unfreeze local
  hypotheses during the operation. This makes sure that generalisation works
  uniformly whether or not any local instances are frozen.

* Treat local defs as fixed during auto-generalisation

  induction' gets confused if we generalise over local definitions since they
  turn into lets when reverted. Ideally, we would handle local defs
  transparently, but that would require a lot of new code. So instead, we at
  least stop auto-generalisation from generalising them (and their
  dependencies).

* Handle infinitely branching types

  induction' and cases' previously did not acknowledge the existence of
  infinitely branching types at all, leading to various internal errors.

New test cases for all these bugs, due to Patrick Massot, were added to the test
suite.
@bors
Copy link

bors bot commented Jun 15, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(tactic/induction): fix multiple cases'/induction' bugs [Merged by Bors] - fix(tactic/induction): fix multiple cases'/induction' bugs Jun 15, 2021
@bors bors bot closed this Jun 15, 2021
@bors bors bot deleted the induction-bugs branch June 15, 2021 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions. t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants