镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Narrow readonly properties of const locals in function expressions - #10015

Closed
Nathan Shively-Sanders (sandersn) wants to merge 5 commits into
masterfrom
narrow-readonly-properties-of-const-locals-in-function-expressions
Closed

Nathan Shively-Sanders (sandersn) wants to merge 5 commits into
masterfrom
narrow-readonly-properties-of-const-locals-in-function-expressions

Conversation

@sandersn

Copy link
Copy Markdown
Member

Fixes #9511

Anders Hejlsberg (@ahejlsberg) do you think this is a good change? It looks OK to me, but I don't have a good intuition for which narrowings are safe and which are not.

Comment thread src/compiler/checker.ts Outdated
return propType;
}
return getFlowTypeOfReference(node, propType, /*assumeInitialized*/ true, /*includeOuterFunctions*/ false);
return getFlowTypeOfReference(node, propType, /*assumeInitialized*/ true, /*includeOuterFunctions*/ isReadonlySymbol(prop));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you need a stronger check here. Say that you have a property access for x.y.z. Right now you're just checking whether z is a readonly symbol, but you really need to check for x.y and x as well.

@mhegazy

Copy link
Copy Markdown
Contributor

Nathan Shively-Sanders (@sandersn) can you refresh this PR.

@sandersn

Copy link
Copy Markdown
Member Author

I refreshed the PR, but it still doesn't check parent properties for readonly-ness. Also, based on our discussion in #9511, narrowing readonly properties ignores that getters will not always return the same value when called multiple times.

@mhegazy
Mohamed Hegazy (mhegazy) deleted the narrow-readonly-properties-of-const-locals-in-function-expressions branch November 2, 2017 21:03
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants