Post NNBD setter/getter consistency requirements

[leafpetersen]

With --no-implicit-casts turned on, the analyzer currently makes the following an error:

class A {
  void set f(int x) {}
  Object get f => 3;
}

I believe this is an unintentional result of making assignable asymmetric. The specification says that:

It is a static warning if a class has
a setter named \code{$v$=} with argument type $T$ and
a getter named $v$ with return type $S$,
and $S$ may not be assigned to $T$.

Turning on --no-implicit-casts essentially re-interprets "$S$ may not be assigned to $T$" to be "$S$ is not a subtype of $T$".

Do we want to treat this as a bug and fix it, or is this the right behavior for the post NNBD world.

In it's defense, this "bug" is enforcing the very sane property that a.f = a.f should not be a type error.

cc @nshahan @munificent @lrhn @eernstg

[eernstg]

Note the overlap between this issue and dart-lang/sdk#36126, dart-lang/sdk#36127, where we decided to clarify our general approach to warnings and then request implementations of the resulting rules.

I think it would make sense to stick to that plan in the following sense:

First we clarify (here) what the relevant relation should be. To me it seems like 'assignable' would make sense, such that we preserve the requirement that a.f = a.f must not be an error, but we might also come up with some other relation.

In any case, we will then have an answer for this issue when we decide whether "getter return type is not <someRelation> to corresponding setter argument type" is a warning or an error (thus unblocking dart-lang/sdk#36126, dart-lang/sdk#36127 as well).

[lrhn]

I think the current getter/setter-with-different-types situation is a little too inconsistent in every case.

If we remove implicit downcasts in NNBD by redefining "assignable to", then we either need to change the use of "assignable to" here to include "or vice versa", or change the language. This is not a cast unless you actually do a.f = a.f, and nobody ever promised you that that would work anyway.

If we change the language, it is a breaking change, whether we require the setter type to be a subtype of the getter type or a supertype. There might be code that does the other thing.
So, if we change the language anyway, how about just requiring the type to be the same?

(I really want to get to a place where getters/setters are not individual members, but linked accessors of a single "property" which has just one type. Even if we don't introduce a combined syntax, everything should still act as if a property is one single thing).

The analyzer warning is just a lint, it can do whatever it wants, but it should probably not let the "is assignable to" relation be affected by "no implicit downcasts" in places where there is no actual cast.

[eernstg]

but it should probably not let the "is assignable to" relation
be affected by "no implicit downcasts" in places where there
is no actual cast.

OK, that makes sense. Here is a concrete example where it is a meaningful design choice that the setter has stronger requirements than the getter:

abstract class Foo { /*Useful interface stuff*/ }
class UninitializedFoo implements Foo { /*Dummy implementations*/ }
class RealFoo implements Foo { /*This implementation will do real work*/ }
// Maybe also `class RealFoo2/3/4/... implements RealFoo`, doing real work.

Foo _fooImpl = UninitializedFoo();
Foo get foo => _fooImpl;
set foo(RealFoo newFoo) => _fooImpl = newFoo;

The point is that we have a property foo, and we want to ensure that it is only ever updated with "real" foo objects, but we want to start out with a dummy implementation (and it shouldn't be null). So readers can only assume that it's a Foo, but writers must ensure that they provide a RealFoo.

I think this serves to illustrate that it can be a meaningful design choice to make the setter more constrained than the getter. Do we wish to preclude such designs?

[lrhn]

Do we wish to preclude such designs?

I do, but that might just be me.

It means you can't do foo = foo; which is potentially surprising (but who'd actually do that?)
I would expect a similarly compelling situation where the setter accepts more things than the getter can return, and it does some automatic conversion internally before storing the value. Which suggests that there isn't one direction which is more natural to allow than the other, and if we allow a discrepancy between the getter type and the setter type, it should allow variation on both directions.

I personally wouldn't use a setter at all for a design like that, but probably have a more specialized function like void updateFoo(RealFoo foo) { _fooImpl = foo; } to change the value received from foo.

I really want to get to the point where we can have syntax like:

Foo foo { 
  get => _fooImpl;
  set(v) { _fooImpl = v; }
}

and not have any reason to use the old syntax.
Then we can treat "properties" as single entities rather than distinct setter/getters. We have already moved towards that by having lookups of foo/foo= stop at the nearest enclosing declaration of foo or foo=.

[munificent]

I really want to get to the point where we can have syntax like:

Foo foo { 
  get => _fooImpl;
  set(v) { _fooImpl = v; }
}

and not have any reason to use the old syntax.

YES PLEASE. I have a sketch of a proposal for something very similar to that sitting on my laptop, waiting for more time to work on it.

[leafpetersen]

We propose to leave this unchanged. TODO: specify that the relationship must be subtyping (that is, we do not special case dynamic as we do in assignability)

[leafpetersen]

This has been specified.

[leafpetersen]

Re-opening this based on feedback from the migration. From @a14n in the Flutter porting work:

I wonder if the rule to have the same type for getter and setter shouldn't be special cased for nullability. It's common to see things like that:

TextSelection? get textSelection => _textSelection;
  TextSelection? _textSelection;
  set textSelection(TextSelection? value) {
    assert(value != null);
    _textSelection = value;
    _hasBeenAnnotated = true;
  }

This code is currently rejected, and is not an unreasonable pattern. Should we re-consider relaxing the assignability requirements, either entirely, or to account for nullability?

cc @Hixie

[a14n]

After checking it doesn't seem to happen too frequently.

[Hixie]

Probably worth fixing the API for the cases where it does happen, rather than changing the language. It's very weird to have a getter be able to return a value that its equivalent setter can't set.

[a14n]

I don't know if it's a bug but with the following code I only see analyzer error for B (The return type of getter 's' is 'String?' which isn't a subtype of the type 'String' of its setter 's') and not for C

class A {
  String get s => '';
  set s(String value) {}
}
// nullable getter and non-nullable setter
class B {
  String? get s => '';
  set s(String value) {}
}
// non-nullable getter and nullable setter
class C {
  String get s => '';
  set s(String? value) {}
}

[eernstg]

That's correct, the rule is basically that s = s should not be an error, which is achieved by requiring that the getter return type is a subtype of the setter parameter type, and that rule is respected in A and C.

By the way, the textSelection example isn't rejected as stated, but this variant is similar to B, and that might be the situation that we're actually discussing:

bool _hasBeenAnnotated = false;
TextSelection? _textSelection;

TextSelection? get textSelection => _textSelection;

set textSelection(TextSelection value) {
  _textSelection = value;
  _hasBeenAnnotated = true;
}

[Hixie]

FWIW, we wouldn't to see C in Flutter code either. The domain of a setter should match the range of its getter.

[leafpetersen]

We discussed this last week in the language meeting, and the general consensus was that we prefer to keep the restriction. Thanks for the input, I'll close this out again.