dart-lang / language

Design of the Dart language
Other
2.65k stars 202 forks source link

Provide a "forced promotion" operator with runtime checking. #1187

Open leafpetersen opened 4 years ago

leafpetersen commented 4 years ago

Dart has made the choice to only do type promotion when it can be done soundly. That is, in situations where an expression cannot be guaranteed to evaluate to a value of the same type, type promotion does not apply. This is particularly notable with fields. Example:

class A {
  int? x = 3;
  void test() {
     if (x != null) {
        print(x.isEven);  //Error, isEven cannot be called on a nullable value
     }
  }
}

In the above example, the comparison to null does not promote x to the non-nullable type int. To allow this would not be sound, since a subclass of A might override x with a getter which returns null on the second invocation, thereby allowing .isEven to be called on null.

Relevant issues:

This issue proposes a possible syntax for providing a different syntactic form which permits unsound promotion of fields and other expressions, inserting a runtime check at each use. The syntax is inspired by Swift implicit unwrapping.

Syntax

Under this proposal, the syntax e is T! is added to the language, where e is an expression satisfying certain syntactic properties and T is a type. The syntactic ambiguity between parsing this as e is T! and (e is T)! is resolved in favor of the former.

The set of expressions that e may take are described as "extended promotion candidates". The set of extended promotion candidates includes at least:

Static Semantics

If e is an extended promotion candidate, then the expression e is T! shall cause e to be promoted to T unconditionally in the true continuation of any branching construct controlled by the instance test.

A write to e may cause demotion as usual. (TODO: pin down whether this is the right choice).

There is no restriction that T be a subtype of the static type of e. This allows promoting side casts when the user knows more about the type hierarchy than is statically visible. (TODO: is this the right choice?).

Dynamic semantics

A forced promotion expression e is T! shall be evaluated as e is T. Within the scope of a forced promotion e is T!, all references to e shall be evaluated as e as T.

Example

class A {
  int? x = 3;
  void test() {
     if (x is int!) {
        try {
           print(x.isEven);   // Statically valid.
        } catch (e) {
          print("Null");
     }
  }
}

class B extends A {
  bool isNull = false;
  int? get x => (isNull = !isNull) ? 3 : null;
}

void main() {
  A().test(); // prints "true"
  B().test(); // prints "Null"
}

cc @Hixie @munificent @lrhn @eernstg @jakemac53 @natebosch @stereotype441

mraleph commented 4 years ago

@leafpetersen Have you considered instead tweaking the current as T (and other expression and statements) to promote-with-a-check iff that make currently illegal program legal? To expand: normally a program like this would not compile

class X {
  T? f;

  void m() {
    if (f != null) {
      f.smth();
    }
  } 
}

The most common (and simplest) fix would be to sprinkle ! after f occurrences in the then-block of the if.

We however could change semantics here (given that normally it would be an incorrect program) and perform this for users as well:

class X {
  T? f;

  void m() {
    if (f != null) {
      f.smth(); // means f!.smth()
    }
  } 
}

Instead of thinking about f in terms of a single promoted type, I suggest we think about it in terms of active promotions (e.g. in this case f! is an active promotion) and say that if expr(f) is a type error, but expr(promote(f)) is not (for one of the active promotions - checked in reverse nesting order) then expr(f) would mean expr(promote(f)) instead.

There are some catches around it however, e.g. it's a bit unclear to me how we would prefer to handle anything that wants to use type of f (e.g. type inference or extension method resolution). For simplicity we could say that type of f remains unaffected, but extension methods are checked against both promoted and non-promoted versions of f.

lrhn commented 4 years ago

I think I would rather have e as T v to introduce a new variable v with the value of e at type T.

The only way e as T! could meaningfully work is if the user knows that the value won't change. In that case, copying it into a different variable shouldn't change anything. The only thing it can't do is promote on e != null, you'll have to use e is Foo v instead (but so would you for e is Foo!)

We are generally trying to remove implicit type checks, to make it clear where code can fail at run-time.. Introducing an operator which creates multiple points of implicit casts seems like a step back in that regard (no matter how convenient it is when it works).

I have been suggesting the variant @mraleph mentions here before, but while it might make more existing Dart code work without migration, it still does so by adding implicit casts. (Or, we could say that e is Foo promotes e to Foo only if it doesn't change value, so we wouldn't have to do type checks, only identity checks ... but the e as T v would definitely be superior).

eernstg commented 4 years ago

We've had e is T v on the table several times before, and e as T v is an obvious generalization of this useful idea. I agree that this construct could make the need for forced promotions a lot less acute.

We have been moving away from dynamic checking in general, but not without exceptions: The introduction of late variables is basically all about allowing the initialization status of a variable to be checked dynamically, based on insertion of dynamic checks at each usage where the outcome isn't statically known. In that sense it is very similar to the treatment of an expression that has had a forced promotion.

Analogously, you could say that e is T! is marking the expression e as having a "late cast" to T in the true continuation, and similarly for e as T!.

This raises the question about the identity of e. So do we consider x to be the same expression as p.x in the case where both resolve to the same top-level variable in a library that is imported without a prefix and also with prefix p? Do we take if (a == b) .. into account and make that imply that a.x and b.x is the same expression? How about x and this.x when they resolve to the same instance getter? It is certainly a new source of complexity that we may need to sort out exactly which expressions are "the same expression".

e is T v and e as T v would help us avoid this complexity, as well as allowing e to be an arbitrary expression.

It could be a problem that these expressions include a declaration of a local variable, and a local variable declaration is otherwise a statement, not an expression. But the fact that we already allow v is T to change the properties of v in the current scope probably means that we have the machinery we need.

lrhn commented 4 years ago

I do believe we have most of the machinery available. We have stalled on x is T v because it's been swept into pattern matching, but I think it can work by itself:

<typePattern> ::= (`var` | `final` <type>?) <id> | <type> <id>?
<cast> ::= <expression> `as` <typePattern>
<check> ::= <expression> `is` `!`? <typePattern>

If we introduce a new variable, the scope of that variable is exactly the same scope which would be affected by promotion of a variable in the same expression (or up until the end of a block statement, whichever is earlier). It's basically the code dominated by the check/cast succeeding (or failing for is!)

mraleph commented 4 years ago

One of the reasons I suggest looking at enabling this for classical operations (instead of introducing new syntax) is that it will automatically benefit users that already understand how type promotion via casting/comparison with null works and expect it to work for (final) fields as well. Expanding language surface area with new syntax leads to cleaner solution from the language perspective, but its less ergonomic/discoverable/understood from user perspective. New syntax will not stop people filing issues asking why "normal" promotion does not work.

stereotype441 commented 4 years ago

I'm not a fan of e is T! because (a) it's not very discoverable, (b) the ! reads to me like part of the type, not an indication that the user is opting into runtime checks, and (c) it's a clumsy syntax for promoting fields to non-nullable (which I believe is going to be what people want to do with this feature most of the time). I have similar concerns about e is T v--to me it doesn't look like a variable declaration.

I really like @mraleph's proposal from https://github.com/dart-lang/language/issues/1187#issuecomment-682352586 that we allow fields to promote whenever it would be a static type error to do otherwise. It's basically saying "if you want field promotion, you can have it; if you don't, you won't have to pay for it". Which is really compelling IMHO. There's a minor implementation feasibility issue in that we can't answer "would it be a static type error" without backtracking, and the CFE and analyzer resolution logic have not been designed with backtracking in mind. But I think we could substitute a criterion based on the context type and the surrounding syntax that would behave identically for most real world code.

The biggest objection I might anticipate is that we're on a general trend to evolve the language away from constructs that can implicitly raise runtime errors, and this feels like a step in the opposite direction from that. But IMHO the user benefit is great enough that it would be worth it.

mraleph commented 4 years ago

e is T v is similar to Java 12 smart casts which have syntax e instanceof T v. I find the syntax somewhat confusing, but workable.

It's worth mentioning that neither e is T! nor e is T v address @Hixie's pain-point examples though: https://github.com/dart-lang/language/issues/1167#issuecomment-682258023

lrhn commented 4 years ago

It won't work with null comparisons, but it does solve some of the issues if you are willing to write a type:

  // _flutterError is a private final field set in the constructor
  if (_flutterError is! Error error)
    properties.add(StringProperty('message', message, quoted: false));
  else
    properties.add(error.toDiagnosticsNode(style: DiagnosticsTreeStyle.whitespace));

// or, optimistically.

  if (_flutterError as Error? error == null) // Introduce new variable, then promote it. promote error here!
    properties.add(StringProperty('message', message, quoted: false));
  else
    properties.add(error.toDiagnosticsNode(style: DiagnosticsTreeStyle.whitespace));

  // _child is a private field of the class
  if (_child is Widget child)
    visitor(child);
jakemac53 commented 4 years ago

I am not in favor of this because:

The pitfalls and usability around this are bad enough that I think it is highly probable that not only would we end up with a lint banning its usage entirely, but we would also end up enforcing that lint internally and in most of our external repos.

jodinathan commented 4 years ago

I think I would rather have e as T v to introduce a new variable v with the value of e at type T.

The only way e as T! could meaningfully work is if the user knows that the value won't change. In that case, copying it into a different variable shouldn't change anything. The only thing it can't do is promote on e != null, you'll have to use e is Foo v instead (but so would you for e as Foo!)

We are generally trying to remove implicit type checks, to make it clear where code can fail at run-time.. Introducing an operator which creates multiple points of implicit casts seems like a step back in that regard (no matter how convenient it is when it works).

I have been suggesting the variant @mraleph mentions here before, but while it might make more existing Dart code work without migration, it still does so by adding implicit casts. (Or, we could say that e is Foo promotes e to Foo only if it doesn't change value, so we wouldn't have to do type checks, only identity checks ... but the e as T v would definitely be superior).

I see myself having to type lots of if (some.prop is Class) (some.prop as Class).method() for simple cases where I really have no need to declare a local variable, so something like if (some.prop is Class as prop) prop.method() would be so nice.
For null check it could be if (some.prop != null as prop) prop.method()

it would be a syntax sugar for var prop = some.prop; if (prop != null) prop.method()

if (some.prop as prop != null) { prop.method() } if (some.prop var prop != null) { prop.method() } if (some.prop use prop != null) { prop.method() } if (some.prop != null use prop) { prop.method() }

leafpetersen commented 4 years ago

@leafpetersen Have you considered instead tweaking the current as T (and other expression and statements) to promote-with-a-check iff that make currently illegal program legal? To expand: normally a program like this would not compile

@mraleph yes, I did.

I'm quite opposed to it. The most consistent feedback I get from our users is that they don't like unexpected runtime failures. Doing this silently on promotion means that you never know whether your code is statically safe, or whether there are lurking runtime errors that you just haven't hit yet.

leafpetersen commented 4 years ago

The only way e as T! could meaningfully work is if the user knows that the value won't change.

@lrhn No, it only requires that the type of the value not change. Or rather, that it not cease to be a subtype of T.

leafpetersen commented 4 years ago

I think I would rather have e as T v to introduce a new variable v with the value of e at type T.

The only way e as T! could meaningfully work is if the user knows that the value won't change. In that case, copying it into a different variable shouldn't change anything. The only thing it can't do is promote on e != null, you'll have to use e is Foo v instead (but so would you for e as Foo!)

There is still appeal to this approach, but I have become less a fan of it after working with null safe code. We have already seen multiple instances of bugs introduced because code which referenced a field was rewritten to bind the field to a variable, and then either a value was no longer written back to the field correctly, or the field was changed and the fresh value was not re-read from the field. It is deceptively hard to do this refactor correctly. One might argue that it will be easier to write correct fresh code like this, but I'm somewhat skeptical.

leafpetersen commented 4 years ago

I really like @mraleph's proposal from #1187 (comment) that we allow fields to promote whenever it would be a static type error to do otherwise.

I really want to push back on this. See my comment above for my general objection to silently inserting runtime checks, but I want to point out that the "whenever it would be a static type error to do otherwise" bit is terrible from a user experience perspective. It means that adding or removing a method call (for example) can completely change the behavior of arbitrary other pieces of code, including adding other unrelated errors. Small example:

extension on Object {
  bool get objectExtension => true;
}
extension _ on num {
  bool get objectExtension => false;
  bool get numExtension => true;
}
class A {
  Object x = 3;
  void test() {
     if (x is num) {
         x + 1; // error if x doesn't promote, so we promote
         assert(!x.objectExtension);  // assertion succeeds, but fails if we delete the addition above
         var y = x;
         assert(y.numExtension);  // static error if we delete the addition.  Unless we plan to retry everything if there's any static error anywhere?
        var l = [x];
        List<Object> l2  = l;
        l.add("hello");  // Worked before I added x+1, now throws a runtime error.
     }
  }
}

Note two things:

This seems super confusing to me.

leafpetersen commented 4 years ago

It's worth mentioning that neither e is T! nor e is T v address @Hixie's pain-point examples though: #1167 (comment)

@mraleph I don't think that's true, why do you say that? It's true you have to write it as e is Null! or e is! Null! or alternatively e is T! where T is the non-null version of the type of e. Both of these are less intuitive, but unless I'm missing something they do solve the problem.

leafpetersen commented 4 years ago

I am not in favor of this because:

  • It doesn't solve the core issue that we want to resolve around null equality checks

Can you elaborate? I think it does.

  • It introduces a very high cognitive load onto users

Can you elaborate? To me it reduces cognitive load. You can write exactly the code you write now: "Check this field is non-null, then write code assuming that it remains non-null". You just have to opt into the "assume that it remains non-null" part.

  • It only makes sense in the context of fields (or a few other things like getters), thus it isn't any less confusing than the current state (users still have to understand that fields don't promote by default).

I don't know what you mean by "only in the context of fields". In general, we can apply this to any piece of syntax that we want. It's just a matter of deciding which ones make sense. I do think it's less confusing than the current state: it's true that users still have to understand that fields don't promote by default, but now they have a way out of it.

  • It isn't familiar

Maybe. It follows pretty naturally from Swift's implicit unwrapping though.

  • It has non-local affects that aren't intuitive
    • It doesn't encourage the typical optimal resolution to this problem, which is introducing a new local variable

See my comment above about this.

  • It introduces potential performance problems
  • It introduces potential runtime errors via implicit casts, which we are trying to remove generally

These are both my strongest objections to this feature as well.

jakemac53 commented 4 years ago
  • It doesn't solve the core issue that we want to resolve around null equality checks

Can you elaborate? I think it does.

You can't use a null equality check, you have to turn that into a type check. Equality checks are currently the recommended way for people to check for null, and we would now be effectively requiring a different way to do this in certain scenarios to get the desired behavior... it seems wrong to me. Existing code with equality checks would have to me migrated to this new pattern, so it doesn't provide as clean of a migration.

There is even an analyzer hint today telling you to use == null.

  • It introduces a very high cognitive load onto users

Can you elaborate? To me it reduces cognitive load. You can write exactly the code you write now: "Check this field is non-null, then write code assuming that it remains non-null". You just have to opt into the "assume that it remains non-null" part.

Because of the things listed below I think it adds a fair bit of complexity to the language that isn't present in other languages or commonly understood. It is an almost entirely Dart specific concept, thus increasing the cognitive load.

  • It only makes sense in the context of fields (or a few other things like getters), thus it isn't any less confusing than the current state (users still have to understand that fields don't promote by default).

I don't know what you mean by "only in the context of fields". In general, we can apply this to any piece of syntax that we want. It's just a matter of deciding which ones make sense. I do think it's less confusing than the current state: it's true that users still have to understand that fields don't promote by default, but now they have a way out of it.

I mean it is only useful in the context of things like fields which don't promote. There are other scenarios sure, but my assumption is that fields are far and away the most common.

The current pushback/confusion I think comes from the lack of understanding that fields don't promote, or why. I don't think this helps that. It is possible that combined with a set of hints it could, but I doubt most users would understand the consequences of the syntax even if the analyzer told them "hey put a ! here and it will promote like it looks like you want it to".

  • It isn't familiar

Maybe. It follows pretty naturally from Swift's implicit unwrapping though.

I don't think this is the same - I don't see anything about is checks in there. That feels a lot more like a real T! type which is equivalent to T? but with runtime checks on access (similar to late?). I haven't fully dove into it though so I could be wrong.

It feels weird for a simple is to introduce an arbitrary number of additional is checks and/or runtime failures later in the function, I definitely would not expect that at all, and I don't think that the ! syntax is a strong enough indicator of what it is actually doing. For instance the late keyword which is sort of similar is a lot more clear about what is happening, imo.

  • It has non-local affects that aren't intuitive
  • It doesn't encourage the typical optimal resolution to this problem, which is introducing a new local variable

See my comment above about this.

I really don't agree with that argument. People assign complex expressions the result of which might change to local variables all the time. To me this is like saying "never use local variables because the result of the expression you assign to them might change".

Yes you definitely need to understand how/where a field is set before assigning it to a local variable and using that instead, but that is always the case. It is a potential pitfall during migrations, sure, but not I think a common one (we have migrated 20+ packages most of which we were not familiar with at all and never hit this issue afaik).

stereotype441 commented 4 years ago

I really like @mraleph's proposal from #1187 (comment) that we allow fields to promote whenever it would be a static type error to do otherwise.

I really want to push back on this. See my comment above for my general objection to silently inserting runtime checks

Yeah, I understand that customers have been asking us to eliminate the sources of silent runtime checks. But customers have been asking for fields to be promotable too, and I don't see a way to give them both. When we were first talking about flow analysis, I definitely felt like eliminating silent runtime checks was more important. But months of patiently explaining to people why fields don't promote, and watching their faces fall in disappointment even when they understand the reason has made me question that. Actually writing null safe code myself, and trying to migrate existing code to null safety, has pushed me over the line to the point where I would personally rather have promotable fields, and pay the penalty in silent runtime checks. I feel like the message we're getting from our customers who are migrating real-world code these days is that they really want field promotion. Are we sure they wouldn't be willing to accept some silent runtime checks in order to get it?

If some customers feel that the silent runtime checks are so dangerous that they would rather not have field promotion, we can always give them a lint to tell them when field promotion is happening, and they can treat it as an error.

but I want to point out that the "whenever it would be a static type error to do otherwise" bit is terrible from a user experience perspective. It means that adding or removing a method call (for example) can completely change the behavior of arbitrary other pieces of code, including adding other unrelated errors.

I actually interpreted it differently. My thinking was: we make the determination of whether to insert a runtime check (and hence get the promoted type) independently at each use site of the field. So my interpretation of your example is:

extension on Object {
  bool get objectExtension => true;
}
extension _ on num {
  bool get objectExtension => false;
  bool get numExtension => true;
}
class A {
  Object x = 3;
  void test() {
     if (x is num) {
        x + 1; // error if x doesn't promote, so we promote
        assert(!x.objectExtension);  // no promotion because `Object` supports `objectExtension`; therefore this refers to the extension on Object, and the assertion fails regardless of whether `x + 1` is present
        var y = x; // no promotion here; static type of `y` is `Object`; if the user had said `num y = x;` we would have promoted.
        assert(y.numExtension);  // static error; no such method on `Object`
        var l = [x]; // Runtime type of the list is `List<Object>`
        List<Object> l2  = l;
        l.add("hello");  // No runtime error.
     }
  }
}

Note two things:

  • removing an addition changes the resolution of extension methods in a separate part of the program.
  • removing/adding an addition changes the inference behavior in ways that can add/subtract static and dynamic errors

This seems super confusing to me.

Agreed, that would be really bad. I think my interpretation avoids these difficulties.

jodinathan commented 4 years ago

It won't work with null comparisons, but it does solve some of the issues if you are willing to write a type:

  // _flutterError is a private final field set in the constructor
  if (_flutterError is! Error error)
    properties.add(StringProperty('message', message, quoted: false));
  else
    properties.add(error.toDiagnosticsNode(style: DiagnosticsTreeStyle.whitespace));

// or, optimistically.

  if (_flutterError as Error? error == null) // Introduce new variable, then promote it. promote error here!
    properties.add(StringProperty('message', message, quoted: false));
  else
    properties.add(error.toDiagnosticsNode(style: DiagnosticsTreeStyle.whitespace));

  // _child is a private field of the class
  if (_child is Widget child)
    visitor(child);

Why can't we make it work with null comparisons?

Your example works for type checks: if (_flutterError as Error? error == null)

for null checks: if (_flutterError use error != null)

leafpetersen commented 4 years ago

@jakemac53

You can't use a null equality check

Fair.

The current pushback/confusion I think comes from the lack of understanding that fields don't promote, or why. I don't think this helps that. It is possible that combined with a set of hints it could, but I doubt most users would understand the consequences of the syntax even if the analyzer told them "hey put a ! here and it will promote like it looks like you want it to".

Well, fair, but as far as I can tell, there is literally exactly one solution to this which doesn't have this problem, which is just to make fields promote, which we're not doing. So I don't know how to take this as actionable feedback on this proposal. I agree it's confusing that fields don't promote, but they don't. The question is, what now?

I really don't agree with that argument. People assign complex expressions the result of which might change to local variables all the time. To me this is like saying "never use local variables because the result of the expression you assign to them might change".

Yes you definitely need to understand how/where a field is set before assigning it to a local variable and using that instead, but that is always the case. It is a potential pitfall during migrations, sure, but not I think a common one (we have migrated 20+ packages most of which we were not familiar with at all and never hit this issue afaik).

The bug fixed here was introduced in this way.

I'm quite sure that I caught other examples of this bug in reviews, but really can't spend the time to read through all of the comment threads to track them down right now.

This CL as far as I know did not encounter this bug, but it was hard to reason through the correctness of the transformations.

I don't want to over-fixate on this, I'm still open to the "binding a variable" solution, but this is an issue that I had not thought about until I went through the experience of migrating code, and I think it's worth keeping in mind.

leafpetersen commented 4 years ago

I actually interpreted it differently. My thinking was: we make the determination of whether to insert a runtime check (and hence get the promoted type) independently at each use site of the field. So my interpretation of your example is:

@stereotype441 I'm going to need a lot of convincing to believe that that interpretation is less confusing than mine. :)

leafpetersen commented 4 years ago

Why can't we make it work with null comparisons?

Your example works for type checks: if (_flutterError as Error? error == null)

for null checks: if (_flutterError use error != null)

Agreed, I see no reason we can't work out a general syntax here.

natebosch commented 4 years ago

But customers have been asking for fields to be promotable too, and I don't see a way to give them both.

Customers have mostly been asking this of the fields which are obviously, to a human reader, promotable. Things like private fields, or fields on classes that the author wishes could be sealed.

My gut reaction to the proposal is that this is likely going to be something that users don't understand, and that the practice will be "if the analyzer complains, add a ! to force it to promote." without understanding why they are doing that. This may be OK if the majority of the time it matches intent, but if it starts getting parroted in unsafe places the runtime failure will be more painful.

jodinathan commented 4 years ago

some use cases

if (obj.prop use prop != null)

if (obj.prop use prop is T)

if (obj?.prop?.deep?.deeper use deeper != null && await deeper.lazy() == 1) {
    deeper.wakeUp();
}

if (await (obj as A).prop1?.prop2() use atLast != null) {
  // atLast is the result of the awaited prop2()
  atLast.doSomething();
}

// final classes
switch (obj.prop use prop) {
    case SomeClass: 
      prop.method();
      break;
}

// final classes per case
switch (obj.prop) {
    case SomeClass use prop: 
      prop.method();
      break;
}

would also help on some other stuff

  MyClass cl;

  if (someBool && (cl = await someFunc()) != null) {
    // cl is used here
  }

  // cl has no use after the if statement because of someBool

  // then, sometimes you end up doing:

  if (someBool) {
    MyClass cl;

    if (cl = await someFunc()) != null) {
    }
  }

  // an easier to write, read and understand IMO:
  if (someBool && await someFunc() use cl != null) {
  }
rakudrama commented 4 years ago

DBC:

One of the patterns in the discussion above seems to be a variant of asking for let v = e1 in e2, but with an extended scope for v.

A less well known feature of C++ is that it a allows a declaration in a condition. This works well with automatic conversion to bool (e.g. null is 'false')

if (int* p = foo()) {
  *p += 1;
}

Can we do something similar with respect to declarations inside a condition?

if (somebool && let cl = await somefunc() in cl != null) {
  // cl in scope here
}

A surprising aspect of the C++ feature is that p is also in scope in the else-branch, and can be assigned.

jodinathan commented 4 years ago

DBC:

One of the patterns in the discussion above seems to be a variant of asking for let v = e1 in e2, but with an extended scope for v.

A less well known feature of C++ is that it a allows a declaration in a condition. This works well with automatic conversion to bool (e.g. null is 'false')

if (int* p = foo()) {
  *p += 1;
}

Can we do something similar with respect to declarations inside a condition?

if (somebool && let cl = await somefunc() in cl != null) {
  // cl in scope here
}

A surprising aspect of the C++ feature is that p is also in scope in the else-branch, and can be assigned.

Declaration with condition does not solve the type check case, ie if (some.prop is T)

Also it gets messy to ready IMO:

if ((int* p = foo()) != 0) {
  *p += 1;
}

and in dart and async stuff:

if (somebool && (var p = await some?.method()) != null) {
}

so I think we should add a keyword that add a variable to the scope that solves null and type check while explicitly telling the developer why it is needed.

I also dislike using the variable outside that specific scope/condition. If that is the case, then there is a real need for a local variable to be manually declared.

jodinathan commented 4 years ago

we've found another interesting way of using a new keyword while developing AngularDart components.

class Animal {}
class Dog implements Animal {
  String name;
}

@Component(template: '<ng-container *ngIf="isDog">It is a dog named ${dog.name} :)</ng-container>')
class SomeClass {
  Dog dog;
  bool isDog = false;

  void inVet(Animal animal) {
    if (isDog = animal is Dog) {
      dog = animal as Dog;
      await dog.bark(times: 5);
    }
  }
}

// the condition in the method inVet could be used with class setters

void inVet(Animal animal) {
    if (isDog = animal is Dog use this.dog) {
      await dog.bark(times: 5);
    }
  }

// this line can be used outside the if condition:
isDog = animal is Dog use this.dog;
eernstg commented 4 years ago

We could also use e is late T / e is! late T, cf. https://github.com/dart-lang/language/issues/1188#issuecomment-684759057.

a14n commented 4 years ago

During the flutter migration I faced this issue a lot. I wanted something to tell in this block I know this field is of this particular type. Basically I wanted to indicate to the langage that every this.field in a block/method should be treated as this.field!. My first simple idea was to be able to use a syntax like T this.field;.

class A {
  int? x = 3;
  void test() {
     // in this method x should/can never be null
     // this line tell that every x in the block should be treated as x!
     int this.x; 
     x.isEven;
     x.isEven;
     x.isEven;
  }
}

would be the same as:

class A {
  int? x = 3;
  void test() {
     x!.isEven;
     x!.isEven;
     x!.isEven;
  }
}

I also like the use of late to spot user hint to the compiler. So we could use late int this.x;

lrhn commented 4 years ago

@a14n This reminds me of Swift's implicitly unwrapped optionals. A type of int! means int?-with-implicit-!-on-use.

let x : int! = maybeInt();
print(x);

is equivalent to:

let x : int? = maybeInt();
print(x!);

We could introduce !-types (not going to be confusing at all!) and allow (dynamically sound) promotion from T? to T! on any variable, not just local ones. Then:

class A {
  int? x = 3;
  void test() {
    x as int!;
    x.isEven;
    x.isEven;
    x.isEven;
  }
}

could then work.

Not sure it's worth it for this problem (it might be worth it as an independent feature), because I think the problem is better solved by introducing a new variable.

Another option is introducing a local get declaration:

class A {
  int? x = 3;
  void test() {
    int get _x => x!;
    _x.isEven;
    _x.isEven;
    _x.isEven;
  }
}

You can already do the same thing with a local function, it's just prettier without parentheses.

a14n commented 4 years ago

Local getter looks interesting. Would it be possible to use the same name ? int get x => this.x!; ?

lrhn commented 4 years ago

Using the same name with this.x would definitely work. If you want to write int get x => x; (or just get x => x; for inference), then it would probably require us to change the scope for name declarations in general. Currently a name declared in a block is scoped to the entire block. If we could limit the scope to code after the declaration, then you can also write var x = x;.

Not sure "local getters" is really worth the complication it would introduce. It doesn't help API design, it's entirely local to a single function. If anything, it could make it harder to read code when you can't assume that a local x does not have side effects.