Skip to content

Editable typedefs stage 1#367

Merged
mergify[bot] merged 18 commits intomainfrom
georgefst/editable-typedefs-1
Apr 14, 2022
Merged

Editable typedefs stage 1#367
mergify[bot] merged 18 commits intomainfrom
georgefst/editable-typedefs-1

Conversation

@georgefst
Copy link
Copy Markdown
Contributor

@georgefst georgefst commented Apr 7, 2022

Closes #267. Although I've edited the "future work" section there, to draw attention to some limitations which were implicit in the original spec, and became clear during implementation.

There is one part of the spec which we don't implement here: it says that we don't add unnecessary holes - see e.g. "when the program does not otherwise typecheck". But in this PR, we are cautious and always add holes where they may be needed, without examining the wider context. Some discussion is still needed around implementating this. The simplest approach seems to be to add the hole, then try to clean up by running the typechecker with smart holes enabled (this is a bit of a sledgehammer, and we might be able to improve performance with a cleverer approach, but I think we already do similar elsewhere). There's a WIP at https://github.com/hackworthltd/primer/tree/georgefst/editable-typedefs-1-smartholes. The implementation might not need to be very complex, but there's no real harm in splitting it out in to a follow-up PR - the work in this one has already been in flight for long enough, without adding anything else.

There are some decisions about name clashing to be tackled as part of #359, #332 etc. For now, we check the minimum e.g. that type names don't clash with each other.

Note that there are some fairly (but not completely) tangential drive-by refactors in the first few commits. I could split the main "Add actions for incrementally editing type definitions" commit up more (i.e. one commit for each new action), but it just doesn't seem like a great use of time after the fact - it would mostly be a lot of busywork with fiddling imports.

Performance could be slightly better in a few places. We have functions like !?, insertAt, adjustAt, etc. which could take constant, rather than linear, time with the right data structures. This is not much of an issue for now as lists will be small in practice, and we have bigger performance issues elsewhere.

As with `moduleDefs` (already a map), we often need to look up elements in it anyway. See, for example, all of the calls to `mkTypeDefMap` which previously existed in the `EvalFull` module (but not `Eval`, where we already required the caller to pass the definitions as a map).
Analogous to `unsafeGlobalName`. Very mildly simplifies some existing code, though we'll use it again shortly.
@georgefst georgefst marked this pull request as draft April 7, 2022 13:12
Comment thread primer/src/Primer/Action.hs
Comment thread primer/src/Foreword.hs
Comment thread primer/src/Primer/App.hs
Comment thread primer/src/Primer/App.hs
Comment thread primer/test/Tests/Action/Prog.hs
We ensure that the student's program continues to typecheck, by adding holes as necessary.
@georgefst georgefst force-pushed the georgefst/editable-typedefs-1 branch from 67d06db to 4b4f0b6 Compare April 11, 2022 10:22
Comment thread primer/src/Primer/Core.hs Outdated
Comment thread primer/src/Primer/App.hs Outdated
@georgefst georgefst marked this pull request as ready for review April 11, 2022 11:01
@georgefst georgefst requested a review from a team April 11, 2022 11:01
Comment thread primer/src/Foreword.hs Outdated
Comment thread primer/src/Primer/App.hs Outdated
Comment thread primer/src/Primer/App.hs Outdated
Comment thread primer/src/Primer/App.hs
Comment thread primer/src/Primer/App.hs Outdated
Comment thread primer/src/Primer/App.hs
Comment thread primer/src/Primer/App.hs
Comment thread primer/src/Primer/App.hs Outdated
Comment thread primer/src/Primer/App.hs Outdated
Comment thread primer/src/Primer/App.hs Outdated
Comment thread primer/src/Primer/App.hs
Comment thread primer/src/Primer/App.hs Outdated
Comment thread primer/src/Primer/App.hs
Comment thread primer/src/Primer/Core.hs Outdated
@georgefst georgefst requested a review from brprice April 13, 2022 17:20
@dhess
Copy link
Copy Markdown
Member

dhess commented Apr 14, 2022

(Note: before this can be merged via Mergify, you'll need to mark every conversation as "resolved." We talked about relaxing this recently, and I apologize that I haven't gotten around to it yet.)

edit #387, but let's merge this first.

@georgefst
Copy link
Copy Markdown
Contributor Author

(Note: before this can be merged via Mergify, you'll need to mark every conversation as "resolved." We talked about relaxing this recently, and I apologize that I haven't gotten around to it yet.)

Thanks, I had forgotten that. Ordinarily I would have left #367 (comment) and #367 (comment) open, since they aren't so much "resolved" as we've just decided they aren't particularly important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge Ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable typedefs stage 1

3 participants