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

Redux: Qualify types as necessary in errors and warnings #4409

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

Conversation

MonoidMusician
Copy link
Contributor

Continuing from #4337. Just fixed merge conflicts so far.

Checklist:

  • Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@MonoidMusician
Copy link
Contributor Author

General design questions/notes:

  1. Where do we want to apply this – e.g. do they need to apply to every type in ErrorMessageHint as well?
  2. It seems like there are particular errors where it will make a big difference, specifically TypesDoNotUnify and KindsDoNotUnify. In those I'd like to also say, “Foo.X was declared in Data.Foo and Bar.X was declared in Test.Types” or something. Perhaps that is enough, and we don't need to requalify every type ever?
  3. Ambiguity in reverse import lookup seems particularly tricky, if the same type gets imported multiple times. In the end it wouldn't matter semantically, but it seems a shame to discard what the user wrote if it corresponds directly. OTOH there are times that the type being displayed in the error did not originate from the same file, so it makes sense that we want to do some work there.
  4. A more pernicious error is if we import a module with a local name that overlaps a global name, that could obscure the origin of the types in errors. E.g. if I accidentally autocomplete Data.List.List coming from Data.List.Lazy (which is the suggested module actually, lol), it adds import Data.List.Lazy as Data.List, and now the requalification cannot distinguish the Data.List.Lazy module from the Data.List module. See QualifiedErrorsUnificationShadow.out. (This is mainly why I want to do item 2 in addition to any requalification added during type printing, but it only targets two specific errors.)
  5. The special type classes are matched based on their qualification, so importing something as Prim could mess them up via shadowing in requalification … but we could convert them to more specific errors sooner. They're only thrown in one place:
    | otherwise = throwError . errorMessage $ NoInstanceFound (srcConstraint className' kindArgs tyArgs conInfo) ambiguous unks
    That doesn't account for Prim.Function though, and is there any other sugar to look out for?
  6. Re-exports are also tricky, because the current algo doesn't pick up the local import of Data.List.Lazy (List) when reprinting List because it is really defined in Data.List.Lazy.Types. See QualifiedErrorsUnificationReexport.out, which should really just print List for the second type. Ugh.

I might end up putting item 2 into its own PR, so at least the obvious “Could not match type List with List” errors get improved … perhaps we want to only show the modules when the constructor names are the same, but I committed this version that always shows it just so we can see what's possible.

@MonoidMusician
Copy link
Contributor Author

It might help to disallow importing modules as Prim. It seems that it already results in a warning, due to the implicit import import Prim as Prim given to every module:

  Module Prim was imported as Prim with unspecified imports.
  As there are multiple modules being imported as Prim, consider using the explicit form:

    import Prim (Function, Record) as Prim

@MonoidMusician
Copy link
Contributor Author

The issue of global modules that haven't been imported locally yet also rears its head in IDE suggestions: because you can only have one span for suggestions, you can't suggest a type and also suggest imports for that type. So we can only have suggestions that actually work if all of the types already are in scope.

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.

None yet

2 participants