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.

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

Normative: Extend specs to observe WebReality for HTMLCloseComment #1493

Closed
wants to merge 2 commits into from

Conversation

leobalter
Copy link
Member

@leobalter leobalter commented Mar 28, 2019

Ref #1479

Observable productions from the current WebReality:

eval('-->');
<script>--></script>

What this does not do/allow:

function fn() {-->
} // THIS REMAINS AS A SYNTAX ERROR WITH OR WITHOUT THE PR

@leobalter leobalter added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Mar 28, 2019
@ljharb ljharb requested a review from a team March 28, 2019 19:55
spec.html Outdated
@@ -40074,6 +40074,7 @@ <h2>Syntax</h2>

SingleLineHTMLCloseComment ::
LineTerminatorSequence HTMLCloseComment
[no |SourceCharacter| here] HTMLCloseComment
Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline, there’s no precedent for something like this in the spec. We’d need to find a better way to express this grammar. @waldemarhorwat can probably help.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly the intent is to allow these comments in exactly the place that the hashbang proposal allows #!. If that's so, I think the easiest thing to do is to extend the HashbangComment production it adds so that it also matches --> SingleLineCommentChars_opt in the Script goal (and then possibly rename the production to InitialComment or something).

Copy link
Member Author

@leobalter leobalter Apr 1, 2019

Choose a reason for hiding this comment

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

@bakkot this has been considered but it's not really possible.

First of all, the hashbang proposal has a grammar and an exception (only allowed at the start of Script or Module). For the current proposal, we don't have a grammar without the preceding requirements. I could possibly have a simple HTMLCloseComment production and say it's only possible at the very start. The problem here is that I can't limit this to the first sequence of "the source text of an ECMAScript Script or Module" only, we'd need to go beyond Script and Module goals so we make it a fit for eval and cases like Function.

Another restriction goes to only specifying --> directly. We need to have it extended to all the productions from HTMLCloseComment including the /* |line terminator| */ -->

The intent is to have the specs for HTMLCloseComment matching web reality. So that should extend to the first sequence of source texts, eval('-->'), Function('-->'), etc, including the productions with the multi line comments.

I'd be happy with other suggestions for the grammar.

Copy link
Collaborator

@jmdyck jmdyck Apr 1, 2019

Choose a reason for hiding this comment

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

I could possibly have a simple HTMLCloseComment production

I don't understand why you'd need to make any change to the HTMLCloseComment production.

and say it's only possible at the very start. The problem here is that I can't limit this to the first sequence of "the source text of an ECMAScript Script or Module" only,

Are you saying it would be insufficient to limit it to the start of Script and Module?

we'd need to go being Script and Module goals so we make it a fit for eval and cases like Function.

For "being", do you mean "beyond"? For eval, the goal symbol is Script, so that's not a problem. But for Function, the body-text goal symbol is FunctionBody (and similarly for other function-constructors), so that's the problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why you'd need to make any change to the HTMLCloseComment production.

My goal is to reflect web reality

Are you saying it would be insufficient to limit it to the start of Script and Module?

yes, this should include parts where SourceText is parsed without these goals.

For "being", do you mean "beyond"?

yes, typo.

For eval, the goal symbol is Script, so that's not a problem. But for Function, the body-text goal symbol is FunctionBody or GeneratorBody, etc, so that's the problem?

If that unblocks the suggestion, I'd like to know what's the suggested production.

So far I'd like to unblock a grammar that reflects the current productions:

-->
/**/ -->
/**/  /* *//*foo*/ -->
/*
*/ /**/ -->

All of them being valid productions as the first sequence of any sourcetext:

  • file (script src)
  • script tag code (no src)
  • eval
  • Function and derivatives (AsyncFunction, GeneratorFunction, etc)
  • possibly non-ecmascript parsing like setTimeout('-->\nconsole.log(42)', 10);

Copy link
Member Author

Choose a reason for hiding this comment

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

If I read the room correctly in the last meeting, I don't believe browsers will change any implementation matching what I'm calling as web reality here. Unlikely is not a guarantee and I honestly believe this is a case to be added to Annex B.

The main complexity here is just how we wanna solve this, not what. I'd recommend we focus on the spec fix otherwise we make this more contentious.

Choose a reason for hiding this comment

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

My recommendation would be similar to @gibson042's, but I'd do it one level down:

SingleLineHTMLCloseComment ::
  LineTerminatorSequence HTMLCloseComment
  HTMLCloseComment (only if no prior input elements have been parsed from the source text being parsed)

Here the intent is that the second expansion of SingleLineHTMLCloseComment can only be done if the SingleLineHTMLCloseComment would be the very first input element produced in the parse of some source text. An illustrative example would be useful to get the idea across.

You'll want to check whether this is actually universally true in all contexts, or, if not, whether we want this to be universally true in all contexts. I think it is true in all of the contexts @leobalter mentioned above. I'm not sure about the context in which the Function parameters are parsed (and that one seems to have a bug in at least implementation encountering an error if function parameters include //).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bakkot's suggestion would allow them at the start of Script and Module (...)

In hashbang we have a grammar production with an extra text setting the restriction.

More precisely, the hashbang proposal adds another InputElement* nonterminal+production and (like all other such nonterminals) specifies when it should be used as the goal symbol for the lexical parser.

How we would set this for HTMLCloseComment?

We could add HTMLCloseComment as another RHS of InputElementHashbangOrRegExp. (This is equivalent to @bakkot's suggestion, I think.)

I'm not objecting, what I am saying we need to end up with a new production anyway.

I'm not sure what you mean by that, since this PR doesn't create a new production. (It just adds a RHS to the SingleLineHTMLCloseComment production.)

Copy link
Member Author

Choose a reason for hiding this comment

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

You'll want to check whether this is actually universally true in all contexts, or, if not, whether we want this to be universally true in all contexts. I think it is true in all of the contexts @leobalter mentioned above

Agreed, and I'll change this PR to match the suggestion from @waldemarhorwat, which I believe it's a good fit for the proposed change, considering where we place the production and how we restrict the production as the first part of the source text.

@jmdyck I'd rather not mix it with the hashbang, these will leave some parts open with unnecessary complexity. The suggestion I first proposed, along with @gibson042's and @waldemarhorwat are, IMO, effective and without much complexity. It's something I'd like to see fixed and never have to touch anymore unless we eventually plan to remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmdyck I'd rather not mix it with the hashbang, these will leave some parts open with unnecessary complexity.

Okay, but that's a very imprecise statement. (Which parts of what open to what?) I've been trying to nail down your objection/qualms/concerns with the mixing-with-hashbang suggestion, and I thought I'd figured it out, but your responses since then have left me puzzled and uncertain. Please confirm if I've correctly characterized the problem with @bakkot's suggestion.

@leobalter leobalter added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Jun 19, 2020
@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@leobalter
Copy link
Member Author

Closing this in favor of #3244

@leobalter leobalter closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants