Use the ghc api to get imports #32

googleson78 · 2022-05-12T11:43:27Z

This replaces our own hand rolled parser.

Fixes #31

This replaces our own hand rolled parser.

We no longer use it!

himportscan/src/HImportScan/ImportScanner.hs

googleson78 · 2022-05-12T11:45:17Z

himportscan/src/HImportScan/ImportScanner.hs

+    -- TODO[GL]: propagate error instead
+    Left err -> do
+      GHC.printBagOfErrors dynFlags err
+      pure Nothing


Maybe I should use error here instead, as was previously done.

I think an error would be preferable. If we don't assume bugs in our code, it would indicate something to fix in the source file.

googleson78 · 2022-05-12T11:46:39Z

This will introduce more work over at #25

facundominguez

Thanks @googleson78! Looks pretty good. I left some comments.

himportscan/src/HImportScan/ImportScanner.hs

facundominguez · 2022-05-12T12:09:04Z

himportscan/src/HImportScan/ImportScanner.hs

+
+  let
+    -- [GL] The fact that the resulting strings here contain the "-X"s makes me a bit doubtful that this is the right approach,
+    -- but this is what I found for now.


usesTH is an heuristic that offers the user a convenient way to show that TH is needed, not an exact detection solution. 🤷

himportscan/src/HImportScan/ImportScanner.hs

facundominguez · 2022-05-12T12:25:54Z

himportscan/src/HImportScan/ImportScanner.hs

+    -- TODO[GL]: propagate error instead
+    Left err -> do
+      GHC.printBagOfErrors dynFlags err
+      pure Nothing


I think an error would be preferable. If we don't assume bugs in our code, it would indicate something to fix in the source file.

himportscan/src/HImportScan/ImportScanner.hs

himportscan/src/HImportScan/GHC/Settings.hs

facundominguez · 2022-05-12T12:51:12Z

himportscan/src/HImportScan/ImportScanner.hs

+        map GHC.unLoc $
+          GHC.getOptions dynFlags sb filePath
+  -- TODO: should both of the files here be filePath?
+  GHC.getImports dynFlags sb filePath filePath >>= \case


getImports can throw exceptions. There are functions to get those formatted too, but perhaps it is fine to have them kill the program without further ado.

We would crash later anyways, because the go part can't parse the message we produce there. This is justified because if the ghc parser fails something is very wrong.

It's fine to pass both filePath as both arguments there because they provide enough info and we aren't that worried about formatting of error messages.

facundominguez · 2022-05-13T11:28:36Z

himportscan/BUILD.bazel

 haskell_toolchain_library(name = "directory")

 haskell_toolchain_library(name = "ghc")

+# Only necessary to be able to import GHC.Platform in HImportScan.GHC.Settings
+# TODO[GL]: we can probably remove this once we only support ghc >=9.2
+haskell_toolchain_library(name = "ghc-boot")


Maybe this is redundant now.

Unfortunately it isn't - it seems I introduced another dependency on this package without knowing. Updated the comment.

himportscan/src/HImportScan/ImportScanner.hs

facundominguez · 2022-05-13T17:21:28Z

In some closed source file I get this error,

gazelle: himportscan: Illegal keyword 'type' (use ExplicitNamespaces to enable)

Probably we will need to enable every extension that affects the parsing of the import/export lists (PatternSynonyms comes to mind too).

I tested with and without runGhc too. There is a difference in performance, where the version with runGhc is 1.8 times slower. I think it would be worth reverting to the hand made dflags.

facundominguez · 2022-05-13T17:22:45Z

Also, note that when we get the error message, there is no indication of the file that produced the failure. We should have himportscan inform that or the user of gazelle won't have a clue.

facundominguez · 2022-05-13T17:26:56Z

I contributed some commits unrelated to my last comments. Let me know if you'd like any of those changed.

googleson78 · 2022-05-15T14:15:39Z

himportscan/src/HImportScan/ImportScanner.hs

@@ -98,7 +99,7 @@ scanImports dynFlags filePath contents = do
    -- This is far from ideal, however handling this better would require being able to communicate errors better to go.
    Left err -> do
      GHC.printBagOfErrors dynFlagsWithExtensions err
-      error "ghc parsing failed"
+      throwIO (GHC.mkSrcErr err)


Does this commit fix the issue where users don't know what parsing failed?

No, it doesn't. I tested this change before writing about it.

Right now, after my change which splits stderr and stdout, we get the parse error that ghc reports, e.g.

/home/googleson78/git/gazelle_haskell_modules/example/package-a/app/Main.hs:9:59: error: parse error on input `PatTrue' | 9 | import PackageA.Other.C(type DataC, DataC(DataC), pattern PatTrue) | ^^^^^^^ himportscan: parse error on input `PatTrue' gazelle: himportscan exited unsuccessfully exit status 1

Is this good enough, or should we explicitly report the file name separately?

googleson78 · 2022-05-16T09:08:52Z

How big is the 1.8 times difference considering all the other processing and generating going on. Is it 1.8 for the whole process, or only for the himportscan part? In other words, should I revert to the fake DynFlags solution?

