Play nice with ghcide #369

robrix · 2019-10-30T18:22:12Z

This PR makes semantic play a little more nicely with ghcide, especially when configuring it to load all of the components.

It does this by:

Eliminating the project-wide language extensions in favour of per-file ones. Given the choice between enabling these globally for all of our packages and configuring ghcide to do the same or enabling them on a file-by-file basis, I strongly prefer to enable them file-by-file, as it makes reasoning locally about what’s going on in the file much easier. (Note that this also makes integration with tools such as hlint easier.)
Enabling more warnings across semantic (and fixing them). Unlike language extensions which are usually conveniences, these are generally very good ideas to have globally enabled.

I’m not quite ready to commit a hie.yaml file because at the present time these require absolute paths to the source directories when used with VS Code, but it is also possible to write a script to provide these, and that should be simple enough for us to integrate in a follow-up PR.

robrix · 2019-10-30T18:47:09Z

.hlint.yaml

@@ -23,7 +23,7 @@
 - warn: {group: {name: default}}

 # Ignore the highly noisy module export list hint
- ignore: {name: Use module export list}
+- ignore: {name: Use explicit module export list}


This one apparently changed.

robrix · 2019-10-30T18:47:16Z

.hlint.yaml

@@ -1,7 +1,7 @@
 # HLint configuration file
 # https://github.com/ndmitchell/hlint

- arguments: [--color=auto, -XDataKinds, -XDeriveFoldable, -XDeriveFunctor, -XDeriveGeneric, -XDeriveTraversable, -XFlexibleContexts, -XFlexibleInstances, -XMultiParamTypeClasses, -XOverloadedStrings, -XRecordWildCards, -XStandaloneDeriving, -XStrictData, -XTypeApplications, -XDerivingVia]
+- arguments: [--color=auto, -XStrictData]


robrix · 2019-10-30T18:47:38Z

bench/Evaluation.hs

-import           Algebra.Graph
-import           Control.Monad


Did you know we didn’t have warnings enabled in the benchmarks?

robrix · 2019-10-30T18:48:45Z

semantic.cabal

-  default-extensions:  DataKinds
-                     , DeriveFoldable
-                     , DeriveFunctor
-                     , DeriveGeneric
-                     , DeriveTraversable
-                     , FlexibleContexts
-                     , FlexibleInstances
-                     , MonadFailDesugaring
-                     , MultiParamTypeClasses
-                     , OverloadedStrings
-                     , RecordWildCards
-                     , StandaloneDeriving
-                     , StrictData
-                     , TypeApplications


All of these have been moved to the .hs files, except StrictData, which does not affect parsing/typechecking, and MonadFailDesugaring, which is obsolete in 8.6+.

robrix · 2019-10-30T18:49:05Z

semantic.cabal

-                     , TypeApplications
+  default-extensions:  StrictData
+  ghc-options:
+    -Weverything


robrix · 2019-10-30T18:49:50Z

src/Analysis/Abstract/Caching/FlowInsensitive.hs

