[modules] Disable HTML-like comments

[modules] Disable HTML-like comments

ES2015 section B.1.3 ("HTML-like Comments") reads:

> The syntax and semantics of 11.4 is extended as follows except that
> this extension is not allowed when parsing source code using the goal
> symbol Module

http://www.ecma-international.org/ecma-262/6.0/#sec-html-like-comments

BUG=v8:5045
LOG=N
R=adamk@chromium.org

[mike3, 2016-05-25 20:40:42]

Description was changed from

==========
[modules] Disable HTML-like comments

ES2015 section B.1.3 ("HTML-like Comments") reads:

> The syntax and semantics of 11.4 is extended as follows except that
> this extension is not allowed when parsing source code using the goal
> symbol Module

http://www.ecma-international.org/ecma-262/6.0/#sec-html-like-comments

BUG=v8:4951
LOG=N
R=adamk@chromium.org
==========

to

==========
[modules] Disable HTML-like comments

ES2015 section B.1.3 ("HTML-like Comments") reads:

> The syntax and semantics of 11.4 is extended as follows except that
> this extension is not allowed when parsing source code using the goal
> symbol Module

http://www.ecma-international.org/ecma-262/6.0/#sec-html-like-comments

BUG=v8:5045
LOG=N
R=adamk@chromium.org
==========

[mike3, 2016-05-25 20:41:21]

Hi Adam,

This patch does not pass mjsunit. The test I added fails a CHECK:

    >>> Running tests for x64.release
    No connection to distribution server; running tests locally.
    === mjsunit/harmony/modules-html-comments ===                    
    ============ Stress 1/2 ============
    ============ Stress 2/2 ============
    #
    # Fatal error in ../src/compiler.cc, line 428
    # Check failed:
!info->shared_info()->feedback_vector()->metadata()->SpecDiffersFrom(
info->literal()->feedback_vector_spec()).
    #

    ==== C stack trace ===============================

     1: 0xb91779
     2: 0x54fd6f
     3: 0x54f872
     4: 0x58539d
     5: 0x549e79
     6: 0x54d592
     7: 0x54c7e0
     8: 0xaa0e4e
     9: 0xaa0cd5
    10: 0x127b65208ba7

The inline comment there reads:

> It's very important that recompiles do not alter the structure of the type
> feedback vector. Verify that the structure fits the function literal.

This is way beyond my experience here. Could you tell me where to start
addressing this problem?

Beyond that, I expect this initial implementation is too naive for `master`.
Does `module_` need to be a configurable flag on Scanner? Or would it be
sufficient to initialize Scanner with the value? If it *does* make sense to
expose `set_module` like this, I have not yet found the appropriate entry
points: the parser tests in cctest are failing, as well.

[adamk, 2016-05-25 20:43:01]

adamk@chromium.org changed reviewers:
+ vogelheim@chromium.org

[adamk, 2016-05-25 20:43:02]

