8000 Rewrite Slurping code by ChrisPenner · Pull Request #2814 · unisonweb/unison · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rewrite Slurping code #2814

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 59 commits into from
Feb 17, 2022
Merged

Rewrite Slurping code #2814

merged 59 commits into from
Feb 17, 2022

Conversation

ChrisPenner
Copy link
Member
@ChrisPenner ChrisPenner commented Jan 18, 2022 •
edited
Loading

Overview

The slurping engine is pretty old at this point and the edge-cases have accumulated. There are now several different parts of the engine which should depend on each other, and it can be pretty tricky to figure out how things are actually calculated. The catalyst for this was #2786 , which is the result of a few edge-cases interacting with each other in a way that's easy to miss.

This rewrite turns slurping into a process with explicit stages, which should make it easier to find where specific checks take place. providing a few additional benefits.

  • The current version tries to calculate all pieces at once, this is tricky to get right since certain steps affect each other (e.g. constructor deprecations depend on which definitions are under consideration)
  • The current version makes it pretty easy for a single definition to end up in multiple buckets
  • The current version can tell you that something is 'blocked', but can't tell you why, or which def is blocking it.
  • Constructors are handled more explicitly, so we have control over whether to show them in output or not.

When you save a scratch file it now splits up the process into:

  1. Parse and typecheck the file
  2. Identify all of the definitions in the file by name
  3. Compute the transitive closure of all variables requested by the user (e.g. the x and its dependencies in add x)
  4. Determine any deprecated constructor names and remove them from the names we're considering.
  5. Analyze each definition and determine its status (e.g. new, update, ctor collision, etc.)
  6. Determine each definition's "transitive status" by folding in the statuses of its dependencies.
  7. Group definitions by their status and convert this mapping to the old SlurpResult type so we get the same output and results from that point onwards.

So why do we need to rewrite this?

Make the change easy, then make the easy change

  • Albert Einstein probably

@ChrisPenner
WIP
1e6c659
@ChrisPenner
WIP
bb52203
@ChrisPenner
WIP
d957c0f
@ChrisPenner
WIP
4e33596
@ChrisPenner
WIP
191ad5b
@ChrisPenner
WIP
0a960c4
@ChrisPenner
WIP
e94448b
@ChrisPenner
Seems to be working 8000
5038744
@ChrisPenner
Checkpoint
9f85ff5
@ChrisPenner
Wire up aliasing
1360594
@ChrisPenner
Feature parity??
fb139d6
@ChrisPenner
WIP
b24a20c
@ChrisPenner
WIP
a88dd94
@ChrisPenner
Use shared SlurpComponent
efb41b5
@ChrisPenner
Ensure we add adds on update
8932523
@ChrisPenner
Actually handle passed vars correctly
b3ffe72
@ChrisPenner
Handle aliases the same < 8000 /code>
fcea773
@ChrisPenner
Fix error message for conflicts
1409f2b
@ChrisPenner
Keep old conflict behaviour for the time being.
3ae9867
@ChrisPenner
Add additional test transcripts from #2786
f9279d2
@ChrisPenner
Fix deprecation detection
dac6243
@ChrisPenner
Check defsToConsider in deprecation checks
b13620a
@ChrisPenner
Make deprecated constructors more robust
d33c200
@ChrisPenner
Only return old slurp result
6b5db96
@ChrisPenner
Add ability-term conflict transcripts
0993de4
@ChrisPenner
Remove unused combinators from old slurp result
cd6f039
@ChrisPenner
Re-use old filterBySlurpResult
6bbc6b3
@ChrisPenner
Comments
e5c314b
@ChrisPenner
Constructor deprecations optimization
10e4389
@ChrisPenner
Docs
09286a3
ChrisPenner
ChrisPenner commented Jan 21, 2022
View reviewed changes
unison-src/transcripts-using-base/tls.output.md Outdated
@@ -29,7 +29,7 @@ test> match (decodeCert.impl (toUtf8 self_signed_cert_pem) with

⍟ These new definitions are ok to `add`:

test.ko630itb5m (Unison bug, unknown term)
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this bug was happening because the old slurping code was filtering watches out of the TypecheckedUnisonFile too aggressively.

ChrisPenner
ChrisPenner commented Jan 21, 2022
View reviewed changes
unison-src/transcripts-using-base/fix2027.output.md
@@ -53,7 +53,6 @@ myServer = unsafeRun! '(hello "127.0.0.1" "0")
change:

⊡ Previously added definitions will be ignored: Exception
Exception.raise
Copy link
Member Author

Choose a reason for hiding this comment

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

We chatted about this and decided there's not much of a reason to print out duplicate constructors when the type itself is already listed.

Adding Constructors to the Slurp Components explicitly gives us a lot more flexibility in the future if we decide to change what we show.

@ChrisPenner ChrisPenner marked this pull request as ready for review January 21, 2022 16:42
@ChrisPenner ChrisPenner changed the title WIP: Rewrite Slurping code Rewrite Slurping code Jan 21, 2022
@aryairani
Copy link
Contributor

Do you think a squash merge would be good for this one?

@ChrisPenner
Copy link
Member Author
ChrisPenner commented Jan 21, 2022 •
edited
Loading

Do you think a squash merge would be good for this one?

I always prefer squash merges when possible yup 🙌🏼 , (unless there's another branch based on what you're merging, but that's not the case here 😄 ).

mitchellwrosen
mitchellwrosen reviewed Jan 26, 2022
View reviewed changes
6D40
aryairani
aryairani approved these changes Feb 5, 2022
View reviewed changes
Copy link
Contributor
@aryairani aryairani left a comment

Choose a reason for hiding this comment

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

Great work, thanks @ChrisPenner. I finally left some comments.

@ChrisPenner ChrisPenner mentioned this pull request Feb 7, 2022
Closed
@ChrisPenner
Copy link
Member Author

I did some benchmarking on typechecking/slurping all of nimbus to make sure this doesn't cause a regression,

trunk:

23.08s

rewrite-slurping

23.15s

So it's safe to say the difference is negligible 👍🏼

@pchiusano pchiusano merged commit 5e827fc into trunk Feb 17, 2022
@pchiusano pchiusano deleted the cp/rewrite-slurping branch February 17, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aryairani aryairani aryairani approved these changes

@mitchellwrosen mitchellwrosen mitchellwrosen approved these changes

Assignees

@ChrisPenner ChrisPenner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChrisPenner @aryairani @mitchellwrosen @pchiusano
0