facundominguez · 2022-05-16T13:42:56Z

Is it 1.8 for the whole process, or only for the himportscan part?

It is 1.8 for the whole process.

In other words, should I revert to the fake DynFlags solution?

I think this would be preferable.

This turns out to be much faster (close to 2 times on real world project).

googleson78 · 2022-05-17T09:39:52Z

I'm very surprised, but currently (or in general?) getImports seems to be totally ignoring "parse (lex?) errors" - for example, if I change import to mport in a file, gazelle still runs "successfully" (implying that himportscan does so as well). It just skips the files that have lex/parse errors in them, even though we should currently be throwing in the Left case of getImports. It seems it's never reached.

I need to investigate more.

googleson78 · 2022-05-18T08:51:24Z

I have confirmed that it returns a Right, and I have also formed a hypothesis on why:

Even if we have a typo, e.g. port or imort, this does not exclude port and imort from being some CPP macros or TH top level splices. Hence, it makes sense for getImports to not report an error.

However, I also checked the behaviour with the current implementation, and it (sensibly) behaves the same way. So I think that this might not be a regression at all (unlike I was previously assuming) and we can safely "ignore" it.

googleson78 · 2022-05-18T09:11:24Z

I added PatternSynonyms, ExplicitNamespaces and MagicHash. Do any others come to mind?

facundominguez · 2022-05-18T13:03:45Z

Do any others come to mind?

They don't. But I think they will be easy to add as we discover them.

Thanks for your insights and your patches.

googleson78 added 3 commits May 12, 2022 14:14

Use a set for importedModules

bb7e557

Use ghc to scan imports

6ace952

This replaces our own hand rolled parser.

Drop parsec stackage dep

37ab8fb

We no longer use it!

googleson78 commented May 12, 2022

View reviewed changes

himportscan/src/HImportScan/ImportScanner.hs Outdated Show resolved Hide resolved

googleson78 commented May 12, 2022

View reviewed changes

facundominguez reviewed May 12, 2022

View reviewed changes

googleson78 added 6 commits May 12, 2022 16:24

Crash instead of returning Maybe

1923cd4

We would crash later anyways, because the go part can't parse the message we produce there. This is justified because if the ghc parser fails something is very wrong.

Remove discussed TODO

0074047

It's fine to pass both filePath as both arguments there because they provide enough info and we aren't that worried about formatting of error messages.

Reverse if not condition

52eb37c

Use field pun for usesTH

6ec74f7

Go through a BS instead of String

4a9a81b

Use runGhc instead of faking DynFlags

813a801

facundominguez reviewed May 13, 2022

View reviewed changes

googleson78 and others added 6 commits May 13, 2022 15:14

Update comment on ghc-boot

5c45866

Rethrow errors

afde376

Don't convert to string when preprocessing a source file

fd0fb8d

Remove moduleNameToText from GHC.Utils

bcca68d

Remove GHC.Utils

b6fd38f

Edit comment on handling parsing failures

9600894

facundominguez reviewed May 13, 2022

View reviewed changes

himportscan/src/HImportScan/ImportScanner.hs Outdated Show resolved Hide resolved

googleson78 commented May 15, 2022

View reviewed changes

Bring back faking DynFlags

40a52cb

This turns out to be much faster (close to 2 times on real world project).

Handle himportscan stdout and stderr separately

55caa5d

Enable and test PatternSynonymss and ExplicitNamespaces

a62dd95

Enable and test MagicHash

eaefe78

googleson78 force-pushed the 31_ghc-api-imports branch from d941324 to eaefe78 Compare May 18, 2022 09:13

facundominguez merged commit 1578ad5 into main May 18, 2022

facundominguez deleted the 31_ghc-api-imports branch May 18, 2022 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the ghc api to get imports #32

Use the ghc api to get imports #32

googleson78 commented May 12, 2022 •

edited

Loading

googleson78 May 12, 2022

facundominguez May 12, 2022

googleson78 May 13, 2022

googleson78 commented May 12, 2022

facundominguez left a comment

facundominguez May 12, 2022

facundominguez May 12, 2022

facundominguez May 12, 2022

facundominguez May 13, 2022

googleson78 May 13, 2022

facundominguez commented May 13, 2022

facundominguez commented May 13, 2022

facundominguez commented May 13, 2022

googleson78 May 15, 2022

facundominguez May 15, 2022

googleson78 May 18, 2022

googleson78 commented May 16, 2022

facundominguez commented May 16, 2022

googleson78 commented May 17, 2022

googleson78 commented May 18, 2022

googleson78 commented May 18, 2022

facundominguez commented May 18, 2022

Use the ghc api to get imports #32

Use the ghc api to get imports #32

Conversation

googleson78 commented May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googleson78 commented May 12, 2022

facundominguez left a 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

facundominguez commented May 13, 2022

facundominguez commented May 13, 2022

facundominguez commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googleson78 commented May 16, 2022

facundominguez commented May 16, 2022

googleson78 commented May 17, 2022

googleson78 commented May 18, 2022

googleson78 commented May 18, 2022

facundominguez commented May 18, 2022

googleson78 commented May 12, 2022 •

edited

Loading