[go: nahoru, domu]

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

improve type promotion in strong mode #25565

Open
7 of 8 tasks
jmesserly opened this issue Jan 22, 2016 · 13 comments
Open
7 of 8 tasks

improve type promotion in strong mode #25565

jmesserly opened this issue Jan 22, 2016 · 13 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). core-l type-enhancement A request for a change that isn't a bug

Comments

@jmesserly
Copy link
jmesserly commented Jan 22, 2016

[Edit Sep 17 2018, eernstg: This seems to be the most generic issue on the topic of improving promotion, so I'd like to position it explicitly as "the place to go" for this discussion; I've added a comment listing other issues related to this topic here. I'll adjust the labels accordingly.]


There are some cases where type promotion won't work. Found while investigating the state of dart-archive/dev_compiler#31.

It roughly falls into a few buckets:

  • if the original type of T of v was dynamic, the test v is SomeType won't promote. This leads to some suboptimal codegen in SDK (see strong mode should allow type promotion from dynamic #25486)
  • if the tested type S is not a subtype of T, the test won't promote. You'd have to track a union type.
  • the type promotion is only valid in the body/else clause of that if statement. There's no analysis of the rest of the method, meaning patterns like if (v is! S) return; don't affect the type of v in the rest of the method body.
  • from Sky, we've heard they would like to do assert(v is S); and have v be treated as S for the rest of the method. Sort of like an unchecked (in production mode) cast, similar to S v2 = v but not requiring a second variable name to be introduced.
  • from package:yaml, assignment to the variable in a closure will disable all promotion, this is true even if that closure hasn't been defined yet (and therefore, cannot possibly mutate the variable).
  • Constrained generic parameter like T extends Event, not promoting to a subtype of Event like InputEvent, see Stack trace from editor evaluating code in CompileTimeConst visitor #327
  • If the variable is mutated, it won't promote. This is the case even if the mutation doesn't affect the type.
  • If the type test wants to allow null, like if (v == null || v is T), it should promote to nullable-T
@jmesserly jmesserly added type-enhancement area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Jan 22, 2016
@jmesserly
Copy link
Author

Moved from dart-archive/dev_compiler#274.

@nex3
Copy link
Member
nex3 commented Jan 26, 2016

I'd like to propose that v == null promote V. I don't know the type theory well enough to say what— maybe dynamic?—but this would support the following pattern:

void fn(T v) {
  // ...
}

void fnTakesObject(Object v) {
  if (v != null && v is! T) return;
  fn(v);
}

This comes up when implementing collections, since they have a number of methods that take Objects but need more specialized behavior for values of the collection's type.

@jmesserly
Copy link
Author

ah, good example. So right now that won't promote because of flow analysis (issue 3 above) but it'd be equivalent to this:

void fnTakesObject(Object v) {
  if (v == null || v is T) {
    fn(v);
  }
}

Doing || generally would take union types, but we could definitely special case null, since that works for any T.

@jmesserly jmesserly added P3 A lower priority bug or feature request and removed Priority-Medium labels Feb 10, 2016
@munificent munificent mentioned this issue Feb 18, 2016
35 tasks
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
@jmesserly jmesserly added P2 A bug or feature request we're likely to work on and removed P3 A lower priority bug or feature request labels Jun 6, 2016
@srawlins
Copy link
Member

To add to the list (unless this is being split into smaller issues?):

  • Promote in ternary: x is SomeType ? x.someField : 'something' should have an understanding that x has a someField field after the ?.

@jmesserly
Copy link
Author
jmesserly commented Sep 20, 2016

@srawlins ternary operators already work:

class C {
  var greeting = 'hello';
}
main() {
  Object c = new C();
  print(c is C ? c.greeting : 'world');
}
$ dartanalyzer --strong test.dart
Analyzing [test.dart]...
No issues found

What you are likely seeing is one of the other problems above. With more info I can help you figure it out.

@matanlurey
Copy link
Contributor

Is this still happening? Will the analyzer and front_end both do the right thing?

@jmesserly
Copy link
Author

Maybe @leafpetersen knows where the language is heading on this?

@leafpetersen
Copy link
Member

@bwilkerson Had a very nice proposal that covered a fair bit of this. It didn't make the cut for Dart 2 just because of time, but I hope we can find a good way to roll it out post 2.

@natebosch
Copy link
Member

if the tested type S is not a subtype of T, the test won't promote. You'd have to track a union type.

I think this is #33932

Some of the others are covered by #29624

@eernstg
Copy link
Member
eernstg commented Sep 17, 2018

It looks like this is the canonical request for improved promotion, which is a topic that has been discussed for a long time (no proposal made it into Dart 2, but surely we'll get there at some point).

I think it would be nice to use this issue as a focal point for that proposal. So here's a list of issues on this topic which contains the issues that I know about (you're welcome to extend the list if you know about a relevant issue that I overlooked):

Moreover, here's a PR that includes an overview of the ideas on the table at the given time:

@eernstg eernstg added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). and removed analyzer-strong-mode labels Sep 17, 2018
@eernstg eernstg removed area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on labels Sep 17, 2018
@eernstg
Copy link
Member
eernstg commented Sep 17, 2018

