-
Notifications
You must be signed in to change notification settings - Fork 213
Provide a "forced promotion" operator with runtime checking. #1187
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
Comments
@leafpetersen Have you considered instead tweaking the current
The most common (and simplest) fix would be to sprinkle We however could change semantics here (given that normally it would be an incorrect program) and perform this for users as well:
Instead of thinking about 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 |
I think I would rather have The only way 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. |
We've had We have been moving away from dynamic checking in general, but not without exceptions: The introduction of Analogously, you could say that This raises the question about the identity of
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 |
I do believe we have most of the machinery available. We have stalled on
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). |
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 |
I'm not a fan of 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. 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. |
It's worth mentioning that neither |
It won't work with // _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); |
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. |
I see myself having to type lots of it would be a syntax sugar for
|
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. |
@lrhn No, it only requires that the type of the value not change. Or rather, that it not cease to be a subtype of |
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. |
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. |
@mraleph I don't think that's true, why do you say that? It's true you have to write it as |
Can you elaborate? I think it does.
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.
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.
Maybe. It follows pretty naturally from Swift's implicit unwrapping though.
See my comment above about this.
These are both my strongest objections to this feature as well. |
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
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.
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
I don't think this is the same - I don't see anything about It feels weird for a simple
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). |
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.
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.
}
}
}
Agreed, that would be really bad. I think my interpretation avoids these difficulties. |
Why can't we make it work with null comparisons? Your example works for type checks: for null checks: |
Fair.
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?
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. |
@stereotype441 I'm going to need a lot of convincing to believe that that interpretation is less confusing than mine. :) |
Agreed, I see no reason we can't work out a general syntax here. |
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 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 |
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) {
} |
DBC: One of the patterns in the discussion above seems to be a variant of asking for A less well known feature of C++ is that it a allows a declaration in a condition. 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 |
Declaration with condition does not solve the type check case, ie 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. |
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;
|
We could also use |
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 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 |
@a14n This reminds me of Swift's implicitly unwrapped optionals. A type of let x : int! = maybeInt();
print(x); is equivalent to: let x : int? = maybeInt();
print(x!); We could introduce 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 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. |
Local getter looks interesting. Would it be possible to use the same name ? |
Using the same name with 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 |
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:
In the above example, the comparison to
null
does not promotex
to the non-nullable typeint
. To allow this would not be sound, since a subclass ofA
might overridex
with a getter which returnsnull
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, wheree
is an expression satisfying certain syntactic properties andT
is a type. The syntactic ambiguity between parsing this ase 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:e0.x
wheree0
is an extended promotion candidatethis
Static Semantics
If
e
is an extended promotion candidate, then the expressione is T!
shall causee
to be promoted toT
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 ofe
. 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 ase is T
. Within the scope of a forced promotione is T!
, all references toe
shall be evaluated ase as T
.Example
cc @Hixie @munificent @lrhn @eernstg @jakemac53 @natebosch @stereotype441
The text was updated successfully, but these errors were encountered: