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

tools: replace custom assert.fail lint rule #12287

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@Trott
Member

Trott commented Apr 9, 2017

Replace custom lint rule for assert.fail() function signature errors
with a restricted-syntax rule.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

tools: replace custom assert.fail lint rule
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

@Trott Trott added the tools label Apr 9, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
Member

Trott commented Apr 9, 2017

@gibfahn

Couple of questions

@@ -110,6 +110,9 @@ rules:
}, {
selector: "ThrowStatement > CallExpression[callee.name=/Error$/]",
message: "Use new keyword when throwing an Error."
}, {
selector: "CallExpression[callee.object.name='assert'][callee.property.name='fail'][arguments.length=1]",

This comment has been minimized.

@gibfahn

gibfahn Apr 9, 2017

Member

Is there a valid use-case for a two argument assert.fail?

Also if assert.fail(null, null, "message") is a valid use-case, we should probably document that.

@gibfahn

gibfahn Apr 9, 2017

Member

Is there a valid use-case for a two argument assert.fail?

Also if assert.fail(null, null, "message") is a valid use-case, we should probably document that.

This comment has been minimized.

@Trott

Trott Apr 9, 2017

Member

This came out of specific errors in the code base. I'm unaware of anyone ever misusing assert.fail() with two arguments in core, and once you are checking for that, the lint message you have to supply gets a lot trickier.

I don't like ramping up non-standard lint rules to be more strict to catch hypothetical problems because as you make the rules more and more strict, you end up generating false positives. But if we were to do that, I'd say let's do it in a different pull request. This one is replacing a custom rule with identical functionality using built-in ESLint capabilities. I'd rather not alter the functionality at the same time.

assert.fail() is an unfortunate API IMO. As far as I can tell, if you supply a truthy value for the third argument, the first two arguments and the fourth argument are all ignored. (I'm not looking at the code so maybe there's an edge case in there where that's not true. I doubt it, though.) And the first two arguments don't make a lot of sense without the fourth argument.

So....the useful signatures are assert.fail(value, value, falsy, value) or assert.fail(ignored, ignored, message).

It's a mess IMO but this lint rule was designed to catch just the one exceedingly common misuse.

@Trott

Trott Apr 9, 2017

Member

This came out of specific errors in the code base. I'm unaware of anyone ever misusing assert.fail() with two arguments in core, and once you are checking for that, the lint message you have to supply gets a lot trickier.

I don't like ramping up non-standard lint rules to be more strict to catch hypothetical problems because as you make the rules more and more strict, you end up generating false positives. But if we were to do that, I'd say let's do it in a different pull request. This one is replacing a custom rule with identical functionality using built-in ESLint capabilities. I'd rather not alter the functionality at the same time.

assert.fail() is an unfortunate API IMO. As far as I can tell, if you supply a truthy value for the third argument, the first two arguments and the fourth argument are all ignored. (I'm not looking at the code so maybe there's an edge case in there where that's not true. I doubt it, though.) And the first two arguments don't make a lot of sense without the fourth argument.

So....the useful signatures are assert.fail(value, value, falsy, value) or assert.fail(ignored, ignored, message).

It's a mess IMO but this lint rule was designed to catch just the one exceedingly common misuse.

This comment has been minimized.

@Trott

Trott Apr 9, 2017

Member

By the way, PR incoming soon to make the API less confusing. :-D

@Trott

Trott Apr 9, 2017

Member

By the way, PR incoming soon to make the API less confusing. :-D

This comment has been minimized.

@gibfahn

gibfahn Apr 9, 2017

Member

Thanks for the detail, definitely not really related to this PR anyway.

Yeah, I find the idea that you have to have the third argument as falsy to actually use the fourth argument a bit hard to wrap my head around.

@gibfahn

gibfahn Apr 9, 2017

Member

Thanks for the detail, definitely not really related to this PR anyway.

Yeah, I find the idea that you have to have the third argument as falsy to actually use the fourth argument a bit hard to wrap my head around.

This comment has been minimized.

@Trott

Trott Apr 9, 2017

Member

PR to make API less confusing: #12293

@Trott

Trott Apr 9, 2017

Member

PR to make API less confusing: #12293

@@ -110,6 +110,9 @@ rules:
}, {
selector: "ThrowStatement > CallExpression[callee.name=/Error$/]",
message: "Use new keyword when throwing an Error."
}, {
selector: "CallExpression[callee.object.name='assert'][callee.property.name='fail'][arguments.length=1]",
message: "assert.fail() message should be third argument"

This comment has been minimized.

@gibfahn

gibfahn Apr 9, 2017

Member

This is really clear if you did assert.fail('message'), but not if you made another mistake (i.e. did assert.fail(false) or something. Maybe something more generic like assert.fail() should take at least three arguments?

@gibfahn

gibfahn Apr 9, 2017

Member

This is really clear if you did assert.fail('message'), but not if you made another mistake (i.e. did assert.fail(false) or something. Maybe something more generic like assert.fail() should take at least three arguments?

This comment has been minimized.

@Trott

Trott Apr 9, 2017

Member

This rule came out of fixing a bunch of actual misuse in the code base. In every single case, it was assert.fail(string) so I'm pretty OK with it the way it is. I can't even fathom what someone would mean by assert.fail(false) to be honest.

@Trott

Trott Apr 9, 2017

Member

This rule came out of fixing a bunch of actual misuse in the code base. In every single case, it was assert.fail(string) so I'm pretty OK with it the way it is. I can't even fathom what someone would mean by assert.fail(false) to be honest.

@cjihrig

cjihrig approved these changes Apr 9, 2017

@gibfahn

gibfahn approved these changes Apr 9, 2017

Trott added a commit to Trott/io.js that referenced this pull request Apr 12, 2017

tools: replace custom assert.fail lint rule
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: nodejs#12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 12, 2017

Member

Landed in b3f2e3b

Member

Trott commented Apr 12, 2017

Landed in b3f2e3b

@Trott Trott closed this Apr 12, 2017

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

gibfahn added a commit that referenced this pull request Jun 18, 2017

tools: replace custom assert.fail lint rule
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: #12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn added a commit that referenced this pull request Jun 20, 2017

tools: replace custom assert.fail lint rule
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: #12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Jul 11, 2017

tools: replace custom assert.fail lint rule
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: #12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Jul 18, 2017

Merged

v6.11.2 proposal #14356

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