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 ] Totality checker misses indirect references to negative data #3043

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

dunhamsteve
Copy link
Contributor

Idris reports a function as total when it indirectly references a negative data type, resulting in a proof of Void. This issue is reported in #527 and #3030. For direct references, the code in References.idr detects the call, but it cuts when it hits an Unchecked.

The totality checking would detect the issue, but data constructor calls are not included in sizeChange list (they do show up in refersTo). I believe this is an oversight, but I may be missing something. My reasoning should be double-checked by someone who knows the totality checker.

This PR changes findSC in CallGraph.idr to include DCon applications in the sizechange graph. I've done this in the TopLevel and Guarded cases. I've also adjusted test cases to reflect the addition entries in size change, and I updated one test to reflect additional partial functions that are detected. I added covering to REPL/Opts.idr in two places because it refers to a negative data type.

It fixes #527, fixes #3030, and is likely to break code that relies on faulty totalily results.

I have an alternative approach in a different branch, where I recursively call calcTermination on each reference of the function at the end of calcTermination. If my assumptions are correct, I believe the present solution is cleaner and provides better error messages - showing the path that fails.

As an example of the issue from the discussion at #3030, this proof of Void is accepted by Idris:

data Oops = MkOops (Not Oops)

total
runOops : Oops -> Not Oops
runOops (MkOops nf) = nf

total
notOops : Not Oops
notOops x = runOops x x

oops : Oops
oops = MkOops notOops

total
boom : Void
boom = runOops oops oops
  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG.md (and potentially also
    CONTRIBUTORS.md).

@andrevidela
Copy link
Collaborator

I'm assuming this is good to go

@andrevidela andrevidela merged commit 7227096 into idris-lang:main Sep 8, 2023
20 checks passed
@dunhamsteve dunhamsteve deleted the issue-3030 branch September 8, 2023 13:06
@mjustus
Copy link
Collaborator

mjustus commented Oct 20, 2023

I am currently porting this to Yaffle and I am wondering whether we should accept functions involving data types that fail the positivity check as total in the first place.

If we didn't, runOops would be considered non-total. Which, I think, is fine but it would probably force us to come up with a more precise positivity checker.

mjustus added a commit to mjustus/Yaffle that referenced this pull request Oct 20, 2023
…rences to partial data

Co-authored-by: Steve Dunham <[email protected]>
edwinb pushed a commit to edwinb/Yaffle that referenced this pull request Oct 30, 2023
…lysis (#29)

* [ new ] mapWithKey for name maps

* [ new ] retrieving value associated with the minimal key from a SortedMap

* [ new ] minimal value in a SortedSet

* [ new ] popping key-value pair from a SortedMap

* [ new ] popping value from a SortedSet

* [ size-change ] implement Ord instance

* [ refactor ] split up Core.Termination

Co-authored-by: Guillaume Allais <[email protected]>

* [ termination ] faithful implementation of size-change graph termination analysis

Co-authored-by: Guillaume Allais <[email protected]>

* [ test ] non-terminating function from issue idris-lang/Idris2#2448

* [ test ] examples from termination paper

* [ port idris-lang/Idris2#2915 ] use `logC` instead of `log`

Co-authored-by: Denis Buzdalov <[email protected]>

* [ port idris-lang/Idris2#2953 ] Issue in totality checking

Co-authored-by: Steve Dunham <[email protected]>

* [ refactor ] renamed `checkDesc` to `checkNonDesc`

* [ port idris-lang/Idris2#3043 ] Totality checker misses indirect references to partial data

Co-authored-by: Steve Dunham <[email protected]>

* [ cleanup ] update expected test output

Data constructors are now treated like functions during size-change
graph generation.

---------

Co-authored-by: Guillaume Allais <[email protected]>
Co-authored-by: Denis Buzdalov <[email protected]>
Co-authored-by: Steve Dunham <[email protected]>
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.

Projections out of a negative record are sometimes seen as total
4 participants