Skip to content
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.

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

Proposal: remove special analyzer behavior for await expressions with "null context". #3648

Closed
stereotype441 opened this issue Mar 11, 2024 · 5 comments
Labels
feature Proposed language feature that solves one or more problems

Comments

@stereotype441
Copy link
Member

Background: I'm trying to update the expression inference rules in inference.md to match what was implemented, so that we can have a rigorous specification of the inference-update-3 logic I landed in https://dart-review.googlesource.com/c/sdk/+/353440. In the process I've discovered some subtle differences between the analyzer and front end type inference of await expressions. This issue addresses one such difference.

In the front end, type inference of an expression always takes place with respect to a type schema (the "context"). In the analyzer, type inference of an expression sometimes takes place with respect to a context, but sometimes takes place with respect to no context at all; the latter circumstance arises when the analyzer uses its standard AstVisitor mechanism to call one of the visit methods in the ResolverVisitor class, and so the visit method's contextType argument takes on the value null. Because of this I am calling this situation a "null context".

In all the circumstances where the analyzer infers an expression using a null context, the front end infers the same expression using a context of _. Furthermore, all but one of the analyzer's visit methods treat a null context the same as they treat a context of _. The one exception is visitAwaitExpression: in this method, if the context is the null context, then the analyzer analyzes the await expression's subexpression using a context of _; otherwise, it analyzes it using a context of FutureOr<_>. Whereas the front end, lacking any notion of a "null context", analyzes the await expression's subexpression using a context of FutureOr<_> in the same circumstances.

For the most part, the way contexts feed into the type inference algorithm is by appearing to the right of <# in the subtype constraint generation algorithm. This algorithm treats a right hand side of _ nearly identically to a right hand side of FutureOr<_>, so it is difficult to come up with an example of code that the analyzer and front end treat differently.

But we can exploit the special behavior of e1 ?? e2, which behaves as follows: if it is type inferred in context _, then e2 is type inferred using the static type of e1 as its context; whereas if it is type inferred in context K, then e2 is type inferred using K as its context.

So here is an example program that is analyzed differently:

Future<T> g<T>(T t) => Future.value(t);

extension on Future<int> {
  void foo() {}
}

test(Future<num>? f) async {
  await (f ?? (g(0)..foo()));
}

main() {}

This example is accepted by the front end, because:

  • await (f ?? (g(0)..foo())) is inferred using a context of _.
  • Therefore, f ?? (g(0)..foo()) is inferred using a context of FutureOr<_>.
  • Therefore, g(0)..foo() is inferred using a context of FutureOr<_>.
  • Therefore, g(0) is inferred using a context of FutureOr<_>.
  • Since the return type of g is Future<T>, and that satisfies FutureOr<_> for all T, downwards inference of g(0) does not constrain the type of T. So the type of T is set to int during upwards inference.
  • Therefore, g(0) has static type Future<int>.
  • Therefore, ..foo() resolves to the extension method foo defined in extension on Future<int>.

But it is rejected by the analyzer, because:

  • await (f ?? (g(0)..foo())) is inferred using a null context.
  • Therefore, f ?? (g(0)..foo()) is inferred using a context of _.
  • Therefore, g(0)..foo() is inferred using a context of Future<num>? (the static type of f).
  • Therefore, g(0) is inferred using a context of Future<num>?.
  • Since the return type of g is Future<T>, and that satisfies FutureOr<num>? only if T <: num, the type of T is set to num during downwards inference.
  • Therefore, g(0) has static type Future<num>.
  • Therefore, ..foo() does not resolve to the extension method foo defined in extension on Future<int>.

Whereas this program is accepted by both the anaylzer and front end:

Future<T> g<T>(T t) => Future.value(t);

extension on Future<int> {
  void foo() {}
}

test(Future<num>? f) async {
  var x = await (f ?? (g(0)..foo()));
}

main() {}

(The only difference is the addition of var x = before the await expression; this causes the analyzer to analyze the await expression using a context of _, just like the front end does).

I propose to base my update of the expression inference rules in inference.md on the front end's behavior in this corner case, and to change the analyzer's behavior to match the front end.

This change could in principle cause the analyzer to reject a program that it previously accepted, however I believe it's not necessary to go through the breaking change process, because any such program is already rejected by the front end.

@dart-lang/language-team any objections?

@stereotype441
Copy link
Member Author

Note: I've separately proposed, in #3571, changing the inference rules for await expressions to reduce the proliferation of unnecessary FutureOrs in contexts. That change is much more potentially breaking than this one (and should probably be done in a language-versioned way), so I'm hoping to move forward with this change first.

@lrhn
Copy link
Member

lrhn commented Mar 14, 2024

Sold. The "context type" type FutureOr<_> is a way of saying that we don't know what this is, or it's a future of something we don't know what is. That's not a good hint, it can't possibly help inference, so it's not a good context type. It can only get in the way of figuring out what the expression actually is.

Should we consider _? as just as useless? "This should be anything, or null".
Should we convert any top type to _?

Generally a union type with _ is meaningless and useless.

@lrhn lrhn added the feature Proposed language feature that solves one or more problems label Mar 14, 2024
@eernstg
Copy link
Member

eernstg commented Mar 15, 2024

SGTM!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Mar 18, 2024
During my work on dart-lang/language#3648, I
ran into some trybot failures with an exception that looked like this:

    RangeError (index): Invalid value: Valid value range is empty: -1
    #0      List.[] (dart:core-patch/growable_array.dart:264:36)
    #1      List.removeLast (dart:core-patch/growable_array.dart:336:20)
    #2      InferenceContext.popFunctionBodyContext (package:analyzer/src/generated/resolver.dart:124:33)
    #3      ResolverVisitor.visitBlockFunctionBody (package:analyzer/src/generated/resolver.dart:1890:38)

Some quick reading of the code revealed that `popFunctionBodyContext`
always removed a single entry from a list, and
`pushFunctionBodyContext` always added a single entry to it. The two
calls were always paired up using a straightforward try/finally
pattern that seemed like it should guarantee proper nesting, making
this exception impossible:

    try {
      inferenceContext.pushFunctionBodyContext(...);
      ...
    } finally {
      ...
      inferenceContext.popFunctionBodyContext(node);
    }

After a lot of headscratching and experimenting, I eventually figured
out what was happening: an exception was being thrown during
`pushFunctionBodyContext`, _before_ it had a chance to add an entry to
the list. But since the exception happened inside the `try` block, the
call to `popFunctionBodyContext` would happen anyway. As a result, the
pop would fail, causing its own exception, and the exception that was
the original source of the problem would be lost.

This seemed like a code smell to me: where possible, the clean-up
logic in `finally` clauses should be simple enough that it can always
succeed, without causing an exception, even if a previous exception
has put data structures in an unexpected state.

And I had gained enough familiarity with the code over the course of
my debugging to see that what we were doing in those `finally` clauses
was more complex than necessary:

- In the ResolverVisitor, we were pushing and popping a stack of
  `BodyInferenceContext` objects using the try/finally pattern
  described above. But we were only ever accessing the top entry on
  the stack, which meant that the same state could be maintained with
  a single BodyInferenceContext pointer, and some logic that can't
  possibly lead to an exception in the `finally` clause:

    var oldBodyContext = _bodyContext;
    try {
      _bodyContext = ...;
      ...
    } finally {
      _bodyContext = oldBodyContext;
    }

- In the ResolverVisitor and the ErrorVerifier, we were also pushing
  and popping a stack of booleans tracking whether the currently
  enclosing function (or initializer) has access to `this`. In the
  ResolverVisitor, this information wasn't being used at all, so it
  could be safely removed. In the ErrorVerifier, it was being used,
  but it was possible to simplify it in a similar way, so that it was
  tracked with a single boolean (`_hasAccessToThis`), rather than a
  stack.

Simplifying this logic brings several advantages:

- As noted above, since it's now impossible for an exception to occur
  in the `finally` clause, exceptions occurring in the `try` clause
  won't get lost, making debugging easier.

- The code should be more performant, since it no longer requires
  auxiliary heap-allocated stacks.

- The code is (IMHO) easier to understand, since the try/catch pattern
  for maintaining the new `_bodyContext` and `_hasAccessToThis` fields
  just involves saving a field in a local variable (and restoring it
  later), rather than calling out to separate classes.

Change-Id: I61ae80fb28a69760ea0b2856a6954b4a68cfcbe1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/358200
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
@stereotype441
Copy link
Member Author

Ok, I'm moving forward with this, via https://dart-review.googlesource.com/c/sdk/+/357521.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Mar 20, 2024
…text".

In the front end, type inference of an expression always takes place
with respect to a type schema (the "context"). In the analyzer, type
inference of an expression sometimes takes place with respect to a
context, but sometimes takes place with respect to no context at all;
the latter circumstance arises when the analyzer uses its standard
AstVisitor mechanism to call one of the visit methods in the
ResolverVisitor class, and so the visit method's contextType argument
takes on the value null. Because of this I am calling this situation a
"null context".

In all the circumstances where the analyzer infers an expression using
a null context, the front end infers the same expression using a
context of _. Furthermore, prior to this change, all but one of the
analyzer's visit methods treated a null context the same as they
treated a context of _. The one exception was visitAwaitExpression: in
this method, if the context was the null context, then the analyzer
analyzed the await expression's subexpression using a context of _;
otherwise, it analyzed it using a context of FutureOr<_>. Whereas the
front end, lacking any notion of a "null context", analyzes the await
expression's subexpression using a context of FutureOr<_> in the same
circumstances.

This change brings the analyzer behavior into line with the front end.

Fixes dart-lang/language#3648.

Bug: dart-lang/language#3648
Change-Id: Ifd77988010d4387ce48eaa20dff4356beec03753
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357521
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
@stereotype441
Copy link
Member Author

Fixed by dart-lang/sdk@f7d5d0c.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Mar 20, 2024
The analyzer's mechanism for passing contexts down the stack while
recursively resolving expressions is to add an optional `contextType`
parameter to some of the `visit` methods in `ResolverVisitor`. This
parameter is only included in `visit` methods that would actually use
it (e.g. `visitDoubleLiteral` doesn't have it, since the analysis of a
double literal doesn't depend on the context).

When the resolver needs to analyze a subexpression, if it needs to
supply a context, it uses `ExpressionImpl.resolveExpression` to
dispatch to the appropriate `visit` method; this passes the context
along to the optional `contextType` parameter. If, on the other hand,
it **doesn't** need to supply a context, it uses the standard
AstVisitor mechanism, which dispatches to the `visit` method without
supplying a context type, so the `contextType` parameter takes on its
default value.

Previous to this CL, the default value of each `contextType` parameter
was `null`; this had the unintended consequence of making a
distinction between a `null` context and a context of `_`
(`UnknownInferredType.instance`). The front end, by contrast, has a
visitor paradigm that allows passing a required argument through to
the `visit` method, so its context parameters are all non-nullable,
and it makes no such distinction.

Prior to addressing language issue 3648
(dart-lang/language#3648), the difference
was only observable for `await` expressions. Now that that issue has
been addressed, there is no user-visible difference between `_` and
the null context. But it's easy to imagine a difference accidentally
sneaking in during future development.

To avoid future bugs, this change makes `_` the default value for each
`contextType` parameter. To do this, it was necessary to change
`UnknownInferredType.instance` from a final variable to a const.

To the best of my knowledge, this change should have no user-visible
effect.

Bug: dart-lang/language#3648
Change-Id: Id74a513366831239df2d56ceac57c2dbe5c5084e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357522
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems
Projects
None yet
Development

No branches or pull requests

3 participants