Skip to content

Conversation

mitchellwrosen
Copy link
Member

Overview

This PR fixes an issue that caused upgrade to behave unintuitively.

The algorithm essentially works as follows (before and after this PR):

  • When upgrading library dependency old to new, render out to a Unison file all dependents of all terms in lib.old (e.g. lib.old.thingy#oldthingy), but with the old segment swapped out for new, then parse and typecheck that.

    For example, consider the namespace:

    lib.old.thingy#oldthingy = ...
    lib.new.thingy#newthingy = ...
    myfoo = #oldthingy + 100
    

    On upgrade old new, we render, parse, and typecheck a Unison file like

    myfoo = lib.new.thingy + 100
    

The issue was to do with aliases. Prior to this PR, additional aliases of things in lib.old.* would essentially prevent upgrading as follows.

Consider the namespace:

lib.old.thingy#oldthingy = ...
lib.new.thingy#newthingy = ...
mythingy#oldthingy = ...
myfoo = #oldthingy + 100

The old implementation would render, parse, and typecheck a Unison file like

myfoo = mythingy + 100

as the "old" reference #oldthingy still had a set of names { lib.new.thingy, mythingy } from which we make the pretty-print environment.

This behavior is especially bad considering dependencies' dependencies are actually in scope for developers (something we neglected to think about when implementing the algorithm the first time around).

While in the example above, the user's project code itself has a name for #oldthingy, it's probably much more common for #oldthingy to be referenced by some dependency that still depends on old (which the user is trying to upgrade to `new):

lib.old.thingy#oldthingy = ...
lib.someOtherDependency.lib.old.thingy#oldthingy = ...

This would also keep #oldthingy alive / prevent an upgrade to #newthingy in the exact same way.

So, the solution implemented here is simply to pretty-print hashes that are directly referenced by the old dependency, i.e. things at the top level of lib.old.* (no matter what's in lib.old.lib.*), as lib.new.* (appropriately suffixified), no matter if there are any other aliases.

@mitchellwrosen mitchellwrosen force-pushed the 23-12-05-fix-upgrade-alias-bug branch from b9f8c92 to 7d9dc85 Compare December 5, 2023 18:16
previously, any alias of an "old" thing would essentially prevent it
from being updated to the new corresponding thing.

this patch attempts to solve that problem by adjusting the PPE we use to
print things
@mitchellwrosen mitchellwrosen force-pushed the 23-12-05-fix-upgrade-alias-bug branch from 7d9dc85 to ee758a2 Compare December 5, 2023 18:17
@mitchellwrosen mitchellwrosen marked this pull request as ready for review December 6, 2023 15:10
@aryairani
Copy link
Contributor

@mitchellwrosen Looks good! Could you update the transcript to view bar also after the upgrade?

@mitchellwrosen
Copy link
Member Author

@aryairani sure, done

@aryairani
Copy link
Contributor

We found another bug where upgrade doesn't work if you haven't loaded any scratch file yet in your ucm session. @mitchellwrosen is working on a fix for that now 🙏

@aryairani aryairani merged commit d0d571d into trunk Dec 6, 2023
@aryairani aryairani deleted the 23-12-05-fix-upgrade-alias-bug branch December 6, 2023 18:25
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.

2 participants