test: Private methods: "shorthand" not permitted -- a receiver is required

[jbhoosreddy]

Tracking issue: #1343

Summary of changes:

1. Add tests for Early SyntaxError if IdentifierName is accessed without receiver.

[jbhoosreddy]

@leobalter @rwaldron for review

[leobalter]

I don't have time for a full review today but one thing that might be interesting here is to limit the reference only to the get operation. #m should be enough.

[leobalter]

like the other case, #m or maybe cloning this case file to a second one with #m = 1 should also be nice.

[leobalter]

none would be evaluated in runtime but should not hurt adding both cases.

[leobalter]

the simple get operation should apply for actual methods, no need to use the call parentheses on them. #m should be enough to trigger the syntaxerror.

[leobalter]

this should be #m to match the pre-existing method.

[jbhoosreddy]

@leobalter for additional review.

[leobalter]

These tests are also part of the edgy cases were we start covering non specified syntax. Similar to #1760.

The narrative for these tests is not really using a "receiver" but making sure that syntax is not described elsewhere.

MemberExpression[Yield, Await] :
  MemberExpression[?Yield, ?Await] . PrivateName

CallExpression[Yield, Await] :
  CallExpression[?Yield, ?Await] . PrivateName

ClassElementName[Yield, Await] :
  PrivateName

There isn't any production of an "isolated" PrivateName reference described beyond the ClassElementNames

---

For this specific case, I think it's fine to add the test cases in this PR, but my only note about this is to avoid rabbit holes where we might eventually add coverage for every non described syntax grammar.

I think for the tests we only need to add the productions above in the info part to show similarities but also inform these are some edge cases where the syntax might misguide to that usage.

I have to say the phrase "Early SyntaxError if IdentifierName is accessed without receiver." distracted me for a while, as if it's something related to a Static Semantics topic in the proposed specs.

---

WDYT, @jbhoosreddy?

[jbhoosreddy]

If this PR is very close to edge cases we don't want to test, I don't mind closing this PR also.

But if we want to keep it I can update the info.

Meanwhile, @leobalter and @rwaldron, do you mind taking a look at #1343 which we are following to generate these tests, maybe we should go through them and see if some of those tests are not valid/appropriate?

[leobalter]

That list is not a spec text. It's not something that we want or not to have specific tests for, the problem is the support to verify the test.

If you go to the proposed specs (class fields and private methods) there is no proposed syntax grammar or static semantics describing these cases. So the nature of this test is literally like writing tests for checking a keyword like if or function are not valid if not followed by the described grammar of function declarations/expressions or if statements. That would allow me to generate a gazillion test files to check everything that is not described.

Even saying something is a edgy case is subjective and I'm using my bias to determine that. One could write Python or Ruby syntax and say it's a syntax error in JS, e.g.

As I told before, this is rabbit hole and if we don't find a fair limit, we could end writing a lot of stuff just because it's not described in the spec.

It adds more value we if first guarantee we cover all the described grammar and static semantics or anything else existing from the specs. The idea is to prove the spec if valid, otherwise what we are writing tests for?

---

I just wanted to add I'm not giving an angry reply or anything, but I the remote conversation is limited to tell I'm proposing a pragmatic discussion over what we define as coverage.

[rwaldron]

That list is not a spec text.

Thanks @leobalter—it's super important to internalize. That list should be scrutinized and validated against the actual spec as authors write tests.

[rwaldron]

Just noticed this, "Updated Productions" is something that (sometimes) appears in proposal specs, but isn't part of the actual specification.

[rwaldron]

This description language is very similar to normative specification prose used to describe "Static Semantics: Early Errors"—however, I cannot find any such early error with this or similar language.

[littledan]

Permitting private names in objects is a feature which was considered and delayed for a future proposal, e.g., see tc39/proposal-decorators@5aa8c82 .

I don't understand the goal of only including tests which are directly listed in the specification. Is the motivation maintainability? Is it documented how the correspondence to spec text has helped maintainability in the past? I wonder if it would be useful to expand the goal to also include regression tests from implementations (on the hypothesis that multiple implementations might share the same bugs), tests to achieve code coverage in implementations (similarly, they may take similar strategies), and tests that verify that previously considered specification alternatives were not selected (like this one).

[leobalter]

I don't understand the goal of only including tests which are directly listed in the specification. Is the motivation maintainability?

We tried several times to explain this over several issues and PRs, I'm not sure how I can explain this better.

If we open a path for things that are not listed in the specification, the possibility to include things is similar to infinity. Choosing what is good to add is always subjective. We would add tests for things "not in the specs", and this can reach to anything.

I wonder if it's worth having a test for, it should be worth to add at least non-normative notes in the specs about it, so we can have something to make a reference to. It could say something like: "the current syntax grammar does not allow a private name reference without a receiver". WDYT, @littledan?

I'd feel way more comfortable having a test I can have some reference, not just possible but uncertain intuition.

I wonder if it would be useful to expand the goal to also include regression tests from implementations

I believe that should be another topic but regressions are welcome if they match Test262 goals as well.

tests to achieve code coverage in implementations

We cover the specs, I'm not sure what are you suggesting here.

tests that verify that previously considered specification alternatives were not selected

Do you mean everyone considerations? We can add such cases like replace everything with the @ sigils, or private identifierName, and here we just start.

[ljharb]

It seems reasonable to always have tests that prohibit earlier obsolete iterations of a proposal - shorthand was once part of it, so someone may have implemented it.

[leobalter]

It seems reasonable to always have tests that prohibit earlier obsolete iterations of a proposal - shorthand was once part of it, so someone may have implemented it.

I need an answer to my question if we could have a note in the spec (currently the proposed one) saying a possible intuitive production is not allowed. If it's worth to have a test, it should be worth to have a note too.

[rwaldron]

It seems reasonable to always have tests that prohibit earlier obsolete iterations of a proposal

Perhaps, but I would draw the line at "things that were there at Stage 3", and I would enforce a marker of some sort that allowed us to quickly dump those tests once the feature was in Stage 4/spec draft, as they might conflict with future Stage 3 features.

Furthermore, including "regression" tests for things that don't make it to Stage 3 or 4, Test262 may impose erroneous and artificial limitations on an engine that may want to extend the syntax or API surface in novel, experimental ways (as long as an extension does not violate the Forbidden Extensions). So, it may be that some engine wants to implement 100% conformant ECMA-262, and then layer on features that are relevant to their goals—which might include allowing some kind of syntax like ({ #foo })—there is nothing in the spec that prevents them from doing so. If private method syntax is to be disallowed from object literal syntax, then perhaps there should be addition to the Forbidden Extensions.

[ljharb]

I'm confused why we'd need a note saying that "not allowed" grammar is disallowed, that seems redundant. In the same way as we'd want tests for specific bugs that were shipped - even if the spec never allowed it - I don't understand why we wouldn't want tests to prevent a "highly likely to be built by mistake" grammar from being shipped.

[rwaldron]

I don't understand why we wouldn't want tests to prevent a "highly likely to be built by mistake" grammar from being shipped.

If it's really that likely, it should be an addition to Forbidden Extensions. In which case, there would be a conformance test to write.

[ljharb]

I don't think it's likely anyone reading the actual spec will implement it, I think it's likely that it already exists in an implementation that was based on an earlier phase of the proposal.

I really don't understand why the slippery slope argument applies here. With this one thing it makes sense to me to have an explicit test preventing it - even if only to reserve the space for future expansion - regardless of whether the spec has additional text forbidding it.

[rwaldron]

I think it's likely that it already exists in an implementation that was based on an earlier phase of the proposal.

That's easily verifiable:

$ eshost -e "class C { #m = 1; get() { return #m; }}"
#### Chakra
SyntaxError: Invalid character

#### V8 --harmony
SyntaxError: Unexpected identifier

#### SpiderMonkey
SyntaxError: illegal character:

#### V8 --harmony_private_fields
SyntaxError: Unexpected identifier

#### V8
SyntaxError: Invalid or unexpected token

#### JavaScriptCore
SyntaxError: Invalid character: '#'

#### jsc
SyntaxError: Invalid character: '#'

#### Chakra-Test
SyntaxError: Invalid character

#### XS
SyntaxError: (host): invalid script

More detailed:

$ v8 --harmony-private-fields
V8 version 7.1.182
d8> class C { #m = 1; get() { return #m; }}
(d8):1: SyntaxError: Unexpected identifier
class C { #m = 1; get() { return #m; }}
                                 ^^
SyntaxError: Unexpected identifier

d8>

$ spidermonkey
js> class C { #m = 1; get() { return #m; }}
typein:1:10 SyntaxError: illegal character:
typein:1:10 class C { #m = 1; get() { return #m; }}
typein:1:10 ..........^
js>

$ javascriptcore
>>> class C { #m = 1; get() { return #m; }}
Invalid character: '#':1
>>>

$ xs -e "class C { #m = 1; get() { return #m; }}"
SyntaxError: evalScript: invalid script

[ljharb]

There's more implementations than just those 5, I'm sure.

Either way, I understand why we wouldn't want negative tests for a growing pile of things. I don't understand why this specific syntax which was a contentious part of the proposal, highly desired by some, and ultimately removed, wouldn't warrant a single test that ensures that it's not implemented.

[leobalter]

If I remember correctly on everything I learned from @allenwb, the tests can't limit the specs or enforce things not specified.

If the concern is to limit or disallow a syntax extension, we have a proper spec topic to add this, but it could also be done in other ways in the specs, not omitting a thing and expecting an implementation to conform with blank specs.

Test262 is a reflect of Ecmascript, and don't mandate anything on implementations. I insist on the fact that if it's not worth adding anything to the spec, it's not valuable to have tests for in this test suite. Maybe it's ok for each implementation case.

[leobalter]

Also...

A conforming implementation of ECMAScript may support program and regular expression syntax not described in this specification.

[ljharb]

Then I guess we should add something to the spec, because the entire point of removing shorthand now was to allow it to be useful for something later, and if the tests for private fields aren't going to ensure that, then they're not serving their full purpose imo.

I suppose this PR should probably be closed until something in the private fields spec, or 262, explicitly prohibits the shorthand; I've filed tc39/proposal-class-fields#137 for that.

[rwaldron]

because the entire point of removing shorthand now was to allow it to be useful for something later, and if the tests aren't going to ensure that, then they're not serving their purpose imo.

I disagree with this reasoning, but I'm open to discussing this with you in good faith: if, without a specified forbidden extension, Test262 includes a test that prohibits this syntax, what do we do when a not-yet-known feature wants to use this syntax, or the feature in question is restored in a future version of specification?

[ljharb]

@rwaldron we'd remove the test, and replace it with a test for the updated functionality.

[rwaldron]

@ljharb

we'd remove the test, and replace it with a test for the updated functionality.

Ok, that would be my preferred course of action as well. Next question: are you on board with Test262 imposing a "marker", in the form of a frontmatter property, that indicates "this test can be considered optional, and may be deleted in future without a conformance penalty"?

[ljharb]

Sure, that sounds fine - i don’t see a problem with an implementation willfully opting out of tests, but I’d prefer having the defaults prevent future constraints on language design.

[rwaldron]

but I’d prefer having the defaults prevent future constraints on language design.

Well, an explicit Forbidden Extension would guarantee that, and then we'd be able to write non-optional tests.

[littledan]

I have no particular problem with adding notes to the specification, but I still am confused by the motivation for the policy in general. (Yes, we have discussed this before, but I still don't think we've come to a shared conclusion.)

If I remember correctly on everything I learned from @allenwb, the tests can't limit the specs or enforce things not specified.

You're right--there is a paragraph in the specification that says this, but I think we should revisit it. We know from experience that actually taking advantage of implementation-specific extensions is bad for compatibility; test262 has been effective in improving compatibility among implementations, and if more tests can be shared, it will be even more effective. At the same time, I'm not aware of people taking advantage of these extensions (unless you count TypeScript or proposals to add to JS, but IMO these are different sorts of things). Of course this will need to be done in TC39, not this repository; apologies for my delay in raising the issue in the proper way. Anyway, I'm happy that test262 makes pragmatic adjustments to expand test coverage to reality, e.g., in adding the locales metadata.

tests to achieve code coverage in implementations

We cover the specs, I'm not sure what are you suggesting here.

I'm suggesting that, if an implementation has tests that cover its code, even if it's somewhat redundant with tests for the same line of spec, that it be shared in some place with other implementations. (I can understand if you disagree, but am I being unclear?)

[littledan]

Ok, that would be my preferred course of action as well. Next question: are you on board with Test262 imposing a "marker", in the form of a frontmatter property, that indicates "this test can be considered optional, and may be deleted in future without a conformance penalty"?

This marker sounds like a good compromise. You might call it something like, "non-forbidden extension". It would be analogous to the locales marker.

[littledan]

tests that verify that previously considered specification alternatives were not selected

Do you mean everyone considerations? We can add such cases like replace everything with the @ sigils, or private identifierName, and here we just start.

You're right that there's potentially an endless number of tests, and it's not reasonable to think about "full coverage" when these are in scope. I'd suggest a pragmatic approach: Bocoup, when trying to fill out test coverage, can go for coverage of each line of the spec test. Other people, when writing tests, can include other things that they find important to test (e.g., regression tests, non-chosen alternatives that they fear might be accidentally supported, etc) which go beyond that initial coverage priority. All correct tests which are not clear 100% duplicates can be shared by test users.

[jbhoosreddy]

Thanks @littledan, @ljharb, @rwaldron and @leobalter.

To get ahead on this issue, and get a consensus on this, should we:

1. Add non-normative notes about these forms not explicitly specified

• (in which case, I'll focus on getting those added for forms raised in this PR and test: Add test for private method in object literal #1760) OR

2. Add a marker to make tests optional

[leobalter]

Reviewing this, I'd like to share my personal opinion as in the current day:

The specs text prevents Test262 from limiting implementations beyond specified, unless specified - sorry for the redundancy - in the Forbidden Extensions section.

I'm against creating multiple goals for this project, and that includes creating optional tests. The metadata should be concise and cause the least possible burden on anyone consuming the tests on this project. Creating more flags to limit the scope of test executions increases the complexity for running the tests from this project. I'm in favor of simplifying the most we can on the metadata, and that should also improve the entrance level for contributions. The single goal of this project should be spec conformance, on what matters to the specs.

My only chance to agree with tests limiting implementations now remains on adding things to the Forbidden Extensions topic or through any other normative text. I stand by the specs as they are provided. I also believe this helps clarification on any plans for the future on the work being proposed. It helps the specs ecosystem and everyone can benefit from that. If the proposal champions are opposed to add these as norms, but only informal convention, I don't see a reason why the tests are so important. Please, remember this project respects formalities, it's not a pet project, otherwise I would be able to relax a lot rules I personally don't agree to, e.g. the copyright headers.

A conforming implementation of ECMAScript may support program and regular expression syntax not described in this specification.

This quote above is normative spec text.

I'll close this PR for now to avoid uncertainty for the authors contributing to this project. I appreciate a lot the work being done, and I'd be happy to reopen if we have changes in the proposed specs.

[littledan]

What's the rationale against @rwaldron 's suggestion at #1782 (comment) ?

[leobalter]

What's the rationale against @rwaldron 's suggestion at #1782 (comment) ?

I already answered that:

I'm against creating multiple goals for this project, and that includes creating optional tests. The metadata should be concise and cause the least possible burden on anyone consuming the tests on this project. Creating more flags to limit the scope of test executions increases the complexity for running the tests from this project. I'm in favor of simplifying the most we can on the metadata, and that should also improve the entrance level for contributions. The single goal of this project should be spec conformance, on what matters to the specs.