Summary representation of constants does not reflect the results of type inference

[stereotype441]

Consider the following code:

class C<T> {
  const C(T x);
}
const Object x = const C(1);
const Object y = const [1];
const Object z = const {1: 1};

When summarizing the above file, the analyzer records the static type of the variables x, y, and z (all of which are Object), but it does not record the inferred type arguments of their initializer expressions. As a result, if another build target attempts to make use of the summary and access the constant values of x, y, and z, it will see values of types C<dynamic>, List<dynamic>, and Map<dynamic, dynamic>, respectively, whereas the correct constant value types are C<int>, List<int>, and Map<int, int>.

Fixing this will require expanding the summary format to store additional type inference information.

[stereotype441]

After further searching, I see that #32290 partially overlaps with this issue (it only covers the case of x in the above example).

[MichaelRFairhurst]

OK, so after a lot of digging and an initial implementation, this is not actually an issue.

We reresolve constant expressions after resynthesis time, including inference, which was almost the approach I took but decided to reresolve at link time because it could have been a bit more performant. After writing a test and trying to get it to fail without my code, I simply couldn't find a single way:

const Object x = [1]; // inferred as List<int>
class C {
  const C() : assert(x is List<int>); // this passes, all the time
}

We thought this would fail because:

const double x = 1; // inferred int 2 double
class C {
  const C() : assert(x is double); // fails
}

But it turns out the reason for this is much much simpler than this bug describes (#35441);

[stereotype441]

After further research, it appears that this is indeed still an issue, and it's a special case of a larger problem: the summary representation of constants is purely syntactic, so it does not take into account the results of type inference. I believe the reason @MichaelRFairhurst didn't see an issue is that when a single file is analyzed, type inference is performed prior to constant evaluation, masking the problem.
The bugs tend to crop up only when multiple files are involved.

Currently I'm aware of two manifestations of this issue: #35908 and #35993. @scheglov is working on fixing this by representing constants in summaries by their values rather than their syntactic form. I'm repurposing this bug to track his work on this.

[scheglov]

@stereotype441 We have a problem. https://dart-review.googlesource.com/c/sdk/+/93961/ implements storing by value, and this fixes #35908 without a need for the workaround. However #35993 is still a problem, and I don't see how to solve it with this approach. Constructor initializers have to be stored syntactically, because the values they produce into fields and super-constructor-invocations depend on the values of constructor parameters.

[stereotype441]

@scheglov Hmm, you're right--that's an unfortunate problem.

Brainstorming some possible approaches to the problem:

1. Keep https://dart-review.googlesource.com/c/sdk/+/93961/, but add a new summary data structure to summaries to represent potentially constant values, e.g. LinkedPotentiallyConstantValue. This data structure would be able to represent (a) a reference to a constructor argument, (b) an operation on one or more potentially constant values, such as addition, multiplication, string concatenation, identical, etc., or (c) an is or as operation on a potentially constant value, or (d) a LinkedConstantValue.

2. As in 1, but just extend the LinkedConstantValue data structure to support the new operations rather than make a new data structure.

3. As in 1, Instead of inventing a new LinkedPotentiallyConstantValue data structure, re-use the old syntactic representation for "potentially constant" parts of constructor initializers.

WDYT? I'm also happy to discuss this in person on Monday.