Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add fix for correct placement of import (#2100) #2116

Merged
merged 2 commits into from
Sep 4, 2021

Conversation

nini-faroux
Copy link
Contributor

hi, I added a fix for the import placement issue. (#2100)

So, the way it worked previously was to use the 'HsModule' type to find the next import position. Specifically it had two cases, when there's no existing imports in the file it would use the 'hsmodDecls' field, with type [LHsDecl name], and otherwise it would use 'hsmodImports', with type [LImportDecl name]'.

In the first case (hsmodDecls) the position would be one above the first element in that list, and this worked fine in general unless there was something above it which wasn't included in the list, like the comment in the example in the issue.

So for the fix, firstly it's changed to use the 'hsmodName' and 'hsmodExports' fields in 'HsModule' instead of 'hsmodDecls' to calculate the position in the case that the file included a 'module ... where' declaration, with no existing imports in the file. So in this case the new import will be added directly under the module declaration. 

But in the case where there is no module declaration and no existing imports I used the same logic as in the pragma plugin, basically the import will be one under the last file-header pragma or else at the very top. I needed to change a few functions to do this, just to pass down the Text version of the file contents, as there wasn't enough information in the 'HsModule' type to manage it otherwise.

I left the other case, where there are existing imports in the file the way it was, as I think inserting the next import directly underneath the last existing import is a sensible thing to do. This would only be a problem I guess if the user has placed the existing import in the wrong place themselves, or if there's another bug that does that.

I also added a good few tests, I hope it covers most of the cases!

@pepeiborra pepeiborra requested a review from berberman August 21, 2021 18:04
@pepeiborra
Copy link
Collaborator

Looks like the 8.6.5 build needs some compatibility fix:

src/Development/IDE/Plugin/CodeAction.hs:88:69: error:
1734
Error:     Module ‘SrcLoc’ does not export ‘HasSrcSpan(..)’
1735
   |
1736
88 | import           SrcLoc                                            (HasSrcSpan (..),
1737
   |                                                                     ^^^^^^^^^^^^^^^
1738

The usual recommendation is to import Development.IDE.GHC.Compat instead of direct GHC modules.

@nini-faroux
Copy link
Contributor Author

Ah ok, that extra import wasn't needed at all so, I'll update it tomorrow.

@nini-faroux
Copy link
Contributor Author

I removed that extra 'HasSrcSpan' import, but actually just checking now I noticed I'll have to amend the expected value of some of the older import tests too, as there's a few of them failing now because of the different behaviour - I can do this tomorrow.

Comment on lines 1293 to 1300
-- | Finds the next valid position for inserting a new import declaration
-- If the file already has existing imports it will be inserted under the last of these,
-- it is assumed that the existing last import declaration is in a valid position
-- If the file does not have existing imports, but has a (module ... where) declaration,
-- the new import will be inserted directly under this declaration (accounting for explicit exports)
-- If the file has neither existing imports nor a module declaration,
-- the import will be inserted at line zero if there are no pragmas,
-- otherwise inserted one line after the last file-header pragma
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-- | Finds the next valid position for inserting a new import declaration
-- If the file already has existing imports it will be inserted under the last of these,
-- it is assumed that the existing last import declaration is in a valid position
-- If the file does not have existing imports, but has a (module ... where) declaration,
-- the new import will be inserted directly under this declaration (accounting for explicit exports)
-- If the file has neither existing imports nor a module declaration,
-- the import will be inserted at line zero if there are no pragmas,
-- otherwise inserted one line after the last file-header pragma
-- | Finds the next valid position for inserting a new import declaration
-- * If the file already has existing imports it will be inserted under the last of these,
-- it is assumed that the existing last import declaration is in a valid position
-- * If the file does not have existing imports, but has a (module ... where) declaration,
-- the new import will be inserted directly under this declaration (accounting for explicit exports)
-- * If the file has neither existing imports nor a module declaration,
-- the import will be inserted at line zero if there are no pragmas,
-- * otherwise inserted one line after the last file-header pragma

Comment on lines 1309 to 1311
-- | Insert the import under the Module declaration exports if they exist, otherwise just under the module declaration
-- If no module declaration exists, then no exports will exist either, in that case
-- insert the import after any file-header pragmas or at position zero if there are no pragmas
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-- | Insert the import under the Module declaration exports if they exist, otherwise just under the module declaration
-- If no module declaration exists, then no exports will exist either, in that case
-- insert the import after any file-header pragmas or at position zero if there are no pragmas
-- | Insert the import under the Module declaration exports if they exist, otherwise just under the module declaration.
-- If no module declaration exists, then no exports will exist either, in that case
-- insert the import after any file-header pragmas or at position zero if there are no pragmas

@@ -209,7 +209,7 @@ extendImportHandler' ideState ExtendImport {..}
it = case thingParent of
Nothing -> newThing
Just p -> p <> "(" <> newThing <> ")"
t <- liftMaybe $ snd <$> newImportToEdit n (astA ps)
t <- liftMaybe $ snd <$> newImportToEdit n (astA ps) ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this do the right thing if the module has a module name but no imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, because of the empty string? I think it will be fine for that case as if the module has a name but no imports the new import will be added directly under the module declaration via the 'hsmodName' type, or via 'hsmodExports' if the declaration contains explicit exports.

But it would be an issue actually when the module has neither a module name nor imports - in that case it relies on the 'fileContents' argument when calling 'findNextPragmaPosition', but if it's just getting an empty string from there it won't always be accurate.

I guess calling 'GetFileContents' earlier in the "extendImportHandler'" function, and then having (fromMaybe "" contents) in the line you highlighted should cover that case? I can check this evening and add a test too.

@pepeiborra
Copy link
Collaborator

Looks like there are some Windows specific test failures related to newlines:

ghcide
1064
  code actions
1065
    insert import
1066
      above comment at top of module:                              FAIL (3.05s)
1067
        test\exe\Main.hs:835:
1068
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\n-- | Some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1069
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\n-- | Some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1070
      above multiple comments below:                               FAIL (3.46s)
1071
        test\exe\Main.hs:835:
1072
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\n-- | Another comment\r\ndata SomethingElse = SomethingElse\r\n\r\n-- | Some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1073
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\n-- | Another comment\r\ndata SomethingElse = SomethingElse\r\n\r\n-- | Some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1074
      above curly brace comment:                                   FAIL (3.10s)
1075
        test\exe\Main.hs:835:
1076
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\n{- Some comment -}\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1077
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\n{- Some comment -}\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1078
      above multi-line comment:                                    FAIL (3.51s)
1079
        test\exe\Main.hs:835:
1080
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\n{- Some multi \r\n   line comment \r\n-}\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1081
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\n{- Some multi \r\n   line comment \r\n-}\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1082
      above comment with no module explicit exports:               FAIL (3.13s)
1083
        test\exe\Main.hs:835:
1084
        expected: "module Test where\r\nimport Data.Monoid\r\n\r\n-- | a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1085
         but got: "module Test where\r\nimport Data.Monoid\n\r\n-- | a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1086
      above two-dash comment with no pipe:                         FAIL (3.49s)
1087
        test\exe\Main.hs:835:
1088
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\n-- no vertical bar comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1089
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\n-- no vertical bar comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1090
      above comment with no (module .. where) decl:                FAIL (3.22s)
1091
        test\exe\Main.hs:835:
1092
        expected: "import Data.Monoid\r\n-- a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1093
         but got: "import Data.Monoid\n-- a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1094
      comment not at top with no (module .. where) decl:           FAIL (3.56s)
1095
        test\exe\Main.hs:835:
1096
        expected: "import Data.Monoid\r\nnewtype Something = S { foo :: Int }\r\n\r\n-- | a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1097
         but got: "import Data.Monoid\nnewtype Something = S { foo :: Int }\r\n\r\n-- | a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1098
      comment not at top (data dec is):                            FAIL (3.55s)
1099
        test\exe\Main.hs:835:
1100
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\ndata Something = Something\r\n\r\n-- | some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1101
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\ndata Something = Something\r\n\r\n-- | some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1102
      comment not at top (newtype is):                             FAIL (3.51s)
1103
        test\exe\Main.hs:835:
1104
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\nnewtype Something = S { foo :: Int }\r\n\r\n-- | a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1105
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\nnewtype Something = S { foo :: Int }\r\n\r\n-- | a comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1106
      with no explicit module exports:                             FAIL (3.14s)
1107
        test\exe\Main.hs:835:
1108
        expected: "module Test where\r\nimport Data.Monoid\r\n\r\nnewtype Something = S { foo :: Int }\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1109
         but got: "module Test where\r\nimport Data.Monoid\n\r\nnewtype Something = S { foo :: Int }\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1110
      add to correctly placed exisiting import:                    FAIL (3.14s)
1111
        test\exe\Main.hs:835:
1112
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\n\r\nimport Data.Char\r\nimport Data.Monoid\r\n\r\n{- Some multi \r\n   line comment \r\n-}\r\nclass Semigroup a => SomeData a\r\n\r\n-- | a comment\r\ninstance SomeData All\r\n"
1113
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\n\r\nimport Data.Char\r\nimport Data.Monoid\n\r\n{- Some multi \r\n   line comment \r\n-}\r\nclass Semigroup a => SomeData a\r\n\r\n-- | a comment\r\ninstance SomeData All\r\n"
1114
      add to multiple correctly placed exisiting imports:          FAIL (3.56s)
1115
        test\exe\Main.hs:835:
1116
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\n\r\nimport Data.Char\r\nimport Data.Bool\r\nimport Data.Eq\r\nimport Data.Monoid\r\n\r\n-- | A comment\r\nclass Semigroup a => SomeData a\r\n\r\n-- | another comment\r\ninstance SomeData All\r\n"
1117
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\n\r\nimport Data.Char\r\nimport Data.Bool\r\nimport Data.Eq\r\nimport Data.Monoid\n\r\n-- | A comment\r\nclass Semigroup a => SomeData a\r\n\r\n-- | another comment\r\ninstance SomeData All\r\n"
1118
      with language pragma at top of module:                       FAIL (3.13s)
1119
        test\exe\Main.hs:835:
1120
        expected: "{-#\160LANGUAGE OverloadedStrings #-}\r\n\r\nmodule Test where\r\nimport Data.Monoid\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1121
         but got: "{-#\160LANGUAGE OverloadedStrings #-}\r\n\r\nmodule Test where\r\nimport Data.Monoid\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1122
      with language pragma and explicit module exports:            FAIL (3.51s)
1123
        test\exe\Main.hs:835:
1124
        expected: "{-#\160LANGUAGE OverloadedStrings #-}\r\n\r\nmodule Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\n-- comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1125
         but got: "{-#\160LANGUAGE OverloadedStrings #-}\r\n\r\nmodule Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\n-- comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1126
      with language pragma at top and no module declaration:       FAIL (3.61s)
1127
        test\exe\Main.hs:835:
1128
        expected: "{-#\160LANGUAGE OverloadedStrings #-}\r\nimport Data.Monoid\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1129
         but got: "{-#\160LANGUAGE OverloadedStrings #-}\r\nimport Data.Monoid\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1130
      with multiple lang pragmas and no module declaration:        FAIL (3.12s)
1131
        test\exe\Main.hs:835:
1132
        expected: "{-# LANGUAGE OverloadedStrings #-}\r\n{-#\160LANGUAGE DataKinds #-}\r\n{-#\160LANGUAGE RankNTypes #-}\r\nimport Data.Monoid\r\n\r\n-- some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1133
         but got: "{-# LANGUAGE OverloadedStrings #-}\r\n{-#\160LANGUAGE DataKinds #-}\r\n{-#\160LANGUAGE RankNTypes #-}\r\nimport Data.Monoid\n\r\n-- some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1134
      with pragmas and shebangs:                                   FAIL (3.59s)
1135
        test\exe\Main.hs:835:
1136
        expected: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\nimport Data.Monoid\r\n\r\n-- some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1137
         but got: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\nimport Data.Monoid\n\r\n-- some comment\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1138
      with pragmas and shebangs but no comment at top:             FAIL (3.12s)
1139
        test\exe\Main.hs:835:
1140
        expected: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\nimport Data.Monoid\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1141
         but got: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\nimport Data.Monoid\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1142
      module decl no exports under pragmas and shebangs:           FAIL (3.55s)
1143
        test\exe\Main.hs:835:
1144
        expected: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\n\r\nmodule Test where\r\nimport Data.Monoid\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1145
         but got: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\n\r\nmodule Test where\r\nimport Data.Monoid\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1146
      module decl with explicit import under pragmas and shebangs: FAIL (3.15s)
1147
        test\exe\Main.hs:835:
1148
        expected: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\n\r\nmodule Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1149
         but got: "{-# LANGUAGE OverloadedStrings #-}\r\n{-# OPTIONS_GHC -Wall #-}\r\n#! /usr/bin/env nix-shell\r\n#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\"\r\n\r\nmodule Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Monoid\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1150
      module decl and multiple imports:                            FAIL (3.52s)
1151
        test\exe\Main.hs:835:
1152
        expected: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Char\r\nimport Data.Array\r\nimport Data.Monoid\r\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1153
         but got: "module Test\r\n( SomeData(..)\r\n) where\r\nimport Data.Char\r\nimport Data.Array\r\nimport Data.Monoid\n\r\nclass Semigroup a => SomeData a\r\n\r\ninstance SomeData All\r\n"
1154

1155
22 out of 22 tests failed (73.80s)

@nini-faroux
Copy link
Contributor Author

Oh ok, so just after the new import it's getting '\n' but expecting '\r\n', I'll have a look this evening.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Other than the failing tests, this looks good to me. Thanks again @nini-faroux !

@nini-faroux
Copy link
Contributor Author

I think it should be ok for the windows tests now too.

No worries! let me know if there's any issues with it. Thanks for the help @pepeiborra and @jneira

@jneira jneira linked an issue Aug 28, 2021 that may be closed by this pull request
@jneira
Copy link
Member

jneira commented Aug 28, 2021

hlint is sad:

Run ./fmt.sh
Downloading and running hlint...
#=#=#                                                                         

######################################################################## 100.0%
                                                                           0.1%
####################################################################      94.5%
######################################################################## 100.0%
ghcide/src/Development/IDE/Core/FileStore.hs:292:9-33: Warning: Use void
Found:
  () <$ uses GetModIface rs
Perhaps:
  Control.Monad.void (uses GetModIface rs)

1 hint

@jneira
Copy link
Member

jneira commented Aug 28, 2021

hlint is sad

i think it is caused by some hlint change, i ve tried to fix it with 6e9d0bb

@ndmitchell
Copy link
Collaborator

I just released a new version of HLint that had this new hint added.

@jneira jneira added the merge me Label to trigger pull request merge label Sep 1, 2021
* Remove HasSrcSpan import

* Amend extendImportHandler' function

* Amend old import tests

* Adapt tests for windows newline issue
@mergify mergify bot merged commit 2358362 into haskell:master Sep 4, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import Regression: Wrong Import Placement on first Import
4 participants