Refactor pragmas plugin

[berberman]

Closes #1413

BTW I still couldn't run pre-commit hook successfully...

$ git commit -m "Refactor pragmas plugin"
stylish-haskell..........................................................Failed
- hook id: stylish-haskell
- exit code: 1

stylish-haskell: 0:0:Language.Haskell.Stylish.Config.loadConfig: fromJSON: expected Bool, but encountered String

CallStack (from HasCallStack):
  error, called at lib/Language/Haskell/Stylish/Config.hs:107:32 in stylish-haskell-0.11.0.3-F2KP2B6JiKw5YfjeWoLVgI:Language.Haskell.Stylish.Config
stylish-haskell: 0:0:Language.Haskell.Stylish.Config.loadConfig: fromJSON: expected Bool, but encountered String

CallStack (from HasCallStack):
  error, called at lib/Language/Haskell/Stylish/Config.hs:107:32 in stylish-haskell-0.11.0.3-F2KP2B6JiKw5YfjeWoLVgI:Language.Haskell.Stylish.Config

pre-commit hook purposed in #1384 enforced formatting files edited in this PR, a good sign!

[Ailrun]

I fixed that issue of the pre-commit hook in #1411, but it still suffers #1416...

[berberman]

I fixed that issue of the pre-commit hook in #1411, but it still suffers #1416...

But this error looks similar to the one reported in #1408, not in #1416

[Ailrun]

I think your nix shell is somewhat outdated, and uses old package... the message says it still uses 0.11.0.3.

[berberman]

Oh! I see. I entered the nix-shell before pulling your fix. Now it works

[pepeiborra]

looks good to me, thanks!

[jneira]

Tests are failing due to the order of pragmas:

test/functional/FunctionalCodeAction.hs:529:
        expected: ["{-# LANGUAGE ScopedTypeVariables #-}","{-# LANGUAGE TypeApplications #-}","module TypeApplications where","","foo :: forall a. a -> a","foo = id @a"]
         but got: ["{-# LANGUAGE TypeApplications #-}","{-# LANGUAGE ScopedTypeVariables #-}","module TypeApplications where","","foo :: forall a. a -> a","foo = id @a"]

[berberman]

Perhaps the order is unstable? I copied endOfModuleHeader from #1235 without any changes. To be investigated...

test/functional/FunctionalCodeAction.hs:529:
expected: ["{-# LANGUAGE TypeApplications #-}","{-# LANGUAGE ScopedTypeVariables #-}","module TypeApplications where","","foo :: forall a. a -> a","foo = id @a"]
but got: ["{-# LANGUAGE ScopedTypeVariables #-}","{-# LANGUAGE TypeApplications #-}","module TypeApplications where","","foo :: forall a. a -> a","foo = id @a"]

[berberman]

No it's stable. I executed the code action manually on my machine (GHC 8.10.3): TypeApplications was always inserted above ScopedTypeVariables.

Perhaps the order is unstable? I copied endOfModuleHeader from #1235 without any changes. To be investigated...

test/functional/FunctionalCodeAction.hs:529:
expected: ["{-# LANGUAGE TypeApplications #-}","{-# LANGUAGE ScopedTypeVariables #-}","module TypeApplications where","","foo :: forall a. a -> a","foo = id @a"]
but got: ["{-# LANGUAGE ScopedTypeVariables #-}","{-# LANGUAGE TypeApplications #-}","module TypeApplications where","","foo :: forall a. a -> a","foo = id @a"]

This error was reported in GHC 8.8.4, and 8.6.5 (maybe all under 8.10), so the order likely depend on the version of GHC...

[jneira]

Hi, i've seen some changes here similar to #1340, concretely the move of endOfModuleHeader, which looks very similar to startOfModuleHeaderin that pr. Should this fix the issue of inserted pragmas before she-bang? That one should be rebased onto this in any case
//cc ishmum123

[berberman]

This PR should fix #555, but I didn't add the corresponding test, which will be finished by #1340 instead. So @ishmum123 should rebase #1340 on this, if we want to merge #1340.

[berberman]

Tests can pass now by adding this, though it's not clear to me...

[jneira]

Well, we could force a ordering (alphabetical? i would say pre 8.10 is the right one) but not sure if it worths tbh

[berberman]

In this test case, getParsedModule should return Nothing, because the module will result in parse error without enabling TypeApplications. So the insert line is always 0, which means TypeApplications will be inserted to the top (and inserting after the shebang will not work). But in tests under GHC 8.10, TypeApplications is inserted under ScopedTypeVariables, so It seems that in pre 8.10 cases GetParsedModule rule does not return Nothing.

Even if GHC parser returns POk instead of PFailed for non-fatal errors like this one since 8.10, parseFileContents should fail immediately when we encounter errors (if I understand correctly), so it's weird that GetParsedModule rule does not return Nothing...

[jneira]

Weird indeed, i think #1340 failed to pass tests for the same reason

[berberman]

Hmm actually #1340 failed to pass is expected. Test added by #1340 uses TypeApplications as the test object. As mentioned above, missing TypeApplications will result in parse error, so "after shebang" won't work in this case. #1340 should change another pragma to test and mark this case as an expected failure.

[jneira]

oh, i see thanks for the insight
what would be the solution in that case (shebang + needed TypeApplications)?
The plugin should suggest add it (to fix the parse error itself) but it would add it in the incorrect position (as we dont have the parsed module)

Maybe that combination is improbable though...

[berberman]

We could use file contents as the fallback. But imo what's really rare is shebang -- inserting pragmas shouldn't have been depended on parsed module...

[ishmum123]

@berberman what would you suggest for #1340 ? Should I expect a different Pragma or remove dependency upon the parsed module?

[berberman]

@ishmum123 Please change another pragma to be tested with shebang.

[berberman]

But in tests under GHC 8.10, TypeApplications is inserted under ScopedTypeVariables, so It
seems that in pre 8.10 cases GetParsedModule rule does not return Nothing.

All tests are finished. The return value of getParsedModule in test case "Adds TypeApplications pragma":

GHC 8.10.4

Nothing

GHC 8.8.4

({ TypeApplications.hs:1:1
    }
 (HsModule
  (Just
   ({ TypeApplications.hs:2:8-23
       }
    {ModuleName: TypeApplications}))
  (Nothing)
  []
  [({ TypeApplications.hs:4:1-23
       }
    (SigD
     (NoExt)
     (TypeSig
      (NoExt)
      [({ TypeApplications.hs:4:1-3
           }
        (Unqual
         {OccName: foo}))]
      (HsWC
       (NoExt)
       (HsIB
        (NoExt)
        ({ TypeApplications.hs:4:8-23
            }
         (HsForAllTy
          (NoExt)
          [({ TypeApplications.hs:4:15
               }
            (UserTyVar
             (NoExt)
             ({ TypeApplications.hs:4:15
                 }
              (Unqual
               {OccName: a}))))]
          ({ TypeApplications.hs:4:18-23
              }
           (HsFunTy
            (NoExt)
            ({ TypeApplications.hs:4:18
                }
             (HsTyVar
              (NoExt)
              (NotPromoted)
              ({ TypeApplications.hs:4:18
                  }
               (Unqual
                {OccName: a}))))
            ({ TypeApplications.hs:4:23
                }
             (HsTyVar
              (NoExt)
              (NotPromoted)
              ({ TypeApplications.hs:4:23
                  }
               (Unqual
                {OccName: a})))))))))))))
  ,({ TypeApplications.hs:5:1-11
       }
    (ValD
     (NoExt)
     (FunBind
      (NoExt)
      ({ TypeApplications.hs:5:1-3
          }
       (Unqual
        {OccName: foo}))
      (MG
       (NoExt)
       ({ TypeApplications.hs:5:1-11
           }
        [({ TypeApplications.hs:5:1-11
             }
          (Match
           (NoExt)
           (FunRhs
            ({ TypeApplications.hs:5:1-3
                }
             (Unqual
              {OccName: foo}))
            (Prefix)
            (NoSrcStrict))
           []
           (GRHSs
            (NoExt)
            [({ TypeApplications.hs:5:5-11
                 }
              (GRHS
               (NoExt)
               []
               ({ TypeApplications.hs:5:7-11
                   }
                (EAsPat
                 (NoExt)
                 ({ TypeApplications.hs:5:7-8
                     }
                  (Unqual
                   {OccName: id}))
                 ({ TypeApplications.hs:5:11
                     }
                  (HsVar
                   (NoExt)
                   ({ TypeApplications.hs:5:11
                       }
                    (Unqual
                     {OccName: a}))))))))]
            ({ <no location info> }
             (EmptyLocalBinds
              (NoExt))))))])
       (FromSource))
      (WpHole)
      [])))]
  (Nothing)
  (Nothing)))