@@ -1,4 +1,4 @@
-{-# LANGUAGE GeneralizedNewtypeDeriving, TypeOperators #-}
+{-# LANGUAGE DeriveFunctor, FlexibleContexts, GeneralizedNewtypeDeriving, TypeApplications, TypeOperators #-}


A lot of these FlexibleContexts additions are going to go away with fused-effects 1.0.

robrix · 2019-10-30T18:50:14Z

src/Analysis/Abstract/Graph.hs

+        vertexAttributes UnknownModule{}  = [ "style" := "dotted, rounded", "shape" := "box", "color" := "red", "fontcolor" := "red" ]
+        vertexAttributes (Variable n _ s) = [ "label" := T.encodeUtf8Builder (n <> " (Variable)"), "tooltip" := T.encodeUtf8Builder (showSpan s), "style" := "rounded", "shape" := "box" ]
+        vertexAttributes (Method   n _ s) = [ "label" := T.encodeUtf8Builder (n <> " (Method)"),   "tooltip" := T.encodeUtf8Builder (showSpan s)  , "style" := "rounded", "shape" := "box" ]
+        vertexAttributes (Function n _ s) = [ "label" := T.encodeUtf8Builder (n <> " (Function)"), "tooltip" := T.encodeUtf8Builder (showSpan s), "style" := "rounded", "shape" := "box" ]


Fallout from the changes to ControlFlowVertex.

robrix · 2019-10-30T18:50:42Z

src/Analysis/PackageDef.hs

@@ -15,7 +15,7 @@ import Prologue
 import Source.Loc

 newtype PackageDef = PackageDef { moduleDefIdentifier :: T.Text }
-  deriving (Eq, Generic, Show)
+  deriving (Eq, Show)


I also took the opportunity to 🔥 a few Generic instances we weren’t using.

robrix · 2019-10-30T18:54:50Z

src/Assigning/Assignment.hs

-- | Match a branch node, matching its children with the supplied 'Assignment' & returning the result.
-branchNode :: (Enum grammar, Ix grammar) => grammar -> Assignment ast grammar a -> Assignment ast grammar a
-branchNode sym child = symbol sym *> children child
-
-- | Match a leaf node, returning the corresponding 'Text'.
-leafNode :: (Enum grammar, Ix grammar) => grammar -> Assignment ast grammar Text
-leafNode sym = symbol sym *> source
-
-- | Wrap an 'Assignment' producing @syntax@ up into an 'Assignment' producing 'Term's.
-toTerm :: Element syntax syntaxes
-       => Assignment ast grammar (syntax (Term (Sum syntaxes) L.Loc))
-       -> Assignment ast grammar         (Term (Sum syntaxes) L.Loc)
-toTerm syntax = termIn <$> location <*> (inject <$> syntax)


These were unused.

robrix · 2019-10-30T18:55:23Z

src/Control/Abstract/Evaluator.hs

+unLoopControl :: LoopControl value -> value
+unLoopControl = \case
+  Break    v -> v
+  Continue v -> v
+  Abort      -> error "unLoopControl: Abort"


Silencing partial selector warnings.

patrickt

A couple questions, but this is a major improvement. The default-extensions field is a prime example of false economy, I think, so kudos for ditching it (even if we are married to StrictData).

patrickt · 2019-10-30T19:26:25Z

bench/Evaluation.hs

-import           Algebra.Graph
-import           Control.Monad


patrickt · 2019-10-30T19:28:06Z

src/Assigning/Assignment.hs

@@ -361,20 +342,18 @@ instance (Enum grammar, Eq1 ast, Ix grammar, Show grammar) => Parsing (Assignmen
  (<?>) :: HasCallStack => Assignment ast grammar a -> String -> Assignment ast grammar a
  a <?> s = tracing (Label a s) `Then` pure

-  unexpected :: HasCallStack => String -> Assignment ast grammar a
+  unexpected :: String -> Assignment ast grammar a


Thought: instead of just ditching this constraint, can we glom it on to the passed String? I feel like call stacks are good and we shouldn’t lose these constraints, but maybe I’m bugging.

I’ve only removed redundant constraints. This call stack was never used, so we haven’t lost anything.

Fair enough. I guess it would still be preserved if we called error here? But we don’t, so no matter.

Yes, it wouldn’t have been redundant in that case, and I wouldn’t have removed it 😁

patrickt · 2019-10-30T19:28:52Z

src/Control/Abstract/Value.hs

@@ -133,17 +133,18 @@ data Function term address value (m :: * -> *) k
  | BuiltIn address BuiltIn (value -> m k)           -- ^ A built-in is parameterized by its parent scope, BuiltIn type, and returns a value.
  | Call value [value] (value -> m k)                -- ^ A Call takes a set of values as parameters and returns a ValueRef.
  | Bind value value (value -> m k)
-  deriving stock (Functor, Generic1)
-  deriving anyclass (HFunctor, Effect)


my beautiful deriving strategies nooooo

(just kidding this is fine)

robrix added 24 commits October 30, 2019 11:44

Avoid binding an unused type variable.

298c37f

Add language extensions to everything.

7262484

Fix some missed language extensions.

a8a3bca

Only enable StrictData by default.

17f860a

Enable hard-mode warnings project-wide.

f68fa5e

Fix partial record selector warnings for LoopControl.

8d6eecc

🔥 partial field selectors for Token.

9770cbd

🔥 partial field selectors for Python.QualifiedName.

29db8b3

Don’t bind unused type variables.

b2ad11a

Disable extensions in the hlint config.

fa865e8

Extract the partial moduleName & span fields into a new type.

29085fd

🔥 partial record selectors from ControlFlowVertex.

a2ec2ae

Fix up the graph style.

6230181

🔥 redundant imports.

22f1e72

🔥 partial record selectors in the integration spec.

b07382d

We don’t actually use NamedFieldPuns here.

3a18af8

🔥 the highly suspect release flag use from the tests.

eeec38a

🔥 the silencing of no export list warnigns.

60886eb

🔥 redundant constraints.

4941cb8

🔥 branchNode/leafNode/toTerm.

890d05d

🔥 redundant constraints.

0c89785

🔥 redundant language extensions.

17fe2b0

Correct how we silence a lint.

50fb138

🔥 redundant Generic & Hashable instances.

c6c729d

robrix commented Oct 30, 2019

View reviewed changes

robrix requested a review from a team October 30, 2019 18:56

patrickt reviewed Oct 30, 2019

View reviewed changes

:fire

cab72da

robrix requested a review from patrickt October 30, 2019 20:29

robrix mentioned this pull request Oct 30, 2019

Multi component repl #373

Merged

1 task

patrickt approved these changes Oct 31, 2019

View reviewed changes

patrickt merged commit 1f796bd into master Oct 31, 2019

robrix deleted the play-nice-with-ghcide branch November 1, 2019 14:18

		@@ -1,4 +1,4 @@
		{-# LANGUAGE GeneralizedNewtypeDeriving, TypeOperators #-}
		{-# LANGUAGE DeriveFunctor, FlexibleContexts, GeneralizedNewtypeDeriving, TypeApplications, TypeOperators #-}

Play nice with ghcide #369

Play nice with ghcide #369

Uh oh!

Conversation

robrix commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrix Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robrix commented Oct 30, 2019 •

edited

Loading

robrix Oct 30, 2019 •

edited

Loading