@jmesserly, I've added a comment at the top of the original text of this issue and adjusted the labels to make this issue "the place to go" for discussions on promotion. I believe it does not fit so well as an 'area-analyzer' issue because the language design needs to be settled first, so I made it an 'area-language' issue; similarly, I removed 'analyzer-strong-mode' because it's now a Dart 2 effort (or 2.x, for some finite x), and the priority (because that priority was presumably concerned with the analyzer implementation).

@jmesserly
Copy link
Author

@eernstg -- that sounds perfect. Yeah feel free to edit my old description to whatever is most is helpful. It's great to have a canonical issue for type promotion work!

@eernstg
Copy link
Member
eernstg commented Jan 27, 2023

Putting check marks on a couple of cases in the initial comment:

In current Dart, it is possible to promote a local variable whose type is dynamic.

  • if the tested type S is not a subtype of T, the test won't promote. You'd have to track a union type.

Also true today.

  • the type promotion is only valid in the body/else clause of that if statement. There's no analysis of the rest of the method, meaning patterns like if (v is! S) return; don't affect the type of v in the rest of the method body.

Today we have a rather complex flow analysis, and there are many ways in which the type of v can be affected by the promotion machinery outside the true-continuation of that test.

  • from Sky, we've heard they would like to do assert(v is S); and have v be treated as S for the rest of the method. Sort of like an unchecked (in production mode) cast, similar to S v2 = v but not requiring a second variable name to be introduced.

We don't do that (because production code cannot be affected by assert statements since they aren't executed), but it can be done using v as S. Flow analysis will then know that this type cast must have succeeded at every location in the code which is reached after having executed the type cast (modulo try/cast and other complex stuff -- but it works! ;-).

  • from package:yaml, assignment to the variable in a closure will disable all promotion, this is true even if that closure hasn't been defined yet (and therefore, cannot possibly mutate the variable).

Kept this one active, the flow analysis basically doesn't try to keep track of which locations in the code are such that the function object definitely hasn't been invoked. So we're still disabling promotion a bit more than we'd like.

In current Dart we do this: There is a kind of type which is known as an intersection type (it's not a general intersection type, just a special case, and only a compile-time phenomenon). This kind of type supports a few special cases, presumably including the case about Event and InputEvent which is mentioned here. So I added the check mark here, too.

  • If the variable is mutated, it won't promote. This is the case even if the mutation doesn't affect the type.

Today there are lots of mutations of local variables which are compatible with promotion. We also have a notion of 'demotion' in the case where a promoted variable v gets assigned the value of an expression whose static type isn't assignable to the promoted type of v, but it is assignable to the declared type of v.

  • If the type test wants to allow null, like if (v == null || v is T), it should promote to nullable-T

I put a check mark here, too, because we can use if (v is T?) and get that promotion.

This seems to indicate that only the part about function objects with potential side effects on local variables isn't yet covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). core-l type-enhancement A request for a change that isn't a bug
Development

No branches or pull requests

9 participants