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

improvements to source locations for error reporting #1821

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

danmatichuk
Copy link
Contributor

@danmatichuk danmatichuk commented Mar 4, 2025

fix for #1744

  • retains more location information when rewriting selectors during the 'noPat' pass to improve error reporting
  • when reporting errors during type inference: prefers the range from the environment if it is contained within the provided range for the error. Otherwise, type checking errors would span the entire definition of a bind, rather than the sub-term where the error occurred.
  • retains information about the original shape of a bind during the 'noPat' pass, in order to provide a better error message when more patterns (bind arguments) are given than are implied by the bind signature

this overrides the provided range to 'recordErrorLoc'
if the current range is contained within the given range
@danmatichuk danmatichuk changed the title add location to selectors when doing a 'noPat' pass improvements to source locations for error reporting Mar 4, 2025
retains information about the original number of bind parameters
when performing the noPat transformation
needed to add new NamedParams constructor
NamedParams -> PatternParams
PatternParams [] -> noParams
@danmatichuk
Copy link
Contributor Author

Here is the new error output for the example given by #1744:

module Bug where

import Array

type Widget = [3]
type Wombat = [4]
type Key = [5]
newtype Info = {
   stuff: Array [5] [6]
}

once: Info -> /*Key ->*/ Widget -> Wombat -> Info
once info key widget wombat = info'
   where
      info' = Info {
         stuff = arrayUpdate info.stuff key 0
      }
[error] at issue1744.cry:12:7--13:28:
  Type signature mismatch.
    Expected number of parameters: 3
    Actual number of parameters: 4
    When defining 'Bug::once: Bug::Info -> [3] -> [4] -> Bug::Info'

The first error shows the result of an additional check that the number of parameters to the binding is not greater than the number of arguments implied by the signature, which subsumes the original type error
It notes the signature and LHS of the binding as the error location, rather than the entire definition.

Re-writing once to instead have fewer binding parameters than the signature would imply:

once: Info -> Key -> Widget -> Wombat -> Info
once info key widget = info'
   where
      info' = Info {
         stuff = arrayUpdate info.stuff key 0
      }

Results in the following error:

[error] at issue1744-2.cry:13:24--13:29:
  Type mismatch:
    Expected type: [4] -> Bug::Info
    Inferred type: Bug::Info
    When checking the type of 'Bug::once'

In this case we don't report that the number of parameters is unexpected, because the error could also be resolved by changing the body of the function into a lambda.
Previously the error range was the entire definition, this change focuses it instead on exactly the term info'.

@danmatichuk danmatichuk marked this pull request as ready for review March 7, 2025 20:18
Copy link
Contributor

@sauclovian-g sauclovian-g left a comment

Choose a reason for hiding this comment

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

All seems plausible, but I've never looked at most of this code before so take it for what it's worth...

Comment on lines +1080 to +1082
let bParams' = case bParams b of
PatternParams _ -> PatternParams pats'
DroppedParams rng i -> DroppedParams rng i
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider introducing a Rename instance for BindParams, as this would allow you to encapsulate the logic of renaming them in that instance.

noParams :: BindParams name
noParams = PatternParams []

data BindParams name =
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand, this data type really only exists for the sake of improving error messages. Is that right? If so, it would be worth stating that in a comment.

Comment on lines +114 to +116
| TooManyParams Name Type Int Int
-- ^ A binder implies more arguments than
-- its type signature
Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave a comment saying what these fields represent. In particular, it's not obvious which Int is the expected number of arguments, and which Int is the actual number of arguments.

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