Skip to content

[Merged by Bors] - chore: bump to nightly-2023-08-17#6019

Closed
kim-em wants to merge 4 commits intomasterfrom
simp_failIfUnchanged
Closed

[Merged by Bors] - chore: bump to nightly-2023-08-17#6019
kim-em wants to merge 4 commits intomasterfrom
simp_failIfUnchanged

Conversation

@kim-em
Copy link
Copy Markdown
Contributor

@kim-em kim-em commented Jul 20, 2023

@kim-em kim-em added blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) awaiting-review blocked-by-core-PR This PR depends on a PR to Core/Std awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. labels Jul 20, 2023
@ghost ghost removed the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Jul 20, 2023
@ghost ghost added merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) labels Jul 27, 2023
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 5, 2023
@github-actions github-actions bot removed the awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. label Aug 6, 2023
@ghost ghost added merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) labels Aug 7, 2023
@kim-em kim-em changed the title chore: adapt to simp failing if it makes no progress chore: bump to nightly-2023-08-17 Aug 17, 2023
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 17, 2023
@kim-em kim-em mentioned this pull request Aug 17, 2023
1 task
@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 17, 2023
Copy link
Copy Markdown
Contributor

@joneugster joneugster left a comment

Choose a reason for hiding this comment

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

Looked through all the files, basically everything is just removing superfluous simp-calls, except one deletion marked below.

In my optinion any_goals simp would be better than _ <;> (try simp) or all_goals (try simp) and it's a bit a shame that <;> simp does not work like any_goals simp.

One thing I did not check is if this PR really modifies all tactics that rely on simp making no progress.

@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 17, 2023
@kim-em kim-em removed the blocked-by-core-PR This PR depends on a PR to Core/Std label Aug 17, 2023
@kim-em kim-em force-pushed the simp_failIfUnchanged branch from 9a539d8 to 6b5c3f9 Compare August 18, 2023 01:37
| `(tactic| whisker_simps) => do
evalTactic (← `(tactic|
simp only [Category.assoc, Bicategory.comp_whiskerLeft, Bicategory.id_whiskerLeft,
simp (config := {failIfUnchanged := false}) only [Category.assoc,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add an explanation of why failIfUnchanged is helpful here? Should whisker_simps take it's own config argument to allow the caller to customize this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It hadn't even occurred to me this counted as a user facing tactic. Mostly it is just used as a component in coherence. But sure, I'll give it a simp configuration.

Copy link
Copy Markdown
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

bors d+

@bors
Copy link
Copy Markdown

bors bot commented Aug 18, 2023

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

@ghost ghost added delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). and removed awaiting-review labels Aug 18, 2023
@kim-em
Copy link
Copy Markdown
Contributor Author

kim-em commented Aug 18, 2023

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Aug 18, 2023
bors bot pushed a commit that referenced this pull request Aug 18, 2023
The major change here is adapting to `simp` failing if it makes no progress.
The vast majority of the redundant `simp`s found due to this change were extracted to #6632.



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@bors
Copy link
Copy Markdown

bors bot commented Aug 18, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title chore: bump to nightly-2023-08-17 [Merged by Bors] - chore: bump to nightly-2023-08-17 Aug 18, 2023
@bors bors bot closed this Aug 18, 2023
@bors bors bot deleted the simp_failIfUnchanged branch August 18, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). ready-to-merge This PR has been sent to bors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants