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

[WIP] Early Errors > Grammar #1343 #1651

Closed
wants to merge 1 commit into from

Conversation

jbhoosreddy
Copy link
Contributor

Summary of changes:

  • No space permitted between # and name (either in definition or usage) 🏃❓
  • Space permitted between receiver and private method access
  • "shorthand" not permitted -- a receiver is required
  • Private methods not permitted in object literals
  • Super private access (super.#x()) is not permitted
  • Private methods may not be deleted

Legend

  • 🏃 means it's in progress
  • ❓ needinfo

@jbhoosreddy
Copy link
Contributor Author

Question re/ early errors: When I try to write test cases for early errors, I run into babel parse errors which bail test262-harness. How can this be fixed? cc @littledan or others maintainers

@ljharb ljharb changed the title [WIP] Early Errors > Grammer #1343 [WIP] Early Errors > Grammar #1343 Jul 26, 2018
@mkubilayk
Copy link
Contributor

I'm also working on a set of early errors for private methods and hitting the same issue as @jbhoosreddy with the babel parser.

@mkubilayk
Copy link
Contributor

I suspect test harness uses a version of the parser that doesn't support private methods syntax properly.

class C {
    #foo() {}
}

I was able to parse the code above using @babel/parser@7.0.0-beta.55. It didn't work with babylon@6.18.0, which is the version installed for test262-harness.

const code = `
class C {
    #foo() {}
}
`;

// parses
const ast = require('@babel/parser').parse(code, {
    sourceType: 'module',
    plugins: [
        'classPrivateMethods'
    ]
});

// throws
const babylonAst = require('babylon').parse(code, {
    sourceType: 'module',
    plugins: [
        'classPrivateMethods'
    ]
});

It looks like classPrivateMethods is not a plugin available in here.

@rwaldron
Copy link
Contributor

The Babel option test262-harness is basically the worst feature ever added to that tool and shouldn't be used at all.

@rwaldron
Copy link
Contributor

Also, we have tons of Class private field tests that need to be restored from git history.

@jbhoosreddy
Copy link
Contributor Author

@rwaldron does test262 not have a way to skip tests? I'm curious why some tests need to be restored from git history? Maybe @mkubilayk and I can PR to add that feature also.

@littledan
Copy link
Member

Great to see more progress on class field tests both in this PR as well as in #1660 . I guess once #1660 lands, we can rebase these fixes on top if there is any more work to do.

I agree with the statements in the checkboxes at the top of this PR; could you explain more about what info you need about the first item, @jbhoosreddy ?

For tests against early errors with Babel, it's probably better to use @jugglinmike 's direct integration of Babylon with test262 at babel/babylon#654 . Maybe this will be able to test the private methods grammar even if the transforms aren't ready yet (see babel/proposals#22 -- the transform is listed as "in progress", even for Babel 7).

Thanks for your fixes to test262-harness's --babelPresets flag, @leobalter and @rwaldron . It definitely wasn't perfect on the first try, and has room for improvement still. I recommended that @jbhoosreddy develop tests for private methods using this mode, but imagined that improving the infrastructure would be part of the project as well. If this recommendation was a poor one, let's figure out the right development mode is. I saw that you marked this flag as deprecated, @rwaldron . I'm wondering, what would you recommend for running test262 tests against Babel transforms?

@rwaldron
Copy link
Contributor

rwaldron commented Aug 6, 2018

I'm curious why some tests need to be restored from git history?

Because they were removed when the spec was split and part of the features were demoted to State 2.

@rwaldron
Copy link
Contributor

rwaldron commented Aug 6, 2018

I suspect test harness uses a version of the parser that doesn't support private methods syntax properly.

I have no problem testing new and existing class public and private fields and methods tests against any engine using test262-harness. I'm not using the babel feature because it's irrelevant when writing tests for specification conformance. It would be useful if I were testing a babel transform against test262 conformance tests.

I'm wondering, what would you recommend for running test262 tests against Babel transforms?

I'd recommend using Babel's test runner for testing Babel transforms: https://github.com/babel/babel/blob/master/Makefile#L99-L100

Test262 tests should be written against the spec, not a specific runtime or transformer. The runtimes and transformers should implement against the spec and the tests.

@littledan
Copy link
Member

I'd recommend using Babel's test runner for testing Babel transforms: https://github.com/babel/babel/blob/master/Makefile#L99-L100

Does this test the transforms, or just the parser?

@rwaldron
Copy link
Contributor

rwaldron commented Aug 6, 2018

@jbhoosreddy I didn't previously recognize that this was meant to be focused on private methods—I thought it was for all new class field and class private (instance and static) features. So far we've been focused only on these:

Which means there is presently no coverage for:

@rwaldron
Copy link
Contributor

rwaldron commented Aug 8, 2018

@jbhoosreddy please add class-methods-private to features.txt and make sure that the feature appears in all relevant tests 👍

@jbhoosreddy
Copy link
Contributor Author

I'm taking this up in #1709

@jbhoosreddy jbhoosreddy closed this Sep 5, 2018
@jbhoosreddy jbhoosreddy deleted the early-errors-grammar branch January 27, 2020 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants