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

fix: segfault in old compiler due to noConfusion assumptions #2903

Merged
merged 1 commit into from
May 10, 2024

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Nov 17, 2023

This fixes #2901, a bug in the old compiler which causes a segfault. The issue is that when visiting noConfusion applications, it assumes that each constructor case has nfields arguments, e.g. head1 = head2 -> tail1 = tail2 -> P has two arguments because List.cons has 2 fields, but in fact propositional fields are skipped by the noConfusion type generator, so for example Subtype.noConfusionType is:

@[reducible] protected def Subtype.noConfusionType.{u_1, u} : {α : Sort u} →
  {p : α → Prop} → Sort u_1 → Subtype p → Subtype p → Sort u_1 :=
fun {α} {p} P v1 v2 ↦
  Subtype.casesOn v1 fun val property ↦ Subtype.casesOn v2 fun val_1 property ↦ 
    (val = val_1 → P) → P

where val = val_1 → P only has the one argument even though Subtype.mk has two fields, presumably because it is useless to have an equality of propositions. Unfortunately there isn't any easy cache or getter to use here to get the number of non-propositional fields, so we just calculate it on the spot.

@nomeata nomeata changed the title fix: fixes #2901 fix: default in old compiler due to noConfusion assumptions Nov 17, 2023
@nomeata
Copy link
Contributor

nomeata commented Nov 17, 2023

I took the liberty to make the PR title more explicit, it helps at least me when looking at notifications. I hope that doesn't step on anyone's toes.

@nomeata nomeata changed the title fix: default in old compiler due to noConfusion assumptions fix: segfault in old compiler due to noConfusion assumptions Nov 17, 2023
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Nov 17, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Nov 17, 2023

  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-11-17' branch does not exist there yet. We will retry when you push more commits. It may be necessary to rebase onto 'nightly' tomorrow. (2023-11-17 14:17:44)
  • 🟡 Mathlib branch lean-pr-testing-2903 build against this PR was cancelled. (2023-11-20 03:50:44) View Log
  • ✅ Mathlib branch lean-pr-testing-2903 has successfully built against this PR. (2023-11-20 05:04:55) View Log
  • 🟡 Mathlib branch lean-pr-testing-2903 build this PR didn't complete normally. (2024-05-10 01:41:41) View Log

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Nov 20, 2023
@nrolland
Copy link

will this go to 4.4 or nightly ?

@semorrison
Copy link
Collaborator

semorrison commented Dec 12, 2023

will this go to 4.4 or nightly ?

@nrolland, when this is merged it will land in the nightly releases. It will then be rolled out as part of v4.5.0-rc1 (likely on Dec 21).

@digama0
Copy link
Collaborator Author

digama0 commented Apr 20, 2024

Ping on this old PR.

@semorrison semorrison enabled auto-merge May 10, 2024 01:22
@semorrison
Copy link
Collaborator

Rebased to make sure there are no interactions with recent changes.

@semorrison semorrison added this pull request to the merge queue May 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 10, 2024
Merged via the queue into leanprover:master with commit 3491c56 May 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in old compiler
5 participants