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

Put the stdlib under the std namespace #1231

Merged
merged 14 commits into from Apr 11, 2023
Merged

Put the stdlib under the std namespace #1231

merged 14 commits into from Apr 11, 2023

Conversation

yannham
Copy link
Member

@yannham yannham commented Apr 6, 2023

Closes #1131.

This PR introduces a top-level value std in which lives the stdlib modules, to make them more discoverable, help completion, and avoid polluting the initial environment.

Design

There was a surprising number of small but non trivial dilemma to solve. The first is where this overarching std module should lie:

  • It sounds appealing to simply generate a record programmatically from Nickel, such that we don't even have to change anything to the current stdlib code. However, this unfortunately doesn't play well with the LSP, which can't handle terms without positions in a file. This would break completion and type information without a serious rethink of how the LSP currently works.

  • If we need a source (can be generated), then we can write a std.ncl file which simply imports the modules:

    {
      std = {
        array = import "array.ncl",
        string = import "string.ncl",
        ..
      }
    }
    

    The issue is that imports don't work for the stdlib, because it's included at compile-time in the binary.

  • Then we can generate this file by doing compile-time concatenation/include_str/substitution. We still have a source code to provide to the LSP (albeit being generated), but we don't have to use imports. Now, the issue is that say a contract failure on std.array.at null null will point to code that exists nowhere (has been generated and concatenated from existing fragments), and the user has no way to find this code anywhere, and thus can't even know what function is failing, depending on the formatting. Hence, we want the std.ncl file to be accessible

  • In the end, I went with squashing the modules together. Later on we might add special imports like import <std/array.ncl> which would allow to split the stdlib into separate files again, but this requires a bit of thinking. In the meantime, this decision put slightly more burden on us maintainers, but don't impact users (LSP is working, the file defining the stdlib is accessible as text).

Typechecking mutually recursive modules with polymorphic functions also proved challenging: thus, the enclosing std = { } is omitted (as opposed to the previous style) and inter-modules reference all goes through the std interface present in the initial typing environment, instead of being directly mutually recursive.

Reviewing

The important commits to review are:

  • Introduce the std namespace (62e7b09)
  • Update the LSP to work with the new std namespace (59d02a6)
  • Update RELEASES.md (0a3c1e6)

The rest is mostly a painful search and replace to update all the existing Nickel code. We've tested the benchmarks as well, to make sure they are up to date. The benchmarks showed a noticeable regression on some examples. Showing the flamegraphs didn't really help, as although the proportion varied a bit, nothing really stood out. Running a bigger example for longer doesn't seem to make that much of a difference, so it might be that loading the stdlib or accessing it has a penalty which shows on micro-benchmarks.

TODO

  • Fix panic in the LSP, because we're adding usages for std, but std doesn't get a Declaration or RecordField item. Should we just add an usage field to any node?
  • Documentation: will be done in a subsequent PR
  • Have a tentative fix for the LSP not completing on std

@yannham yannham marked this pull request as draft April 6, 2023 17:24
@github-actions github-actions bot temporarily deployed to pull request April 6, 2023 17:28 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 7, 2023 09:22 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 7, 2023 10:19 Inactive
@yannham yannham marked this pull request as ready for review April 7, 2023 10:36
@github-actions github-actions bot temporarily deployed to pull request April 7, 2023 10:44 Inactive
Copy link
Member

@vkleen vkleen left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few typos in comments 👍

RELEASES.md Outdated Show resolved Hide resolved
src/typecheck/mod.rs Outdated Show resolved Hide resolved
stdlib/std.ncl Outdated Show resolved Hide resolved
stdlib/std.ncl Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request April 7, 2023 12:27 Inactive
@yannham yannham changed the title Task/std namespace Put the stdlib under the std namespace Apr 7, 2023
@yannham
Copy link
Member Author

yannham commented Apr 7, 2023

Ah, completion for the stdlib is broken. I think I understood why, will probably fix in a coming PR, but I prefer to have a solution lined up before merging this one.

@github-actions github-actions bot temporarily deployed to pull request April 7, 2023 13:49 Inactive
yannham and others added 13 commits April 11, 2023 14:57
This commits put the different modules of the stdlib, previously living
directly at the top-leve in the initial environment (`array`, `string`,
etc.) under the `std` top-level record. This makes the stdlib more
structured, give an unified entry point (including for completion and
querying) and avoid polluting the environment with name that could clash
with e.g. local function parameters.

This changes came with a number of trade-offs. The LSP isn't happy if
something like `std` isn't defined in one location, so we have to write
or at least generated actual code for `std` (instead of programmatically
building the record, for example).

We also want the users to be able to read this file, even if generated,
because otherwise diagnosing a contract error would be quite hard.

There were also other subtleties with respect to typechecking mutually
recursive modules. In the end, the stdlib is currently squashed into one
file (plus the module internals, but which is special) `std.ncl`, which
content is bound to `std` in the initial environment.
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
@github-actions github-actions bot temporarily deployed to pull request April 11, 2023 14:14 Inactive
@yannham
Copy link
Member Author

yannham commented Apr 11, 2023

With the help of @ebresafegaga, we could restore the stdlib completion with a one-liner in 6186118.

@github-actions github-actions bot temporarily deployed to pull request April 11, 2023 15:19 Inactive
@yannham yannham merged commit 80edc42 into master Apr 11, 2023
4 of 5 checks passed
@yannham yannham deleted the task/std-namespace branch April 11, 2023 15:51
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.

Namespacing stdlib modules
2 participants