Detect single-assignment bindings

[samwgoldman]

Flow should be able to determine that x is never re-assigned. I would be interested to discuss how we might accomplish this in this issue.

var x: ?number = 1;

if (x) {
  setTimeout(() => {
    (x : number) // we can't know
  })
}

Possible solution: on each init_env, store a boolean mutated property with the new scope entry initially set to false. When an assignment happens, flip mutated to true. Then, whenever we need to havoc the env, we could only do so for scope entries where mutated is true.

Is that sound?

[bhosmer]

Sadly it's not quite that simple... but it's an interesting situation:

1. Just to restate the background, the reason why we clear refinements going into a closure is that the closure can run anywhere, not just within the scope of the refinement test.
2. In your example, that's not a problem because the variable is never reassigned, making it a de facto const. So the if the refinement test holds at all, it holds everywhere, not just inside the if.
3. But the annotation muddies the water: it deliberately widens the type of the initial value, and we want to respect annotations, except within the scope of a refinement test. If we used item 2 to propagate the refinement everywhere, we'd effectively be rendering the annotation meaningless.
4. Note that if we take the annotation out, there's no need for the refinement.

So I guess the direct answer is that it's easy enough to detect that x is never re-assigned - in fact, an implementation of const would have to do exactly that. And if it were useful, you could do it in a preliminary pass, yielding (not mutated but) const flag which would allow you to make stronger assertions about the variable's type state as you went through the usual set/get/refine/havoc process. But the use-case as given is problematic-slash-unnecessary per the above, I think.

[samwgoldman]

Thanks for putting it so clearly! I think my problem was that I started without specific problem code in mind, but just a principle. When I started playing around with more realistic code, I noticed that it's actually quite difficult to confuse flow w.r.t. refinements and non-constant bindings. Good!

I will note that it's slightly surprising that the type checker would treat the following code snippet differently from the one above (I understand why it does this):

/* @flow */

var x: ?number = 1;

if (x) {
  var y = x;
  setTimeout(() => {
    (y : number) // OK
  })
}

That aside, I still think there could be some use for tracking constant bindings. A realistic example comes from a change I tried to make a while ago in #318.

var s = "hello";
var f = () => React.createClass({ propTypes: { s: React.PropTypes.oneOf([s]) } });
s = "oops";
f() // runtime check and static type disagree

If we knew that the s binding were constant, we would be able to translate that into a proptype instead of defaulting to any, as we do now. This is a very specific case, because even a constant binding to an array wouldn't be good enough—we could only accept an array literal of constant bindings.

So, not a great example again, but is it enough to show the usefulness of tracking constant bindings? You mentioned that an implementation of const would have to do that, which mine (#431) does do, but it just errors on a rebinding and I don't try to track bindings which are not declared as const. I would be happy to roll a more general version into that PR.

[bhosmer]

Aha - the example actually reinforces the point, though, check it out: y is assigned only once, from a (refined to) number rvalue, and there's no widening annotation. So it's guaranteed to always be a number, and thus safe. (We're not doing anything special to recognize it as a single assignment, it's just that the complete set of assignments yields a number type.)

So again, it's the annotation that starts the dominos falling in the previous example: it means that the refinement is required to narrow x back down to number, but that refinement is busted by the fact that the closure can run anywhere.

In the #318 example, I'm not entirely sure what's going on, but it looks to me like it's again a victim of closures running anywhere: we definitely can't be sure whether s is "hello" or "oops" when it runs.

Unless I'm missing something, for me the real takeaway here is the need for const - even if we were to track de facto const vars, the problem with exploiting them is the lack of developer intent (consider e.g. what happens when somebody reassigns what is by all appearances an ordinary var, maybe in a place that's distant from its definition, and starts getting type errors on all the stuff that was using the thing as if it were a bona fide const).

I haven't been following #431 (which, my bad) but I'll take a look now.

[samwgoldman]

So my take-away here is: yes, we can track de facto const vars, but it's
not clear that we should. Your scenario where a code change causes type
errors from an inferred de-facto const becoming non-const does seem
problematic. After considering this, I do agree that, unless I can come up
with more compelling use cases for inferring const, it's probably not worth
trading potentially confusing type errors caused by seemingly unrelated
code changes.

I opened this issue after being convinced by a tweet from @wycats. Yehuda,
any interest in chiming in here?

I'll think a bit more on use cases for inferred const and close this if I
can't find some that would justify the cost of possibly confusing people.

On Tue, Jun 2, 2015 at 7:27 AM, Basil Hosmer notifications@github.com
wrote:

Aha - the example actually reinforces the point, though, check it out: y
is assigned only once, from a (refined to) number rvalue, and there's
no widening annotation. So it's guaranteed to always be a number, and thus
safe. (We're not doing anything special to recognize it as a single
assignment, it's just that the complete set of assignments yields a number
type.)

So again, it's the annotation that starts the dominos falling in the
previous example: it means that the refinement is required to narrow x back
down to number, but that refinement is busted by the fact that the closure
can run anywhere.

In the #318 #318 example, I'm not
entirely sure what's going on, but it looks to me like it's again a victim
of closures running anywhere: we definitely can't be sure whether s is
"hello" or "oops" when it runs.

Unless I'm missing something, for me the real takeaway here is the need
for const - even if we were to track de facto const vars, the problem with
exploiting them is the lack of developer intent (consider e.g. what happens
when somebody reassigns what is by all appearances an ordinary var, maybe
in a place that's distant from its definition, and starts getting type
errors on all the stuff that was using the thing as if it were a bona fide
const).

I haven't been following #431 #431
(which, my bad) but I'll take a look now.

—
Reply to this email directly or view it on GitHub
#482 (comment).

[bhosmer]

Yeah. Further, though, and sorry to be tiresome: the issue with the first example is specifically that it's problematic to override an annotated type with a narrower inferred type - even though we can see that the latter holds throughout the variable's lifetime. Without the annotation, one naturally gets the benefit of assignment tracking - per your second example. :)

You can think of it as a precedence ordering, strongest first:

1. explicit refinement test
2. type annotation
3. union of assigned types

The first example has an error because it can't rely on (1), due to closures running anywhere, but would instead need for (3) to be higher precedence than (2). Make sense?

[wycats]

What would the benefits of Flow detecting de-facto const bindings be?

[samwgoldman]

OK, so I've been thinking on this a bit, and I did come up with what I believe is a compelling use case where the benefit outweighs the cost¹.

Flow doesn't currently support computed object keys. We could support them today for string literals (e.g., { ["foo"]: "foo" }), but that isn't very interesting/useful. For many string-typed variables, we will have literal information that we could potentially use to construct an object type with a known key (e.g., { [foo]: "foo" } where foo is some string variable), except that the variable might change if the object is constructed in a closure.

This is a very similar use case for React components in #318, but a lot more applicable to every day code.

Now, we could leverage const here, once that lands, and safely use const-annotated variables as computed object keys. However, one of the goals for flow is to understand JavaScript as it is written, and I posit that enough JavaScript will be written with computed object keys for de-facto-const-but-not-const-annotated vars to make this logic worthwhile. This is obviously speculation, so it's up for debate.

1. The cost here being a scenario where a developer makes a change and sees a resulting type error that is "far away" from the change.

[TrySound]

@samwgoldman Looks like it works now. Can you confirm?
https://flow.org/try/#0G4QwTgBAHgXBD8A7ArgWwEYFNIF4IEYBuAKGIEsAzCACigEoIBvYiCAZ0wBcAVM1TAPbJO1agxwA+Ji1Y0oEOCgzYGAelUQA7pggBjEIgDknCAGtEAzTIC+dYtaA

[SamChou19815]

We have the ability to detect effectively-const variables for a while.