Skip to content
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.

Sign up for GitHub

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

Scripts should not be considered in a block #1339

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Scripts should not be considered in a block #1339

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06a1367
Scripts are not considered "in a block"
macmv Jun 18, 2021
cb40725
Duplicate parameter names are now checked for functions
macmv Jun 18, 2021
f37b4b1
Parameter names hashset now uses Box<str> instead of String
macmv Jun 20, 2021
c9190ca
Added duplicate parameter test
macmv Jun 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions boa/src/syntax/parser/function/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ mod tests;

use crate::{
syntax::{
ast::{
node::{self},
Punctuator,
},
ast::{node, Punctuator},
lexer::{InputElement, TokenKind},
parser::{
expression::Initializer,
Expand All @@ -25,7 +22,7 @@ use crate::{
},
BoaProfiler,
};
use std::io::Read;
use std::{collections::HashSet, io::Read};

/// Formal parameters parsing.
///
Expand Down Expand Up @@ -66,6 +63,7 @@ where
cursor.set_goal(InputElement::RegExp);

let mut params = Vec::new();
let mut param_names = HashSet::new();

if cursor.peek(0)?.ok_or(ParseError::AbruptEnd)?.kind()
== &TokenKind::Punctuator(Punctuator::CloseParen)
Expand All @@ -76,14 +74,19 @@ where
loop {
let mut rest_param = false;

let position = cursor.peek(0)?.ok_or(ParseError::AbruptEnd)?.span().start();
let next_param = match cursor.peek(0)? {
Some(tok) if tok.kind() == &TokenKind::Punctuator(Punctuator::Spread) => {
rest_param = true;
FunctionRestParameter::new(self.allow_yield, self.allow_await).parse(cursor)?
}
_ => FormalParameter::new(self.allow_yield, self.allow_await).parse(cursor)?,
};
if param_names.contains(next_param.name()) {
return Err(ParseError::general("duplicate parameter name", position));
Copy link
Member

Choose a reason for hiding this comment

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

Could we add the parameter name in the error message? That way it's a bit more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to, but the general function requires a &'static str, which I cannot make from a format! macro. We would need to change the ParseError type to take an Into<Box<str>> in order for this to work. I'm not sure if that is within the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it like this for now then :)

}

param_names.insert(next_param.name().to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is creating a HashSet<String>, right? Is it possible to do it with just &str or similar? It would be nice to minimize allocations.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, at some point we want to intern all these strings, but for now this is not possible. The only optimization that could be done reasonably fast would be to change the Stri g for a Box<str>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that still minimizes the space used by the HashSet...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change this to a Box<str>. I couldn't think of a better way to check for duplicate names (except maybe storing their hashes?).

params.push(next_param);

if cursor.peek(0)?.ok_or(ParseError::AbruptEnd)?.kind()
Expand Down
2 changes: 1 addition & 1 deletion boa/src/syntax/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,6 @@ where
type Output = StatementList;

fn parse(self, cursor: &mut Cursor<R>) -> Result<Self::Output, ParseError> {
self::statement::StatementList::new(false, false, false, true, &[]).parse(cursor)
self::statement::StatementList::new(false, false, false, false, &[]).parse(cursor)
}
}