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

Experiment: enable concise top-level imports #435

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rvanasa
Copy link
Contributor

@rvanasa rvanasa commented Nov 29, 2022

Adds a generated lib.mo file to enable the following import style:

import { Nat; Array } "mo:base";

TODO:

  • Add unit test(s)
  • Validate lib.mo via CI

@crusso
Copy link
Contributor

crusso commented Nov 30, 2022

Interesting, but I wonder if moc is smart enough to dead-code the (many) unused fields of Lib.mo . We should verify that first otherwise folks will pull in all sort of code they don't use, bloating binaries. @nomeata might know the answer off the top of his head.

On slack, I was actually suggesting we should add another form of abbreviated package import to motoko

Something like:

import X0,  ..., Xn  "mo:<package>";

that literally expands to:

import X0 "mo:<package>/X0";
...
import Xn "mo:<package>/Xn";

So you don't just import everything but only the libraries you specify.

@rvanasa
Copy link
Contributor Author

rvanasa commented Nov 30, 2022

Makes sense, +1.

@nomeata
Copy link
Contributor

nomeata commented Nov 30, 2022

Interesting, but I wonder if moc is smart enough to dead-code the (many) unused fields of Lib.mo . We should verify that first otherwise folks will pull in all sort of code they don't use, bloating binaries. @nomeata might know the answer of the top of his head.

The frontend, type checker and IR-to-IR passes will spend time on all the modules, but the backend will skip all unused code. At least if it's just modules and functions; probably not tate like in the stable memory module.

@crusso
Copy link
Contributor

crusso commented Dec 2, 2022

Interesting, but I wonder if moc is smart enough to dead-code the (many) unused fields of Lib.mo . We should verify that first otherwise folks will pull in all sort of code they don't use, bloating binaries. @nomeata might know the answer of the top of his head.

The frontend, type checker and IR-to-IR passes will spend time on all the modules, but the backend will skip all unused code. At least if it's just modules and functions; probably not tate like in the stable memory module.

The state of experimentalstablememory is, I believe, hidden within the compiler primitives, so probably won't affect this either.
Is the sugar bad - at least it avoids the redundant work.

@nomeata
Copy link
Contributor

nomeata commented Dec 2, 2022

The sugar would lift a convention (name the import like the basename of the file) into a language feature. Not sure if that’s advisable. And does it gain much over @rvanasa’s syntax? If the only benefit is that it’s faster to compile, I’d rather make the compiler optimize that syntax :-)
(I guess the benefit is that it wouldn’t need a generated lib.mo. I could imagine the compiler synthesizing a lib.mo when a directory doesn’t have one, like Webservers generate a directory index if there is no index.html.)

@crusso
Copy link
Contributor

crusso commented Dec 5, 2022

import { Nat; Array } "mo:base";

does that already work, by defaulting "mo:base" to mo:base/Lib", or is that just a typo that should read.

import { Nat; Array } "mo:base/Lib";

Otherwise it feels a bit like a convention too, no?

@nomeata
Copy link
Contributor

nomeata commented Dec 5, 2022

I thought the lib.mo is implicit if you import a directory? Or did we drop that?

@rvanasa
Copy link
Contributor Author

rvanasa commented Dec 5, 2022

Yep, this already works as shown!

I had to limit the number of imports, though: CI output

@crusso
Copy link
Contributor

crusso commented Dec 5, 2022

I thought the lib.mo is implicit if you import a directory? Or did we drop that?

I think I never documented it because I didn't understand what it was doing... and maybe Andreas had objected? I can't even find the design doc you wrote now...

Yep, undocumented:
(https://internetcomputer.org/docs/current/developer-docs/build/cdks/motoko-dfinity/language-manual#imports-and-urls)

@crusso
Copy link
Contributor

crusso commented Dec 5, 2022

Yep, this already works as shown!

I had to limit the number of imports, though: CI output

Ah, no, I think that's because the base test tries to import all libraries and compile them to wasmtime, which ExperimentalStableMemory.mo isn't able to do. Just a hunch though.

My hunch is that excluding the IC related libraries (ExperimentalXXX.mo and CertifiedVariables) would be enough (and maybe even too much).

That might suggest that the compiler isn't dead-coding as much as we think though...

@crusso
Copy link
Contributor

crusso commented Dec 5, 2022

Don't worry, this can wait....

@rvanasa
Copy link
Contributor Author

rvanasa commented Dec 6, 2022

It looks like we can add everything except (at least one of the) Experimental modules. Seems reasonable to either include or exclude all of them for consistency.

@rvanasa rvanasa changed the title Enable concise top-level imports Experiment: enable concise top-level imports Dec 6, 2022
@crusso
Copy link
Contributor

crusso commented Dec 8, 2022

Instead of relying on Lib.mo, we could, perhaps, extend this to profiles JS.mo (JS playground) Wasi.mo (wasmtime) and IC.mo and maybe use the default Lib.mo for a least common denominator.

I think Rand.rand() might still be a (fixable) problem.

Crazy?

And we should fix the compiler to error gracefully, not crash. I'll take a look next week to see why it's crashing.

@rvanasa
Copy link
Contributor Author

rvanasa commented Dec 8, 2022

Instead of relying on Lib.mo, we could, perhaps, extend this to profiles JS.mo (JS playground) Wasi.mo (wasmtime) and IC.mo and maybe use the default Lib.mo for a least common denominator.

+1! Perhaps we could also use this as an opportunity to capitalize what are currently specified as main.mo and lib.mo in the compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants