I'm not sure about the "you could subsitute with one of these values" section; usually, when I encounter an UnknownName error, it's because I:

• forgot to import something,
• used a function I was intending to implement but haven't done so yet, or
• misspelled a function name.

In each case I know what I want to put there, so having a list of other things I could put there is unlikely to be helpful, especially because we already have holes for asking the compiler "what can I put here". I would prefer either having a "did you mean: something" section based mostly on edit distance for misspellings, or not having it at all.

• used a function I was intending to implement but haven't done so yet, or

The "has the inferred type" section above seems to cover this case

• forgot to import something,

Perhaps a "did you mean to import [something] from one of these modules?" message is also displayed when the name is an exact match for an existing unimported function

• misspelled a function name.

If there are functions within a small edit distance list these as "did you mean...", possibly prioritizing functions which also appear in the list of matches for the type hole

Just to clarify: yes, I think suggesting values of the unknown name which haven't been imported or suggesting values a small edit distance away from that name would both be fantastic, but both of these are probably best tackled separately; for now, just adding the inferred type would probably be best.

@hdgarrood I agree that the plain type search results without some sort of edit distance comparison isn't the most helpful. I've taken it out, and the error message is now a lot more readable.

@spicydonuts I think there is a lot we can do with edit distance comparison, and I really like the idea of suggesting possibly forgotten imports. I agree with Harry that this could be done in a follow up PR.

For now, I have updated this PR to:

• remove type search results from UnknownValueHint error
• move reporting of UnknownValueHint to the module level so that multiple UnknownValueHint errors can be reported for a single module

Here is an updated example:

module Main where

import Prelude

import Effect (Effect)

bar :: Effect Unit
bar = do
  boo <- a 10
  let x = foo "hello"
  pure boo

Previously, this would fail with an UnknownName error, but now it fails with a UnknownValueHint error, stating

[1/2 UnknownValueHint] src/Main.purs:9:10

  9    boo <- a 10
              ^

  Unknown name 'a' has the inferred type

    Int -> Effect Unit

  in value declaration bar

[2/2 UnknownValueHint] src/Main.purs:10:11

  10    let x = foo "hello"
                ^^^

  Unknown name 'foo' has the inferred type

    String -> t0

  in the following context:

    boo :: Unit

  in value declaration bar

  where t0 is an unknown type

I just realized a problem with the current implementation - the holes created by an unknown value (or just a plain Hole) can spuriously cause AmbiguousTypeVariables or NoInstanceFound caused by unknown types. Here's an example:

module Main where

import Prelude

import Effect (Effect)

bar :: Effect Unit
bar = ?a \_ -> do
  pure 10

And

module Main where

import Prelude

import Effect (Effect)

bar :: Effect Unit
bar = foo \_ -> do
  pure 10

Both fail with

[1/1 NoInstanceFound] src/Main.purs:9:3

  9    pure 10
       ^^^^^^^

  No type class instance was found for

    Control.Applicative.Applicative t0

  The instance head contains unknown type variables. Consider adding a type annotation.

  while applying a function pure
    of type Applicative t0 => t1 -> t0 t1
    to argument 10
  while inferring the type of pure 10
  in value declaration bar

  where t0 is an unknown type
        t1 is an unknown type

While the real error in both of them is that there is a Hole or an unknown value. This is because the UnknownName/Hole errors are first collected as warnings and then escalated as errors later, whereas other errors are thrown immediately.

I'm not sure how big of a deal this is with Holes (or if it can even be helped), but for UnknownName errors, I think that this is incorrect behavior. It should report the unknown name before reporting errors down the line.

I'm thinking of ways to remedy this. One idea that I have is to modify some of the throwErrors to check for UnknownName warnings that have accumulated, and report those first before any other error.

You could wrap the original UnknownName error you would have thrown inside the error constructor, and modify the error rendering function to handle that case specially, perhaps.

I wasn't satisfied with the stateful approach I originally took, so I have created an explicit AST traversal that takes an Expr and returns all of the holes and unknown values within that Expr. This allows throwing all unknown name errors first in the case of a NoInstanceFound or AmbiguousTypeVariables error (which could have been caused by an unknown name hole in the first place). I have also added an extra hint message when the error is possibly caused by a hole, as I illustrated above.

Other than the problems with unknown name holes causing NoInstanceFound or AmbiguousTypeVariables errors (which should be fixed now), this seems to be working really well. I've been using it at work for about a week and I haven't run into any issues.

Let me know of any feedback, and I'd be happy to address it and try to get this over the line! I'm going to address the duplicated error code tomorrow when I get a chance.

I have made a few changes that hopefully make this PR more focused/easier to review. At first I started to make possible improvements to errors messages for errors that are possibly caused by the existence of typed holes. However, in favor of having this PR be smaller and easier to review, I have reverted those changes and only included changes pertaining to the original issue. I can do follow up work on the other issues in subsequent PRs, if wanted.

The main change since @hdgarrood's last review adds a mechanism to avoid unknown name holes from causing spurious AmbiguousTypeVariable and NoInstanceFound errors. To do this, I added a function which throws UnknownName errors contained in an expression which has an AmbiguousTypeVariable or NoInstanceFound error before throwing said error. Note that this bypasses the logic which raises all unknown value holes into errors at the module level, meaning that if there are unknown names in some other binding group in the module, they will not be reported as errors. However, I think that this case is infrequent enough that this is acceptable.

Has anyone had a chance to look over this? I'm happy to change my approach with a little guidance, if needed!

Note that this bypasses the logic which raises all unknown value holes into errors at the module level, meaning that if there are unknown names in some other binding group in the module, they will not be reported as errors. However, I think that this case is infrequent enough that this is acceptable.

Can you give me an example that triggers this case?

Note that this bypasses the logic which raises all unknown value holes into errors at the module level, meaning that if there are unknown names in some other binding group in the module, they will not be reported as errors. However, I think that this case is infrequent enough that this is acceptable.

Can you give me an example that triggers this case?

Sure, something like this:

module Test where

import Prelude
import Effect (Effect)

foo :: Effect Unit
foo = b \_ ->
  pure 10

bar = f 10

Will fail with one UnknownName error (for b), instead of two.
Now that I look at it again I think that the error has the wrong source span - I'm looking into that now

@natefaubion I pushed a proposed update to the typechecking logic for the cases where there is an unknown name in a expression. It alleviates the problem of throwing a spurious (NoInstanceFound/AmbiguousTypeVariables) The two cases that cause problems (NoInstanceFound and AmbiguousTypeVariables) can only be caused when the typechecker is solving unsolved types. To remedy this, one thing we could do is not do the processing of unknown types when an expression contains an unknown name. This will sometimes cause errors like

[3/4 UnknownName] src/Test.purs:10:7

  10  bar = f 10
            ^

  Unknown value f has the inferred type

    Int -> t0

  in value declaration bar

But at least we have given some indication of what the type should be, and we have also prevented the spurious errors that were previously caused. This has the added benefit of not reporting the wrong source spans, like my previous attempt did. What do you think? Is it OK to report unknown types for unknown name errors?

@natefaubion bumping this PR. I still am unsure of the best way to resolve the AmbiguousTypeVariables/NoInstanceFound errors that can be caused by trying to infer the types of unknown names. I have a proposed solution above but it's definitely not the best.

Thanks!