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

Parse error on private identifier optional chain #35987

Conversation

joeywatts
Copy link
Contributor

Previously, this error was reported in the checker, so JS files with checkJs: false were not erroring on this invalid syntax.

Addresses the optional chaining part of #35962.

propertyAccess.name = parseRightSideOfDot(/*allowIdentifierNames*/ true, /*allowPrivateIdentifiers*/ true);
if (questionDotToken || expression.flags & NodeFlags.OptionalChain) {
propertyAccess.flags |= NodeFlags.OptionalChain;
if (isPrivateIdentifier(propertyAccess.name)) {
parseErrorAtRange(propertyAccess.name, Diagnostics.An_optional_chain_cannot_contain_private_identifiers);
propertyAccess.name = createMissingNode(SyntaxKind.Identifier, /*reportAtCurrentPosition*/ false);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to just put the privatename identifier on the node so that the language service still works

Copy link
Member

Choose a reason for hiding this comment

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

Should this be a grammar error in the binder @sandersn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was looking around and it seems like diagnostics from the binder and checker are discarded for JS files that have checkJs off.

const includeBindAndCheckDiagnostics = !isTsNoCheck && (sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX ||
sourceFile.scriptKind === ScriptKind.External || isCheckJs || sourceFile.scriptKind === ScriptKind.Deferred);

Previously, this error was reported in the checker, so JS files with
checkJs: false were not erroring on this invalid syntax.
@joeywatts joeywatts force-pushed the unchecked-js-private-name-optional-chaining branch from d6abc43 to f3cdef3 Compare January 3, 2020 22:39
@ajafff
Copy link
Contributor

ajafff commented Jan 4, 2020

@joeywatts where is this restriction documented? I searched in the proposal specs and didn't find anything useful.
I tried optional chaining with private properties in Node.js 13.5 and it worked without error (something like a?.b?.#c?.d). So either this is a bug in V8 or this restriction doesn't really exist.

@joeywatts
Copy link
Contributor Author

See the syntax for Optional Chaining in the ES2020 Specification. It only supports IdentifierName, [ Expression ], Arguments, or TemplateLiteral after ?. (so no PrivateIdentifier). The spec text for the class fields proposal does not modify optional chaining to support private identifiers.

@DanielRosenwasser DanielRosenwasser merged commit f84b2d2 into microsoft:master Jan 8, 2020
@DanielRosenwasser
Copy link
Member

I think that the only reason that it's not allowed is because the two proposals developed separately. I would be happy to bring forward a proposal to remove the restriction (because why not I guess?) unless @littledan wants to add PrivateIdentifier to OptionalChain for the class fields proposal.

@littledan
Copy link

@DanielRosenwasser I didn't communicate or justify it well, but disallowing the combination was intentional on my part. However, now TC39 has revisited this and decided that the combination should be allowed, see tc39/proposal-class-fields#301. So, this change should be revisited (reverted?).

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.

5 participants