Scripts should not be considered in a block #1339

macmv · 2021-06-18T07:58:52Z

This allows scripts like these to compile:

"use strict";

function Hello() {}

macmv · 2021-06-18T08:12:32Z

Huh. This test failed:

    let scenario = r#"
    'use strict';
    function f(a, b, b) {}
    "#;

    let mut context = Context::new();

    let string = dbg!(forward(&mut context, scenario));

    assert!(string.starts_with("Uncaught \"SyntaxError\": "));

And it was passing before because functions were not allowed at all in strict mode. Fixing this now.

EDIT: This is meant to test that a function cannot have duplicate parameters.

Razican · 2021-06-18T08:22:20Z

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,897	78,897	0
Passed	26,865	26,842	-23
Ignored	15,628	15,628	0
Failed	36,404	36,427	+23
Panics	0	0	0
Conformance	34.05%	34.02%	-0.03%

Fixed tests:

test/built-ins/Number/S9.3.1_A3_T2.js [strict mode] (previously Failed)
test/built-ins/Number/S9.3.1_A3_T2.js (previously Failed)
test/built-ins/Number/S9.3.1_A3_T1.js [strict mode] (previously Failed)
test/built-ins/Number/S9.3.1_A3_T1.js (previously Failed)

Broken tests:

test/language/expressions/logical-assignment/lgcl-or-assignment-operator-non-simple-lhs.js [strict mode] (previously Passed)
test/language/expressions/logical-assignment/lgcl-nullish-assignment-operator-non-simple-lhs.js [strict mode] (previously Passed)
test/language/expressions/logical-assignment/lgcl-and-assignment-operator-non-simple-lhs.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/parenthesized-lefthandsideexpression-assignment-assignmentexpression-1.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/direct-callexpression-arguments.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/parenthesized-primaryexpression-objectliteral.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/direct-lefthandsideexpression-assignment-assignmentexpression-1.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/direct-lefthandsideexpression-assignment-assignmentexpression-0.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/direct-lefthandsideexpression-assignment-assignmentexpression-2.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/parenthesized-lefthandsideexpression-assignment-assignmentexpression-2.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/parenthesized-callexpression-arguments.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/direct-arrowfunction-1.js [strict mode] (previously Passed)
test/language/expressions/assignmenttargettype/parenthesized-lefthandsideexpression-assignment-assignmentexpression-0.js [strict mode] (previously Passed)
test/language/expressions/assignment/target-assignment-inside-function.js [strict mode] (previously Passed)
test/language/statements/async-function/dflt-params-duplicates.js [strict mode] (previously Passed)
test/language/statements/async-function/early-errors-declaration-duplicate-parameters.js [strict mode] (previously Passed)
test/language/statements/async-function/rest-param-strict-body.js [strict mode] (previously Passed)
test/language/statements/async-function/early-errors-declaration-NSPL-with-USD.js [strict mode] (previously Passed)
test/language/statements/function/param-duplicated-strict-2.js [strict mode] (previously Passed)
test/language/statements/function/param-duplicated-strict-3.js [strict mode] (previously Passed)
test/language/statements/function/param-duplicated-strict-1.js [strict mode] (previously Passed)
test/language/statements/function/dflt-params-duplicates.js [strict mode] (previously Passed)
test/language/statements/function/rest-param-strict-body.js [strict mode] (previously Passed)
test/language/statements/function/13.1-5gs.js [strict mode] (previously Passed)
test/language/statements/function/use-strict-with-non-simple-param.js [strict mode] (previously Passed)
test/built-ins/Number/prototype/toExponential/this-is-0-fractiondigits-is-0.js [strict mode] (previously Passed)
test/built-ins/Number/prototype/toExponential/this-is-0-fractiondigits-is-0.js (previously Passed)

macmv · 2021-06-18T08:23:43Z

I cannot believe that broke so many tests. I will do my best to fix some of them, but no promises.

macmv · 2021-06-18T08:26:39Z

After looking over a few of these, I don't think it will be easy to make any of them pass. Most of them expected to fail, were in strict mode, and also happened to define a function. But now that they aren't failing, we get lower coverage.

Razican · 2021-06-18T08:32:08Z

After looking over a few of these, I don't think it will be easy to make any of them pass. Most of them expected to fail, were in strict mode, and also happened to define a function. But now that they aren't failing, we get lower coverage.

It does happen sometimes that some tests are passing because they are supposed to fail when parsing, and they were failing on parsing due to a different error. No worries, if the implementation is correct, we will merge it, even if it uncovers some hidden errors :)

macmv · 2021-06-18T08:33:38Z

It does happen sometimes that some tests are passing because they are supposed to fail when parsing, and they were failing on parsing due to a different error. No worries, if the implementation is correct, we will merge it, even if it uncovers some hidden errors :)

Sounds good to me 👍

Razican

Looks good to me! Check my comment in case it makes sense :)

Razican · 2021-06-18T08:33:19Z

boa/src/syntax/parser/function/mod.rs

            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));


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

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.

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

RageKnify

Looks good to me, just suggested a possible optimization, but I'm not sure it's possible.

RageKnify · 2021-06-19T11:55:54Z

boa/src/syntax/parser/function/mod.rs


+            param_names.insert(next_param.name().to_string());


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.

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>.

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

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?).

Scripts are not considered "in a block"

06a1367

Duplicate parameter names are now checked for functions

cb40725

Razican approved these changes Jun 18, 2021

View reviewed changes

Razican requested review from Lan2u, jasonwilliams, RageKnify and HalidOdat June 18, 2021 08:34

RageKnify approved these changes Jun 19, 2021

View reviewed changes

Razican approved these changes Jun 19, 2021

View reviewed changes

macmv added 2 commits June 20, 2021 15:09

Parameter names hashset now uses Box<str> instead of String

f37b4b1

Added duplicate parameter test

c9190ca

Razican merged commit b5d2ce8 into boa-dev:master Jun 21, 2021

Razican added this to the v0.13.0 milestone Aug 24, 2021

Razican added the bug Something isn't working label Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripts should not be considered in a block #1339

Scripts should not be considered in a block #1339

macmv commented Jun 18, 2021

macmv commented Jun 18, 2021 •

edited

Loading

Razican commented Jun 18, 2021

macmv commented Jun 18, 2021

macmv commented Jun 18, 2021

Razican commented Jun 18, 2021

macmv commented Jun 18, 2021

Razican left a comment

Razican Jun 18, 2021

macmv Jun 18, 2021

Razican Jun 19, 2021

RageKnify left a comment

RageKnify Jun 19, 2021

Razican Jun 19, 2021

RageKnify Jun 19, 2021

macmv Jun 20, 2021

Scripts should not be considered in a block #1339

Scripts should not be considered in a block #1339

Conversation

macmv commented Jun 18, 2021

macmv commented Jun 18, 2021 • edited Loading

Razican commented Jun 18, 2021

Test262 conformance changes:

macmv commented Jun 18, 2021

macmv commented Jun 18, 2021

Razican commented Jun 18, 2021

macmv commented Jun 18, 2021

Razican left a comment

Choose a reason for hiding this comment

Razican Jun 18, 2021

Choose a reason for hiding this comment

macmv Jun 18, 2021

Choose a reason for hiding this comment

Razican Jun 19, 2021

Choose a reason for hiding this comment

RageKnify left a comment

Choose a reason for hiding this comment

RageKnify Jun 19, 2021

Choose a reason for hiding this comment

Razican Jun 19, 2021

Choose a reason for hiding this comment

RageKnify Jun 19, 2021

Choose a reason for hiding this comment

macmv Jun 20, 2021

Choose a reason for hiding this comment

macmv commented Jun 18, 2021 •

edited

Loading