Privates fields should get promoted

[Hixie]

One can statically prove that this code does not have a null dereference risk:

class A {
  int _foo;

  void test() {
    if (_foo != null)
      print(_foo + 1);
  }
}

As far as I can tell, there is no way, even with implements and noSuchMethod and other shenanigans, for _foo to ever be null on the line with the print.

cc @leafpetersen who pointed out that some similar cases aren't so cut and dry because privates can be implemented using noSuchMethod.

[lrhn]

If there is no class extending A and overriding _foo, and no mixin declaring _foo in the same library, and you know for certain that evaluating print doesn't run code which might modify _foo through a reference to the same instance, then you can probably be sure that _foo doesn't change.

This is a very simplistic case, and very many conditions, which it will be hard for users to grok. That makes the promotion you might get very fragile - one small change, say adding log("testing"); between the test and the use, would invalidate the conditions (because log might have a reference to the object and the ability to call a method which changes the value of _foo - which I assume is possible since you didn't make _foo final).

A promotion this specific is not a good language feature. Small changes can invalidate code and force you to rewrite anyway.

[Hixie]

The call to a function with a reference is an interesting point.

[leafpetersen]

If the field is final, I think it makes it a bit more robust. Note that in case it's not clear, it's important in this example that this is a this call, since otherwise an implementation from another package via noSuchMethod is possible.

[Hixie]

The amount of time I see code like the following while doing the framework migration really makes me wish we could do this even if it only works in narrow cases.

  // _dependencies is a private member of this class
  final bool hadDependencies = (_dependencies != null && _dependencies!.isNotEmpty) || _hadUnsatisfiedDependencies;

  // _dependencies is a private member of this class
  _dependencies ??= HashSet<InheritedElement>();
  _dependencies!.add(ancestor);

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

  // _child is a private field of the class
  if (_child != null)
    visitor(_child!);

  // _inheritedWidgets is a private field
  if (incomingWidgets != null)
    _inheritedWidgets = HashMap<Type, InheritedElement>.from(incomingWidgets);
  else
    _inheritedWidgets = HashMap<Type, InheritedElement>();
  _inheritedWidgets![widget.runtimeType] = this;

[leafpetersen]

See here for a quick proposal on syntax for a forced promotion operator to allow unsound (runtime checked) promotion .

[Hixie]

I don't think we should need new syntax for the cases I list above. They're all entirely safe cases as far as I can tell. I can see an argument that it would lead to questions about "why does it work here but not in this identical-seeming case", but I think we can actually answer that question (maybe even literally in the analyzer, part of the message could be something like "could not guarantee that type didn't change after call to X on line Y column Z" or whatever).

[leafpetersen]

I don't think we should need new syntax for the cases I list above.

Not all of those examples can be safely done, but we could try to identify a set of criteria that are safe in existing dart. It would look something like:

• final, private field
• accessed as an implicit or explicit this call
• some set of restrictions to ensure that mixin shenanigans don't happen (e.g. no other fields with this name in the library)

This would only cover one or two of your cases above though, since several of them are not final.

[Hixie]

I'm curious what shenanigans could invalidate the cases I've described above. Can you elaborate?

[leafpetersen]

I'm curious what shenanigans could invalidate the cases I've described above. Can you elaborate?

I missed listing under the criteria above the obvious one: that the field is not overridden in a subclass. But with mixins, you can essentially induce a non-local override. That is, if I have:

class A {
  final int _privateField = 3;
}

class B {
  int _backingStore = 0;
  int get _privateField => _backingStore++;
}

Then another library can do the following:

class C extends A with B {
}

void main() {
  new C()..test()..test()..test();
}

This prints out:

0
1
2

So to do sound promotion, you need to ensure that mixin overrides can't happen in this way.

[Hixie]

yikes. that seems like a footgun. Can we make it a violation to mix in a type that would have a private field collision without having indicated with "extends" or "on" and @OverRide that it's intentional? This seems like it would cause many more problems (and missed optimisation opportunities) than just this minor issue with field promotion...

[leafpetersen]

yikes. that seems like a footgun. Can we make it a violation to mix in a type that would have a private field collision without having indicated with "extends" or "on" and @OverRide that it's intentional? This seems like it would cause many more problems (and missed optimisation opportunities) than just this minor issue with field promotion...

We attempted to do so as part of Dart 2, but we only half succeeded. The analyzer actually emits an error on the code above, as we had specified for Dart 2, but the CFE doesn't. That does mean that it should be easy to patch this up. I have a vague sense though that there's another tricky way to induce this that we'd also have to patch up, but I can I can't page it in right now.

[leafpetersen]

I filed an issue with a concrete proposal for this here, I'm going to close this out.