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

Babel parser typescript should throw when accessors do not agree in visibility #13125

Open
1 task
JLHwung opened this issue Apr 9, 2021 · 6 comments
Open
1 task

Comments

@JLHwung
Copy link
Contributor

JLHwung commented Apr 9, 2021

Bug Report

  • I would like to work on a fix!

Current behavior

Parsed successfully.

Input Code

class C99 {
	private get Baz():number { return 0; }
	public set Baz(n:number) {} // error - accessors do not agree in visibility
}

Expected behavior

Babel parser should throw when accessors do not agree in visibility

Babel Configuration (babel.config.js, .babelrc, package.json#babel, cli command, .eslintrc)
REPL

Possible Solution

Additional context

Found this issue when reviewing #13089.

@babel-bot
Copy link
Collaborator

Hey @JLHwung! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@thorn0
Copy link
Contributor

thorn0 commented Apr 9, 2021

Should this really be considered a syntax error, not a semantic one?

@JLHwung
Copy link
Contributor Author

JLHwung commented Apr 9, 2021

I think if an error can be thrown in compile time (in contrast runtime is running the type checking), then any early error belongs to syntax error.

@fedeci
Copy link
Member

fedeci commented Apr 19, 2021

Should we check for duplicate methods too?
REPL

@JLHwung
Copy link
Contributor Author

JLHwung commented Apr 19, 2021

Duplicate public methods are allowed in ECMAScript:

class C { foo() {}; foo(v) {} }

The TS error falls to diagnostic errors IMHO.

@fedeci
Copy link
Member

fedeci commented Apr 19, 2021

I think we can check it here by looking for some other methods in classBody with kind === "get" || kind === "set" and the same name as the method to push.

pushClassMethod(
classBody: N.ClassBody,
method: N.ClassMethod,
isGenerator: boolean,
isAsync: boolean,
isConstructor: boolean,
allowsDirectSuper: boolean,
): void {
const typeParameters = this.tsTryParseTypeParameters();
if (typeParameters && isConstructor) {
this.raise(typeParameters.start, TSErrors.ConstructorHasTypeParameters);
}
// $FlowIgnore
if (method.declare && (method.kind === "get" || method.kind === "set")) {
this.raise(method.start, TSErrors.DeclareAccessor, method.kind);
}
if (typeParameters) method.typeParameters = typeParameters;
super.pushClassMethod(
classBody,
method,
isGenerator,
isAsync,
isConstructor,
allowsDirectSuper,
);
}

However it is almost useless if a user can specify multiple getters/setters with same name:

class C {
  public get foo() {}
  private get foo() {}
  public set foo(v) {}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants