Refactor pragmas plugin #1417

berberman · 2021-02-21T05:25:47Z

~~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 · 2021-02-21T05:28:17Z

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

berberman · 2021-02-21T05:32:55Z

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 · 2021-02-21T05:34:40Z

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

berberman · 2021-02-21T05:39:16Z

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

pepeiborra

looks good to me, thanks!

jneira · 2021-02-21T11:37:00Z

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 · 2021-02-21T13:12:11Z

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 · 2021-02-21T13:40:46Z

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 · 2021-02-22T06:23:12Z

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 · 2021-02-22T06:31:40Z

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 · 2021-02-22T06:35:13Z

test/functional/FunctionalCodeAction.hs

+-- TODO: Why CPP???
+#if __GLASGOW_HASKELL__ < 810
+                    [ "{-# LANGUAGE ScopedTypeVariables #-}"
+                    , "{-# LANGUAGE TypeApplications #-}"
+#else
                    [ "{-# LANGUAGE TypeApplications #-}"
                    , "{-# LANGUAGE ScopedTypeVariables #-}"
+#endif


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

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

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...

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

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.

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...

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...

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

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

berberman · 2021-02-22T12:14:28Z

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 · 2021-02-22T20:40:52Z

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

berberman added 2 commits February 21, 2021 13:17

Refactor pragmas plugin

c90fd07

Revert some format changes

7c771c6

Revert some format changes

8c0ea29

berberman added 6 commits February 21, 2021 13:40

Run pre-commit hook

298be74

Fix and format class test

46fe2a0

Merge branch 'master' into refactor-pragmas

b24fb6d

Fix and format func-test (code action)

4007428

Merge branch 'master' into refactor-pragmas

f9653d0

Update test

7cdb788

pepeiborra approved these changes Feb 21, 2021

View reviewed changes

Update test

88f01d3

berberman force-pushed the refactor-pragmas branch from ab99b2e to 88f01d3 Compare February 21, 2021 13:58

berberman added 3 commits February 21, 2021 23:11

Merge branch 'master' into refactor-pragmas

c728684

Merge branch 'master' into refactor-pragmas

2f0fa18

Don't look contents in endOfModuleHeader

eebf6b6

berberman commented Feb 22, 2021

View reviewed changes

berberman marked this pull request as draft February 22, 2021 09:16

berberman force-pushed the refactor-pragmas branch 4 times, most recently from 380a65a to 7643361 Compare February 22, 2021 10:17

berberman force-pushed the refactor-pragmas branch from 7643361 to 8622380 Compare February 22, 2021 10:54

berberman force-pushed the refactor-pragmas branch from 8622380 to eebf6b6 Compare February 22, 2021 12:39

berberman marked this pull request as ready for review February 22, 2021 14:18

jneira merged commit 1c45629 into haskell:master Feb 22, 2021

berberman deleted the refactor-pragmas branch February 23, 2021 02:22

jneira mentioned this pull request Apr 15, 2021

Insert pragmas after shebang or to existing pragma list #1731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pragmas plugin #1417

Refactor pragmas plugin #1417

berberman commented Feb 21, 2021 •

edited

Loading

Ailrun commented Feb 21, 2021

berberman commented Feb 21, 2021

Ailrun commented Feb 21, 2021

berberman commented Feb 21, 2021

pepeiborra left a comment

jneira commented Feb 21, 2021

berberman commented Feb 21, 2021

berberman commented Feb 21, 2021

jneira commented Feb 22, 2021

berberman commented Feb 22, 2021

berberman Feb 22, 2021

jneira Feb 22, 2021

berberman Feb 22, 2021

jneira Feb 22, 2021

berberman Feb 22, 2021

jneira Feb 22, 2021

berberman Feb 22, 2021

ishmum123 Feb 22, 2021

berberman Feb 22, 2021

berberman commented Feb 22, 2021 •

edited

Loading

jneira commented Feb 22, 2021

Refactor pragmas plugin #1417

Refactor pragmas plugin #1417

Conversation

berberman commented Feb 21, 2021 • edited Loading

Ailrun commented Feb 21, 2021

berberman commented Feb 21, 2021

Ailrun commented Feb 21, 2021

berberman commented Feb 21, 2021

pepeiborra left a comment

Choose a reason for hiding this comment

jneira commented Feb 21, 2021

berberman commented Feb 21, 2021

berberman commented Feb 21, 2021

jneira commented Feb 22, 2021

berberman commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berberman commented Feb 22, 2021 • edited Loading

jneira commented Feb 22, 2021

berberman commented Feb 21, 2021 •

edited

Loading

berberman commented Feb 22, 2021 •

edited

Loading