Skip to content

Towards the prim module#1088

Merged
nomeata merged 26 commits intomasterfrom
joachim/prim-module
Jan 10, 2020
Merged

Towards the prim module#1088
nomeata merged 26 commits intomasterfrom
joachim/prim-module

Conversation

@nomeata
Copy link
Contributor

@nomeata nomeata commented Jan 9, 2020

this starts implementing the plan in
#867 (comment)

No values are in scope by default.
In bare uses of Motoko: Use import Prim "mo:prim" to get
Prim.debugPrint etc.
In real uses, the stdlib should provide everything you need.

Quite some churn in the test suite. Went through run and fail so
far.

this starts implementing the plan in
#867 (comment)

No values are in scope by default.
In bare uses of Motoko: Use `import Prim "mo:prim"` to get
`Prim.debugPrint` etc.
In real uses, the `stdlib` should provide everything you need.

Quite some churn in the test suite. Went through `run` and `fail` so
far.
@nomeata nomeata force-pushed the joachim/prim-module branch from 3a55d9f to ee0d31d Compare January 9, 2020 16:51
@nomeata
Copy link
Contributor Author

nomeata commented Jan 9, 2020

I hit a snag: Nested modules.

Although they are not supported in drun, we do use local actors in our test suite, e.g. in:

import Prim "mo:prim";
Prim.debugPrint (debug_show true);
let _ = actor {
  Prim.debugPrint (debug_show false);
  Prim.debugPrint (debug_show 1);
};
()

Previously this compiled due to a horrible hack about prelude definitions in the backend. But the hack does not apply to Prim (which looks like any old let-binding then), so this is now a non-closed actor.

What to do? Maybe simply stopping to use local actors in the test suite (or only in the interpreter)?

I guess there are actually just a few of these. Disabling on drun/stub seems to be fine.

(Working on updating test/drun)

@nomeata nomeata force-pushed the joachim/prim-module branch from f606155 to 8bc9992 Compare January 9, 2020 17:25
@nomeata nomeata mentioned this pull request Jan 9, 2020
and move the test projects into its own directory, to avoid spurious
recompilation of the `lsp-int` binary.
but somehow this does not compile. @ggreif, can you help?
@nomeata
Copy link
Contributor Author

nomeata commented Jan 9, 2020

@ggreif I have some problem adjusting test/random. I would expect joachim/prim-module-qc to work. Maybe the msum is doing something strange. Would you mind having a look at that commit and help me out (e.g. by pushing one that works :-)?

@nomeata nomeata force-pushed the joachim/prim-module branch from b7a4c47 to c3b6a88 Compare January 9, 2020 18:32
@ggreif
Copy link
Contributor

ggreif commented Jan 10, 2020

@ggreif I have some problem adjusting test/random. I would expect joachim/prim-module-qc to work. Maybe the msum is doing something strange. Would you mind having a look at that commit and help me out (e.g. by pushing one that works :-)?

I am working on this. Just for the record, one can run a subset of the tests by (e.g.)

env PATH=$PATH:src $(nix-build -A qc-motoko)/bin/qc-motoko -p failures

which will leave behind specific .mo files, which then can be inspected/massaged in isolation.

It appears that "import Prim ..." is prefixed on every line, but it is only allowed at the top of the program. This is for the successes test (corresponding to "tests.mo"), all the others are one-liners (IIRC), so they should be fine.

@ggreif
Copy link
Contributor

ggreif commented Jan 10, 2020

@nomeata please merge origin/joachim/prim-module-qc, I'll watch the CI.

@nomeata nomeata mentioned this pull request Jan 10, 2020
@nomeata nomeata marked this pull request as ready for review January 10, 2020 14:06
@nomeata
Copy link
Contributor Author

nomeata commented Jan 10, 2020

Ok, CI is happy.

Copy link
Contributor

@kritzcreek kritzcreek left a comment

Choose a reason for hiding this comment

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

This looks great, do you think we want to hold off on this until after Davos? This is definitely going to break code in linkedup (having to add the Prim import) and I don't know when the next DFX release is happening so we might end up dropping this change on them very shortly before the demo.

parse "mo:foo/bar" = Ok (Package ("foo", "bar"))
parse "mo:foo" = Ok (Package ("foo", ""))

parse "mo:prim" = Ok (Prim)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not saying it needs to happen in this PR, but these comments look like they should be unit tests?

This module should contain everything that cannot be implemented in plain
Motoko. It is available via `import Prim "mo:prim"`. Normal user code would
usually not import that module directly, but through the stdlib, which takes
care of providing a prober module structure, e.g. exposing Array_tabulate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
care of providing a prober module structure, e.g. exposing Array_tabulate
care of providing a proper module structure, e.g. exposing Array_tabulate

Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

This is big. I have just skimmed the surface. But I agree, this needs to be done before lots of big (d)apps grow in the wild. Is LinkedUp informed about upcoming changes? Also @lsgunnlsgunn should be in the loop so she is prepared.

nomeata and others added 3 commits January 10, 2020 15:33
Co-Authored-By: Gabor Greif <gabor@dfinity.org>
Co-Authored-By: Gabor Greif <gabor@dfinity.org>
@nomeata
Copy link
Contributor Author

nomeata commented Jan 10, 2020

This is big.

Actually it isn’t so big:

  • In prelude.ml you can see the prelude and the prim.
  • A little bit of code in the pipeline to make "mo:prim" work.
  • A lot of mechanical updating of the testing.

Would you prefer one PR that adds the infrastructure (unchanged prelude, empty prim_module), and a separate that actually moves things over?

Is LinkedUp informed about upcoming changes? Also @lsgunnlsgunn should be in the loop so she is prepared.

Judging from updating the produce exchange, there isn’t much to do. Outside users should usually not import Prim, but should use the stdlib (e.g. Array.tabulate, Iter.range).

The biggest one is Mostly debugPrint, which should be properly exported through the stdlib (maybe Debug.print?). We can do that in a separate PR shortly afterwards.

We can maybe port LinkedUp ourself and provide a PR with the changes needed for that.

@nomeata
Copy link
Contributor Author

nomeata commented Jan 10, 2020

Would you prefer one PR that adds the infrastructure (unchanged prelude, empty prim_module), and a separate that actually moves things over?

Actually, that sounds like a good idea. Will do that.

nomeata added a commit that referenced this pull request Jan 10, 2020
this make `import "mo:prim"` import the module in the `prim_module`
string in `prelude.ml`. This is currently empty; the code is added in
PR #1088.
@nomeata
Copy link
Contributor Author

nomeata commented Jan 10, 2020

See #1093. Now with a warning upon --package prim ..

mergify bot pushed a commit that referenced this pull request Jan 10, 2020
* Infrastructure for the hard-coded prim module

this make `import "mo:prim"` import the module in the `prim_module`
string in `prelude.ml`. This is currently empty; the code is added in
PR #1088.

* Print an error upon `--package prim ...`

* Update src/prelude/prelude.ml

Co-Authored-By: Gabor Greif <gabor@dfinity.org>

Co-authored-by: Gabor Greif <ggreif@gmail.com>
Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

I think this is good to go, with following caveats:

  • heads-up to @enzoh and other teams
  • needs mention in Motoko user manual
  • snippets for range and hashInt so that users can include it themselves.

Please choose the merge time with consideration of the Bronze situation.

@nomeata
Copy link
Contributor Author

nomeata commented Jan 10, 2020

This looks great, do you think we want to hold off on this until after Davos?

The merge conflicts in the test suite will not be fun, but maybe that’s reasonable.

OTOH, if we prepare the stdlib so that they don’t need to import prim (just stdlib/Debug), and we give them a PR against LinkedUp with the changes, then it would be fine, wouldn't it?

Co-Authored-By: Gabor Greif <gabor@dfinity.org>
nomeata added a commit that referenced this pull request Jan 10, 2020
this anticipates #1088: With this change, the LinkedUp code can be
changed to use only the stdlib, but _no_ definitions from the prelude.
This means that #1088 would not break LinkedUp.
@nomeata
Copy link
Contributor Author

nomeata commented Jan 10, 2020

See dfinity/linkedup#3 for how small the fallout on Linkedup is

@kritzcreek
Copy link
Contributor

If they're fine with it that's great. I just didn't want to put one more thing on their plate.

mergify bot pushed a commit that referenced this pull request Jan 10, 2020
* Stdlib: Export some conversion functions also in Nat and Int

this anticipates #1088: With this change, the LinkedUp code can be
changed to use only the stdlib, but _no_ definitions from the prelude.
This means that #1088 would not break LinkedUp.

* Likewise, export Array.{init/tabulate}

* Expose Debug.print
@nomeata
Copy link
Contributor Author

nomeata commented Jan 10, 2020

Yes, that’s absolutely right in asking for caution here (and I am glad you made me look at LinkedUp).

@nomeata nomeata merged commit 8e485d3 into master Jan 10, 2020
@nomeata nomeata deleted the joachim/prim-module branch January 10, 2020 17:34
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