Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add implicit import proposal #500

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

TristanCacqueray
Copy link

The goal of this proposal is to enable direct access to any available module without requiring an import statement. This removes an unnecessary restriction, making the language simpler.

Rendered

@TristanCacqueray
Copy link
Author

Hi, first time contributor here. I could not find a previous proposal for this feature, so I figure I should try to make one. Though I don't know how GHC resolves symbols and manages imports, thus if this is possible, I will need help to write the specifications.

@JakobBruenker
Copy link
Contributor

Note that ghci already supports this, and when you run :set in ghci it claims to have enabled -fimplicit-import-qualified.

However, trying to use this flag in a file doesn't actually seem to do anything - even though ghc doesn't complain about a missing flag.

One question is whether this should use an extension as in the proposal, or just use the existing flag, but make it function outside of ghci.

@nomeata
Copy link
Contributor

nomeata commented Apr 6, 2022

One question is whether this should use an extension as in the proposal, or just use the existing flag, but make it function outside of ghci.

If it comes, it should be an extension: It changes how to interpret the source file, and other tools would have to understand it as well.

@Ericson2314
Copy link
Contributor

How do fully qualified names leading to modules within the current package work? Currently GHC's "downsweep" figuring out module dependencies can function without reading entire modules. Shall we change it to do a pass looking for fully qualified imports?

@JakobBruenker
Copy link
Contributor

If it comes, it should be an extension

In that case I would suggest removing -fimplicit-import-qualified as part of this proposal, and instead turning on the extension by default in GHCi (similarly to how MonomorphismRestriction is turned off in GHCi - though weirdly, that doesn't seem to be mentioned by :set?)

proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
-------------------
TBD: estimate development and maintenance costs.

This extension may improve the language's learnability for novice users by:
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that beginners usually don't use any language extensions, I don't think this would really affect beginners (unless it is enabled by default, of course). That would change however if this extension became part of a future GHC20XX language.

Copy link
Author

Choose a reason for hiding this comment

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

I can't think of a situation where this extension would not be useful, thus I'm hopeful it could be added to a future GHC20XX language.

TristanCacqueray and others added 5 commits April 6, 2022 20:51
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
@TristanCacqueray
Copy link
Author

Thank you all for the prompt feedback. I've incorporated your comments in the last commit.

@JakobBruenker
Copy link
Contributor

JakobBruenker commented Apr 8, 2022

Shall we change it to do a pass looking for fully qualified imports?

I think this is an argument in favor of not enabling such a feature by default, since it'd likely be better if such a pass only has to be done if implicitly imported names are actually used in a module - though of course how strong of an argument this is depends on how long such a pass would actually take.

This is mitigated by libraries/applications (i.e. the cases where compile time matters) typically having cabal files that specify Haskell2010 as the default language, though I don't know if this will remain the case in the future.

proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
proposals/0000-implicit-import.rst Outdated Show resolved Hide resolved
@phadej
Copy link
Contributor

phadej commented Apr 8, 2022

How this would compare with #283 if it is undormanted.

I'm a fan of project specific fat preludes, and would e.g. re-export Data.Text as T. So what this proposal suggest that I could use Data.Text.pack everywhere doesn't add value, as I'd rather use T.pack everywhere.

Or using #283, my fat prelude could just re-export whole Data.Text.

I see #283 as superior. It gives more control, and still keeps imports contained in the header of the module.

TristanCacqueray and others added 2 commits April 8, 2022 15:40
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
Co-authored-by: David Feuer <David.Feuer@gmail.com>
@TristanCacqueray
Copy link
Author

@phadej I guess the main difference with #283 is that this proposal doesn't need a curated list of module: If the fat prelude doesn't export, say Servant, then that does not address the issue raised in the motivation.

@TristanCacqueray
Copy link
Author

It seems like we can leverage the existing code used by GHCi. I took a stab at implementing this in GHC, and the change is rather minimal: https://gitlab.haskell.org/TristanCacqueray/ghc/-/tree/ImplicitQualifiedImport

@guibou
Copy link
Contributor

guibou commented Apr 9, 2022

What about instances?

import statement also imports instances and there is no control on this right now. An implicitely introduced import qualified will also implicitly import instances and ones may argue that it may be too much implicit. This should be discussed in the interaction section.

@TristanCacqueray
Copy link
Author

What about instances?

If I understand correctly, the implementation proposed in the last commit should not affect instances. Only the names that throws Not in scope errors are handled differently.

Thank you for the feedback, I think all the suggestion are now part of the proposal. Please let me know if I missed something.

@JakobBruenker
Copy link
Contributor

JakobBruenker commented Apr 10, 2022

the implementation proposed in the last commit

GHCi seems to import instances from used modules:

ghci> (++"!") <$> Data.Map.fromList [(1,"foo")]
fromList [(1,"foo!")]

instance Functor (Map k) is defined in Data.Map.Internal.

Edit: No, it's not quite so straightforward after all:

$ ghci -XNoImplicitPrelude
ghci> import GHC.Show
ghci> show ((\x _ -> x) (5.6 :: GHC.Float.Float) (GHC.Float.eqFloat 1.2 3.4))

<interactive>:2:1: error:
    * No instance for (Show GHC.Types.Float)

GHC.Float provides instance Show Float, so I'm not sure what the difference here is. Could be because this instance is an orphan?

@TristanCacqueray
Copy link
Author

If I understand correctly, non orphaned instances are available whenever the typeclass or the type is in scope.

While trying this out, I realize that the proposed implementation does not work for local module, where the implicit dependencies need to be declared in the ModuleGraph. Thus in the last commit I added a mention that the GHC.Driver.Make.downsweep process needs to be modified so that it parse the whole module.

@TristanCacqueray
Copy link
Author

In the last commit of my ghc ImplicitQualifiedImport branch I updated the downsweep pass to use the Lexer in order to add all the qualified names to the module graph. That seems to work as a proof of concept, though I can't tell what are the implications :)

@nomeata
Copy link
Contributor

nomeata commented Apr 11, 2022

I was wondering if there is a problem with Template Haskell, and the staging it introduces into the processing of a file. But it seems there is none: lexing still happens all at once, so that's good. And spliced code wouldn't have to be considered by this change (in a way, it already behaves as this), so also fine. Good!

I wonder if it is incompatible, at least morally, with local modules as envisioned by #283: with that, it's no longer clear how to map from what looks like a qualified name to the module one should import (it could be any prefix).

Overall it's an intriguing and interesting proposal, and my mind making up process is still ongoing. If I hadn't done some Ocaml programming recently I might have said “surely no”, but I'm not so sure anymore.

@TristanCacqueray
Copy link
Author

@santiweight If I understand correctly, such new import syntax would still need a pragma. And the advantage of this proposal being just a pragma is that it can be enabled globally by adding it to the list of default-extensions.

I am also not in favor of @simonmar's suggestion: even though that would work most of the time, I think the proposal would provide a better user experience if it also works with local modules.

@TristanCacqueray
Copy link
Author

@Ericson2314 After reading through 2022-ghc-modularity I think I better understand what you meant and I totally agree this work would make our life easier. Thank you so much for that!

I believe I have addressed the remaining salient points in the last commit, and unless there is an objection, I think this is ready for the committee.

@nomeata
Copy link
Contributor

nomeata commented May 5, 2022

Thanks, submission received and assigned to @Icelandjack. @Icelandjack, looks like you didn’t accept the repo invitation yet, so I couldn’t assign it to you here on Github. But you are on it :-)

@nomeata nomeata added the Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion label May 5, 2022
@nomeata nomeata changed the title Add implicit import proposal Add implicit import proposal (under review) May 5, 2022
@simonpj
Copy link
Contributor

simonpj commented May 5, 2022

This demonstrates how the implicit dependency can be discovered using the Lexer

I would love to see this sentence (currently buried in Costs and Drawbacks) made much more explicit and promient. Explain what is the "implicit dependency" and why it matters.

I don't have a strong opinion about the utility (from a programmer's POV) of this proposal. The design is simple enough (good) but the costs are not trivial:

  • More code paths -- a bit more complexity. Nothing major, but more.
  • Dependency computation needs to read every token in the file.

So I'm not very enthusiastic, but I'd be swayed by lots of people saying "this would make my life better"

@TristanCacqueray
Copy link
Author

@simonpj please find an update to the Costs and Drawbacks section in the last commit. I have removed the mention of the lexer because I think it is an implementation detail.

I believe the cost can be kept small by:

  • updating the getImports code to use the lexer instead of the parser.
  • sharing the tokenization work done by the make driver with the parsing layer.

Perhaps it is not optimal to do these changes now, and I would be happy to check on the upcoming modularization work and see how to accommodate this extension efficiently.

@aspiwack
Copy link
Contributor

aspiwack commented May 6, 2022

@doyougnu

In particular, the original motivation for implicit imports in OCaml was as a solution for ad-hoc polymorphism. But Haskell+GHC has world-class support for ad-hoc polymorphism. IMO OCaml's solution is quite good because it reuses machinery that is well-supported and very powerful in the language, i.e., its module system and specifically functors.

This is a misunderstanding. Ocaml has always had the ability to refer to other modules without an import statement. In fact, there is no import statement in Ocaml. The modular implicits that you are referring to are not about imports, but about discharging module argument automatically in the manner of type classes. This could happen with or without explicit import statements.

@doyougnu
Copy link

doyougnu commented May 6, 2022

This is a misunderstanding. Ocaml has always had the ability to refer to other modules without an import statement. In fact, there is no import statement in Ocaml. The modular implicits that you are referring to are not about imports, but about discharging module argument automatically in the manner of type classes. This could happen with or without explicit import statements.

Aha! Thank you for the correction @aspiwack and taking the time to respond. I much prefer having correct information over being right!

@nomeata
Copy link
Contributor

nomeata commented Jul 10, 2022

Gentle ping about shepherding this proposal, @Icelandjack :-)

@goldfirere
Copy link
Contributor

This has been pending a recommendation from @Icelandjack for about 2.5 months -- @Icelandjack, do you think you can get to this recommendation? I think we may need to reassign if not. Thanks!

@nomeata nomeata assigned simonmar and unassigned Icelandjack Aug 1, 2022
@nomeata
Copy link
Contributor

nomeata commented Aug 1, 2022

I’ve reassigned to Simon Marlow.

@TristanCacqueray
Copy link
Author

I'm still interested in pushing this forward as I think it is a valuable addition to the language. It seems like the most valuable use-case is to make the extension work with modules that are defined outside of the current package (e.g. Debug.Trace). Thus, It might be best to implement the proposal in two stages:

  1. Introducing the extension and making it work by subsuming the -fimplicit-import-qualified GHCi flag.
  2. Making the extension work for modules defined in the current package by changing the dependency "downsweep" process.

The first stage seems relatively straightforward to implement, and the only downside is that the user will get a Not in scope error when trying to use the feature with a local module. Therefore, If such implementation is feasible, I'd be happy to update the proposal with this two-stage plan.

@simonmar
Copy link

Committee shepherd here. I've read through all the comments (phew).

One thing that would inform our discussion is to know the impact on compile time (and space) of doing a full lexical scan of every module during the downsweep. Would it be possible to get some numbers on that for something large, e.g. GHC itself?

@TristanCacqueray
Copy link
Author

@simonmar Thank you for following up on this proposal. That's a good idea, I'll try to get those performance numbers using the initial implementation.

@TristanCacqueray
Copy link
Author

Thanks to @mpickering suggestions, I think I managed to measure the cost of the lexical scan in this make-lexical-analysis branch by calling lexTokenStream to count qualified names right after pre-processing the imports. Though I'm not familiar with the code base, and it would be great if someone could validate the test implementation.

I measured a full compiler build as well as a single rebuild of the GHC module:

# baseline clean GHC build
2,892,047,868,432 bytes allocated in the heap 
Total   time  3400.765s  (3490.170s elapsed)

# lexical clean GHC build
2,914,811,164,216 bytes allocated in the heap 
Total   time  3469.633s  (3558.010s elapsed)

# baseline GHC module rebuild
    9,707,615,104 bytes allocated in the heap
Total   time    10.789s  ( 13.420s elapsed)

# lexical GHC module rebuild
   32,644,612,384 bytes allocated in the heap
Total   time   24.613s  ( 27.360s elapsed)

I noticed the rebuild does re-preprocessed all the dependencies, eventhough they didn't changed, thus perhaps this could be improved by caching the scan result?

@simonpj
Copy link
Contributor

simonpj commented Aug 30, 2022

I have reflected on this proposal and, while I see its merits, I lean against it. It's a matter of taste but the cost/benefit ratio comes out too high for me -- more complexity [both design and implementation] without increasing expressiveness, and what is, to me, a small increase in convenience. And stylistically I positively like listing dependencies!

But I accept that others may differ.

I'm a bit shocked that some compile time metric might triple (which seems to be what the above figues show). That might be a show-stopper anyway; or it might be a measurement glitch, or overcome with cleverer implementation. But design first!

@TristanCacqueray
Copy link
Author

It seems like the above figures show the cost of doing an extra lexical scan. Perhaps we could mitigate the overhead by sharing the lexical scan done in the downsweep process with the main pipeline? That may be a good improvement, for example, to replace the current getImport parser.

Unless we can reduce the cost/benefit ratio, then I'm perfectly happy to retract this proposal.
In any case, thank you all for taking the time to evaluate the proposal.

@simonmar
Copy link

sharing the lexical scan done in the downsweep process with the main pipeline

That's not going to save very much in the case when just one module needs to be recompiled, because we're only saving the lexical analysis of that one module.

To make this extension efficient, we would have to cache (on disk) the precomputed dependencies of each source file. That's a lot of extra complexity, and I think probably tips the balance into "not worth it" territory.

@TristanCacqueray
Copy link
Author

To make this extension efficient, we would have to cache (on disk) the precomputed dependencies of each source file.

That sounds like a valuable feature on its own, for example, as suggested by @Ericson2314 Mod.import counter proposal. I guess we should mark this extension as "dormant" until such feature is implemented.

@nomeata nomeata added Dormant The proposal is not being actively discussed, but can be revived and removed Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion labels Aug 31, 2022
@nomeata
Copy link
Contributor

nomeata commented Aug 31, 2022

Marking this as dormant as per author's request. Thanks for your contribution!

@adamgundry adamgundry changed the title Add implicit import proposal (under review) Add implicit import proposal Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dormant The proposal is not being actively discussed, but can be revived
Development

Successfully merging this pull request may close these issues.

None yet