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

Panic with local definiton that uses constraint guards without a type signature #1685

Closed
RyanGlScott opened this issue Jun 12, 2024 · 2 comments · Fixed by #1686
Closed

Panic with local definiton that uses constraint guards without a type signature #1685

RyanGlScott opened this issue Jun 12, 2024 · 2 comments · Fixed by #1686
Labels
bug Something not working correctly type-guards

Comments

@RyanGlScott
Copy link
Contributor

If you give this Cryptol this program:

module Bug where

f : ()
f = g
  where
    g | 0 == 0 => ()

Then Cryptol will panic:

$ ~/Software/cryptol-3.1.0/bin/cryptol Bug.cry 
┏━╸┏━┓╻ ╻┏━┓╺┳╸┏━┓╻  
┃  ┣┳┛┗┳┛┣━┛ ┃ ┃ ┃┃  
┗━╸╹┗╸ ╹ ╹   ╹ ┗━┛┗━╸
version 3.1.0
https://cryptol.net  :? for help

Loading module Cryptol
Loading module Bug
cryptol: You have encountered a bug in Cryptol's implementation.
*** Please create an issue at https://github.com/GaloisInc/cryptol/issues

%< --------------------------------------------------- 
  Revision:  65397a491bddc3b4e8f41053106ce8859387d662
  Branch:    release-3.1.0 (uncommited files present)
  Location:  [TypeCheck] checkMonoB
  Message:   Used constraint guards without a signature at 
             (at Bug.cry:6:5--6:6, g)
CallStack (from HasCallStack):
  panic, called at src/Cryptol/Utils/Panic.hs:21:9 in cryptol-3.1.0-inplace:Cryptol.Utils.Panic
  panic, called at src/Cryptol/TypeCheck/Infer.hs:1622:17 in cryptol-3.1.0-inplace:Cryptol.TypeCheck.Infer
%< ---------------------------------------------------

Note that making slight changes to the program will change the panic to a proper Cryptol error message:

  • If you give g a type signature:

    module Bug where
    
    f : ()
    f = g
      where
        g : ()
        g | 0 == 0 => ()
    

    Then Cryptol will error thusly:

    $ ~/Software/cryptol-3.1.0/bin/cryptol Bug.cry 
    ┏━╸┏━┓╻ ╻┏━┓╺┳╸┏━┓╻  
    ┃  ┣┳┛┗┳┛┣━┛ ┃ ┃ ┃┃  
    ┗━╸╹┗╸ ╹ ╹   ╹ ┗━┛┗━╸
    version 3.1.0
    https://cryptol.net  :? for help
    
    Loading module Cryptol
    Loading module Bug
    [error] at Bug.cry:7:5--7:6:
      Local declaration `g` may not use constraint guards.
      Constraint guards may only appear at the top-level of a module.
    
  • If you use constraint guards at the top level without a type signature, as in this program:

    module Bug where
    
    f | 0 == 0 => ()
    

    Then Cryptol will error thusly:

    $ ~/Software/cryptol-3.1.0/bin/cryptol Bug.cry 
    ┏━╸┏━┓╻ ╻┏━┓╺┳╸┏━┓╻  
    ┃  ┣┳┛┗┳┛┣━┛ ┃ ┃ ┃┃  
    ┗━╸╹┗╸ ╹ ╹   ╹ ┗━┛┗━╸
    version 3.1.0
    https://cryptol.net  :? for help
    
    Loading module Cryptol
    Loading module Bug
    At Bug.cry:3:1--3:2: Declarations using constraint guards require an explicit type signature.
    
@RyanGlScott RyanGlScott added bug Something not working correctly type-guards labels Jun 12, 2024
@RyanGlScott
Copy link
Contributor Author

This issue involves checkNumericConstraintGuardsOK. This function makes the assumption that by the time it is invoked, all numeric constraint guard expressions have been checked to have a type signature. (See the comment here.) This isn't actually true, however. The code in Cryptol.Parser.ExpandPropGuards does check that top-level numeric constraint guards have type signatures, but it does not check any constraint guards in where expressions.

Two ways we could fix this:

  1. Make Cryptol.Parser.ExpandPropGuards recurse into where expressions and reject any nested constraint guards it encounters. (This would make this error message later in the typechecker unreachable, so we could remove it.)
  2. Do not assume that by the time checkNumericConstraintGuardsOK is invoked, we have checked all constraint guards to have a type signature.

I am leaning more towards option (1), since this is the sort of thing that we could easily detect (and reject) in the parser, well before we get to the typechecker.

@yav
Copy link
Member

yav commented Jun 14, 2024

Yeah, I also think (1) is the way to go.

RyanGlScott added a commit that referenced this issue Jun 14, 2024
Numeric constraint guards have very particular syntactic requirements:

* They must always be used in top-level definitions.
* Their definitions must always be accompanied by a top-level type signature.

Previously, we checked these requirements in a combination of places in the
parser and the typechecker. The checks in the typechecker weren't very
thorough, however, and they failed to catch local definitions without type
signatures that use constraint guards, as seen in #1685.

This patch moves all of these syntactic checks to the parser (in
`Cryptol.Parser.ExpandPropGuards`). We now recurse into expressions to check
for local definition that use constraint guards and error if we encounter one.
This ensures that by the time we reach the typechecker, all constraint guard
expressions are at least syntactically valid.

Fixes #1685.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something not working correctly type-guards
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants