-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Give a semantic error on with statements #176
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
Conversation
@@ -5195,8 +5195,7 @@ module ts { | |||
} | |||
|
|||
function checkWithStatement(node: WithStatement) { | |||
checkExpression(node.expression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to talk about how we should still typecheck the expression (since there's no with
craziness there) and how the error message is inaccurate (since symbolless expressions like 3 > ''
ought to error anyway), but this is actually inline with the old compiler so I'm inclined to say it really doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we've always just thrown up our hands on everything in a 'with' block
Ignore the Travis-CI failure |
Give a semantic error on with statements
@@ -5284,7 +5284,7 @@ module ts { | |||
|
|||
function checkWithStatement(node: WithStatement) { | |||
checkExpression(node.expression); | |||
checkSourceElement(node.statement); | |||
error(node.expression, Diagnostics.All_symbols_within_a_with_block_will_be_resolved_to_any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small tweak. can you just underline the 'while' token (i think you can get it with the scanner). it seems weird to underling the expression since you're complaining about the 'while' block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Should be the with
keyword.
lgtm |
👍 |
Pretty straightforward. Giving a semantic error on all 'with' statements, and skipping type check for them.