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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qualify types as necessary in errors and warnings #4337

Conversation

hdgarrood
Copy link
Contributor

@hdgarrood hdgarrood commented May 23, 2022

Will hopefully fix #1647, reported by me a mere 7 years ago. 馃く

The idea is to make a note of the Imports for the current module before type checking, and then if there are any errors, construct a reverse lookup of more or less the same information in the Imports, so that we can go from fully-qualified names back to names that are qualified as they would be in the source file, and then "requalify" any types in error messages according to that reverse lookup.

While this does what we want sometimes (see e.g. the new tests TypeQualification{1,2,3}), it doesn't quite do what we want most of the time, and I think one of the main reasons it's not doing what we want is because of missing cases in onTypesInErrorMessage.

Checklist:

  • Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
  • Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • 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)

@hdgarrood hdgarrood mentioned this pull request May 23, 2022
2 tasks
@hdgarrood hdgarrood marked this pull request as draft May 23, 2022 18:38
@hdgarrood
Copy link
Contributor Author

I'm hoping I can find some time to pick this up again soon, but I think what I have here works as a proof of concept, so I'm interested to hear what people think of this approach. Alternatives I can think of:

  • Don't desugar names at all, and instead keep something the Imports around during type checking. Disadvantages: much more invasive
  • Add a new field to the Qualified data type to keep the original qualified names of things around as they were written in the source file. Disadvantages of this: it doesn't account for things that aren't written in the source file. For example:
module Main where

import Data.List as L

x = L.fromFoldable [1,2,3]

It would be nice to be able to produce a suggested type signature that says x :: L.List here, so that the user can simply insert that into the source file and have things Just Work; this approach allows us to easily do that.

I'm also tempted to get rid of the new field in TypeRenderOptions and instead have the type renderer always print all qualifications that are present, and let the caller strip qualifications if that's what they want. In general I think keeping qualifications is the correct default.

@garyb
Copy link
Member

garyb commented May 25, 2022

I've not had a chance to look at the meat of this yet, but I'm excited! I did take a stab at this years ago too but my branch got behind master and the diff was too much to deal with.

Keeping Imports around to handle names is how the compiler worked in the very early days, and it was a nightmare IMO. I'm interested to look into what you've done here, because my approach to this was just to include more data in Qualified, as per your second suggestion.

@rhendric
Copy link
Member

I don't know how much the changes to golden test outputs reflect a work-in-progress state of the implementation versus what you're trying to achieve, but a number of them are, if not mistakes, steps backward in my opinion. Broadly:

Provided none of those objections pose difficulties for the approach, the requalification pass makes a lot of sense to me.

@hdgarrood
Copy link
Contributor Author

Completely agree - this is far from what I鈥檇 consider ready.

@JordanMartinez
Copy link
Contributor

This has been superceded by #4409

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.

Qualify types as necessary in hints in warnings/errors
4 participants