Analyzer needs a compatibility mode to support non-NNBD-aware clients

[stereotype441]

Recently we updated the analyzer's mock SDK to be NNBD-enabled (545b80e). This turned out to break clients that use the mock SDK for their unit tests, but don't yet properly know how to handle NNBD types.

We'd thought that this would not be an issue, because NNBD types look just like legacy types, but with a new getter indicating their nullability. We'd thought that clients that don't yet properly know how to handle NNBD types would simply ignore that new getter. Unfortunately that didn't work, because those clients still make use of the analyzer's TypeSystem object to do things like subtype checking, and the TypeSystem object does understand the NNBD types. So there's a mismatch between the client's NNBD awareness and the TypeSystem object's NNBD awareness, and that's breaking invariants about subtype relationships that the clients rely on.

Fortunately, today, this is not a huge deal, because it is only affecting unit tests for analyzer clients, and we can always roll back the mock SDK change to un-break them. But when we un-fork the SDK, it's going to become a huge deal, because then it will affect code generators running in production.

As I see it, there are three broad approaches to solving this problem:

1. Prior to unforking the SDK, update all clients of the analyzer to properly handle NNBD types.

2. Add a "compatibility mode" to the analyzer allowing clients to opt out of seeing the consequences of NNBD types, and prior to unforking the SDK, update all clients of the analyzer to make use of this mode. Later, when a client has been modified to properly handle NNBD types, it can switch off compatibility mode.

3. Same as 2, but make compatibility mode the default behavior of the analyzer.

Option 1 doesn't seem feasible; it would delay unforking the SDK too much. Option 2 might be feasible, but it's scary; pushing this sort of change out through the ecosystem (even just in our internal codebase) sometimes takes weeks. Option 3 has the advantage that we can address it entirely inside the analyzer codebase, but the disadvantage that it forces compatibility mode to be a global switch, rather that something that the client opts into in a fine-grained manner.

However, we still haven't figured out exactly how to go about implementing this compatibility mode, and the way we solve that problem may make option 3 impossible. A few options we've discussed are:

• When in compatibility mode, the analyzer could analyze all code as though it's opted out, but ignore errors like having an unexpected ? after a type name. We'd also have to change the analyzer's summary resynthesizer so that in compatibility mode, it erases type nullabilities. Advantage: it's a global switch, so option 3 is possible. Disadvantage: suppressing all the errors may be difficult (for example, do we switch off flow analysis, and if so, do we have to suppress a lot of type errors?)

• The way compatibility mode could work is that every time a client gets an element (e.g. from an AST), they have to call .toLegacyElement. Advantage: easy to implement in the analyzer. Disadvantage: since it's not a global switch, so option 3 isn't feasible. Disadvantage: since the client must remember to call .toLegacyElement whenever they access an element, there is a greater danger of forgetting to do so in a necessary place and not discovering until later, when it's not convenient to update the client (e.g. when trying to migrate a package that makes use of the client as a code generator).

• Implement compatibility mode using a boolean flag in the TypeSystem class; when in compatibility mode, it ignores nullabilities when making type comparisons. Advantage: it's a global switch, so option 3 is possible. Advantage: a relatively small amount of analyzer code is affected. Disadvantage: we would also need to make DartType.operator== ignore nullabilities when in compatibility mode, but DartType.operator== doesn't have access to the TypeSystem object.

• Implement compatibility mode using a static boolean flag that affects the behavior of the TypeSystem class and DartType.operator==. Advantage: it's a global switch, so option 3 is possible. Advantage: a relatively small amount of analyzer code is affected. Disadvantage: injecting behaviors like this through a static bool seems like a poor design. Disadvantage: if two analyzer clients coexist in the same isolate, they might fight over the setting of the flag. (Note: I'm not sure whether this happens when using package:build, or whether package:build segregates code generators into separate isolates).

[stereotype441]

Here's another possible implementation approach (and my current favorite).

• Implement compatibility mode using a boolean flag in the TypeSystem class; when in compatibility mode, it ignores nullabilities when making type comparisons. (Note: we already have a boolean isNonNullableByDefault in the TypeSystem class, but we don't yet use it in methods like isSubtypeOf)
• Add a method to TypeSystem for checking whether two types are the same; this method respects the boolean flag.
• Change the behavior of DartType.operator== and DartType.hashCode so that they ignore nullabilities.
• Change the analyzer API so that by default, clients receive a TypeSystem that has compatibility mode enabled.

The steps above are sufficient to allow us to safely unfork the SDK. The steps below are about getting proper functionality once NNBD is enabled:

• Add something to the analyzer API to allow a client that knows how to properly handle NNBD types to request an NNBD-aware TypeSystem (one with compatibility mode disabled). When we're ready to add NNBD support to an analyzer client, we'll change it to use this API to request the TypeSystem object.
• Internally in the analyzer, ensure that we use an NNBD-aware type system when analyzing code that is opted in to NNBD (assuming we implement compatibility mode using the existing TypeSystem.isNonNullableByDefault flag, we already do this).
• Stop using DartType.operator== and DartType.hashCode inside the analyzer, and use the new TypeSystem method for checking whether two types are the same.
• Deprecate DartType.operator== and DartType.hashCode (note: this addresses technical debt issue We should probably deprecate DartType.operator== and DartType.hashCode #40507). Clients can wait until they're ready to support NNBD to stop using them.

@scheglov @bwilkerson @MichaelRFairhurst @jcollins-g what do you think?

[scheglov]

1. Any analysis should be done as the specification requires, including all type operations. We may only change behavior when type operations are done by the client. We cannot use isNonNullableByDefault flag of TypeSystem for this, it is the flag that specifies if this flag is for a migrated library.

2. Adding a new flag to TypeSystem and threading it to every place is cumbersome. Adding new APIs for clients to access new "really working" TypeSystem instances means that we accumulate more technical debt to remove later (make current typeSystem working as "real", deprecate previously created "real" getter, move everyone, remove created "real").

3. So, I'd rather created one static flag that controls operations on types after analysis is done. Using static fields is ugly, so we want to migrate clients quickly.

4. To support multiple clients in the same isolate we could use some helper method in analyzer, like runWithNullSafetyTypesAfterAnalysis(() {...}). So, the client not just sets the flag once and forget, but wraps code that knows about NNBD. For generators it might be just one point, the generate method.

5. If I understood it correctly, we want to release a minor version of analyzer that has such flag, so that there is at least one non-breaking change of analyzer that works with un-forked SDK. If just one version is all we need, then right after this we can remove the flag and publish the breaking version. So, any client upgrading analyzer will have to adapt. And the ugly flag can go away almost immediately. The run method will live one more version, but will do nothing with the flag, will just run the given function.

[stereotype441]

I'm open to the static variable approach (though I think we actually need to use a zone-local variable--see my further comments below).

1. Any analysis should be done as the specification requires, including all type operations. We may only change behavior when type operations are done by the client. We cannot use isNonNullableByDefault flag of TypeSystem for this, it is the flag that specifies if this flag is for a migrated library.
2. Adding a new flag to TypeSystem and threading it to every place is cumbersome. Adding new APIs for clients to access new "really working" TypeSystem instances means that we accumulate more technical debt to remove later (make current typeSystem working as "real", deprecate previously created "real" getter, move everyone, remove created "real").

You're right, my proposal would introduce some technical debt.

3. So, I'd rather created one static flag that controls operations on types after analysis is done. Using static fields is ugly, so we want to migrate clients quickly.

Something I really like about this idea is that it addresses the issue of what to do about DartType.operator==, since DartType.operator== can just look at the static field.

4. To support multiple clients in the same isolate we could use some helper method in analyzer, like runWithNullSafetyTypesAfterAnalysis(() {...}). So, the client not just sets the flag once and forget, but wraps code that knows about NNBD. For generators it might be just one point, the generate method.

The analyzer API used by code generators is asynchronous, so it would be a little more complex than this. But I think it would still be doable if we use zone-local storage rather than a static variable, e.g.:

static final zoneKeyForNullSafetyTypes = Object();
R enableNullSafetyTypes<R>(R body()) => runZoned<R>(
    body, {zoneKeyForNullSafetyTypes: true});
class TypeSystem {
  static bool _usingNullSafetyTypes => Zone.current[zoneKeyForNullSafetyTypes] ?? false;
  ...
}

5. If I understood it correctly, we want to release a minor version of analyzer that has such flag, so that there is at least one non-breaking change of analyzer that works with un-forked SDK. If just one version is all we need, then right after this we can remove the flag and publish the breaking version. So, any client upgrading analyzer will have to adapt. And the ugly flag can go away almost immediately. The run method will live one more version, but will do nothing with the flag, will just run the given function.

I don't think we can immediately remove the flag and publish the breaking version, because we want to keep rolling the latest version of the analyzer into our internal codebase without causing any breakages. So we'll have to keep the flag around until all internally-used analyzer clients have been made sufficiently NNBD-aware that they won't be broken by hardcoding it to true.

Also, since the purpose of all this machinery is to make sure that we don't have breakages when the SDK is unforked, it would be nice to wait until the unfork happens before ripping it out again, just to make sure there aren't any unanticipated breakages.

But I think that's ok. Having the flag around is technical debt, but it's technical debt that we should be able to resolve before we publish full NNBD support, and I think it's lower impact technical debt than what I'd proposed.

[stereotype441]

@scheglov should we close this issue or is there additional work to be done?

[stereotype441]

We don't know of any additional work to be done. We may encounter more issues when we actually do the unfork, and if that happens we'll address them as they arise.