{"action":"created","issue":{"url":"https://api.github.com/repos/Microsoft/TypeScript/issues/9998","repository_url":"https://api.github.com/repos/Microsoft/TypeScript","labels_url":"https://api.github.com/repos/Microsoft/TypeScript/issues/9998/labels{/name}","comments_url":"https://api.github.com/repos/Microsoft/TypeScript/issues/9998/comments","events_url":"https://api.github.com/repos/Microsoft/TypeScript/issues/9998/events","html_url":"https://github.com/Microsoft/TypeScript/issues/9998","id":167984634,"number":9998,"title":"Trade-offs in Control Flow Analysis","user":{"login":"RyanCavanaugh","id":6685088,"avatar_url":"https://avatars.githubusercontent.com/u/6685088?v=3","gravatar_id":"","url":"https://api.github.com/users/RyanCavanaugh","html_url":"https://github.com/RyanCavanaugh","followers_url":"https://api.github.com/users/RyanCavanaugh/followers","following_url":"https://api.github.com/users/RyanCavanaugh/following{/other_user}","gists_url":"https://api.github.com/users/RyanCavanaugh/gists{/gist_id}","starred_url":"https://api.github.com/users/RyanCavanaugh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RyanCavanaugh/subscriptions","organizations_url":"https://api.github.com/users/RyanCavanaugh/orgs","repos_url":"https://api.github.com/users/RyanCavanaugh/repos","events_url":"https://api.github.com/users/RyanCavanaugh/events{/privacy}","received_events_url":"https://api.github.com/users/RyanCavanaugh/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/Microsoft/TypeScript/labels/Design%20Notes","name":"Design Notes","color":"bfdadc"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":13,"created_at":"2016-07-28T00:03:48Z","updated_at":"2016-08-01T07:27:15Z","closed_at":null,"body":"Some rough notes from a conversation @ahejlsberg and I had earlier about trade-offs in the control flow analysis work based on running the real-world code (RWC) tests. For obvious reasons I'll be comparing what Flow does with similar examples to compare and contrast possible outcomes.\r\n\r\nThe primary question is: When a function is invoked, what should we assume its side effects are?\r\n\r\nOne option is to be *pessimistic* and reset all narrowings, assuming that any function might mutate any object it could possibly get its hands on. Another option is to be *optimistic* and assume the function doesn't modify any state. Both of these seem to be bad.\r\n\r\nThis problem spans both locals (which might be subject to some \"closed over or not\" analysis) and object fields.\r\n\r\n#### Optimistic: Bad behavior on locals\r\n\r\nThe TypeScript compiler has code like this:\r\n```ts\r\nenum Token { Alpha, Beta, Gamma }\r\nlet token = Token.Alpha;\r\nfunction nextToken() {\r\n\ttoken = Token.Beta;\r\n}\r\nfunction maybeNextToken() {\r\n\tif (... something ...) {\r\n\t\tnextToken();\r\n\t}\r\n}\r\n\r\nfunction doSomething() {\r\n\tif (token !== Token.Alpha) {\r\n\t\tmaybeNextToken();\r\n\t}\r\n\t// is this possible?\r\n\tif (token === Token.Alpha) {\r\n\t\t// something happens\r\n\t}\r\n}\r\n```\r\nOptimistically assuming `token` isn't modified by `maybeNextToken` incorrectly flags `token === Token.Alpha` as an impossibility. However, in other cases, this is a *good* check to! See later examples.\r\n\r\n#### Optimistic: Bad behavior on fields\r\n\r\nThe RWC suite picked up a \"bug\" that looked like this:\r\n```ts\r\n// Function somewhere else\r\ndeclare function tryDoSomething(x: string, result: { success: boolean; value: number; }): void;\r\n\r\nfunction myFunc(x: string) {\r\n\tlet result = { success: false, value: 0 };\r\n\t\r\n\ttryDoSomething(x, result);\r\n\tif (result.success === true) { // %%\r\n\t\treturn result.value;\r\n\t}\r\n\t\r\n\ttryDoSomething(x.trim(), result);\r\n\tif (result.success === true) { // ??\r\n\t\treturn result.value;\r\n\t}\r\n\treturn -1;\r\n}\r\n```\r\nThe `??` line here is *not* a bug in the user code, but we thought it was, because after the `%%` block runs, the only remaining value in `result.success`'s domain is `false`.\r\n\r\n#### Pessimistic: Bad behavior on locals\r\n\r\nWe found actual bugs (several!) in partner code that looked like this:\r\n```ts\r\nenum Kind { Good, Bad, Ugly }\r\nlet kind: Kind = ...;\r\nfunction f() {\r\n\tif (kind) {\r\n\t\tlog('Doing some work');\r\n\t\tswitch (kind) {\r\n\t\t\tcase Kind.Good:\r\n\t\t\t\t// unreachable!\r\n\t\t}\r\n\t}\r\n}\r\n```\r\nHere, we detected the bug that `Kind.Good` (which has the falsy value `0`) is not in the domain of `kind` at the point of the `case` label. However, if we were fully pessimistic, we couldn't know that the global function `log` doesn't modify the global variable `kind`, thus incorrectly allowing this broken code.\r\n\r\n#### Pessimistic: Bad behavior on fields, example 1\r\n[A question on flowtype SO is a good example of this](http://stackoverflow.com/questions/38531182/weird-method-cannot-be-called-on-possibly-null-undefined-value)\r\n\r\nA smaller example that demonstrates the behavior:\r\n```ts\r\nfunction fn(arg: { x: string | null }) {\r\n    if (arg.x !== null) {\r\n        alert('All is OK!');\r\n        // Flow: Not OK, arg.x could be null\r\n        console.log(arg.x.substr(3));\r\n    }\r\n}\r\n```\r\nThe problem here is that, pessimistically, something like this might be happening:\r\n```ts\r\nlet a = { x: 'ok' };\r\nfunction alert() {\r\n    a.x = null;\r\n}\r\nfn(a);\r\n```\r\n\r\n#### Pessimistic: Bad behavior on fields, example 2\r\n\r\nThe TS compiler has code that looks like this (simplified):\r\n```ts\r\nfunction visitChildren(node: Node, visit: (node: Node) => void) {\r\n\tswitch(node.kind) {\r\n\t\tcase SyntaxKind.BinaryExpression:\r\n\t\t\tvisit(node.left);\r\n\t\t\tvisit(node.right); // Unsafe?\r\n\t\t\tbreak;\r\n\t\tcase SyntaxKind.PropertyAccessExpression:\r\n\t\t\tvisit(node.expr);\r\n\t\t\tvisit(node.name); // Unsafe?\r\n\t\t\tbreak;\r\n\t}\r\n}\r\n```\r\nHere, we discriminated the `Node` union type by its `kind`. A pessimistic behavior would say that the second invocations are unsafe, because the call to `visit` may have mutated `node.kind` through a secondary reference and invalidated the discrimination.\r\n\r\n\r\n### Mitigating with (shallow) inlining / analysis\r\n\r\nFlow does some assignment analysis to improve the quality of these errors, but it's obviously short of a full inlining solution, which wouldn't be even remotely practical. Some examples of how to defeat the analysis:\r\n```ts\r\n// Non-null assignment can still trigger null warnings\r\nfunction fn(x: string | null) {\r\n    function check1() {\r\n        x = 'still OK';\r\n    }\r\n\r\n    if (x !== null) {\r\n        check1();\r\n        // Flow: Error, x could be null\r\n        console.log(x.substr(0));\r\n    }\r\n}\r\n```\r\n```ts\r\n// Inlining is only one level deep\r\nfunction fn(x: string | null) {\r\n    function check1() {\r\n        check2();\r\n    }\r\n    function check2() {\r\n        x = null;\r\n    }\r\n    \r\n    if (x !== null) {\r\n        check1();\r\n        // Flow: No error\r\n        console.log(x.substr(0)); // crashes\r\n    }\r\n}\r\n```\r\n\r\n#### Mitigating with `const` parameters\r\n\r\nA low-hanging piece of fruit is to allow a `const` modifier on parameters. This would allow a much faster fix for code that looks like this:\r\n```ts\r\nfunction fn(const x: string | number) {\r\n  if (typeof x === 'string') {\r\n    thisFunctionCannotMutateX();\r\n    x.substr(0); // ok\r\n  }\r\n}\r\n```\r\n\r\n#### Mitigating with `readonly` fields\r\n\r\nThe `visitChildren` example above might be mitigated by saying that `readonly` fields retain their narrowing effects even in the presence of intervening function calls. This is technically unsound as you may have both a `readonly` and non-`readonly` alias to the same property, but in practice this is probably very rare.\r\n\r\n#### Mitigating with other options\r\n\r\nRandom ideas that got thrown out (will add to this list) but are probably bad?\r\n * `pure` modifier on functions that says this function doesn't modify anything. This is a bit impractical as we'd realistically want this on the vast majority of all functions, and it doesn't really solve the problem since lots of functions only modify *one* thing so you'd really want to say \"`pure` except for `m`\"\r\n * `volatile` property modifier that says this \"this property will change without notice\". We're not C++ and it's perhaps unclear where you'd apply this and where you wouldn't."},"comment":{"url":"https://api.github.com/repos/Microsoft/TypeScript/issues/comments/236508140","html_url":"https://github.com/Microsoft/TypeScript/issues/9998#issuecomment-236508140","issue_url":"https://api.github.com/repos/Microsoft/TypeScript/issues/9998","id":236508140,"user":{"login":"yortus","id":1252361,"avatar_url":"https://avatars.githubusercontent.com/u/1252361?v=3","gravatar_id":"","url":"https://api.github.com/users/yortus","html_url":"https://github.com/yortus","followers_url":"https://api.github.com/users/yortus/followers","following_url":"https://api.github.com/users/yortus/following{/other_user}","gists_url":"https://api.github.com/users/yortus/gists{/gist_id}","starred_url":"https://api.github.com/users/yortus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yortus/subscriptions","organizations_url":"https://api.github.com/users/yortus/orgs","repos_url":"https://api.github.com/users/yortus/repos","events_url":"https://api.github.com/users/yortus/events{/privacy}","received_events_url":"https://api.github.com/users/yortus/received_events","type":"User","site_admin":false},"created_at":"2016-08-01T07:27:15Z","updated_at":"2016-08-01T07:27:15Z","body":"@maiermic your post was pretty complex, so I may have misunderstood it, but it looks like you are describing what control flow anaylsis already does, plus extending it to 'look into' the functions being called as well. If so, how is this different to the inlining approach mentioned by @RyanCavanaugh in the OP?\r\n"}}
{"actor":{"display_login":"yortus"}}