Skip to content

Add test that the kernel checks relevances in Case binders#21883

Open
SkySkimmer wants to merge 1 commit intorocq-prover:masterfrom
SkySkimmer:test-case-relevance
Open

Add test that the kernel checks relevances in Case binders#21883
SkySkimmer wants to merge 1 commit intorocq-prover:masterfrom
SkySkimmer:test-case-relevance

Conversation

@SkySkimmer
Copy link
Copy Markdown
Contributor

@SkySkimmer SkySkimmer commented Apr 3, 2026

Before 6fae78c (8.17) relevance of Case binders was checked by checking the lambdas produced by expand_case.

Before 0cfe16a (8.18) expand_case would use the relevance from the Case instead of from the inductive data (after it used the relevance from the inductive unsubstituted by the univ instance until 269daf2 (9.0)).

If we had skipped 6fae78c the later patches would have introduced an inconsstency as typechecking would have trusted the Case relevances, but they would be used in conversion.

I think it's worth a test to ensure we don't introduce this possible bug by mistake.

To make testing practical the check is turned into a regular error instead of assert failure.
The test is an output test as otherwise we risk producing some other error if something changes.

@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label Apr 3, 2026
@SkySkimmer SkySkimmer requested a review from a team as a code owner April 3, 2026 06:57
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Apr 3, 2026
@SkySkimmer SkySkimmer added this to the 9.3+rc1 milestone Apr 3, 2026
Before 6fae78c (8.17) relevance of
Case binders was checked by checking the lambdas produced by expand_case.

Before 0cfe16a (8.18)
expand_case would use the relevance from the Case instead of
from the inductive data (after it used the relevance from the
inductive unsubstituted by the univ instance until
269daf2 (9.0)).

If we had skipped 6fae78c the later
patches would have introduced an inconsstency as typechecking would
have trusted the Case relevances, but they would be used in
conversion.

I think it's worth a test to ensure we don't introduce this possible
bug by mistake.

To make testing practical the check is turned into a regular error
instead of assert failure.
@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label Apr 3, 2026
@SkySkimmer SkySkimmer force-pushed the test-case-relevance branch from 9bbdf28 to 8749a74 Compare April 3, 2026 07:00
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Apr 3, 2026
@ppedrot ppedrot self-assigned this Apr 3, 2026
@yannl35133
Copy link
Copy Markdown
Contributor

Can you add the check there as well?

rocq/kernel/environ.ml

Lines 314 to 316 in 5a4f0c2

Context.
{ binder_name = nas.(i).binder_name;
binder_relevance = UVars.subst_instance_relevance u na.binder_relevance }

(maybe the functions can even be merged together)

@SkySkimmer
Copy link
Copy Markdown
Contributor Author

Can you add the check there as well?

Is that expected to be called on unchecked data?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants