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 ] False positives checking for conflicts in coverage checking #3478

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dunhamsteve
Copy link
Collaborator

Description

This fixes #3477. Idris considers 2 = length xs as empty while checking missing cases in coverage checking, and it incorrectly accepts this as covering:

test : (xs : List Int) ->  2 = length xs -> (Int, Int)
test (x1 :: x2 :: x3 :: []) pf = (x1, x2)

There are two changes here:

I removed the clearDefs in conflictNF to allow the length xs to expand when xs is known. Let me know if there is a reason we don't allow expansions here. It passes tests and it seems to be necessary to enable Idris to automatically use the 2 = length xs.

Without this change, the following impossible clauses are required:

test : (xs : List Int) ->  2 = length xs -> (Int, Int)
test (x1 :: x2 :: []) pf = (x1, x2)
test (x1 :: Nil) Refl impossible
test (x1 :: x2 :: x3 :: _) Refl impossible

The second change addresses the root cause. When checking for clashes between types in conflictMatch, idris checks if the heads clash, and if not it proceeds to check the arguments. This is a problem when S (S Z) is compared to length xs. S and length do not clash, but it is not correct to then compare S Z against xs. To remedy this, it is now only checking the spines in the case of matching data or type constructors. The helper function returns Nothing to indicate the spine should be checked.

Krivine.idr was relying on this bug. The code was covering, but coverage checking was accepting it for the wrong reason. It was discounting the other case when looking at Element (EvalContext{}) _ vs (::) (Arr{}) ctx (here :: is a function, not a constructor). Element vs (::) was not clashing, so it proceeded to compare the arguments and saw that EvalContext{} and Arr{} clashed (values headed by different constructors). After this change, it needed an impossible clause to unblock the (::).

I've also added a test case, fixed a typo in a comment, and added a log message that I use do diagnose these impossible issues. This will break code that relies on false positives in coverage, but given that the rest of papers and frex are uneffected, I suspect that is rare.

@@ -24,20 +24,20 @@ conflictMatch : {vars : _} -> List (Name, Term vars) -> Bool
conflictMatch [] = False
conflictMatch ((x, tm) :: ms) = conflictArgs x tm ms || conflictMatch ms
where
clash : Term vars -> Term vars -> Bool
clash : Term vars -> Term vars -> Maybe Bool
Copy link
Member

@gallais gallais Feb 5, 2025

Choose a reason for hiding this comment

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

Can you please document the meaning of this Maybe? Or should we even define
e.g. data ClashingDecision = Distinct | Same | Incomparable
with the idea that we are only checking the arguments if the heads are Same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was on the fence about whether to use an enum or not. Coming back to this a week later and having to get my bearings again, I think it would be clearer if I did that.

@dunhamsteve dunhamsteve changed the title Issue 3477 [ fix ] False positives checking for conflicts in coverage checking Feb 6, 2025
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.

Not covering pattern compiles with auto proof on list length
2 participants