FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Redux: Qualify types as necessary in errors and warnings by MonoidMusician · Pull Request #4409 · purescript/purescript · GitHub

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

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

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

Conversation

Copy link
Copy Markdown
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)

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL