gren-lang / compiler

Compiler for the Gren programming language
https://gren-lang.org
Other
379 stars 23 forks source link

Revamping the import system #220

Open robinheghan opened 1 year ago

robinheghan commented 1 year ago

Each feature here could be presented as an independant proposal, but since they all center around the same system, it makes sense to present them as one.

Every module is available without an explicit import

Just as it says on the tin. If you don't need to specify an alias or import things unqualified, then you don't need an import statement at all. This should greatly reduce the need for imports, and encourage using qualified references.

No more default imports

The recent operators as syntax sugar proposal would benefit from being able to choose which function is called when an operator is used. This doesn't work too well with default imports.

Making every module available without explicit import statements helps somewhat, but there are certain values that would be nice to have imported by default, like Just and Nothing.

To help with this, the CLI will get a generate command which can generate a new module with a specific set of imports included in the template. This way, the default imports are explicit, and can be removed or changed according to the whims of the programmer.

A from keyword

We don't really have a good way of dealing with collisions in module names. This proposal simply adds a from keyword that can be used to select which package a module should be imported from, as a way to select a winner between colliding modules.

import MyModule from "skinney/package"
import MyOtherModule from "application"

Move documentation comment above module declaration

This will be more inline with how documentation comments work in all other parts of the language, and also avoid ambuguity when the import section is empty (which is more likely if the above proposals make it into the language).

Other than moving the location of the documentation comment, no other changes are made to the module's documentation comment.

Dissallow modules sharing a name or alias

Gren currently allows two modules to share a name (common example is aliasing Array.Extra as Array, to merge those two namespaces). This makes it hard to tell where a function really resides, especially when the import section isn't available (diffs or tutorials). It can also make it harder for external tools to know where a value comes from.

Suggestion removing this ability.

boxed commented 1 year ago

Every module is available without an explicit import

I don't understand what this means. You mean every module in my own project or the entire system? Won't this in both cases lead to potential namespace collisions a lot?

jfmengels commented 1 year ago

Every module is available without an explicit import

A few downsides for this:

  1. discovering dependencies for this module becomes a lot trickier, since instead of looking at the imports, you need to look at the entire module. Examples:

I'm sure other uses-cases like this exist, even though they will be quite rare.

  1. IIRC Gren can have dots in aliases right? I'd imagine that that can easily lead to conflicts.

No more default imports

This can make sense only if the previous feature is supported. But would also require qualified usages for basic functions, which I'm not sure is an experience you'd want 🤔

Btw, with these changes, it's likely you won't have any imports anymore . A reminder that if you try to document the first declaration and you have no imports nor a module documentation, then that documentation will act like the module documentation, not the declaration 's documentation.

To help with this, the CLI will get a generate command which can generate a new module with a specific set of imports included in the template. This way, the default imports are explicit, and can be removed or changed according to the whims of the programmer.

Just to be clear, this is a a command to generate a new module with some imports already there, right? It's not a customizable prelude? I don't think it's a great remedy to the problem you're introducing. Tools like elm-review or editors will try to remove all the imports right away.

I think you'll find Basics imported on 99% of all projects too, so might as well include it directly too. Do we need Platform.Cmd imported by default, probably not no. So I think you could alter the prelude to have less things, but I wouldn't remove everything from it.

(I don't think a prelude will be great either btw)

A from keyword

Sounds reasonable. Maybe you could even provide from which source-directory you could import the module. A recent use-case I had was for creating an Elm benchmark from a module and it's previous version (to test perf improvements). I had to copy the old version into a new folder and, to avoid naming conflicts, had to prefix all the module names (+ imports) with something, which was tedious. If you could specify the source-directory, this would have been trivial.

Re-expose

Could you re-expose the constructors as well? If so, I think this sounds good. If not, then I'm not sure what the value is over adding a type alias. You would have two ways of doing the same thing.

Multiple exposings

This one I think is not a great proposal. The @docs we currently have is already great because it easily lets you customize how to present and interleave the different pieces of the API. Having "@docs for others" (or a similar style) makes that just less obvious (harder to find where "two" is documented through ctrl-F).

The separation of exposing is what bugs me the most though. Often, if I need to see if an API exposes a function that does X, it's much faster to scan for the names in the exposing list than the docs. With this proposal, the exposing are far in between and it becomes a hassle to find what I'm looking for.

The problem you're introducing trying to solve here is about not having missing @docs notation. If I'm right, then I think there are better solutions.

We could have a "@docs for others" without the duplicate exposing, though I think that would actually worden the resulting docs.

We could have a compiler error when you're missing @docs. We have it for packages, but not for applications and non-exposed modules. That's what elm-review-documentation is for, until there is a compiler check.

joakin commented 1 year ago

Very nice proposal 👌

There is a couple of things I’d like to comment on:

Maybe the one that doesn’t seem very necessary is the multiple exposing one. You could argue you can interleave comments in the exposing list too, with a similar result. Something like this

​module MyPackage
​
​{-| MyPackage is just an example
​-}
​    
exposing
​    {-| ## Ones!
​
         ​Docs for one
    ​-}
    ​(one
​
    ​{-| ## Others!
​
    ​docs for others
    ​-}
    , two, three
    )

Although maybe it is not so aesthetically pleasing as the multiple exposing one.

—-

Would you consider changing exposing to export or exports. It feels like the natural opposite of imports, and it is used in other languages like JS.

—-

What is the problem of keeping a small focused default import? It really helps readability for the basic types of the language. There are many things in Basics that could be moved to better named modules so that accessing them qualified would read nice, like all of the math functions, which could go under Math (Math.sin, etc). Then the basics default import could be stripped down to the bare types and constructors every app will use, and you don’t need the generate cli shenanigans and all files look a bit cleaner.

robinheghan commented 1 year ago

@boxed

I don't understand what this means.

It means that import My.Module (without any other keywords) become redundant. It means you can do things like Dict.empty or Array.map without doing any importing at all.

@jfmengels

A few downsides for this:

These are good points. A long term goal of Gren is to rewrite the compiler in Gren. That would include making the most of the compiler a package which can be relied on for things like formatting, linting, analysis etc.

Would that help in this case? The compiler has the same basic need of figuring out which packages have changed in order to do a incremental re-compile.

IIRC Gren can have dots in aliases right? I'd imagine that that can easily lead to conflicts.

Right. However, you would only get a collision if you were to use a conflicting name in practice, if that helps? If you define an alias, which happens to collide with another module, you won't be having a conflict as the compiler will already know which module you're referring too.

A reminder that if you try to document the first declaration and you have no imports nor a module documentation, then that documentation will act like the module documentation, not the declaration 's documentation.

Not if exposing is its own statement ;)

Just to be clear, this is a a command to generate a new module with some imports already there, right? It's not a customizable prelude? I don't think it's a great remedy to the problem you're introducing. Tools like elm-review or editors will try to remove all the imports right away.

Right. And again, a good point, but considering the operators as sugar proposal (https://github.com/gren-lang/compiler/issues/219), you might want to whitelist certain imports anyway?

I think you'll find Basics imported on 99% of all projects too, so might as well include it directly too. Do we need Platform.Cmd imported by default, probably not no. So I think you could alter the prelude to have less things, but I wouldn't remove everything from it.

You will find that Gren's prelude (specifically Basic) is smaller than in Elm. If parametric modules (accepted) and operators as syntax (under consideration) is implemented, the only pieces remaining in Basic are some of the functions listed under "Function Helpers", and those are small enough to be in a exported clause at the top of the file. Or maybe it won't hurt to have them in seperate modules either 🤷

Sounds reasonable. Maybe you could even provide from which source-directory you could import the module. A recent use-case I had was for creating an Elm benchmark from a module and it's previous version (to test perf improvements). I had to copy the old version into a new folder and, to avoid naming conflicts, had to prefix all the module names (+ imports) with something, which was tedious. If you could specify the source-directory, this would have been trivial.

In Gren you can have local dependencies. So you make a copy of the previous version to a new folder, change the name to jfmengels/old-project, import it as a local dependency and then use the from keyword with an alias to seperate between current and new projects.

Could you re-expose the constructors as well?

Yes

Having "docs for others" (or a similar style) makes that just less obvious

Not sure I understand your criticism.

My proposal essentially makes a @docs statement an actual keyword, and moves it out of the comments. That way, there's no need to specify it at the top of the file. The module docs will still be reading order, just as it is today.

Would you consider changing exposing to export or exports. It feels like the natural opposite of imports, and it is used in other languages like JS.

I think this is a valid argument. Would cause a ton of breakage though 😅 I'll consider it!

There are many things in Basics that could be moved to better named modules so that accessing them qualified would read nice, like all of the math functions, which could go under Math (Math.sin, etc)

Gren already has a Math module :D

With the operators as syntax sugar proposal, we wouldn't want the typical operator names to be a default import (or you wouldn't be able to override them), and with both that proposal and parametric modules, most functions will move to seperate modules anyway (compare will move to String, Int, Float etc.)

If you don't have to specify imports, then there's no need to have a Debug or Array module import either.

So, in my mind at least, the future set of default imports would be

import Maybe exposing (Maybe(..))
import Function (identity, always) -- do we even need these?

And the operator functions you'd want to be explicit anyway in order to be able to override them.

joakin commented 1 year ago

Would that help in this case? The compiler has the same basic need of figuring out which packages have changed in order to do a incremental re-compile.

I'm not sure what elm-review does, but for a compiler (at least for one I worked on), for figuring out the dependencies of a module I needed to do the parsing of the module to get the imports out of it. In that case, given the unique syntax of Gren, doing the parse of the module would give you the modules it depends on as well from the code, so in both cases you have to do parsing.

Gren already has a Math module :D

My bad!

So, in my mind at least, the future set of default imports would be

import Maybe exposing (Maybe(..))

This reminds me that we do use a lot the import statements for bringing in the main type of the imported module for using in type signatures.

With this proposal, given less imports will be used, it means likely that people will move to use qualified type names for type signatures, which in my opinion can be ugly. Or end up having to write most imports anyways just to bring a type to scope. Something to ponder about.

name: Person -> Maybe.Maybe Int.Int

This module system looks a lot like OCaml and F#. I know in OCaml they usually name the main type of a module as t, it is kind of a convention, because all modules are available everywhere, so they'd do something like:

name: Person -> Maybe.t Int.t

I don't think F# has the same convention, they do name their types with readable names, but I'm not sure how they deal with modules, because they use open Module a lot. So maybe in Maybe they have a Maybe type and a Maybe.Maybe submodule that has the functions and uses the type in the "parent" module. So when you do open Maybe you bring in Maybe type and Maybe module to scope. Not sure though, and doesn't help much with this proposal.

boxed commented 1 year ago

Hmm. Could we have a local Array.gren module in your own project and that gets merged? That could be very nice as a way to extend modules. Extending modules is a thing I really liked in Objective-C back in the day. Made tons of stuff just way cleaner.

laurentpayot commented 1 year ago

This proposal simply adds a from keyword that can be used to select which package a module should be imported from, as a way to select a winner between colliding modules.

import MyModule from "skinney/package"
import MyOtherModule from "application"

In the JS community the import X from A syntax is generally considered a design mistake because it makes autocompletion of A tricky for the IDE when you write it (I don’t know for the compiler once written). Many JS folks say from A import X should have been used instead. Maybe also in Gren?

wolfadex commented 1 year ago

Since no goal is stated regarding the removal of the import section, I'm going to assume that it's viewed as unnecessary. With that assumption, If I'm in the middle of a 5k loc Gren file and see Foo.fn I should be able to assume that this is using the module Foo. However if we allow aliasing then I do have to check the import section first, specifically for aliases. An example, with the assumption this file is thousands of lines long:

doSomething = 
    internalFn
        |> Task.andThen otherInternal
        |> Task.attempt ResultingMsg

What module is Task coming from? We might assume that it's gren/core, but what if (borrowing form a recent Elm Slack discussion) it's acutally import Concurrent.Task as Task. The next dev comes into this file and jumps straight into updating the code to to add a timestamp to ResultingMsg

doSomething = 
    internalFn
        |> Task.andThen otherInternal
        |> Task.map2 (\now other -> { now, other } Time.now
        |> Task.attempt ResultingMsg

They make the incorrect assumption that we're using the core Task module and use the matching gren/time module. They should have used Concurrent.Time.now but they weren't aware of the alias.

A nice error message will likely help them, something like

I found a Task.Task x a

2306 |        |> Task.map2 Time.now
                           ^^^^^^^^
when I was expecting a Concurrent.Task.Task x a

but by this point we're already wasting the developer's time.

robinheghan commented 1 year ago

@laurentpayot bear in mind, from is not a required keyword. It's only to be used when there are multiple modules with the same name in your application, which should be rare.

@wolfadex Got it. I've been thinking of requiring that aliases are unique, which would make the example you posted above not compile. I believe Roc has the same decision. Would that work for you?

@boxed I'm leaning towards not allowing merging of module names. While convinient, it does add some confusion, see @wolfadex 's comment above.

laurentpayot commented 1 year ago

@laurentpayot bear in mind, from is not a required keyword. It's only to be used when there are multiple modules with the same name in your application, which should be rare.

Not so rare in JS/TS at least :wink: image

robinheghan commented 1 year ago

@laurentpayot I see what you're saying. But in JS, the thing that immedietly follows the import keyword is the things you want imported, right? In Gren/Elm, what follows the import keyword is where you want it imported from.

laurentpayot commented 1 year ago

@robinheghan I’m a bit confused. Do you mean we could have something like this in Gren?

import MyModule from "skinney/package" exposing (foo)
robinheghan commented 1 year ago

@laurentpayot yes.

But from "skinney/package" is only required if import MyModule exposing (foo) would give a compiler error due to there being two or more modules that is called MyModule.

laurentpayot commented 1 year ago

@robinheghan Oh I see now. That makes sense :+1:

joakin commented 1 year ago

Hmm. Could we have a local Array.gren module in your own project and that gets merged? That could be very nice as a way to extend modules. Extending modules is a thing I really liked in Objective-C back in the day. Made tons of stuff just way cleaner.

This is exactly the stuff that is being mentioned, when discussing having multiple aliases.

Right now it is possible to alias more than one module to the same name in the imports, which means that you can fake extend a module by providing functions from another module to the same name.

It’s on Robin to decide if this is a behavior he wants to keep with this import system overhaul, or if it is something that he wants to disallow.

A probably incomplete summary:

The pros:

The cons:

I personally love being able to write clean code and use this pattern for merging Extra modules with the original module, makes me feel in can extend built-ins and the code reads a lot better for it.

I’d prefer it if I could do this and the people that don’t like it can do their own thing and enforce lints to disallow duplicate aliases rather than removing this nice little feature.

robinheghan commented 1 year ago

Just edited the proposal.

icidasset commented 1 year ago

Disallow modules sharing a name or alias

I agree with joakin here, this is actually something I really like about Gren/Elm. It improves readability a lot. It's something I used heavily in my first Gren package. I had to put stuff in different modules to avoid recursive imports, but I then alias them all to the same module in the code that uses the package for readability.

(see Markdown example: https://packages.gren-lang.org/package/icidasset/shikensu-gren/version/4.0.0/overview)

I agree that it makes it a bit harder to tell where a function resides, but I think most people that have experience with writing Gren/Elm code look at the imports first before assuming where the function comes from. I think the good outweighs the bad in this case.

That said, maybe there's a way to write my code better so I can avoid the aliasing? Would re-expose fix this?

Love all the other sub-proposals 👏

GabrielBoehme13 commented 5 months ago

Every module is available without an explicit import

I really don't like this.

One thing I've grown to appreciate about Elm modules is their self-documenting design – you can look at the top of a module and see exactly:

  1. what's being exported from that module, and
  2. what's being imported into that module.

This proposal takes away that second benefit from Gren, for the very minor convenience of not having to explicitly specify your imports. This creates problems for anyone coming into your codebase (or your future self coming back to an old codebase) and trying to understand how all of the modules are related to one another – you'd have to scroll through each module and look for the names of all the modules being implicitly imported. This doesn't seem like a worthwhile tradeoff to me.

If the goal is to encourage the use of qualified references, I think your other sub-proposals are already pointed in that direction – I'm not seeing how this particular sub-proposal is necessary to achieve that goal.

If the goal is to allow the programmer to write their initial code more quickly, importing available modules without having to manually key in the related import statements, then a better approach would be to have the Gren formatter detect the use of unimported modules and automatically add the relevant import statements for you – retaining the self-documenting benefit of having explicit import statements without requiring them to be keyed in manually.

robinheghan commented 5 months ago

Hi Gabriel, thanks for chiming in!

This proposal takes away that second benefit from Gren

This is interesting.

Do people actually read the list of imports before reading looking up definitions in a module? I get it for modules containing < 100 lines of code, because then it's actually possible to keep a list of imports in your head, sort of.

But in the projects I've worked on, with hundreds of modules and some modules containing 1000-2000 lines of code with 20-30 imports, the import statements are pure noise to me.

I've ended up always writing imports of the form import <module> where exposing is allowed if I'm going to refer to a type that has the same name as a module, and aliases are allowed if the module contains several parts. Aliases must remain the same across modules of the project, though. I've found this way to scale with the size of the project.

This proposal would make the way I write import statements to be the easy way, which means you're more likely to see qualified references and know which constant is being referred to regardless of import statements.

you'd have to scroll through each module and look for the names of all the modules being implicitly imported.

When is it relevant to know which modules are being imported into a module? 🤔

If this is a true need, this can be implemented as an operation in the compiler: gren docs My.Module --list-imports, or something.

then a better approach would be to have the Gren formatter detect the use of unimported modules and automatically add the relevant import statements for you

This is an option. Although it would only work on source code, not in the repl.

boxed commented 5 months ago

I would like to chime in here that in Swift this is how it works. I find this jarring, but I'm also a hobby level Swift coder. It's easy to get name conflicts for one, which is annoying. Swift also has quite bad compile times, and I wonder if this has something to do with that. I don't know...

Although it would only work on source code, not in the repl.

I think that's ok personally. The repl is for interactive use and should have some different tradeoffs if you can cut down on typing. A repl could also do this:

> foo = pi * 4
import Math
> 

ie, print the imports (maybe in another color) that are implicitly run.

robinheghan commented 5 months ago

I would like to chime in here that in Swift this is how it works.

I could be wrong, but in my memory Swift has one namespace per package. So everything in a single package is in the same scope, more or less. Usually you attach methods to class and struct so it isn't too bad, but there's still a much higher chance of name conflicts compared to what I'm proposing.

I also belive qualified imports isn't common in Swift (unqualified being the norm), which is another way in which my proposal differs.

boxed commented 5 months ago

Ah, yea ok, I misremembered what the original proposal was 😅 Just having access to the module names seems much more sane to me, even though I personally don't like module.symbol type code. From having lived in Python land for ~15 years I find that it works perfectly fine there like that. 🤷