warn on a statement expression "use asm"; not in a processing directive

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
The idea of "use asm" is to provide good developer feedback.  However, if their mistake is not putting "use asm" in the processing directives region according to the grammar, we won't say anything.  It seems friendly to issue a warning for statement expressions of the form "use asm"; in non-process-directive position.
Yeah.  I hit this with an example like this:

let asmModule = Function("g2", "foreign", "heap2", "{\
    'use asm';\
    let HEAP8 = new g2.Int8Array(heap2);\
    function f() { return 99 }\
    return {f: f};\
}")

The braces were the problem.
(Assignee)

Updated

6 years ago
Blocks: 854599
(Assignee)

Comment 2

6 years ago
Created attachment 744375 [details] [diff] [review]
patch

This patch warns for any expression statement "use asm"; not in the Directive Prologue of a function.  I would've liked to avoid passing canHaveDirectives to statement(); the annoying thing is that maybeParseDirective is called *after* we've seen the TOK_STRING in statement().

I also took the liberty of converting the fallthrough case of TOK_NAME to 'return expressionStatement' so it can be symmetric with the TOK_STRING case and easier to read.
Attachment #744375 - Flags: review?(jorendorff)
Comment on attachment 744375 [details] [diff] [review]
patch

Review of attachment 744375 [details] [diff] [review]:
-----------------------------------------------------------------

Great tests.

::: js/src/frontend/Parser.cpp
@@ +4468,5 @@
> +      case TOK_STRING:
> +        if (!canHaveDirectives && tokenStream.currentToken().atom() == context->names().useAsm) {
> +            if (!report(ParseWarning, false, null(), JSMSG_USE_ASM_DIRECTIVE_FAIL))
> +                return null();
> +        }

OK, this is a fine place for this. Maybe we should move all directive processing here, someday.
Attachment #744375 - Flags: review?(jorendorff) → review+

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a45f1dcd603c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.