Skip to content

Conversation

@ppedrot
Copy link
Member

@ppedrot ppedrot commented Jul 19, 2021

The current behaviour was to indiscriminately try to apply on any goal unfold hints for transparent constants from discriminated databases, since they were not indexed by the head constant. In particular, this might have unfolded the constant even if it was at the head of the goal, contrarily to what the documentation claimed.

I think it was written this way out of fear of the dnet pattern algorithm. In discriminated mode, transparent constants are considered to match everything, but maybe the author of the code was confused by the complexity of the dnet implementation.

This patch changes the semantics of auto tactics in fringe cases, and should be mostly backwards compatible.

Fixes #14874: Exponential blowup with Hint Unfold.

@ppedrot
Copy link
Member Author

ppedrot commented Jul 19, 2021

Here is a minimal example of a fringe case.

Create HintDb foo discriminated.

Definition F (n : nat) := n.

Inductive Pred : nat -> Prop := pred : forall n, Pred (S n).

#[local]
Hint Unfold F : foo.

#[local]
Hint Resolve pred : foo.

Print HintDb foo.

Goal forall n, Pred (F (S n)).
Proof.
intros n.
solve [ auto with foo ]. (* with this PR auto leaves the goal unchanged and this line fails *)
Qed.

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 19, 2021

Hey, I have detected that there were CI failures at commit 3d8c49d without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 1bba7d6 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following targets: ci-hott, ci-math_classes.

@ppedrot
Copy link
Member Author

ppedrot commented Jul 20, 2021

CI shows that only HoTT, math-classes and (by transitivity) corn are broken. It seems quite a small change in the end thus, so maybe it's worth pursueing and writing overlays. @mattam82 what do you think?

@ppedrot ppedrot added the needs: discussion Further discussion is needed. label Jul 20, 2021
@ppedrot
Copy link
Member Author

ppedrot commented Jul 21, 2021

Let's close for the time being, I'll reopen after the deadzone known as summer.

@ppedrot ppedrot closed this Jul 21, 2021
@ppedrot ppedrot reopened this Sep 13, 2021
@ppedrot ppedrot force-pushed the hint-unfold-head-constant-index branch from 3d8c49d to 641dd02 Compare September 13, 2021 08:12
ppedrot added a commit to ppedrot/HoTT that referenced this pull request Sep 13, 2021
ppedrot added a commit to ppedrot/math-classes that referenced this pull request Sep 13, 2021
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Sep 13, 2021

Hey, I have detected that there were CI failures at commit 641dd02 without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 89818ca succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following targets: ci-hott, ci-math_classes.

1 similar comment
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Sep 13, 2021

Hey, I have detected that there were CI failures at commit 641dd02 without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 89818ca succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following targets: ci-hott, ci-math_classes.

@ppedrot ppedrot force-pushed the hint-unfold-head-constant-index branch from 641dd02 to 6e2ce47 Compare September 13, 2021 11:39
@ppedrot ppedrot removed the needs: discussion Further discussion is needed. label Sep 13, 2021
@ppedrot ppedrot marked this pull request as ready for review September 13, 2021 11:41
@ppedrot ppedrot requested a review from a team as a code owner September 13, 2021 11:41
@ppedrot
Copy link
Member Author

ppedrot commented Sep 13, 2021

Why are there CI failures in ml-api and refman? In the first case I don't see how it's related to this PR and in the second I don't even see any outstanding error.

ppedrot added a commit to ppedrot/math-classes that referenced this pull request Sep 14, 2021
@ppedrot ppedrot force-pushed the hint-unfold-head-constant-index branch from 6e2ce47 to 2486663 Compare September 14, 2021 09:56
spitters added a commit to rocq-community/math-classes that referenced this pull request Sep 17, 2021
@ppedrot ppedrot force-pushed the hint-unfold-head-constant-index branch from 2486663 to 64bc59f Compare September 21, 2021 13:32
Alizter added a commit to HoTT/Coq-HoTT that referenced this pull request Sep 21, 2021
@ppedrot ppedrot force-pushed the hint-unfold-head-constant-index branch from 64bc59f to cc790db Compare September 21, 2021 15:59
@ppedrot ppedrot added kind: fix This fixes a bug or incorrect documentation. kind: cleanup Code removal, deprecation, refactorings, etc. labels Sep 21, 2021
@ppedrot ppedrot added this to the 8.15+rc1 milestone Sep 21, 2021
@ppedrot
Copy link
Member Author

ppedrot commented Sep 21, 2021

All overlays were merged, so this is ready to go as-is and needs an assignee.

@SkySkimmer SkySkimmer self-assigned this Sep 22, 2021
@SkySkimmer
Copy link
Contributor

Maybe we could have a test for this?

@ppedrot
Copy link
Member Author

ppedrot commented Sep 22, 2021

Do you want a positive or a negative test?

@SkySkimmer
Copy link
Contributor

Why not both?

@SkySkimmer
Copy link
Contributor

ping

The current behaviour was to indiscriminately try to apply on any goal
unfold hints for transparent constants from discriminated databases, since
they were not indexed by the head constant. In particular, this might have
unfolded the constant even if it was at the head of the goal, contrarily to
what the documentation claimed.

I think it was written this way out of fear of the dnet pattern algorithm. In
discriminated mode, transparent constants are considered to match everything,
but maybe the author of the code was confused by the complexity of the dnet
implementation.

This patch changes the semantics of auto tactics in fringe cases, and should
be mostly backwards compatible.

Fixes rocq-prover#14874: Exponential blowup with Hint Unfold.
@ppedrot ppedrot force-pushed the hint-unfold-head-constant-index branch from cc790db to c120f97 Compare September 24, 2021 12:12
@ppedrot
Copy link
Member Author

ppedrot commented Sep 24, 2021

@SkySkimmer test added.

@SkySkimmer
Copy link
Contributor

This test passes in master, shouldn't we test the changed behaviour?

@ppedrot
Copy link
Member Author

ppedrot commented Sep 24, 2021

You're right, let me add another discriminating test.

@ppedrot ppedrot force-pushed the hint-unfold-head-constant-index branch from c120f97 to 7e8de11 Compare September 24, 2021 12:22
@SkySkimmer
Copy link
Contributor

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 7330b04 into rocq-prover:master Sep 24, 2021
@ppedrot ppedrot deleted the hint-unfold-head-constant-index branch September 24, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: cleanup Code removal, deprecation, refactorings, etc. kind: fix This fixes a bug or incorrect documentation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exponential blowup with Hint Unfold

2 participants