+vogelheim who recently made a similar change for other reasons. His patch
(since reverted), only made a single change to scanner.cc, I'm curious if it was
wrong (see https://codereview.chromium.org/1801203002).

https://codereview.chromium.org/2009963002/diff/1/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2009963002/diff/1/src/parsing/scanner.cc#newc...
src/parsing/scanner.cc:45: module_(false),
Please call this "allow_html_comments_" instead.

[mike3, 2016-05-25 21:10:14]

https://codereview.chromium.org/2009963002/diff/1/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2009963002/diff/1/src/parsing/scanner.cc#newc...
src/parsing/scanner.cc:45: module_(false),
On 2016/05/25 20:43:02, adamk wrote:
> Please call this "allow_html_comments_" instead.

Acknowledged.

[vogelheim, 2016-05-27 08:07:10]

On 2016/05/25 20:43:02, adamk wrote:
> +vogelheim who recently made a similar change for other reasons. His patch
> (since reverted), only made a single change to scanner.cc, I'm curious if it
was
> wrong (see https://codereview.chromium.org/1801203002).

Probably. I only looked at full HTML comments (<!-- .. -->) and didn't treat the
strange edge cases of only end-of-comment marker. I no longer remember whether I
just overlooked those or whether - for that particular case - that was the
better method. I probably just overlooked it, though.

[vogelheim, 2016-05-27 08:08:49]

I suspect this is broken. Please be sure that

1, Parser + Pre-Parser still work the same, and
2, this still works when functions within a module are being re-parsed for
whatever reason.

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:937: scanner_.set_allow_html_comments(parsing_module_);
So... if a file is being parsed top-to-bottom, The Parser will come through here
and will tell the Scanner of whether to parser HTML comments or not. But if
function has been lazy-parsed or needs to be re-compiled later because of
(de-)optimization, how would the Scanner then know what to do?

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:937: scanner_.set_allow_html_comments(parsing_module_);
Also, do we know whether Parser + Pre-Parser will still behave the same?

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.cc#...
src/parsing/scanner.cc:458: if (c0_ == '-' && !allow_html_comments_) {
nitpick: It seems more logical to check this at the calling site; but that's
admittedly a matter of taste.

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.h#n...
src/parsing/scanner.h:809: // Whether this scanner should recognize HTML-like
comment tokens.
Please be clear on what this does.

As I read the code,
- allow_html_comments_ == true means that an HTML comment will be reported as
individual tokens (and thus usually as syntax error), while
- !allow_html_comments_ means the current behaviour, namely that HTML comments
will be recognized & skipped.

I would have assumed it to be the other way round... In either case, the comment
doesn't really clarify this.

[mike3, 2016-05-27 15:53:22]

Thanks for the review! I've researched how the scanner is used a bit more, and
I now believe that this flag needs to be set in `Scanner::Initialize`. This is
partly because that seems to be the function call that is common across the
distinct parsing operations, but it is also because `Scanner::Initialize`
invokes `Scanner::SkipWhitespace`, whose behavior is dependent on the
script/module distinction.

I've implemented this by extending `Scanner::Initialize` with a second boolean
parameter, `allow_html_comments`. This is a victim of the boolean trap, which I
don't like, and it also seems like a very ostentatious way to handle a
relatively minor distinction for the scanner. I've considered two alternatives:

1. Re-implement `Scanner::set_allow_html_comments` and invoke it immediately
   prior to `Scanner::Initialize` at all call sites. That seems cumbersome and
   fragile.
2. Extend `Scanner::Initialize` to instead accept an instance of `ParseInfo`,
   as its second parameter, letting the method evaluate `info->is_module()`.
   That seems a little less ostentatious, but it likely violates the
   "separation of concerns" principle. It also buries the lead a little.

...but I think the boolean trap is preferable to either. I am open to
suggestion, of course.

I have been unable to verify whether this satisfies the
optimization/deoptimization cases you mentioned because the mjsunit error I
reported earlier persists. I've been reading through the code a bit (and
talking with Caitlin on IRC), but I've made very little progress in my
understanding of Type Feedback Vectors and related concerns. I was hoping that
with this latest version, you might be able to suggest some ideas regarding
what is going wrong here.

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.cc#...
src/parsing/scanner.cc:458: if (c0_ == '-' && !allow_html_comments_) {
On 2016/05/27 08:08:49, vogelheim wrote:
> nitpick: It seems more logical to check this at the calling site; but that's
> admittedly a matter of taste.

Maybe, but it's good taste. I was suffering from tunnel vision when I wrote this
(still kind of intimidated by the source). I'll move this to the call site and
update the other references to occur earlier (e.g. no need to scan all the way
through "-->" before checking if these are enabled).

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2009963002/diff/20001/src/parsing/scanner.h#n...
src/parsing/scanner.h:809: // Whether this scanner should recognize HTML-like
comment tokens.
On 2016/05/27 08:08:49, vogelheim wrote:
> Please be clear on what this does.
> 
> As I read the code,
> - allow_html_comments_ == true means that an HTML comment will be reported as
> individual tokens (and thus usually as syntax error), while
> - !allow_html_comments_ means the current behaviour, namely that HTML comments
> will be recognized & skipped.
> 
> I would have assumed it to be the other way round... In either case, the
comment
> doesn't really clarify this.

Yes, you are right. I'm afraid I applied Adam's feedback thoughtlessly by simply
re-naming the variable I had initially called `module_`. Changing to
`allow_html_comments_` obviously inverts the semantic meaning of the flag, so I
need to update its usage accordingly.

[mike3, 2016-05-30 21:57:00]

I've extended JSFunction to carry a flag for whether it originated from module
code or not. This seems very heavy-handed to me, especially for such a simple
feature.

Currently, the `is_module` bit of the ParseInfo object is correctly set during
eager compilation, but when optimization occurs, there is a corresponding lazy
compilation stage where the bit is *not* set correctly.

When optimization is occuring, `Compiler::GetOptimizedCode` is invoked. This
method creates a CompilationJob for a provided JSFunction. CompilationJobs have
a CompilationInfo property named "info", and these have a ParseInfo property.

Because the Compiler only has knowledge of the JSFunction at this stage, I
initially thought the JSFunction itself had to be extended to carry the
information about whether it originated from module code or not. Now I'm not
so sure.

The ParseInfo constructor in use accepts a reference to a JSFunction, and I'm
now trying to see if it is possible to use
`function->context()->IsModuleContext()` to set `is_module` on the ParseInfo
object. No luck so far, though; I'm still not sure how/when/why the context
becomes a module context.

Adam or Daniel: do you think I'm getting warmer with either the latest patch or
the `function->context()` approach?

[mike3, 2016-06-06 22:08:33]

> Hi Mike,
> 
> ... talked to Yang; we have a plan now:
> 
> - ParseInfo, which I originally suggested, is the wrong place as it doesn't
>   actually live very long.
> - SharedFunctionInfo is close, but duplicates the info unnecessarily.
> - Yang suggests the Script object, which is much more logical since
>   being-a-module is indeed a property of the script, and not of the function.
> - ParseInfo can be constructed from JSFunction, SharedFunctionInfo, or Script,
>   where JSFunction knows its SharedFunctionInfo, and SharedFunctionInfo knows
>   its Script.
>     - In other words, the script should always be around when we need it.
>     - We should be able to always reconstruct the flag from the Script, in
>       ParseInfo constructor (except the default one)
> - is_module is passed through the API to
>   Compiler::GetSharedFunctionInfoForScript, which also constructs the Script
>   instance.
> 
> 
> So... I think I need to make you jump through one more hoop:
> 
> - Extend v8::internal::Script to store whether the script is_module or not.
> - Set that value from Compiler::GetSharedFunctionInfoForScript.
> - Ensure ParseInfo constructors will always read the value & properly
>   reconstruct is_module.  (like set_native)
> 
> The value should probably be stored in the Script::flags_ field, like
> compilation_type, hide_source, compilation_state & others.
> 
> This is sufficiently self-contained that this should IMHO be its own CL. This
> one should then be a rather straight-forward adaption.

Thanks for the feedback! I think I follow you. I may have questions when I
begin writing code, but I'll let you know if I do.

Regarding submitting as a separate CL: on their own, those changes don't seem
testable. Is there some mechanism for testing internals like that? Or am I
missing some aspect of the change you are recommending?

Finally: this change seems to be further emphasizing tension between V8's use
of the word "script" (source code that may be module code) and the
specification's use (source code that is specifically distinct from module
code). I'm not sure this should be addressed as part of this work, but I am
curious if you see it as a problem generally.

[adamk, 2016-10-12 20:41:03]

Hi Mike,

Did you still want to give a shot at fixing this bug? I think it may be simpler
than we originally thought (due to some facts about how modules are compiled).
Let me know if you're still interested, and we can chat about how to simplify
the approach (for one thing, I'm not sure we actually need your "track module
code" patch).

[mike3, 2016-10-16 16:07:29]

Hey Adam,

Sure, I'd love to follow through on this patch--thanks for checking back with
me! I've seen a lot of activity in service of bug 1569, and I was starting to
worry that my work for "track module code" [1] would be obsoleted. It sounds
like that's the case, but I understand how these things go. Should I close that
ticket?

In either case, I'm all ears for any input you have here!

[1] https://codereview.chromium.org/2065453002/

On 2016/10/12 20:41:03, adamk wrote:
> Hi Mike,
> 
> Did you still want to give a shot at fixing this bug? I think it may be
simpler
> than we originally thought (due to some facts about how modules are compiled).
> Let me know if you're still interested, and we can chat about how to simplify
> the approach (for one thing, I'm not sure we actually need your "track module
> code" patch).