Skip to content
Permalink
Branch: master
Commits on Dec 5, 2019
  1. [optional-chaining] fix to parenthesized optional chain assignment

    mvitousek authored and facebook-github-bot committed Dec 5, 2019
    Summary: [this](https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVATApgYxgQwCcswA3IsADzAC4wA7AVxhjAB8wBvMAT1q7AAvfkwC2AIyyEwAX1noAFJQD8AOh4BKVcIC8YACwAmANxA) should error. it doesn't because, in order to allow field assignments and field deletions to use the same code, i was using the `optional_chain` function to evaluate the object being deleted from/written to, and uses the filtered, non-nullable result type. this diff changes that to only be the case when it's a deletion of an optional chain; otherwise we just use the normal `expression`
    
    Reviewed By: panagosg7
    
    Differential Revision: D18803649
    
    fbshipit-source-id: 365a846b2d9ce021e50a14ad70c44de8371b9ad1
Commits on Nov 22, 2019
  1. Fix order of evaluation on jsx children for correct refinement

    mvitousek authored and facebook-github-bot committed Nov 22, 2019
    Summary:
    Flow currently evaluates the children of a jsx expression before it evaluates the expression's attributes. This is contrary to the actual behavior of compiled JSX expressions at runtime, and leads to refinements getting invalidated in the wrong order.
    
    Compare the behavior of flow https://fburl.com/grv8wua8 with the output produced by  babel https://fburl.com/95kvrez6
    
    This diff moves the call to `collapse_children`, which typechecks jsx children to after the checking of the attributes.
    
    Reviewed By: gabelevi
    
    Differential Revision: D18642975
    
    fbshipit-source-id: 965e9a81f6e80084af9a20cdb27842a890c4edb8
