Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133543003:
[parser] report errors for invalid binding patterns in async formal parameters (Closed)
Description[parser] report errors for invalid binding patterns in async formal parameters
BUG=v8:4483, v8:5190
R=littledan@chromium.org, nikolaos@chromium.org
Patch Set 1 #
Total comments: 15
Patch Set 2 : fixup #
Total comments: 1
Messages
Total messages: 14 (3 generated)
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
PTAL (sorry for the noise with this CL, I am not happy with depot-tools-auth at all)
Description was changed from ========== [parser] report errors for invalid binding patterns in async formal parameters BUG=v8:4483, v8:5190 R=littledan@chromium.org, nikolaos@chromium.org patch from issue 2131823002 at patchset 1 (http://crrev.com/2131823002#ps1) ========== to ========== [parser] report errors for invalid binding patterns in async formal parameters BUG=v8:4483, v8:5190 R=littledan@chromium.org, nikolaos@chromium.org ==========
caitp@igalia.com changed reviewers: - caitpotter88@gmail.com
LGTM, modulo my two comments. https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:926: if (!classifier->is_valid_binding_pattern()) { Is it possible that we have both !is_valid_async_arrow_formal_parameters and !is_valid_binding_pattern? In that case, we'd be reporting two errors here. Maybe switch the "if" to an "else if"? BTW, the same seems to be the case with the error message in line 916. Can it be that we have !is_valid_arrow_formal_parameters, is_async, and at least one of !is_valid_async_arrow_formal_parameters and !is_valid_binding_pattern? If yes, maybe add "*ok &&" to the condition of line 919. https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:1033: bool marge_partterns, bool* ok); Unless I'm missing the pun, "merge_patterns" :-)
Thanks. I think Dan is working on an alternative approach, which is simpler/less of a hack, but maybe a little more expensive. I think we should wait for that CL to be out for review before landing this. https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:926: if (!classifier->is_valid_binding_pattern()) { On 2016/07/11 13:19:50, nickie wrote: > Is it possible that we have both !is_valid_async_arrow_formal_parameters and > !is_valid_binding_pattern? In that case, we'd be reporting two errors here. > Maybe switch the "if" to an "else if"? > > BTW, the same seems to be the case with the error message in line 916. Can it > be that we have !is_valid_arrow_formal_parameters, is_async, and at least one of > !is_valid_async_arrow_formal_parameters and !is_valid_binding_pattern? If yes, > maybe add "*ok &&" to the condition of line 919. Yeah, this is true --- the WebKit style is to just add `return` statements rather than using `else if` control flow, which I think I prefer. Not sure what Chromium/Google's styleguide says about this. If people are okay with it, I prefer to just add return statements where they're missing. https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:1033: bool marge_partterns, bool* ok); On 2016/07/11 13:19:50, nickie wrote: > Unless I'm missing the pun, "merge_patterns" :-) just a typo, will fix
Thanks. I think Dan is working on an alternative approach, which is simpler/less of a hack, but maybe a little more expensive. I think we should wait for that CL to be out for review before landing this.
https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:926: if (!classifier->is_valid_binding_pattern()) { On 2016/07/11 13:24:42, caitp wrote: > On 2016/07/11 13:19:50, nickie wrote: > > Is it possible that we have both !is_valid_async_arrow_formal_parameters and > > !is_valid_binding_pattern? In that case, we'd be reporting two errors here. > > Maybe switch the "if" to an "else if"? > > > > BTW, the same seems to be the case with the error message in line 916. Can it > > be that we have !is_valid_arrow_formal_parameters, is_async, and at least one > of > > !is_valid_async_arrow_formal_parameters and !is_valid_binding_pattern? If > yes, > > maybe add "*ok &&" to the condition of line 919. > > Yeah, this is true --- the WebKit style is to just add `return` statements > rather than using `else if` control flow, which I think I prefer. Not sure what > Chromium/Google's styleguide says about this. If people are okay with it, I > prefer to just add return statements where they're missing. Yes, I think that adding return statements is a better option here.
https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2351: if (merge_patterns) { I find this logic of when to merge_patterns to be really confusing. Maybe better to notice the binding pattern error when it's generated, rather than propagating it like this and noticing it later. I tried that approach out at https://codereview.chromium.org/2139063002/ . https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2865: classifier->ReplaceBindingPatternError(location, message, arg); I don't understand why this case is added when we're going to reach ArrowFormalParametersUnexpectedToken a couple lines lower. Also, why replace here rather than add to the list like normal? Do you have an example of an error message that improves due to this? If this is about getting a better error message rather than reporting an error in the first place, we should have a message test for it. https://codereview.chromium.org/2133543003/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2133543003/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7719: "var f = async({ foo = async(a) => 1 })", How is this last error related to your patch?
https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2351: if (merge_patterns) { On 2016/07/11 23:22:47, Dan Ehrenberg wrote: > I find this logic of when to merge_patterns to be really confusing. Maybe better > to notice the binding pattern error when it's generated, rather than propagating > it like this and noticing it later. I tried that approach out at > https://codereview.chromium.org/2139063002/ . That doesn't really work, because then we could fail when parsing `async(<assignment pattern>);` for no real reason... https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2865: classifier->ReplaceBindingPatternError(location, message, arg); On 2016/07/11 23:22:47, Dan Ehrenberg wrote: > I don't understand why this case is added when we're going to reach > ArrowFormalParametersUnexpectedToken a couple lines lower. Also, why replace > here rather than add to the list like normal? Do you have an example of an error > message that improves due to this? If this is about getting a better error > message rather than reporting an error in the first place, we should have a > message test for it. ArrowFormalParameters don't really matter for async functions. What matters is that the parameters to the `async(params...)` call are each valid binding patterns. (And also, valid AsyncArrowFormalParameters because of the banned `await` word). https://codereview.chromium.org/2133543003/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2133543003/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7719: "var f = async({ foo = async(a) => 1 })", On 2016/07/11 23:22:47, Dan Ehrenberg wrote: > How is this last error related to your patch? The idea is to make sure that even though async arrow functions still work correctly, async calls also still work correctly and don't incorrectly report errors that they should not.
https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2351: if (merge_patterns) { On 2016/07/11 23:32:24, caitp wrote: > On 2016/07/11 23:22:47, Dan Ehrenberg wrote: > > I find this logic of when to merge_patterns to be really confusing. Maybe > better > > to notice the binding pattern error when it's generated, rather than > propagating > > it like this and noticing it later. I tried that approach out at > > https://codereview.chromium.org/2139063002/ . > > That doesn't really work, because then we could fail when parsing > `async(<assignment pattern>);` for no real reason... I don't understand the issue. The binding pattern is only checked if it's followed by an arrow (the same case for turning it into a comma expression). Do you have a test case in mind? https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2865: classifier->ReplaceBindingPatternError(location, message, arg); On 2016/07/11 23:32:24, caitp wrote: > On 2016/07/11 23:22:47, Dan Ehrenberg wrote: > > I don't understand why this case is added when we're going to reach > > ArrowFormalParametersUnexpectedToken a couple lines lower. Also, why replace > > here rather than add to the list like normal? Do you have an example of an > error > > message that improves due to this? If this is about getting a better error > > message rather than reporting an error in the first place, we should have a > > message test for it. > > ArrowFormalParameters don't really matter for async functions. What matters is > that the parameters to the `async(params...)` call are each valid binding > patterns. (And also, valid AsyncArrowFormalParameters because of the banned > `await` word). Could you write a test for it?
https://codereview.chromium.org/2133543003/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:901: !this->IsIdentifier(expr)) { Merging the conditions and removing the "else" changed the semantics. I suggest that you keep the two ifs.
https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2351: if (merge_patterns) { On 2016/07/11 23:37:57, Dan Ehrenberg wrote: > On 2016/07/11 23:32:24, caitp wrote: > > On 2016/07/11 23:22:47, Dan Ehrenberg wrote: > > > I find this logic of when to merge_patterns to be really confusing. Maybe > > better > > > to notice the binding pattern error when it's generated, rather than > > propagating > > > it like this and noticing it later. I tried that approach out at > > > https://codereview.chromium.org/2139063002/ . > > > > That doesn't really work, because then we could fail when parsing > > `async(<assignment pattern>);` for no real reason... > > I don't understand the issue. The binding pattern is only checked if it's > followed by an arrow (the same case for turning it into a comma expression). Do > you have a test case in mind? I tried the following with Dan's patch and they seem to work fine, so I don't understand what Caitlin suggests here. d8> function async(x) { print("async", x, "FTW!"); } d8> var await=42; d8> f = async(1); async 1 FTW! d8> f = async(x=1); async 1 FTW! d8> f = async([x,y]=[1,2]); async 1,2 FTW! d8> async(x=1); async 1 FTW! d8> async([x,y]=[1,2]); async 1,2 FTW! d8> async(await); async 42 FTW! d8> async(await=17); async 17 FTW!
https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2133543003/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2351: if (merge_patterns) { On 2016/07/12 13:11:53, nickie wrote: > On 2016/07/11 23:37:57, Dan Ehrenberg wrote: > > On 2016/07/11 23:32:24, caitp wrote: > > > On 2016/07/11 23:22:47, Dan Ehrenberg wrote: > > > > I find this logic of when to merge_patterns to be really confusing. Maybe > > > better > > > > to notice the binding pattern error when it's generated, rather than > > > propagating > > > > it like this and noticing it later. I tried that approach out at > > > > https://codereview.chromium.org/2139063002/ . > > > > > > That doesn't really work, because then we could fail when parsing > > > `async(<assignment pattern>);` for no real reason... > > > > I don't understand the issue. The binding pattern is only checked if it's > > followed by an arrow (the same case for turning it into a comma expression). > Do > > you have a test case in mind? > > I tried the following with Dan's patch and they seem to work fine, so I don't > understand what Caitlin suggests here. > > d8> function async(x) { print("async", x, "FTW!"); } > d8> var await=42; > > d8> f = async(1); > async 1 FTW! > > d8> f = async(x=1); > async 1 FTW! > > d8> f = async([x,y]=[1,2]); > async 1,2 FTW! > > d8> async(x=1); > async 1 FTW! > > d8> async([x,y]=[1,2]); > async 1,2 FTW! > > d8> async(await); > async 42 FTW! > > d8> async(await=17); > async 17 FTW! What are either of you even talking about? Using plain english, what he suggested does not work. What he implements in his patch is different from what he suggested here --- so that's besides the point. Anyways, his patch works, cool, lets do it. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
