feat: add names to modules by brprice · Pull Request #375 · hackworthltd/primer

brprice · 2022-04-11T14:41:52Z

No description provided.

brprice · 2022-04-11T15:30:07Z

Tracking comment for progress towards implementing the FR (I may split this out into a new pr):

Hierarchical names (i.e. change a ModuleName into a list of names): see feat!: hierarchical module names #385
An action to rename the editable module: see feat: action to rename a module #379

Notes for reviewers:
This is an annoyingly large PR, but much of it is mechanical. It may be sensible to split it after the commit "refactor: use modules where possible", as the initial commits are some refactoring to set up, and the last commit is a large one that touches everywhere a global name is modified, to add a module prefix.
~~The pr will not pass CI due to an intentionally failing test that I have opened a thread about below.~~

georgefst · 2022-04-14T12:09:43Z

Note that we should update the comment about checking all modules in applyProgAction's RenameType case.

See #367 (comment).

This is in preperation for adding module names, which will require some renaming of primitives. We will export some identifiers for these names instead of writing them as literal strings, to make any future renames easier.

This is in preperation for adding module names, which will require some renaming of primitives.

This is not yet needed, as currently primitive types are always in scope -- the '...WithPrims' just adds primitive terms. But this will change shortly.

Instead of using 'defaultTypeDefs', define and use 'builtinModule' and 'primitiveModule'. This is in preparation for adding module names and taking modules more seriously. There is one theoretical drawback: in Tests.EvalFull we lose the ability to generate definitions for global variables. However, we never actually used this ability, so there is no loss in practice. (However, we may want to re-instate this ability in the future so we can generate an arbitrary context in these tests.)

brprice · 2022-04-25T13:35:02Z

After rebasing this onto main, I am seeing sporadic CI failures in type_preservation, e.g.

Use '--hedgehog-replay "Size 37 Seed 10684577349138219122 13324058589961643495"' to reproduce.
Use -p '$0=="test/Test.hs.Tests.EvalFull.type preservation"' to rerun this test only.

https://buildkite.com/hackworthltd/primer/builds/514#d236758d-3a20-4f3a-815b-d38e61c81420
I will investigate before merging, but given that it happened on the very first commit, I expect it is a pre-existing bug.

This is a large commit, but most of it is pretty mechanical. NB: we remove the test 'unit_rename_def_capture'. It tested that we could not rename a definition in such a way that a reference would be captured by a local binding. However, now that global variables have module-qualified names, this is no longer possible: a local binding is not considered to shadow any globals. BREAKING CHANGE: this change requires a database migration, as it changes the representation of `Prog`. However, since this is just serialised to json and stored as a blob in the DB, it requires no schema changes. Since we have no programs we need to preserve, we decided not to bother with a migration. This means that DBs created before this commit will not load with a primer containing this commit.

brprice · 2022-04-25T16:09:23Z

After rebasing this onto main, I am seeing sporadic CI failures in type_preservation

This is indeed a pre-existing bug. I'll track this in #407.

Let's merge this PR -- I retried CI and it now passes (the failing test is a hedgehog test which only sporadically fails)

brprice force-pushed the brprice/module-names branch 2 times, most recently from 5fbbe33 to 460cbf1 Compare April 11, 2022 14:58

brprice linked an issue Apr 11, 2022 that may be closed by this pull request

Add identifiers to modules #320

Closed

brprice force-pushed the brprice/module-names branch from a822e8b to c6582b5 Compare April 11, 2022 16:17

Base automatically changed from brprice/refactor-internalerror-text to main April 11, 2022 16:33

brprice force-pushed the brprice/module-names branch from 93d9334 to d1b6d42 Compare April 12, 2022 14:40