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

Pedantic ghcide #3751

Merged
merged 21 commits into from
Aug 22, 2023
Merged

Pedantic ghcide #3751

merged 21 commits into from
Aug 22, 2023

Conversation

joyfulmantis
Copy link
Collaborator

@joyfulmantis joyfulmantis commented Aug 10, 2023

This is taking much more time than I had anticipated, so I'm going get this merged in, and then if I have time later, finish the rest then
Currently the following warnings are fixed

  • redundant-imports
  • name-shadowing
  • unused-top-binds
  • unused-local-binds
  • orphans
  • unused-matches
  • simplifiable-class-constraints

These warnings don't make sense to fix

  • unrecognised-pragmas
  • unused-packages

These warnings are currently left to fix (some of these also might not make sense to fix)

  • dodgy-imports
  • missing-signatures
  • duplicate-exports
  • dodgy-exports
  • incomplete-patterns
  • ambiguous-fields
  • overlapping-patterns
  • incomplete-record-updates

I have also drastically simplified the CPP in the imports section. Whereas before there were several layers of nested if-else CPP flags spread throughout the imports, now there are very clear single-layer if clauses at the end of the standard imports that are ordered by the ghc version they are for.

@joyfulmantis joyfulmantis marked this pull request as draft August 10, 2023 13:13
@joyfulmantis joyfulmantis marked this pull request as ready for review August 18, 2023 12:32
@joyfulmantis joyfulmantis changed the title WIP Pedantic ghcide Pedantic ghcide Aug 18, 2023
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, the CPP is way better. It seems to me that you've come up with a policy for how to write the big blocks of CPP in imports - perhaps you could write it down in a Note somewhere and refer to it from the files where we do this a lot? That would help us keep to it in future.

ghcide/ghcide.cabal Show resolved Hide resolved
ghcide/ghcide.cabal Show resolved Hide resolved
@@ -111,6 +110,10 @@ import HieDb.Utils
import qualified System.Random as Random
import System.Random (RandomGen)

#if !MIN_VERSION_ghc(9,4,0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we had to add this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was necessary to clean up redundant imports (it's not used GHC >= 9.4 (Or it's possibly rexported by someone else for those versions?!))


#if MIN_VERSION_ghc(9,0,1)
import GHC.Tc.Gen.Splice
#endif

#if MIN_VERSION_ghc(9,0,1) && !MIN_VERSION_ghc(9,2,1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases like this where it really is multiple ways of getting the same names, I think it can be nice to structure it with #elses? Not sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe else CPP is not worth it in the imports section. As this statement uses an && symbol, it would need to nested CPP, and then you're pretty much back to where we started.

ghcide/src/Development/IDE/Core/FileStore.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/UseStale.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/LSP/HoverDefinition.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good, just one thing I don't think is an improvement

ghcide/src/Development/IDE/Core/UseStale.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/GHC/Compat/Outputable.hs Outdated Show resolved Hide resolved
@michaelpj michaelpj added the merge me Label to trigger pull request merge label Aug 22, 2023
@joyfulmantis joyfulmantis merged commit e0d82e7 into haskell:master Aug 22, 2023
45 checks passed
@fendor fendor mentioned this pull request Aug 25, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants