X Tutup
Skip to content

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

Open
MonoidMusician wants to merge 12 commits intomasterfrom
mm/diagnostic-qualification
Open

Redux: Qualify types as necessary in errors and warnings#4409
MonoidMusician wants to merge 12 commits intomasterfrom
mm/diagnostic-qualification

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.

@nwolverson
Copy link
Contributor

because you can only have one span for suggestions, you can't suggest a type and also suggest imports for that type

There's no reason this can't be a thing, we can easily extend suggestions to have additional edits

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

X Tutup