Establish documentation links for non-promotion reasons #44900

stereotype441 · 2021-02-09T15:04:29Z

(Parent issue #44897)

We are currently in the process of adding functionality to the CFE and analyzer to explain type promotion failures to the user. For example in the following code:

class C {
  int? i;                  // (1)
  f() {
    if (i == null) return;
    i.isEven;              // (2)
  }
}

the error message generated at (2) will contain a context message (pointing to (1)) explaining that i couldn't be promoted because it's a field.

We'll have other context messages for other non-promotion reasons (e.g. the variable has been written to since it was promoted, the variable is write captured, etc.)

It would be nice if these context messages could contain links to documentation pages explaining the reasons for the type promotion failures in more detail. This is particularly important for field non-promotion, since it's a common user trap.

Note that this bug covers both writing the documentation and linking it up to the context messages.

The text was updated successfully, but these errors were encountered:

stereotype441 · 2021-02-09T15:04:59Z

I'm not sure who to assign this to: perhaps @munificent or @kwalrath?

kwalrath · 2021-02-09T16:42:04Z

This reminds me of diagnostic messages, which are written by the engineer and are in the SDK repo, but are edited by the writing team and published on dart.dev. For details, ask @bwilkerson and check out the comments at the top of https://github.com/dart-lang/site-www/blob/master/src/tools/diagnostic-messages.md.

bwilkerson · 2021-02-09T16:53:10Z

The expectation is that someone will write a separate document describing promotion or at least the reasons why is sometimes fails. I believe that's because we expect the size of the document to be unwieldy for the diagnostic messages page.

My hope is that the diagnostic will include a link to the diagnostic messages entry, not to the promotion page directly, and that we can link to the promotion page from the appropriate diagnostic messages (there will be several of them) for users that need more information than is available from the diagnostic messages page.

We might need slightly different support for the CFE-based tools than for analyzer, because the diagnostics they produce in these cases might not be a 1-to-1 match to what analyzer is producing, and in either case, analyzer needs to keep the URL(s) separate from the messages so that clients can treat them as links.

kwalrath · 2021-02-09T17:07:37Z

It's easy enough for us to provide a go link and/or page as a target for these explanations, but writing up the explanations in a finished way is going to take some time. (Maybe @munificent will have that time sooner than I will?)

stereotype441 · 2021-03-24T21:24:48Z

@munificent @kwalrath It's probably time to start working on this. At a minimum we need to figure out what the link URL should be explaining about the lack of field promotion, so that I can commit it prior to the stable branch cut (which I believe is scheduled for Apr 6). We don't necessarily have to have the content ready until the release happens.

In the long term we'll want other pages (or other sections of the same page) to explain about other reasons why promotion might fail (e.g. the variable was written to between test and usage, or the variable is written to in a local function), but it's not certain how many of those reasons will be supported by the branch cut date. We should probably decide on the link URLs for all these cases now, so that I can add support as I can before the branch cut. I can get y'all a complete list of the possible type promotion failure reasons in the next day or two.

kwalrath · 2021-03-24T22:12:14Z

I'd prefer to have one page rather than many, to enable in-page search and keep the website URLs manageable. If this doc lives on dart.dev, the URL would probably be something like dart.dev/tools/<filename>, where <filename> might be non-promotion-reasons. (This is parallel to https://dart.dev/tools/diagnostic-messages.)

If you expect people to type in these URLs, then we could provide one or more go links, as well. They could be either to the page or to sections within the page. E.g. we could use one of the following schemes:

dart.dev/go/non-promotion
dart.dev/go/non-promo, dart.dev/go/non-promo-field, dart.dev/go/non-promo-write-captured, ...
dart.dev/go/non-promo, dart.dev/go/non-promo-f, dart.dev/go/non-promo-wc, ...
dart.dev/go/non-promo, dart.dev/go/non-promo-1, dart.dev/go/non-promo-2, ...

bwilkerson · 2021-03-24T22:18:27Z

I can get y'all a complete list of the possible type promotion failure reasons in the next day or two.

Could you also get a list of the analyzer diagnostics with which these context messages are associated? After we've settled on a URL for this documentation I'd like to add a link to it from the documentation for each of those diagnostics.

stereotype441 · 2021-03-25T17:11:39Z

Ok, here's all the reasons I'm aware of why type promotion might fail. This is kind of a brain dump so sorry for the length. Note that these examples are all pretty contrived; hopefully someone can dig around some successfully migrated code for more realistic examples :)

Probably not all of these are worth writing about. But I think it would be good to reserve a "go" link for each of them, so that I can point the error messages to those links as I implement them. Initially all the "go" links could resolve to a common "catch all" article about lack of type promotion, and then we could change them if/when we discover that we have more to say about a particular case.

The thing the user is trying to promote isn't a local variable, e.g.:

