Add prefer-default-parameters rule

[medusalix]

Some notes: I've tried to keep the rule simple by only reporting literal assignments (see this comment). Combining parameters with e.g. a return value of another function or variable might be desired in some cases and should in my opinion not be reported. I've had to work around arrow functions with only one parameter and no parentheses (foo => {}) to make sure the default parameter gets inserted correctly by including the missing parentheses.

Fixes #29

---

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

• #29: Prefer default parameters

---

IssueHunt has been backed by the following sponsors. Become a sponsor

[fisker]

Sorry for the delay.

Very good format and clear logic. ❤️

Can you update code with code in function body to multiple lines? Should easier to read.

[fisker]

One more thing, since the fix is not safe , we should use suggestion api, but don't worry, we can easily convert it. example

[medusalix]

I think we only need check the last parameter.

We still need to check all of them to see if it's actually one of the parameters, but I chose to skip the fixing if it's not the last parameter.

Can you update code with code in function body to multiple lines?

Done. I kept the formatting of some of the tests to make sure that the fixer correctly formats one-line functions.

One more thing, since the fix is not safe , we should use suggestion api.

I've noticed that there are more cases where a fix would actually break the code, so I chose to use the suggestions API exclusively:

function abc(foo) {
	foo += 'bar';
	foo = foo || 'bar';
}

[fisker]

We still need to check all of them to see if it's actually one of the parameters, but I chose to skip the fixing if it's not the last parameter.

I think we can simple this logic, only report one problem each time on one function. but we need consider following cases

function foo(a, b, c=1){} check b not c,
function foo(a, b, ...c){} also check b. Update: sorry, should ignore both c and b

[fisker]

Another edge case, since the assignment can be used as value.

function foo(a){
 doSomething(
   a = a || 1
)

doAnotherThing(a)
}

or

function foo(a){
 const b = a = a || 1
}

So, we should choose leave a there or ignore those case.

My suggestion, alway leave a there unless parent is ExpressionStatement.

[fisker]

@medusalix What do you think the suggestion above?

[medusalix]

@fisker Sorry for the late reply, I'm pretty busy right now, I'll take a look at your suggestions when I get the time.

[medusalix]

Another edge case, since the assignment can be used as value.

Yeah, that's already checked by the selector: ExpressionStatement[expression.type="AssignmentExpression"]. I've tested both of your examples.

[fisker]

1. Please fix tests I added.
2. ?? operator is supported by eslint now, please check it too.

[medusalix]

I moved one of your test cases because it can actually be fixed. References are now checked using findVariable.

[fisker]

I moved one of your test cases because it can actually be fixed. References are now checked using findVariable.

Good, you checked the foo

[fisker]

I think the problem we can't resolve here is

function abc(foo) {
	foo = foo || 'bar';
	console.log(foo);
}

VS

function abc(foo) {
	console.log(foo);
	foo = foo || 'bar';
}

function abc(foo) {
	useFoo();
	foo = foo || 'bar';

	function useFoo() {
		console.log(foo);
	}
}

VS

function abc(foo) {
	notUseFoo();
	foo = foo || 'bar';

	function notUseFoo() {
	}
}

[sindresorhus]

Bump

[medusalix]

I think I've found a pretty good solution for the first point @fisker outlined above:

1. If the parameter is being reassigned, make sure that there are no references to the parameter before the default-assignment:

foo = foo || 123; // Reference 1 and 2
console.log(foo); // Reference 3
// First reference is the default-assignment; invalid, can be fixed.

console.log(foo); // Reference 1
foo = foo || 123; // Reference 2 and 3
// First reference is the console.log statement; valid.

2. If the parameter is default-assigned to a new variable, make sure that there are no additional references to the old parameter at all:

const bar = foo || 'bar'; // Reference 1
console.log(foo, bar);    // Reference 2
// More than one reference; valid.

const bar = foo || 'bar'; // Reference 1
console.log(bar);
// Only one reference; invalid, can be fixed.

[medusalix]

I've done some tests with the hasSideEffect function. I believe implementing side-effect checking defeats the purpose of this rule. Consider the following example:

function abc(foo) {
	let bar = true;
	bar = false;

	foo = foo || 123;
	console.log(foo);
}

This example would be ignored by the rule because hasSideEffect returns true for the bar = false statement, even though it doesn't have any effects on the foo parameter at all.

@fisker I think ignoring cases where there is any CallExpression before the default-assignment would be a better solution. Other side-effect causing statements would be detected by the reference checking. What do you think?

[fisker]

I'm running out of ideas. 😄

[medusalix]

@fisker I've implemented the CallExpression check I described in my last comment and added lots of tests for it.

[sindresorhus]

// @fisker

[fisker]

I forgot the review progress, I read all comments from beginning again, seems good enough, we can still improve it if any issue found.