upgrade fails on type-preserving ability handler changes

[ceedubs]

... in certain cases

Description

Upgrade can fail to automatically upgrade code like handle ... with foo even when the updated foo has the same type signature as the old foo.

This is a strange one, but I've run into it 3 times today with Http.handler coming transitively through several libs (aws, etc).

This is a pretty small reproduction, but might not be minimal. I'm not sure if it's specific to request handlers, but that's where I keep seeing it. I've spoofed library dependencies by directly adding to lib in my scratch file, but as far as I can tell it mimics the actual behavior.

Failing transcript

```ucm
.> project.create-empty upgrade-handler
upgrade-handler/main> builtins.merge

structural ability lib.Abort where
  abort : a

lib.foo_1_0_0.base_1_0_0.Abort.handler : Request {Abort} r -> r
lib.foo_1_0_0.base_1_0_0.Abort.handler = cases
    { r } -> r
    { abort -> _ } -> bug "error message 1"

bar = handle '(abort) with Abort.handler

upgrade-handler/main> add

lib.foo_1_0_1.base_1_0_1.Abort.handler : Request {Abort} r -> r
lib.foo_1_0_1.base_1_0_1.Abort.handler = cases
    { r } -> r
    { abort -> _ } -> bug "error message 2"

upgrade-handler/main> update
upgrade-handler/main> upgrade foo_1_0_0 foo_1_0_1

## Transcript failure

The transcript failed due to an error in the stanza above. The error is:

  I couldn't automatically upgrade foo_1_0_0 to foo_1_0_1.
  However, I've added the definitions that need attention to the
  top of scratch.u.

I'm not sure how to show it in the transcript, but when I run into this in the real world I get a cryptic compile error:

The handler used here

131 |     with lib.aws_4_0_3.lib.httpclient_5_0_2.Http.handler

has type _15 but I'm expecting a function of the form Request e a -> o.

[ceedubs]

It seemed to need to be a transitive dependency. Is it that update rips off the first level of lib when it prints code to your scratch file but this case is 2 layers deep?

[mitchellwrosen]

@ceedubs hrm, perhaps I'm missing something here, but the current behavior is looking correct.

When you upgrade foo bar, if there is a dependency on something in foo that does not have a corresponding name in bar, then your upgrade will fail.

That's what's happening here: base_1_0_0.Abort.handler exists in foo_1_0_0 but not foo_1_0_1.

So, for this real-world error message

  The handler used here

    131 |     with lib.aws_4_0_3.lib.httpclient_5_0_2.Http.handler

  has type _15 but I'm expecting a function of the form Request e a -> o.

I'd say you have a dependency in your non-lib code on lib.aws_4_0_3.lib.httpclient_5_0_2.Http.handler, and furthermore the only source of names for that hash exist in lib.aws_4_0_3, which you are trying to delete.

[ceedubs]

I guess that I see what you are saying. But also this behavior is pretty bewildering for a user. I guess that I'm used to the days of patches where the names didn't matter.

Now that names are playing an increasingly important role in update/ugprade/etc I'm warming up to your previous proposal that term names from transitive deps shouldn't be considered in-scope. Though I still think that we would want type names from transitive deps, and it might require some sort of transition period to not be too painful for users.

[mitchellwrosen]

I think we could definitely try to do better here – but which part is (most) bewildering? Any "upgrade from dependency v1 to dependency v2" command has to deal with the case that something in dependency v1 no longer exists in v2.

Perhaps those should be called out explicitly? Like, instead of just saying "I put stuff in scratch.u" it could say "and also blah was deleted, so watch out for that out-of-scope error".

This message also seems like it could easily be improved:

  The handler used here

    131 |     with lib.aws_4_0_3.lib.httpclient_5_0_2.Http.handler

  has type _15 but I'm expecting a function of the form Request e a -> o.

It seems some type unification error has taken precedence over a simple "variable out of scope" error, or something.

[ceedubs]

I think that there are a few things that make this confusing:

• In my original code I just wrote handle req with Http.handler and it wasn't clear to me that I was pinning very specifically to lib.aws_4_0_3.lib.httpclient_5_0_2.Http.handler. So it's surprising that if the aws lib updates its http dependency suddenly I'm not going to be able to cleanly upgrade.
• For a direct dependency, upgrade will be aware that httpclient_5_0_2.Http.handler has been updated to httpclient_5_0_3.Http.handler and will propagate this change for me. But when httpclient happens to be a transitive dependency, suddenly httpclient_5_0_2.Http.handler and httpclient_5_0_3.Http.handlerare treated as though they have no relationship to each other. As a user I'm thinking about it more as "oh the Http.handler from the HTTP client library" that I may or may not be aware I'm actually inheriting through the aws library. In my mind the 5_0_2 and 5_0_3 are just kind of there to help me eyeball what version of the library it is as opposed to part of a term name.

Having said all of that, I don't really think that the answer is to try to make upgrade recursive down lib namespaces or something. Maybe first-class dependencies will help in the long run, but in the short to medium term I'm starting to think that we should just prevent people from referencing terms that are only in transitive deps.