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

no-extra-semi false positive with `ExportNamedDeclaration` & `ExportDefaultDeclaration` #4915

Closed
mathiasbynens opened this Issue Jan 11, 2016 · 13 comments

Comments

Projects
None yet
6 participants
@mathiasbynens
Copy link
Contributor

commented Jan 11, 2016

The semicolons in these examples are incorrectly flagged as unnecessary:

// ExportNamedDeclaration
export const x = 42;
// ExportDefaultDeclaration
export default 42;
@eslintbot

This comment has been minimized.

Copy link

commented Jan 11, 2016

@mathiasbynens Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Jan 11, 2016

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

No, those are unnecessary due to ASI. Perhaps you are thinking about the case when certain expressions follow?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

no-extra-semi is not supposed to warn on ASI locations (that's the job of the semi rule). This rule is supposed to warn on things like function declarations (which are unambiguously parsed as statements even without a semicolon in most cases), in other words things that shouldn't need a semicolon ever. I think exports require semicolons (or ASI) but I'm not sure.

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Ah, I see. So no-extra-semi just flags EmptyStatements? Yes, the export declarations will consume that semicolon, so there should be no EmptyStatements here and thus no report.

@btmills

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

@mathiasbynens what version of ESLint and what parser are you using? Both of those examples pass as test cases added off of master and v1.10.3.

@mathiasbynens

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2016

@btmills I noticed this while submitting a patch to babel/babel and triggering the CI build. They seem to be using eslint v1.8.0.

If this has been fixed in the mean time, all the better! (Sorry for not testing this myself before, I was short on time and wanted to file the issue before I forgot.)

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Confirmed previously fixed.

@nzakas nzakas closed this Jan 11, 2016

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

@nzakas I'm not seeing any unit tests in no-extra-semi around export statements. Were they covered in some other test case?

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Actually, no-extra-semi

@nzakas nzakas reopened this Jan 11, 2016

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Oops, premature Enter. no-extra-semi has never flagged this case, so I'm curious if that's not the rule in question.

@mathiasbynens please paste the actual output you're seeing (note we ask for this in the first comment).

@mathiasbynens

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2016

@nzakas: https://travis-ci.org/babel/babel/jobs/101628941#L116 (that’s all I know; didn’t think it was gonna be very helpful)

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

@mathiasbynens we only ask for information that's helpful. :) Sometimes people misread or misrepresent an actual error, that's why we ask for the complete console output. It really helps cut down on the amount of time it takes to triage a bug.

So this is definitely in no-extra-semi, but I can't reproduce. I also can't see how the rule ever would flag such code based on the implementation. Per @platinumazure's comment, I'll add some tests to be sure, but pretty certain this is most likely an issue on Babel's side (possibly with the way it's treating code before passing to ESLint).

ilyavolodin added a commit that referenced this issue Jan 12, 2016

Merge pull request #4924 from eslint/issue4915
Update: Add module tests to no-extra-semi (fixes #4915)
@mathiasbynens

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2016

More tests (tc39/ecma262#284):

However, the ExportDeclaration grammar doesn’t include the semicolon for the following cases:

  • export HoistableDeclaration, e.g. export function foo() {}
  • export ClassDeclaration, e.g. export class Bar {}
  • export default HoistableDeclaration, e.g. export default function baz() {}
  • export default ClassDeclaration, e.g. export default class Qux {}

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.