Commits on Nov 21, 2019
  1. [optional-chaining] Optional chains used for typeof, instanceof, and …

    mvitousek authored and facebook-github-bot committed Nov 21, 2019
    …Array.isArray refinements
    
    Summary:
    Similar to D18458524, which added support for refinements in literal comparisons with optional chains, this diff adds support for type comparisons with optional chains. For the most part this is a simple extension of the existing work in D18458524, in order to generate refinements for cases like this:
    ```
    class C { ... }
    declare var x: ?{
      arr: Array<number>,
      obj: {...},
      c: C,
      maybe_void: number | void
    };
    if (Array.isArray(x?.arr)) { ... }
    if (typeof x?.obj === "object") { ... }
    if (x?.c instanceof C) { ... }
    ```
    
    In all of these conditionals, we can be certain that if the condition returns true, then `x` is not null or undefined, because short-circuiting always produces `undefined`, and
    1. `Array.isArray(undefined) === false`
    2. `typeof undefined === "undefined"`
    3. For all X, `undefined instanceof X === false`.
    
    Case 2 above means that we need to flip the predicates generated from the optional chain when looking at a program like:
    ```
    if (typeof obj?.maybe_void === "undefined") { ... }
    ```
    In that case, we don't know whether the conditional succeeding means that `obj` is null or undefined or if `obj.maybe_void` is void--it could be either. But in the else case of the if, we know that `obj` is not null or undefined and that `obj.maybe_void` is a number.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18496275
    
    fbshipit-source-id: 8eb2a23fe57838ee670e2894234e75ee0dbc8cf2
  2. [optional-chaining] Evaluate subexpressions of optional chains in ref…

    mvitousek authored and facebook-github-bot committed Nov 21, 2019
    …ined environments
    
    Summary:
    In the current design for optional chains, as we walk up the chain we generate predicates that will hold if the chain does not short-circuit (as well as ones that will hold if it does). We use these in conditionals, but they can actually be useful when typechecking the chain itself as well. Consider:
    ```
    declare var obj: ?{val: number, fun: (number) => string};
    ```
    What can we do if we want to call `obj.fun`, if `obj` exists, on `obj.val`? Currently we'd write:
    ```
    if (obj) {
      obj.fun(obj.val);
    }
    ```
    That's three whole lines! Unacceptable.
    We might be tempted to write
    ```
    obj?.fun(obj?.val);
    ```
    but `obj?.val` has type `number | void`, and `obj.fun`, if it exists, only takes `number`. However, we can realize that if `obj` is null or void, then we actually wouldn't get to the point of calling `obj.fun` on anything! We would short-circuit and no function would be called in the first place. Therefore, we can actually just write
    ```
    obj?.fun(obj.val) // no optional chaining on the argument!
    ```
    If we're evaluating the argument, then we haven't short-circuited, and so we can refine `obj` to being not null or void before we evaluate the argument subexpression.
    
    This diff adds this feature, which turns out to be pretty simple--we just refine the environment of subexpressions (arguments to calls, or indexes to indexed member accesses) with the non-short-circuiting branch of the the conditional before evaluating them. We already have these refinements, if any, because we've been building them up as we go, and there shouldn't be much overhead because we only have refinements to apply when there's an optional chain operator lower on the chain.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18483827
    
    fbshipit-source-id: 9d3f07f1d3f350b479f63948ca27a3b745218d0d
  3. Better error messages for refining read-only properties

    mvitousek authored and facebook-github-bot committed Nov 21, 2019
    Summary: We generate some really bad error messages when we try to refine an object as having a property with `PropExistsP` when the property is read-only. See https://flow.org/try/#0CYUwxgNghgTiAEA3W8AqARASgLngbwFowBXGOAOwBdc8B7AM3oGcRKB1AS2EoAtdziAWwBGIGAF9xAbgBQHevAAUGTADoSZEFQCU+eOJlA for an example (the first error). With this diff, we thread a reason through with the `PropExistsP` refinement so we have something better to point at.
    
    Reviewed By: dsainati1
    
    Differential Revision: D18495253
    
    fbshipit-source-id: edb8de49287e8ff8b979605924329800f79d2494
  4. [optional-chaining] Optional chains used for null, void, and literal …

    mvitousek authored and facebook-github-bot committed Nov 21, 2019
    …refinements
    
    Summary:
    Similar to D18458524, this diff adds the ability to refine optional chain expressions by comparing to literals. Taking a program like this:
    ```
    declare var obj: ?{ a: ?{b: number, c?: number} };
    if (obj?.a?.b === 42) {
      ...
    }
    ```
    We know that when an optional chain short-circuits it produces `undefined`, so if the equality check passes, then the chains must not have short circuited. This means that in addition to the refinement `obj.a.b is equal to 42` that we would normally have added, we can also add the refinements `obj is not null or void` and `obj.a is not null or void` to our heap refinements. This diff adds the ability to create these sorts of refinements by hooking the `condition_of_maybe_sentinel` function, used in generating refinements on comparison operations, into the `optional_chain` function, which produces predicates based on whether optional chains short-circuit or not.
    
    One tricky thing, which I try to discuss in the comments, comes up when we compare optional chains to null or void. If we have an expression like this:
    ```if (obj?.a?.c === undefined) { ... }
    ```
    then, unlike the `=== 42` case above, we'll hit the truthy case of the `if` when the chain does short circuit, since short-circuiting produces undefined. However, in the else case of the if, we can refine `obj` and `obj.a.` to not be null or void--we flip the truthy and falsy branches of the predicates that we built when we traversed the optional chain. The same is true for an expression like
    ```if (obj?.a?.c == null) { ... } // non-strict equality
    ```
    since `undefined == null`, but it is *not* true for
    ```if (obj?.a?.c === null) { ... } // strict equality
    ```
    since optional chain short-circuiting always produces `undefined` (even if the short-circuiting was triggered by `null`) which is not strictly equal to undefined.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18458524
    
    fbshipit-source-id: 0d88e93544859d4adffe3325d8a2e56d175102fa
  5. [optional-chaining] Optional chains generate refinements

    mvitousek authored and facebook-github-bot committed Nov 21, 2019
    Summary:
    This diff allows refinements to be generated from testing the truthyness of optional chains.
    
    In a program like this one,
    
    ```
    declare var a: ?{b?: {c?: {data: number}};
    if (a?.b?.c) {
      ...
    }
    ```
    we can consider what conditions have to be true for the expression `a?.b?.c` to be truthy. Short-circuiting of optional chains always results in `undefined`, which is falsy, so this expression must not have short circuited. This, in turn, means that `a` is not nullish, `a.b` is not nullish, and `a.b.c` is truthy. Therefore, this program is equivalent to an expanded program like
    ```
    declare var a: ?{b?: {c?: {data: number}};
    if (a != null && a.b != null && a.b.c) {
      ...
    }
    ```
    This diff implements refinement generation for optional chains like the one above, allowing users to use optional chains in conditionals and get useful refinements. The main addition is to the `optional_chain` function, which traverses chains of property accesses, and which now also (if the `predicates` parameter is true) builds refinement predicates as it goes through the chain.
    
    When the `optional_chain` function encounters an optional chain operator `?.` in the context of a property read or indexed read, the expression to the left of the operator (the object being read from) is refined to be not null or void in the truthy branch, and if that expression is itself is a member access, its object is refined to have the property being read. In other words,
    ```
    if (a.b?.c) {...}
    ```
    generates the refinements `a.b not null or void` and `a has property b`, in addition to the refinements `a.b.c is truthy` and `a.b has property c` (which is what would normally/previously be generated by `if (a.b.c) {...}`).
    
    This diff also makes some minor modifications to the predicate/guard system in general, mainly by introducing `PropNonMaybeP`, which is a predicate similar to the existing `PropExistsP`, but where `PropExistsP(prop)` checks that an object has a truthy property `prop`, `PropNonMaybeP(prop)` checks that the object has a non-nullish property `prop`.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18355800
    
    fbshipit-source-id: 3ab94e6e7013f70b797e9b9c37970f77d4ebbf4d
Commits on Nov 14, 2019
  1. Remove sketchy-null check on property existence predicates

    mvitousek authored and facebook-github-bot committed Nov 14, 2019
    Summary:
    We generate sketchy-null errors when we're checking the truthyness of a type that can be either a falsy non-null/void value and null/void. Currently, however, we also check for sketchy-null errors when checking whether a type has a given property, using the `PropExistsP(name)` predicate, which holds on some type `t` when `t` has a property `name`.
    
    This check signals a sketchy-null when the target type `t` may be either null or some falsy value, which isn't super useful since it only happens during a lookup on that type--it can never be the "top level" of a conditional, which is where the conflation of falsey values and undefined is most dangerous.
    
    It looks like the *intent* behind this might have been to check whether the *property type* is null-or-maybe-falsey, but this is always checked anyways since in the one place where `PropExistsP` predicates are generated, an `ExistsP` pred on the overall expression is also generated. This redundancy can be observed in the (very small) warning diff--for each of the removed warnings, there is another warning that is not removed which reports the same thing, but with better positioning.
    
    Reviewed By: dsainati1
    
    Differential Revision: D18462349
    
    fbshipit-source-id: 22143d2bfc2c63b5aeefedf7421e93ff290c23b7
  2. [optional-chaining] Make optional chains sensitive to refinement

    mvitousek authored and facebook-github-bot committed Nov 14, 2019
    Summary:
    This diff makes optional chain property lookups correctly have refined heap types: it makes programs like the following well-typed, up to unnecessary optional chain lints:
    ```
    declare var a: ?{b: number};
    if (a && a.b) {
      (a?.b: number);
    }
    ```
    Before this diff, optional chains did not pick up refinements, and in the above program, we would get an error because `a?.b` could be `undefined`. This diff does *not* cause optional chains to generate refinements--as before, we're not able to use `if (a?.b) { ... }` to refine anything (that's a later diff on the stack).
    
    One important bit added in this diff is the `~allow_optional` parameter to `Refinement.key`, which specifies whether optional members can have refinement keys (normally, they cannot). If it's set to true, which it is when we're looking up refinements in the environment, `a?.b` generates the same refinement key as `a.b`, and so we find refinements on `a.b`. It's set to false when we're *setting* refinements in the heap, in which case `a?.b` is treated as non-refinable--this way, we don't accidentally refine `a.b` to be (for example) equal to undefined just because `a?.b` is. As refinements for optional chaining continues to be built, though, we may be able to remove this flag, because we'll actually generate sensible refinements from optional chains.
    
    Reviewed By: dsainati1
    
    Differential Revision: D18288817
    
    fbshipit-source-id: b48288aad110bc0964b9422a438f3a0e4b0a07e8
Commits on Nov 8, 2019
  1. [optional-chaining] Remove OptionalChainVoid and modify reasons for b…

    mvitousek authored and facebook-github-bot committed Nov 8, 2019
    …etter error messages
    
    Summary:
    The `OptionalChainVoid` internal type is no longer needed -- we're not passing "imaginary" voids up through optional chains any more after D18211760.
    
    This diff removes that type, and it also adds a little bit of improved reason handling. According to the spec (https://github.com/tc39/proposal-optional-chaining#base-case), both `null` and `undefined` are converted to `undefined` in optional chains. This can lead to bad error messages like this: https://flow.org/try/#0MYewdgzgLgBAhgLnmAnjAPjMBXANrmAXhgEYBmMgdgG4AoUSWAIyWgCcBLMAcwyzwLE4AfgB0cakA
    
    The error in the program is actually correct, because in the `null` case for `a`, `a?.a` has type `undefined`-- but it has a reason description of `null`, pointing at the original `null` annotation. This diff adds a new reason desc, `RVoidedNull`, for these `VoidT`s that originate from `null`s short-circuiting optional chains; hopefully the result will be clearer error messages. The textual description of `RVoidedNull` is "undefined (result of null short-circuiting an optional chain)"
    
    Reviewed By: jbrown215
    
    Differential Revision: D18340917
    
    fbshipit-source-id: 460757017d99ede21cf6be175169a94431552b80
  2. [optional-chaining] Optional chaining deletion

    mvitousek authored and facebook-github-bot committed Nov 8, 2019
    Summary:
    The spec for optional chaining specifies that optional chains can be targets of deletion (https://github.com/tc39/proposal-optional-chaining#optional-deletion). This diff adds support for that.
    
    This necessitates exposing the `recur_chain` implementation function that was originally scoped inside of `subscript`, with the rationale that in an expression like `delete a?.b.c`, we need the "non-nullable" output of `a?.b` which is what `recur_chain` provides, since if `a` was nullable we wouldn't get to the deletion in the first place. Likewise, if the deletion is an expression like `delete a?.b`, we pass `a` through an `OptionalChainT` that filters out the nullable components of its type before performing the deletion.
    
    One tricky bit arises due to refinements: normally, an expression like `delete a.a` adds to the refinement environment that `a.a` has type undefined, similar to if we wrote `a.a = undefined`. On the other hand, `delete a?.a` should not add that to the environment, so it's not just a matter of treating `delete a?.a` as identical to `delete a.a` up to optional chaining. Instead, this diff adds some additional bookkeeping to prevent this refinement.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18337198
    
    fbshipit-source-id: f9a6751e07b77817a0713156583ab3d68238db04
Commits on Nov 5, 2019
  1. [optional-chaining] Optional chaining for methods

    mvitousek authored and facebook-github-bot committed Nov 5, 2019
    Summary:
    This diff builds support for optional method chains.
    
    The tricky bit for optional method chains, beyond what is needed for optional chains in general, is that method calls combine both property lookup and function calling into a single flow: `X.method(args)` will generate a flow `X ~> MethodT(method, args)`, which will look up `method` in `X` and then flow the result to `CallT(args)`. With optional chaining, *either* the lookup or the call can be optional, or both. If the call is optional (`X.method?.(args)`), we need to intercede in the logic of `X ~> MethodT` in order to filter out the nullish case for the method type before it is flowed to a `CallT`.
    
    The approach this diff takes is to add a `ChainM` method action, which represents optional chain filtering of nullish types before they're called. When a type flows into a `MethodT` with a `ChainM` method action, the method (which may be nullish) is looked up, and then flowed into an `OptionalChainT` representing the optional chaining of the call. The non-nullish components of the method type are then flowed into a `CallT` and the process proceeds as usual.
    
    One thing that complicates this story is that `funcalltype`s contain a "this" type, which in method calls is the method's receiver (`X` in `X.method()`). Normally, this can just be generated in statement.ml by getting the type of the target of the method lookup. However, what if this is an optional access, e.g. `X?.method()`? If we naively got the type of `X` here, the "this" type of the `funcalltype` would be `?X` (assuming that `X` is an instance of class `X`). But in the `FunT ~> CallT` flow that eventually occurs, the "this" type of the `funcalltype` will flow into the "this" type of the actual function, which in the case of this method would be `X`. This would result in a type error, and it also doesn't really make sense--in order for us to have even gotten to the function call in the first place, the receiver would have to not be nullish, becuase the optional chain would have already short-circuited if it was null.
    
    Instead, this diff delays the binding of the receiver to the `funcalltype`'s "this" type until after the nullish types have been filtered out of it. It does so by adding a little additional bit of logic to optional chaining, by (when necessary) including a tvar in the `OptionalChainT` that takes non-nullish LBs of the optional chain as its LBs--in other words, when nullish types have been filtered away, the rest of the types flow into this tvar in addition to flowing into a use_t as described in the previous diff in this stack. This tvar is used as the "this" type for the `funcalltype`.
    
    Most of the work of setting this up happens in statement.ml, and it is a little complicated, needing to look two levels deep into the chain and considering both whether the method lookup and the method call is optional, as well as handling refinements and test hooks etc. I've tried to document it, but I'm happy to write up as much as is needed in any part that is hard to comprehend.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18302022
    
    fbshipit-source-id: 5d8abe863beb98268b725d3ea924fdbf00c28c06
  2. [optional-chaining] Refactor optional chaining helper functions

    mvitousek authored and facebook-github-bot committed Nov 5, 2019
    Summary: This is a simple refactoring of the helper functions used by optional chaining. Some behavior has been factored out into separate functions, and will (in upcoming diffs) be called separately. We also thread through a little more info, which will be used for method chains.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18302023
    
    fbshipit-source-id: 08ba769e518999c0f399f259d40b488649743768
  3. [optional-chaining] Create `method_action` type to prepare for option…

    mvitousek authored and facebook-github-bot committed Nov 5, 2019
    …al chain method calls
    
    Summary: To support optionally-chained method calls, we will need to handle the case where we look up a method on a receiver and find that it is optional, then "filter out" the nullish components of the type before passing the non-null function to a `CallT`. This work is built in later diffs, but this diff modifies `MethodT` (and relatedly, `CallElemT`) so that instead of containing a `funcalltype` representing the call that the looked-up method will flow to, it contains a new ocaml type called `method_action`. Method actions are currently just `CallM`, which represents the current, non-optional behavior of methods where the looked-up method is directly called. `CallM`s are converted to `CallT`s after methods are looked up. In later dfiffs we will add `ChainM`s, which are converted to `OptionalChainT`s and support optional chain method calls, but this diff just sets up for that.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18302024
    
    fbshipit-source-id: 12101d9e5adeff5773c2015210d23bf122af085e
Commits on Nov 4, 2019
  1. [optional-chaining] Simpler (and more amenable to method calls) appro…

    mvitousek authored and facebook-github-bot committed Nov 4, 2019
    …ach to optional chaining
    
    Summary:
    This diff changes the implementation of optional chaining to one that is (I hope) easier to understand, doesn't use mutable references, and which helps make implementing optional chained method calls much more straightforwards (the next diff on the stack).
    
    Before, an OptionalChainT represented *everything* in an optional chain, and contained a list of opt_use_ts (a use_t with the output tvar factored out). When something non-nullish flowed to an OptionalChainT, it would kick off an imperative loop of flowing the LHS sequentially to the opt_use_ts. Building these chains is a little hacky and relies on using references to change reasons "out from under" given points in the chain. This works, but it is a little hard to understand and fragile.
    
    This diff changes the behavior of OptionalChainT to represent just one "link" in the chain. When a type flows into an OptionalChainT, if it's nullish, it flows to a tvar in the OptionalChainT which represents short-circuiting, and if it's not nullish, it flows directly to a use_t; in other words, an OptionalChainT just filters nullish LBs into a special accumulator tvar, and passes other types through.
    
    We build OptionalChainTs in statement.ml by recursively descending through chains, e.g. `a?.b?.c`. At each level, we produce an output that contains "non-nullish success" and "failure/short-circuit" types. The "success" type is passed upwards through the chain and used as the receiver for further operators, while the "short-circuit" type (which includes the accumulator tvars of any OptionalChainTs in the chain as lower bounds) is, if necessary, set as a lower bound to the overall type of the expression.
    
    So, if we have
    ```
    declare var a: ?{b?: {c: number};
    var d = a?.b?.c
    ```
    we'll generate the flows
    ```
    ?{b?: {c: number}} ~>
      OptionalChainT (success=GetPropT("b", tout=TVAR1), short-circuit=TVAR2));
    TVAR1 ~>
      OptionalChainT (success=GetPropT("c", tout=TVAR3), short-circuit=TVAR4));
    TVAR2 ~> TVAR_final
    TVAR4 ~> TVAR_final
    TVAR3 ~> TVAR_final
    ```
    The first flow above in turn kicks off:
    ```
    void ~> TVAR2
    {b?: {c: number}} ~> GetPropT("b", tout=TVAR1)
    ^ not optional any more!
    ?{c: number} ~> TVAR1
    ```
    and similar for the other OptionalChainT flow.
    
    The result is that the type of `d` is `TVAR_final`, with lower bounds `number`, `void` (from `a` being optional), and `void` (from `b` being optional).
    
    This diff *shouldn't* cause any difference in behavior, it's just cleanup and set-up for methods. However, there is a small error diff. I can't repro why the errors weren't caught with the old approach, but they're legit errors.
    
    Reviewed By: panagosg7
    
    Differential Revision: D18211760
    
    fbshipit-source-id: fc263de127f88aa917046b446057b956a7ac73f8
Commits on Oct 30, 2019
  1. [parser][optional-chaining] Sequences of calls can be optional chains

    mvitousek authored and facebook-github-bot committed Oct 30, 2019
    Summary: Chains of function calls aren't correctly tracking optionality: currently, `f?.()()` is equivalent to `(f?.())()`, where the outer call is a `Call` and not an `OptionalCall`, which means that optional chain calls can't be short-circuited. This diff fixes that by setting `in_optional_chain` to true when encountering an initial optional call. Closes [#8159](#8159).
    
    Reviewed By: gabelevi
    
    Differential Revision: D18189600
    
    fbshipit-source-id: 9184d041513022e4176b4f7ba7e5252dfb845183
Commits on Oct 16, 2019
  1. Check that `module` is undefined before special-cases of `module.export`

    mvitousek authored and facebook-github-bot committed Oct 16, 2019
    Summary: We special case module.exports in a couple places in statement.ml, which is incorrect if a variable named `module` has been declared in scope. This change guards the special-casing of module.exports to ensure that it hasn't been shadowed by a variable.
    
    Reviewed By: samwgoldman
    
    Differential Revision: D17933077
    
    fbshipit-source-id: f44240c15a06aa080f21091f8ce66c7bdaa6f969
Commits on Sep 24, 2019
  1. [autocomplete] Trigger autocomplete with a space in JSX

    mvitousek authored and facebook-github-bot committed Sep 24, 2019
    Summary:
    >JSX autocomplete rates are very low. Theory: because there is no trigger character (`.`), the client isn't asking for completions automatically, so the requests we see are the user explicitly hitting ctrl+space.
    
    >Is it possible to trigger autocomplete with a space when inside JSX?
    
    This diff:
    
    * Makes " " (space) a trigger character for the textDocument/completion LSP method, so the client sends an completion request when " " is typed
    * When receiving a completion LSP request, track whether the request has the `Invoked` kind or the `TriggerCharacter` kind. I *think* the request will be `Invoked` when it is triggered by ctrl+space, or when identifier characters (like [a-zA-Z0-9]) are being typed, while it's `TriggerCharacter` when one of the triggers ("." and now " ") is typed.
    * When setting autocomplete hooks, only allow the "autocomplete_id" hook to be triggered when the request was `Invoked`, not when it was caused by `TriggerCharacter`. This maintains the current behavior where typing a space in a "normal" block of code doesn't trigger autocomplete.
    * Don't have that condition for the "autocomplete_jsx" hook: it will now fire on spaces inside JSX blocks. (also no need for this condition on "autocomplete_member" since that only fires after "." anyways).
    
    We now also log whether an autocomplete request was generated by `Invoke` or `TriggerCharacter` and which trigger character it was. This requires modifying the Hack LSP interface to meet the LSP spec, which passes the trigger character in a request's `CompletionContext`: https://microsoft.github.io/language-server-protocol/specification#textDocument_completion
    
    Reviewed By: gabelevi
    
    Differential Revision: D17240763
    
    fbshipit-source-id: 433ce56b17f987dae226166f2a1d28bf00a8ef14
  2. Better handling of Object.defineProperty and friends

    mvitousek authored and facebook-github-bot committed Sep 24, 2019
    Summary:
    Currently Flow special-cases `Object.defineProperty`, `Object.defineProperties`, and `Object.create` in ways that don't correspond with their specifications or with their libdef definitions in `core.js`. Each of these methods take `PropertyDescriptors`, which are defined in spec as
    ```
    type PropertyDescriptor<T> = {
        enumerable?: boolean,
        configurable?: boolean,
        writable?: boolean,
        value?: T,
        get?: () => T,
        set?: (value: T) => void,
        ...
    };
    ```
    But the special-cased implementation of these methods requires that they have a `value` property, whose type is used to initialize the property on the Flow object type that is being created or modified. The rest of the properties are ignored--which means that the `get` property can return something of an entirely different type.
    
    This diff alters that behavior to correspond to the spec by actually looking up the definition of `PropertyDescriptor` and flowing the arguments of these `Object` methods into that definition. The methods are still special cased, but instead of requiring a `value` property contrary to spec, the type for the object property is determined by creating a tvar `X` and then flowing the property descriptor argument into `PropertyDescriptor<X>`. `X` then becomes the type on the ObjT for the property. This corresponds to spec and improves soundness, because now if the type of `value` and the type of `get` disagree, an error will be raised.
    
    There is also some additional work to correspond to spec by allowing `Object.defineProperty` to take a type argument, which will be used instead of the tvar above. Finally, `Object.freeze` is also changed to allow a type argument, as allowed in the spec.
    
    Reviewed By: panagosg7
    
    Differential Revision: D17081593
    
    fbshipit-source-id: d7b04582a0ebc74204381caf7d23c30c569169a2
Commits on Sep 23, 2019
  1. Implement checking and refinements for delete

    mvitousek authored and facebook-github-bot committed Sep 23, 2019
    Summary:
    Implements logic for member and variable deletion.
    The basic goal of this diff is to prevent this kind of unsoundness:
    ```
    declare var x: {foo: number}
    delete x.foo;
    (x.foo: number) // Unsound!
    ```
    This diff accomplishes that by raising an error on `delete x.foo`; on the other hand, if `x` was defined as `{foo?: number}`, the deletion would be ok.
    
    For the most part, deleting a member or element can be modeled just as flowing `VoidT` to that member or element, and that is all that is needed for cases like the above example. Unfortunately, it's more complicated for objects with indexers:
    ```
    declare var x: {[string]: number}
    delete x.foo // arguably, should be fine
    ```
    Objects with indexers are already unsound with respect to undefined, so it would be inconsistent to error when we delete something from them. To allow for this behavior (and analogous behavior for arrays) to be accepted, we unfortunately need to thread an extra bit of information through `SetPropT` and `SetElemT`, which specifies whether the type represents a regular member/element assignment or a deletion. (We could create new types for `DeletePropT` etc, and it could be done if that's preferred, but the behavior of deletion and assignment is *almost* the same, so I went with using the same types but adding an extra parameter.) We also thread some information back from property lookups, to represent whether the lookup succeeded via an indexer or otherwise, and when a deletion hits an indexer prop, we don't flow `VoidT` to the prop like we otherwise would.
    
    Since we're mostly just reusing the member assignment machinery, refinements work out of the box, and we error when deleting from read-only props, `$ReadOnlyArray`s, etc.
    
    Reviewed By: panagosg7
    
    Differential Revision: D16867974
    
    fbshipit-source-id: 9d504f5b2bc8e42e6cf871fedfc1fdf943623aa7
Commits on Sep 18, 2019
  1. v0.108.0

    mvitousek authored and facebook-github-bot committed Sep 18, 2019
    Reviewed By: gabelevi
    
    Differential Revision: D17464010
    
    fbshipit-source-id: 9223ad980b5611826f3cc89703da1fdf80f3f23d
Commits on Sep 16, 2019
  1. Fix for comparison of number-bounded generics

    mvitousek authored and facebook-github-bot committed Sep 16, 2019
    Summary:
    `flow_comparison` chokes on `EmptyT`, which means that generics with `number` upper bounds can't be compared. Fix is to allow `flow_comparison` to accept `EmptyT`.
    
    Fixes #8017
    
    Reviewed By: mroch
    
    Differential Revision: D17289380
    
    fbshipit-source-id: c2d87d740f4e9a5f670d69b68ea01da1574c2b78
Commits on Sep 4, 2019
  1. [autocomplete] Fix crash on types with default arguments

    mvitousek authored and facebook-github-bot committed Sep 4, 2019
    Summary:
    Attempting to autocomplete a property of a type with default type arguments results in the autocomplete command crashing:
    
    given the file `poly.js`:
    ```
    //flow
    
    type T<S=number> = {x: S};
    declare var a: T<>;
    a.
    ```
    flow crashes as below:
    
    ```
    [.../flow/tests/autocomplete]$ flow autocomplete 5 3 < poly.js
    Error: (Failure "cannot instantiate non-class type PolyT")
    Raised at file "stdlib.ml", line 33, characters 17-33
    Called from file ".../flow/src/typing/members.ml", line 373, characters 19-42
    ...
    ```
    This occurred because the type parameter instantiation algorithm used by `members.ml` ignores default type arguments.
    
    This diff replaces the algorithm with a simplified version of the algorithm used for "actual" type instantiation in `flow_js.ml`, which handles default arguments correctly.
    
    Reviewed By: mroch
    
    Differential Revision: D17175083
    
    fbshipit-source-id: d4435a4f540ee7480ca5aacfeaeb16d7bb11af79
  2. [autocomplete] Evaluate EvalT in members.ml

    mvitousek authored and facebook-github-bot committed Sep 4, 2019
    Summary:
    This diff allows `members.ml` to evaluate `EvalT` types when looking for members, and therefore will allow values with that type to have their members autocompleted. This *hopefully* will fix the bulk of autocomplete failures*
    
    It seems like the expectation was that `EvalT`s would have been evaluated before reaching `members.ml`, but that assumption can be false if the evaluation of the type is suspended by a type alias. For example:
    ```
    type t = $ReadOnly<{a: number}>;
    declare var x: t;
    x.
    //^^^ Autocomplete fails
    ```
    
    Here a raw, unevaluated `EvalT` is reached by members.ml, and before this diff it wouldn't be able to suggest anything for `x.?`. (Note that it *would* have worked if `x` was directly declared to have type `ReadOnly<...>` rather than going through `type t`, because the annotation would have been evaluated to `ObjT` before it reached members.ml.)
    
    This diff just allows `EvalT` types to be evaluated using the same functions that flow_js.ml uses to evaluate them (this necessitates making one helper function visible outside of flow_js).
    
    Reviewed By: mroch
    
    Differential Revision: D17172551
    
    fbshipit-source-id: 8a5bf9385d48c553073e11a785695f816a8abb40
  3. [autocomplete] Improve behavior of extract_type by removing implicit …

    mvitousek authored and facebook-github-bot committed Sep 4, 2019
    …preconditions
    
    Summary:
    The `extract_type` function in members.ml, which is used in autocomplete, has several unenforced preconditions on its input: it assumes that, if necessary, `resolve_type` and `resolve_builtin_class` have already been called on its argument if necessary, and if that is not the case, it returns `FailureUnhandledType`, which leads to an autocomplete failure with an unexpected type. This precondition did not always hold: some recursive calls of `extract_type` neglected to call these functions on their arguments. For example, autocomplete would fail on values of type `?Array<number>`, because `Array<number>` isn't resolved before its type is extracted.
    
    This diff removes those preconditions by having `extract_type` call `resolve_type` when necessary, and folding the behavior of `resolve_builtin_class` into `extract_type` entirely (because that function is not used anywhere else). This fixes a source of unexpected type errors* during autocomplete.
    
    This diff also adds a new kind of failure that can be generated by `extract`. Currently, there's no way to tell whether a `FailureUnhandledType` originates in `extract_type` (if the type doesn't have a corresponding object-like type containing its members) or if in `extract_members` (if the provided object-like type doesn't actually contain members). This diff adds `FailureUnhandledMembers` for the latter case, which should help debug future autocomplete failures.
    
    Reviewed By: mroch
    
    Differential Revision: D17141837
    
    fbshipit-source-id: 1817f881bbb09863d8f749aab8f7174f31c1fac4
Commits on Sep 3, 2019
  1. [autocomplete][easy] Minor change to allow autocompletion of OptionalT

    mvitousek authored and facebook-github-bot committed Sep 3, 2019
    Summary: Lets autocomplete treat `OptionalT` the same as `MaybeT`
    
    Reviewed By: gabelevi
    
    Differential Revision: D17141838
    
    fbshipit-source-id: 834907eaeafeeb458f7a9ca51311ba17aaa58a7c
Commits on Jul 12, 2019
  1. [types-first] Add a context flag for types-first phase

    mvitousek authored and facebook-github-bot committed Jul 12, 2019
    Summary:
    Currently there's no way to know during typechecking whether the AST being checked is a full file or a types-first signature. This diff adds a `phase` flag to the context that records that information. When types-first is enabled, the `Merging` phase indicates that the context is being used to check a signature AST, while the `Checking` phase indicates that a whole file is being checked.
    
    When the classic architecture is being used, the phase is always `Checking`.
    
    Reviewed By: panagosg7
    
    Differential Revision: D16174152
    
    fbshipit-source-id: 1c8dcdd94b6004d82330965d720baaf01b92f315
Commits on Jun 11, 2019
  1. [trusted-types] Enable trust inference on all type annotations

    mvitousek authored and facebook-github-bot committed Jun 11, 2019
    Summary:
    This diff enables trust inference by making all type annotations (and targets of casts) trust-inferred. Most of the action is in the `convert` function of `type_annotation`, where all types are now given trust inference variables that are initialized to unknown.
    
    Several new test cases are included, and a lot of old test cases now get different results. This is because prior to this change, a type annotation like `var x: number` would treat `number` as untrusted, and if it flowed into a trusted type--`var y: $Trusted<number>`--an error would be raised. Now, this is totally fine unless `number` has had an explicitly tainted type or `any` flow into it.
    
    Reviewed By: dsainati1
    
    Differential Revision: D14669361
    
    fbshipit-source-id: 0d221e1d9856e755bde4a171ddc9042ecb7422d8
Commits on Jun 9, 2019
  1. [trusted-types] Merge_js handles trust variables

    mvitousek authored and facebook-github-bot committed Jun 8, 2019
    Summary:
    This diff adds logic to `merge_js` to handle merging trust variables. It follows basically the same logic used to merge tvars: First, it resolves the trust var, which is easier than it is for tvars, since unresolved trust variables maintain their current trust status, and then it adds the var to a reduced trust graph and generates a stable ID.
    
    Resolving a trust var just takes the current trust status and "fixes" it by substituting away any unknown trust or privacy information. Currently this just converts "unknown" trust to taintedness and "unknown" privacy to public; this is not what we ultimately want (since it means that anything exported from a module that is not explicitly marked as `$Trusted` becomes tainted) but getting intermodular trust working correctly will take some care, and this approach is conservative.
    
    Reviewed By: dsainati1
    
    Differential Revision: D14669362
    
    fbshipit-source-id: 8fe773a11b0dd6bdb5bf194268171079a67419c1
Commits on Jun 8, 2019
  1. [trusted-types] Core implementation of trust inference

    mvitousek authored and facebook-github-bot committed Jun 8, 2019
    Summary:
    This diff provides the core implementation of trust inference. It's still unused, because trust variables are never created, but it contains the logic for propagating trust across the trust graph. The main work in this diff is in the `trust_checking` module, which now handles both fixed trust and trust variables. I've tried to add detailed documentation about the algorithms used, also discussed briefly here.
    
    As implemented in D14610542, trust variables point to nodes in the trust graph which contain a mutable trust data representing the currently known trust of the variable, and a set of upper and lower bound variables.
    
    * When a type with fixed trust (e.g. `any`) flows into a type with inferred trust, the trust of the lower bound is propagated on the trust graph to the trust variable of the upper bound, and from there to its upper bound types.
    * Vice versa for when an inferred trust type flows into a fixed trust type.
    * When a type with inferred trust flows into another type with inferred trust, we flow the trust variable of the lower bound to the trust variable of the upper bound, e.g. `X ~> Y`. When this occurs, we add `Y ∪ upper_bound_variables(Y)` to `X`'s upper bound variables and to each of `X`'s lower bound variables's upper bound variables. This maintains the invariant that the upper bounds of a trust variable are closed: if `X ~> Y ~> Z`, then `Z` is in `X`'s upper bound variables. We also flow the current trust of the lower bound into the upper bound and vice versa.
    
    Reviewed By: dsainati1
    
    Differential Revision: D14614055
    
    fbshipit-source-id: 738106177fc01340b49b8d8d3f8b25c51ab330df
Commits on Jun 7, 2019
  1. [trusted-types] Add infrastructure to the context to support trust in…

    mvitousek authored and facebook-github-bot committed Jun 7, 2019
    …ference
    
    Summary: Adds a `trust_graph` field to the context to track constraints on trust variables. Also creates a definition for constraints on trust variables, based on `constraint.ml` for constraints on tvars, but simpler: instead of the bounds of a variable containing upper- and lower-bound types (in addition to upper and lower variables), the bounds of a trust variable just contains its current `trust` value.
    
    Reviewed By: dsainati1
    
    Differential Revision: D14610542
    
    fbshipit-source-id: cd9f15a8179baff49b3f6e2fdfab4fa89e479ce6
Commits on May 24, 2019
  1. [refactor] Make TypeAppT a type, not a def_type

    mvitousek authored and facebook-github-bot committed May 24, 2019
    Summary: It doesn't make sense for a type app to have a trust value, and type apps don't obviously correspond to values in the way that most other `def_t`s do.
    
    Reviewed By: dsainati1
    
    Differential Revision: D15484245
    
    fbshipit-source-id: b9249c03c9742c9611d6c81b3fb253d5ba175413
Commits on May 23, 2019
  1. [refactor] Move verbose printing to debug_js to make usable without d…

    mvitousek authored and facebook-github-bot committed May 23, 2019
    …epending on flow-js
    
    Summary: Refactor `print_if_verbose` and similar out of `flow_js.ml`. These functions don't inherently belong in that module and having them there makes it a pain to use them from other modules, which would be nice to prevent `flow_js` from getting even larger. I've put them in a module within `debug_js`; they depend on `debug_js` and have a kinda similar purpose, and they're in a module (rather than the `debug_js` toplevel) so that they can be `open`ed by `flow_js` without having to import all the definitions in `debug_js`.
    
    Reviewed By: dsainati1
    
    Differential Revision: D14610540
    
    fbshipit-source-id: c725f12fdb57e281de33e47a968f6e0c8b633ad9
Commits on May 22, 2019
  1. [trusted-types][refactor] Move trust checking operations into separat…

    mvitousek authored and facebook-github-bot committed May 22, 2019
    …e module
    
    Summary: Future diffs are going to add a lot of additional machinery to trust checking in order to enable trust inference. This diff moves the `trust_flow` functions (the main entry points for trust checking) into a separate module in order to make this work cleaner. Since `flow_js` provides functions used by `trust_flow`, we make this module a functor to allow mutual recursion with flowjs (similar to `assert_ground`)
    
    Reviewed By: dsainati1
    
    Differential Revision: D14610541
    
    fbshipit-source-id: cba64e3bde2f73653c075347b9df67736398ff81
  2. [trusted-types] Trust data in types can be either a trust status or a…

    mvitousek authored and facebook-github-bot committed May 22, 2019
    …n index for inference, with compact representation
    
    Summary:
    Currently, all `DefT`s contain trust information, but we assume that the trust of a type is fully known. This isn't where we want to end up, though: we'd love to actually *infer* whether a given type is trusted or untrusted, unless the user specifies that it must be trusted.
    
    To do this inference, we need to be able to track interactions between types. For example, if `any` flows into `number`, then that `number` needs to be marked as untrusted (aka tainted). We'll accomplish this by reusing some of the ideas behind Flow's tvar inference, but in order to do so, the type whose trust is being inferred (in this case `number`) needs to have an index into a graph of trust inference variables, just like tvars contain an index into the context graph.
    
    This diff alters the information contained within `DefT`s to be either a representation of trust (in the cases where the trust of a type is known or user-specified) or an index into a trust graph (currently unimplemented). The `trust.ml` module now provides two primary types: the `trust` type, as before, represents fully-known trust information, and the `trustdata` type contains either a `trust` value or an index into the graph. The `trustdata` type is now what appears in `DefT`s, allowing them to be either fixed trust or inferred trust.
    
    The specifics of `trustdata` is complicated due to performance issues. What we'd really like is for the `trustdata` type to look like
    ```
    type trustdata =
    | Inferred of ident
    | Fixed of trust
    ```
    In other words, it should be either an ident or a trust value, and (importantly) client modules should be able to tell which is which--this should be an externally visible distinction. Unfortunately, since all `DefT`s contain a `trustdata` value, this results in a huge memory hit. As a result, we're now using a much more compact representation: a value of type `trustdata` is an int with the following layout:
    
    ```
    trustdata:
    [ tag | data ...... ]
      ^     ^
    1 bit   62 bits
    ```
    If `tag` is 0, then `data` should be interpreted as trust information. Trust information is now itself represented as an int, with values 0 to 3 representing different trust info (trusted public, untrusted private, etc).
    
    If `tag` is 1, then `data` should be interpreted as an identifier, which (in future diffs) will be a pointer into a graph for trust inference.
    
    This is all internal implementation details within `trust.ml`, of course. External modules use an API to manipulate `trustdata` values. The `expand` function takes a `trustdata` value and expands it to our ideal representation of `trustdata` as above, a datatype of either `Inferred` or `Trusted`. There are also predicates `is_infer` and `is_trust`, and a `trustdata` value can be directly converted to a `trust` value with `as_trust` and likewise for `as_infer` for inference identifiers (these functions assert false if the `trustdata` value is not actually a `trust` value or inference ident, respectively).
    
    Reviewed By: dsainati1
    
    Differential Revision: D14605940
    
    fbshipit-source-id: ad078f9bf2a029fe6d370a43918eb92bcfc5165a
Older
You can’t perform that action at this time.