class C {
  int? i;
  f() {
    if (i != null) return;
    print(i.isEven); // ERROR
  }
}

Note that it's an instance field in this example, but it could also be an instance getter, or a static field or getter, or a top level variable or getter. Also we don't allow promoting this (even though it would be sound to do so; there were some technical reasons that this would have been difficult and we didn't think it would be worth it)

The variable the user is trying to promote might have been written to since it was promoted, e.g.:

f(bool b, int? i, int? j) {
  if (i != null) return;
  if (b) {
    i = j; // (1)
  }
  if (!b) {
    print(i.isEven); // (2) ERROR
  }
}

In this example, when flow analysis hits (1), it demotes i from non-nullable int back to nullable int?. A human user can tell that the access at (2) is safe, because there's no code path that includes both (1) and (2), but flow analysis isn't smart enough to see that, because it doesn't track correlations between conditions in separate if statements. The user could fix the problem by coalescing the two if statements:

f(bool b, int? i, int? j) {
  if (i != null) return;
  if (b) {
    i = j;
  } else {
    print(i.isEven);
  }
}

Note that in straight-line control flow cases like these (no loops), flow analysis takes into account the right hand side of the assignment in order to decide whether to demote. That means the user could also fix this by changing the type of j to int.

The thing the user is trying to promote might have been written to in a previous iteration of a loop, and so the promotion was invalidated, e.g.:

f(Link? p) {
  if (p != null) return;
  while (true) { // (1)
    print(p.value); // ERROR
    var next = p.next;
    if (next == null) break;
    p = next; // (2)
  }
}

What's happening here is that when flow analysis reaches (1), it looks ahead and sees the write to p at (2). But since it's looking ahead, it hasn't yet figured out the type of the right-hand side of the assignment, so it doesn't know whether it's safe to retain the promotion or not. So it's conservative and it invalidates the promotion. The user could fix this by moving the null check to the top of the loop, e.g.:

f(Link? p) {
  while (p != null) {
    print(p.value);
    p = p.next;
  }
}

This situation can also arise in switch statements if there's a case block that has a label, because the user can use labeled switch statements to construct loops, e.g.:

f(int i, int? j, int? k) {
  if (j == null) return;
  switch (i) {
    label:
    case 0:
      print(j.isEven);
      j = k;
      continue label;
  }
}

The variable the user is trying to promote might have been written to in a try block, and we are now in a catch block, e.g.:

f(int? i, int? j) {
  if (i == null) return;
  try {
    i = j; // (1)
    if (i == null) return; // (2)
  } catch (...) {
    print(i.isEven); // ERROR
  }
}

In this case, flow analysis doesn't consider i.isEven safe, because it has no way of knowing when in the try block the exception might have occurred, so it conservatively assumes that it might have happened between (1) and (2), when i was potentially null.

Similar situations can occur between try and finally blocks, and between catch and finally blocks.

Because of a historical artifact of how the implementation was done, these try/catch/finally situations don't take into account the right-hand side of the assignment, similar to what happens in loops.

The type the user is trying to promote to isn't a subtype of the variable's current promoted type (or wasn't a subtype at the time of the promotion attempt), e.g.:

f(Object o) {
  if (o is Comparable) { // (1)
    if (o is Pattern) { // (2)
      print(o.matchAsPrefix('foo')); // (3) ERROR
    }
  }
}

In this example, o is promoted to Comparable at (1), but it's not promoted to Pattern at (2), because Pattern is not a subtype of Comparable. (The rationale is that if we did promote, then the user wouldn't be able to access methods on Comparable anymore). Note that just because Pattern isn't a subtype of Comparable doesn't mean the code at (3) is dead; if the user has defined a class that implements both Comparable and Pattern, then (3) would be reachable.

The variable the user is trying to promote has been write captured by a local function or function expression, e.g.:

f(int? i, int? j) {
  if (i == null) return;
  var foo = () {
    i = j;
  };
  print(i.isEven); // ERROR
}

Flow analysis reasons that as soon as the definition of foo is reached, it might get called at any time, therefore it's no longer safe to promote i at all. As with loops, this demotion happens regardless of the type of the right hand side of the assignment.

The variable the user is trying to promote is written to outside of a closure or function expression, and we are currently inside the closure or function expression, e.g.:

f(int? i, int? j) {
  if (i == null) return;
  var foo = () {
    print(i.isEven); // ERROR
  };
  i = j; // (1)
}

Flow analysis reasons that there is no way of telling when foo might get called, therefore it might get called after the assignment at (1), and thus the promotion might no longer be valid. As with loops, this demotion happens regardless of the type of the right hand side of the assignment. A particularly nasty case of this one looks like this:

f(int? i) {
  i ??= 0;
  var foo = () {
    print(i.isEven); // ERROR
  };
}

In this case, a human reader can see that the promotion is safe because the only write to i (a) writes a non-null value, and (b) happens before foo is ever created. But flow analysis is not that smart. See dart-lang/language#1536.

The variable the user is trying to promote is write captured outside of a closure or function expression, and we are currently inside the closure or function expression trying to promote it, e.g.:

f(int? i, int? j) {
  var foo = () {
    if (i == null) return;
    print(i.isEven); // ERROR
  };
  var bar = () {
    i = j;
  };
}

Flow analysis reasons that there's no way of telling what order foo and bar might be executed in; in fact, bar might even get executed halfway through executing foo (due to foo calling something that calls bar). So it's not safe to promote i at all inside foo.

stereotype441 · 2021-03-25T17:12:56Z

I can get y'all a complete list of the possible type promotion failure reasons in the next day or two.

Could you also get a list of the analyzer diagnostics with which these context messages are associated? After we've settled on a URL for this documentation I'd like to add a link to it from the documentation for each of those diagnostics.

@bwilkerson Ok, I can do that, but it will be an ever-expanding list as I work on the "why not promoted" logic. Is it ok if I wait until after the branch cut so that I can give you a definitive list?

bwilkerson · 2021-03-25T17:17:54Z

The documentation can be updated after the branch point, so yes.

The other option is to tell me where to start looking for uses and I can do the searching myself.

stereotype441 · 2021-03-25T18:09:20Z

The documentation can be updated after the branch point, so yes.

The other option is to tell me where to start looking for uses and I can do the searching myself.

Sure, just look for any uses of ResolverVisitor.computeWhyNotPromotedMessages-- the return value should always be passed fairly promptly to an ErrorReporter method, so it should be fairly easy to see which error codes might be affected.

stereotype441 · 2021-03-25T19:42:03Z

These links are not live yet, but they will be filled in with documentation explaining subtleties of type promotion in more detail. Bug: #44900 Change-Id: I3e1865597fc5c56495a8108c8a4de98cab0c3e4b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193749 Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]>

stereotype441 · 2021-04-01T18:31:10Z

@kwalrath as of c25e748, the analyzer and CFE are now making use of the following links:

So we will definitely need these links to go somewhere useful by the time we ship stable. I'll keep you posted if I implement enough functionality to require any of the other links.

kwalrath · 2021-04-05T17:00:14Z

I'd like to have redirects for those URLs (even if it's just a placeholder page) by the time this change gets to dev channel.

stereotype441 · 2021-04-05T17:04:52Z

I'd like to have redirects for those URLs (even if it's just a placeholder page) by the time this change gets to dev channel.

FYI, the change will land in dev version 2.13.0-192.0.dev.

stereotype441 · 2021-04-14T16:32:05Z

I can get y'all a complete list of the possible type promotion failure reasons in the next day or two.

Could you also get a list of the analyzer diagnostics with which these context messages are associated? After we've settled on a URL for this documentation I'd like to add a link to it from the documentation for each of those diagnostics.

@bwilkerson I believe this is a complete list of the analyzer diagnostics with which the analyzer is capable of associating these context messages, as of 94162a2 (the beta branch for the 2.13 stable release):

CompileTimeErrorCode.ARGUMENT_TYPE_NOT_ASSIGNABLE
CompileTimeErrorCode.CONST_FIELD_INITIALIZER_NOT_ASSIGNABLE
CompileTimeErrorCode.FIELD_INITIALIZER_NOT_ASSIGNABLE
CompileTimeErrorCode.INVALID_ASSIGNMENT
CompileTimeErrorCode.INVALID_USE_OF_NULL_VALUE (note: there's a code path for this but I suspect it doesn't happen in practice yet, since the only non-promotion reasons we currently report on don't apply to cases where the unpromoted type is Null)
CompileTimeErrorCode.UNCHECKED_INVOCATION_OF_NULLABLE_VALUE
CompileTimeErrorCode.UNCHECKED_METHOD_INVOCATION_OF_NULLABLE_VALUE
CompileTimeErrorCode.UNCHECKED_OPERATOR_INVOCATION_OF_NULLABLE_VALUE
CompileTimeErrorCode.UNCHECKED_PROPERTY_ACCESS_OF_NULLABLE_VALUE
CompileTimeErrorCode.UNCHECKED_USE_OF_NULLABLE_VALUE
CompileTimeErrorCode.UNCHECKED_USE_OF_NULLABLE_VALUE_AS_CONDITION
CompileTimeErrorCode.UNCHECKED_USE_OF_NULLABLE_VALUE_AS_ITERATOR
CompileTimeErrorCode.UNCHECKED_USE_OF_NULLABLE_VALUE_IN_SPREAD
CompileTimeErrorCode.UNCHECKED_USE_OF_NULLABLE_VALUE_IN_YIELD_EACH

kwalrath · 2021-05-12T17:46:20Z

@stereotype441 & @bwilkerson, I'm about to start editing the page to add Paul's content.

Please let me know if there's anything I should be aware of before I get too deep into this.

happy-san · 2021-07-30T16:03:41Z

Hey! I have the following use-case:

class C {
  final int? i;            // (1)
  
  C(this.i);
  
  f() {
    if (i != null) {
      i.isEven;            // (2)
    }
  }
}

The instance field i is declared final. So I expected it to be promoted at (2).

From the comment on a similar example,

Note that it's an instance field in this example, but it could also be an instance getter, or a static field or getter, or a top level variable or getter. Also we don't allow promoting this (even though it would be sound to do so; there were some technical reasons that this would have been difficult and we didn't think it would be worth it)

Is this the reason why i is not promoted in this case?

NVM, found #1188

eernstg · 2021-07-30T17:07:05Z

Check out field-promotion.

The core point is that it is unsound to promote a an instance variable because evaluation of an instance variable amounts to execution of a getter (which is generated implicitly for each instance variable), and that getter can be overridden:

class A {
  final int? i;
  void foo() {
    if (i != null) i.isEven; // Not safe!
  }
  A(this.i);
}

class B implements A {
  get i => Random().nextBool() ? 1 : null;
}

stereotype441 added the area-documentation Prefer using 'type-documentation' and a specific area label. label Feb 9, 2021

stereotype441 mentioned this issue Feb 9, 2021

Meta-issue for "why not promoted" functionality #44897

Open

15 tasks

kwalrath mentioned this issue Feb 11, 2021

Add details for non-promotion causes/workarounds dart-lang/site-www#2940

Closed

kwalrath self-assigned this Mar 25, 2021

johnpryan mentioned this issue Apr 8, 2021

Type promotion doesn't work when a closure is used #45622

Closed

ghost mentioned this issue Apr 14, 2021

Type promotion doesn't work when a closure is used dart-lang/language#1582

Open

kwalrath mentioned this issue Apr 14, 2021

Placeholder page & go links for non-promotion docs dart-lang/site-www#3148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Establish documentation links for non-promotion reasons #44900

Establish documentation links for non-promotion reasons #44900

stereotype441 commented Feb 9, 2021 •

edited

Loading

stereotype441 commented Feb 9, 2021

kwalrath commented Feb 9, 2021 •

edited

Loading

bwilkerson commented Feb 9, 2021

kwalrath commented Feb 9, 2021

stereotype441 commented Mar 24, 2021

kwalrath commented Mar 24, 2021

bwilkerson commented Mar 24, 2021

stereotype441 commented Mar 25, 2021 •

edited

Loading

stereotype441 commented Mar 25, 2021

bwilkerson commented Mar 25, 2021

stereotype441 commented Mar 25, 2021

stereotype441 commented Mar 25, 2021 •

edited by kwalrath

Loading

stereotype441 commented Apr 1, 2021

kwalrath commented Apr 5, 2021

stereotype441 commented Apr 5, 2021

stereotype441 commented Apr 14, 2021

kwalrath commented May 12, 2021

happy-san commented Jul 30, 2021 •

edited

Loading

eernstg commented Jul 30, 2021 •

edited

Loading

Establish documentation links for non-promotion reasons #44900

Establish documentation links for non-promotion reasons #44900

Comments

stereotype441 commented Feb 9, 2021 • edited Loading

stereotype441 commented Feb 9, 2021

kwalrath commented Feb 9, 2021 • edited Loading

bwilkerson commented Feb 9, 2021

kwalrath commented Feb 9, 2021

stereotype441 commented Mar 24, 2021

kwalrath commented Mar 24, 2021

bwilkerson commented Mar 24, 2021

stereotype441 commented Mar 25, 2021 • edited Loading

stereotype441 commented Mar 25, 2021

bwilkerson commented Mar 25, 2021

stereotype441 commented Mar 25, 2021

stereotype441 commented Mar 25, 2021 • edited by kwalrath Loading

stereotype441 commented Apr 1, 2021

kwalrath commented Apr 5, 2021

stereotype441 commented Apr 5, 2021

stereotype441 commented Apr 14, 2021

kwalrath commented May 12, 2021

happy-san commented Jul 30, 2021 • edited Loading

eernstg commented Jul 30, 2021 • edited Loading

stereotype441 commented Feb 9, 2021 •

edited

Loading

kwalrath commented Feb 9, 2021 •

edited

Loading

stereotype441 commented Mar 25, 2021 •

edited

Loading

stereotype441 commented Mar 25, 2021 •

edited by kwalrath

Loading

happy-san commented Jul 30, 2021 •

edited

Loading

eernstg commented Jul 30, 2021 •

edited

Loading