[Merged by Bors] - Allow BindingPattern in function parameters
#1666
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Revert "redefining FormalParameter to include BindingPattern" This reverts commit 248339b.
jedel1043
added
ast
jedel1043
changed the title
BindingPattern in function parameters #1601BindingPattern in function parameters
Test262 conformance changes:
Fixed tests (409):Broken tests (56): |
raskad
requested changes
Oct 14, 2021
There was a problem hiding this comment.
Nice work. I did a first review.
Could you look at the tests that are failing now? Based on the names of those tests, most of them seem to be related to strict mode. My guess is, that we have to add some conditions to the is_simple evaluation for FormalParameterList like defined here: https://tc39.es/ecma262/#sec-static-semantics-issimpleparameterlist
raskad
reviewed
Oct 14, 2021
raskad
reviewed
Oct 14, 2021
raskad
reviewed
Oct 14, 2021
raskad
reviewed
Oct 14, 2021
raskad
reviewed
Oct 14, 2021
jedel1043
requested changes
Oct 14, 2021
Razican
reviewed
Oct 23, 2021
boa/src/builtins/function/mod.rs
Outdated
| @@ -16,6 +16,7 @@ use std::{ | |||
| borrow::Cow, | |||
| fmt, | |||
| ops::{Deref, DerefMut}, | |||
| option::Option, | |||
There was a problem hiding this comment.
Isn't this in the prelude? Can we avoid this import?
boa/src/object/jsobject.rs
Outdated
| @@ -109,7 +109,6 @@ impl JsObject { | |||
| pub fn equals(lhs: &Self, rhs: &Self) -> bool { | |||
| std::ptr::eq(lhs.as_ref(), rhs.as_ref()) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This line removal seems unrelated to the changes here, and I prefer to have a line between two functions, it makes the code clearer.
boa/src/syntax/ast/node/mod.rs
Outdated
| @@ -693,6 +723,7 @@ unsafe impl Trace for PropertyName { | |||
| /// are using different indents in their source files. This fixes | |||
| /// any strings which may have been changed in a different indent | |||
| /// level. | |||
|
|
|||
There was a problem hiding this comment.
I'd prefer to keep the documentation together with the code, and it seems there was no related change here.
|
@raskad Hey, the code causing this CI to fail does not exist in the changed version, is one of the errors but FormalParameter::name() is not even called in this line. |
|
There my be a new change on main for that function. can you try a rebase? |
|
@am-a-man When I checkout your branch, I get the errors now. |
Codecov Report
@@ Coverage Diff @@
## main #1666 +/- ##
==========================================
- Coverage 50.91% 50.85% -0.07%
==========================================
Files 199 199
Lines 17709 17803 +94
==========================================
+ Hits 9016 9053 +37
- Misses 8693 8750 +57
Continue to review full report at Codecov.
|
|
@jedel1043 please review the requested changes |
jedel1043
requested changes
Oct 30, 2021
| None => Position::new(1, 1), | ||
| }, | ||
| ))); | ||
| for param_name in param.names().iter() { |
There was a problem hiding this comment.
Suggested change
| for param_name in param.names().iter() { | |
| for param_name in param.names() { |
The iter call is unnecessary in a lot of these, the for expression automatically calls into_iter if you pass something implementing the IntoIterator trait.
| .parse(cursor)?; | ||
|
|
||
| let init = if let Some(t) = cursor.peek(0)? { | ||
| // Check that this is an initilizer before attempting parse. |
There was a problem hiding this comment.
Suggested change
| // Check that this is an initilizer before attempting parse. | |
| // Check that this is an initializer before attempting parse. |
Comment on lines
210
to
222
| let init = if let Some(t) = cursor.peek(0)? { | ||
| // Check that this is an initilizer before attempting parse. | ||
| if *t.kind() == TokenKind::Punctuator(Punctuator::Assign) { | ||
| Some( | ||
| Initializer::new(true, self.allow_yield, self.allow_await) | ||
| .parse(cursor)?, | ||
| ) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
This should be simpler with a functional approach:
Suggested change
| let init = if let Some(t) = cursor.peek(0)? { | |
| // Check that this is an initilizer before attempting parse. | |
| if *t.kind() == TokenKind::Punctuator(Punctuator::Assign) { | |
| Some( | |
| Initializer::new(true, self.allow_yield, self.allow_await) | |
| .parse(cursor)?, | |
| ) | |
| } else { | |
| None | |
| } | |
| } else { | |
| None | |
| }; | |
| let init = cursor.peek(0)?.filter(|t| { | |
| // Check that this is an initilizer before attempting parse. | |
| *t.kind() == TokenKind::Punctuator(Punctuator::Assign) | |
| }).map(|_| | |
| Initializer::new(true, self.allow_yield, self.allow_await).parse(cursor) | |
| ).transpose()?; |
Applies to all patterns similar to this.
There was a problem hiding this comment.
@jedel1043
above changes gives errors because borrow occurs at cursor.peek() and *cursor is needed in the same closure,
should I implement below code using .cloned or is there an efficient way to do it?
let init = cursor.peek(0)?.cloned().filter(|t| {
// Check that this is an initializer before attempting parse.
*t.kind() == TokenKind::Punctuator(Punctuator::Assign)
}).map(|_|
Initializer::new(true, self.allow_yield, self.allow_await).parse(cursor)
).transpose()?;
There was a problem hiding this comment.
let init = cursor.peek(0)?;
let init = init.filter(..)
...There was a problem hiding this comment.
same problem still exists
There was a problem hiding this comment.
Then the problem must be map. Hmm... does this work?
let init = cursor.peek(0)?
.filter(|t| {
// Check that this is an initilizer before attempting parse.
*t.kind() == TokenKind::Punctuator(Punctuator::Assign)
}).map(|t| {
drop(t);
Initializer::new(true, self.allow_yield, self.allow_await).parse(cursor)
}).transpose()?;If it doesn't, just clone. I prefer focusing on the readability of the code, we can optimize later.
There was a problem hiding this comment.
filter expects a bool
expected type `bool`
found enum `Result<Node, parser::error::ParseError>`
There was a problem hiding this comment.
I reedited the code, had a missing \n
There was a problem hiding this comment.
the original error again
jedel1043
approved these changes
Oct 30, 2021
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
jedel1043
requested changes
Oct 31, 2021
|
@jedel1043 Cargo.lock is already a file in current project, my branch failed in security audit because of do you want me to delete Cargo.lock or just revert to the previous version? |
|
Sorry! I thought it was a lock file for the boa crate 😅 |
|
bors r+ |
|
👎 Rejected by code reviews |
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
bors retry |
bors bot
pushed a commit
that referenced
this pull request
Nov 1, 2021
<!--- Thank you for contributing to Boa! Please fill out the template below, and remove or add any information as you feel neccesary. ---> This Pull Request fixes/closes #1601 It changes the following: - implements `FormalParameter` using `syntax::ast::node::declaration::Declaration` as ``` pub struct FormalParameter { declaration: Declaration, is_rest_param: bool, } ``` - changes `fn test_formatting(source: &'static str)` in `ast::node` to setup tests based on the new struct definition - changes in various tests using `FormalParameter` - changes in files using `FormalParameter` What it does not change: - if the function uses `FormalParameter` but does not seem affected due to the changed definition it is not altered while keeping all the required features i.e. if it wasn't affected but ended up giving undesired (not error) behaviour it has been changed Co-authored-by: Aman Kumar <57605821+am-a-man@users.noreply.github.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
BindingPattern in function parametersBindingPattern in function parameters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request fixes/closes #1601
It changes the following:
FormalParameterusingsyntax::ast::node::declaration::Declarationasfn test_formatting(source: &'static str)inast::nodeto setup tests based on the new struct definitionFormalParameterFormalParameterWhat it does not change:
FormalParameterbut does not seem affected due to the changed definition it is not altered while keeping all the required features i.e. if it wasn't affected but ended up giving undesired (not error) behaviour it has been changed