GHCi does not need a main function
ClosedPublic

Authored by RolandSenn on Sep 19 2018, 7:45 AM.

Details

Summary

In GHCi we don't check anymore, whether a main function is exported.

Test Plan

make test TEST=T11647

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
RolandSenn created this revision.Sep 19 2018, 7:45 AM
RolandSenn retitled this revision from GHCi: A module without export list must re-export imported main (#11647) to GHCi does not honour implicit `module Main (main) where` for re-exported `main`s.Sep 19 2018, 7:49 AM
RolandSenn edited the summary of this revision. (Show Details)
osa1 accepted this revision.Sep 20 2018, 1:29 AM

Thanks! LGTM but added an inline comment.

Perhaps @mpickering should take a look before merging as he worked on this code before.

compiler/typecheck/TcRnExports.hs
136 ↗(On Diff #18056)

I think this comment is confusing because the existing comment already says that Trac #11647 should be handled correctly:

EXCEPT in interactive mode, when we behave as if he had
written "module Main where ..."

Because module Main where exports everything in scope, main imported in another module should also be exported.

So really this diff doesn't add a new exception, it just fixes what the existing comment says the code does.

This revision is now accepted and ready to land.Sep 20 2018, 1:29 AM
mpickering requested changes to this revision.Sep 20 2018, 9:28 AM

I chatted with Omer about this. Can you try the suggestion I left inline?

compiler/typecheck/TcRnExports.hs
151 ↗(On Diff #18056)

What happens if you just remove this special case and make no other changes?

176 ↗(On Diff #18056)

This function should compare RdrName rather than strings.

177 ↗(On Diff #18056)

I don't think this function will work if main is imported implicitly.

testsuite/tests/typecheck/should_run/T11647.stdout
1 ↗(On Diff #18056)

What's the purpose of this file?

This revision now requires changes to proceed.Sep 20 2018, 9:28 AM
RolandSenn marked 2 inline comments as done.Sep 20 2018, 3:08 PM

Thanks for this code review!
I'll rework this patch.

compiler/typecheck/TcRnExports.hs
136 ↗(On Diff #18056)

I'll remove my additonal lines in this comment!

151 ↗(On Diff #18056)

If we remove the line

| ghcLink dflags == LinkInMemory = Nothing

we can no longer load a module without a module header and without a main function
into GHCi .

eg, if we load the module

add :: Int -> Int -> Int
add x y = x + y`

into GHCi, we immediately get the error message:

Test2.hs:1:1: error:
    Not in scope: ‘main’
    Perhaps you meant ‘min’ (imported from Prelude)
  |
1 | add :: Int -> Int -> Int
  | ^

This is obvious, because we now export the name 'main' in the otherwise clause, but we don't have any main function.

The 2 lines

Reason: don't want to complain about 'main' not in scope
in interactive mode

in the comment, try to explain this.

176 ↗(On Diff #18056)

Excellent comment! I'll change the type (and the logic)

177 ↗(On Diff #18056)

Unfortunately, you are right!

I'll look into this. Either I have to search the implicit imports too, or to look up the name main in the variables in scope. For both of them, I have to learn how it's done, and then to see, which one is simpler.

testsuite/tests/typecheck/should_run/T11647.stdout
1 ↗(On Diff #18056)

The ghci script of the current test, does not only load the two modules, it also runs the main function. This is done to make sure, that the name main is really in found and can be run. That's why the test is in the should_run subdirectory.

If I remove this file without changing the ghci script, then the test driver immediately reports the error:

@@ -0,0 +1 @@
+T11647
*** unexpected failure for T11647(ghci)
RolandSenn marked 2 inline comments as done.Sep 20 2018, 3:12 PM

Perhaps you should modify check_main instead to not look for main when loading a module into GHCi.

testsuite/tests/typecheck/should_run/T11647.stdout
1 ↗(On Diff #18056)

Just don't print anything then :) ?

RolandSenn updated this revision to Diff 18101.Sep 23 2018, 5:15 AM
  • Merge branch 'master' into T11647
  • Improve patch for Trac #11647
RolandSenn edited the summary of this revision. (Show Details)Sep 23 2018, 5:26 AM

@mpickering Thank you for the hint about check_main. The patch is now much simpler. I also tested with the -fobject-code flag and switching between GHC and GHCi and I didn't get any error messages.

@mpickering After having read D5189#142767, I tested Trac #11567 with a version that included the patch D5189 but not this patch and found:
Patch D5189 does not fix Trac #11567. Therefore I kindly ask you to review the new version of this patch.

mpickering added inline comments.Nov 1 2018, 4:04 AM
compiler/typecheck/TcRnDriver.hs
1761–1765

Looks like the right idea but I'm not confident this is the right way to check if we are loading a module into GHCi.

RolandSenn added inline comments.Nov 3 2018, 11:13 AM
compiler/typecheck/TcRnDriver.hs
1761–1765

Interesting point!

In module ghc/Main.hs (in the functions main and main') the --interactive flag is processed. If set, the following values are set: ghcMode to CompManager, hscTarget to HscInterpreted, ghcLink to LinkInMemory. In addition the following flags are set: Opt_ImplicitImportQualified, Opt_IgnoreOptimChanges, Opt_IgnoreHpcChanges.
The fact that the --interactive flag was set is (as I read the code) not stored in any data structure, so it's no more available in the module compiler/typecheck/TcRnDriver.hs.

To decide on interactive mode, we have to look at the above mentioned flags:
CompManager value we cannot use, it is used also for backpack and with the --make mode. HscInterpreted can be changed with the -fobject-code flag.
LinkInMemory is also set for eval mode. This is fine, because my fix should work
in eval mode too. The additional 3 Opt_... flags are of no value to check the interactive mode. Therefore I decided to use LinkInMemory.

There exits also the function compiler/basicTypes/Module.hs:isInteractiveModule. However the result is dependent of a module. Either we are in interactive mode for all modules, or we are not in interactive mode, again for all modules. Why should this be dependent of a module? Therefore I didn't use this function.

To have a better criteria for GHCi, in D5122: Fix for #13862: Optional "-v" not allowed with :load in GHCi I tried to store the --interactive flag somewhere in the DynFlags structures, but got stopped by the code reviewer.

How would you check on interactive mode?

mpickering accepted this revision.Nov 6 2018, 3:35 PM

Thanks @RolandSenn, do you need to change the title of the revision as well?

compiler/typecheck/TcRnDriver.hs
1761–1765

Great answer!

This revision is now accepted and ready to land.Nov 6 2018, 3:35 PM
RolandSenn retitled this revision from GHCi does not honour implicit `module Main (main) where` for re-exported `main`s to GHCi does not need a main function.Nov 7 2018, 3:19 AM
This revision was automatically updated to reflect the committed changes.