Skip to content
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

Use SingletonStrT in case of const literal declarations #4175

Closed
wants to merge 2 commits into from
Closed

Use SingletonStrT in case of const literal declarations #4175

wants to merge 2 commits into from

Conversation

agentcooper
Copy link
Contributor

@agentcooper agentcooper commented Jun 14, 2017

// @flow

const x = 'X';
const y: typeof x = 'Y';

Before: no errors.

After:

Cannot assign 'Y' to y because string [1] is incompatible with typeof x [2].

        1│ // @flow
        2│
        3│ const x = 'X';
 [2][1] 4│ const y: typeof x = 'Y';
        5│

Fixes #2639.

This is my first attempt to dive into Flow codebase, not sure if the implementation is sane. I would like to get some comments on this. Will add tests if everything is ok.

@agentcooper
Copy link
Contributor Author

@samwgoldman @mroch @gabelevi any comments?

@samwgoldman
Copy link
Member

Sorry for the radio silence on this. I really like this idea! Going to import this and have the rest of the team weigh in.

@facebook-github-bot
Copy link
Contributor

@samwgoldman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@agentcooper
Copy link
Contributor Author

any updates on this?

@peter-leonov
Copy link
Contributor

@samwgoldman 🙏 please, implement this feature! It's so of a principle of least astonishment :)

@francorisso
Copy link

Please please make this happen

@mrkev
Copy link
Contributor

mrkev commented Jun 15, 2018

Oh hey, I looked a bit into what happened. Looks like this interfered with React default props for some reason, and then it fell off the radar (the flow team is busy). I'll try to find out if this is still an issue, I really like this idea.

Mind fixing the merge conflict though @agentcooper?

@agentcooper
Copy link
Contributor Author

@mrkev I'll try to update the code over the weekend

@mrkev
Copy link
Contributor

mrkev commented Jun 15, 2018

sounds good!

@agentcooper agentcooper reopened this Jun 17, 2018
@agentcooper
Copy link
Contributor Author

@mrkev 👀

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrkev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@agentcooper
Copy link
Contributor Author

@mrkev any updates on this?

I'm trying to rebase my diff again, but I can't solve one compiler error:

File "src/typing/statement.ml", line 2499, characters 45-55:
Error: This function has type Context.t -> Loc.t Ast.Expression.t -> Type.t
       It is applied to too many arguments; maybe you forgot a `;'.

and line 2499 is:

              | VariableDeclaration.Const -> expression ~is_const:true cx expr

Here is the full patch I'm trying to apply:

diff --git a/newtests/gen_flow_files_command/test.js b/newtests/gen_flow_files_command/test.js
index da5be6130..b373edfee 100644
--- a/newtests/gen_flow_files_command/test.js
+++ b/newtests/gen_flow_files_command/test.js
@@ -146,7 +146,7 @@ export default suite(({addFile, addFiles, flowCmd}) => [
           // @flow
 
           declare export default number;
-          declare export var str: string;
+          declare export var str: "asdf";
 
 
         `,
diff --git a/src/typing/statement.ml b/src/typing/statement.ml
index e210a296a..8f3582ce3 100644
--- a/src/typing/statement.ml
+++ b/src/typing/statement.ml
@@ -2494,7 +2494,11 @@ and variable cx kind ?if_uninitialized (_, vdecl) = Ast.Statement.(
         Type_inference_hooks_js.(dispatch_lval_hook cx name loc (Val t));
         (match init with
           | Some ((rhs_loc, _) as expr) ->
-            let rhs = expression cx expr in
+            let rhs =
+              match kind with
+              | VariableDeclaration.Const -> expression ~is_const:true cx expr
+              | _ -> expression cx expr
+            in
             (**
              * Const and let variables are not declared during evaluation of
              * their initializer expressions.
@@ -2596,8 +2600,8 @@ and mixin_element_spread cx (loc, e) =
     Flow.flow_t cx (arr, DefT (reason, ArrT (ArrayAT(tvar, None))));
   )
 
-and expression ?(is_cond=false) cx (loc, e) : Type.t * unit Ast.Expression.t' =
-  let t, e' = expression_ ~is_cond cx loc e in
+and expression ?(is_cond=false) ?(is_const=false) cx (loc, e) : Type.t * unit Ast.Expression.t' =
+  let t, e' = expression_ ~is_cond ~is_const cx loc e in
   Env.add_type_table cx loc t;
   t, e'
 
@@ -2610,11 +2614,11 @@ and this_ cx loc = Ast.Expression.(
 and super_ cx loc =
   Env.var_ref cx (internal_name "super") loc
 
-and expression_ ~is_cond cx loc e : Type.t * unit Ast.Expression.t' =
+and expression_ ~is_cond ~is_const cx loc e : Type.t * unit Ast.Expression.t' =
   let ex = (loc, e) in Ast.Expression.(match e with
 
   | Ast.Expression.Literal lit ->
-      literal cx loc lit, Ast.Expression.Literal lit
+      literal ~is_const cx loc lit, Ast.Expression.Literal lit
 
   (* Treat the identifier `undefined` as an annotation for error reporting
    * purposes. Like we do with other literals. Otherwise we end up pointing to
@@ -3935,7 +3939,7 @@ and identifier cx name loc =
   t
 
 (* traverse a literal expression, return result type *)
-and literal cx loc lit = Ast.Literal.(match lit.Ast.Literal.value with
+and literal ?(is_const=false) cx loc lit = Ast.Literal.(match lit.Ast.Literal.value with
   | String s ->
       (* It's too expensive to track literal information for large strings.*)
       let max_literal_length = Context.max_literal_length cx in
@@ -3944,7 +3948,9 @@ and literal cx loc lit = Ast.Literal.(match lit.Ast.Literal.value with
         then Literal (None, s)
         else AnyLiteral
       in
-      DefT (annot_reason (mk_reason RString loc), StrT lit)
+      if is_const
+      then DefT (annot_reason (mk_reason RString loc), SingletonStrT s)
+      else DefT (annot_reason (mk_reason RString loc), StrT lit)
 
   | Boolean b ->
       DefT (annot_reason (mk_reason RBoolean loc), BoolT (Some b))

@benwiley4000
Copy link

Hm, any status on this? Would be really nice to have - removes tons of needless (redundant) annotations in constant export files (as discussed here: #4279 (comment))

@mrkev
Copy link
Contributor

mrkev commented Aug 27, 2018

Welp sorry, got busy with other projects. IIRC this was breaking some stuff with object literals (object literal strings were thought to be constant? So {a: 'foo'} was the inferred type for const a = {a: 'foo'}?). I might be wrong sorry, it was a while ago 😅

@goodmind
Copy link
Contributor

@mrkev any info about this PR?

@mrkev
Copy link
Contributor

mrkev commented Apr 1, 2019

Have there been any updates? I remember this broke the tests. 😅

@peter-leonov
Copy link
Contributor

Just two years and ten months so far, too early to fix tests 😈

@goodmind
Copy link
Contributor

goodmind commented Apr 2, 2019

I mean if author still wants to pursue this PR, Flow team is more willing to accept PRs lately, than 2 years ago

@agentcooper
Copy link
Contributor Author

Sorry, folks, but I'm not interested in working on this anymore. I've switched to TypeScript since then. My contribution experience there was much better.

Feel free to re-use the code for another PR.

@goodmind
Copy link
Contributor

goodmind commented Apr 2, 2019

Thank you! I re-opened this PR #7607

@mrkev
Copy link
Contributor

mrkev commented Apr 4, 2019

Closing this one in that case 👍

@mrkev mrkev closed this Apr 4, 2019
@goodmind goodmind removed the Has PR label Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

typeof string literal
9 participants