GHC 8.6.5

({ TypeApplications.hs:1:1
    }
 (HsModule
  (Just
   ({ TypeApplications.hs:2:8-23
       }
    {ModuleName: TypeApplications}))
  (Nothing)
  []
  [({ TypeApplications.hs:4:1-23
       }
    (SigD
     (NoExt)
     (TypeSig
      (NoExt)
      [({ TypeApplications.hs:4:1-3
           }
        (Unqual
         {OccName: foo}))]
      (HsWC
       (NoExt)
       (HsIB
        (NoExt)
        ({ TypeApplications.hs:4:8-23
            }
         (HsForAllTy
          (NoExt)
          [({ TypeApplications.hs:4:15
               }
            (UserTyVar
             (NoExt)
             ({ TypeApplications.hs:4:15
                 }
              (Unqual
               {OccName: a}))))]
          ({ TypeApplications.hs:4:18-23
              }
           (HsFunTy
            (NoExt)
            ({ TypeApplications.hs:4:18
                }
             (HsTyVar
              (NoExt)
              (NotPromoted)
              ({ TypeApplications.hs:4:18
                  }
               (Unqual
                {OccName: a}))))
            ({ TypeApplications.hs:4:23
                }
             (HsTyVar
              (NoExt)
              (NotPromoted)
              ({ TypeApplications.hs:4:23
                  }
               (Unqual
                {OccName: a})))))))))))))
  ,({ TypeApplications.hs:5:1-11
       }
    (ValD
     (NoExt)
     (FunBind
      (NoExt)
      ({ TypeApplications.hs:5:1-3
          }
       (Unqual
        {OccName: foo}))
      (MG
       (NoExt)
       ({ TypeApplications.hs:5:1-11
           }
        [({ TypeApplications.hs:5:1-11
             }
          (Match
           (NoExt)
           (FunRhs
            ({ TypeApplications.hs:5:1-3
                }
             (Unqual
              {OccName: foo}))
            (Prefix)
            (NoSrcStrict))
           []
           (GRHSs
            (NoExt)
            [({ TypeApplications.hs:5:5-11
                 }
              (GRHS
               (NoExt)
               []
               ({ TypeApplications.hs:5:7-11
                   }
                (EAsPat
                 (NoExt)
                 ({ TypeApplications.hs:5:7-8
                     }
                  (Unqual
                   {OccName: id}))
                 ({ TypeApplications.hs:5:11
                     }
                  (HsVar
                   (NoExt)
                   ({ TypeApplications.hs:5:11
                       }
                    (Unqual
                     {OccName: a}))))))))]
            ({ <no location info> }
             (EmptyLocalBinds
              (NoExt))))))])
       (FromSource))
      (WpHole)
      [])))]
  (Nothing)
  (Nothing)))

[jneira]

@beberman many thanks, great work and lot of useful info in the way