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

RuleTester: Cannot test failing suggestion fixer #13723

Closed
DMartens opened this issue Sep 30, 2020 · 7 comments
Closed

RuleTester: Cannot test failing suggestion fixer #13723

DMartens opened this issue Sep 30, 2020 · 7 comments

Comments

@DMartens
Copy link

@DMartens DMartens commented Sep 30, 2020

Tell us about your environment
Environment Info:

Node version: v14.12.0
npm version: v6.14.2
Local ESLint version: v7.10.0 (Currently used)
Global ESLint version: Not found

What did you do?
I am currently creating a custom rule with a suggestion fixer.
Sometimes the suggestion is not applicable and I return null directly:

{
	...
	suggestions: [{
		desc: '...',
		fix(fixer) {
			if (cannotFix) {
				return null;
			}
		},
	}]

Then if I test this in the rule tester like this:

{
	errors: [{
		suggestions: [{ output: '...' }]
}

I get the error: TypeError: Cannot read property 'range' of null.
I investigated and the basic flow is:

  1. Report Translator adds all existing suggestions even if their fix is null
  2. Rule Tester just passes the suggestion along without checking
  3. Source Code Fixer cannot access the range property as the value is null

I expect returning null from a fixer to be supported behaviour because it works if I convert the suggestion to a fix.

Are you willing to submit a pull request to fix this bug?
Yes

@DMartens DMartens changed the title RuleTester: RuleTester: Cannot test failing suggestion fixer Sep 30, 2020
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 30, 2020

Thanks for the issue!

By the LintMessage specification, fix: EditInfo is mandatory in suggestions, meaning that suggestion must provide a fix.

ReportTranslator already throws an error if the suggestion object returned from the rule doesn't have fix() method, but it doesn't check the produced value.

A bug fix would be to update ReportTranslator to throw if fix() returns null.

The rule should check upfront whether a fix is possible, and then just not return the whole suggestion object if the fix cannot be applied. In tests, asserting that there were no suggestions can be done with suggestions: [] (or any falsy value instead of []).

However, returning null from a fix() method is well-known and widely used in core rules (although it seems to be undocumented?), so I think it would be reasonable to support that in suggestions, since it's a convenient way to put all the fixing logic in one place.

If fix() returns null, ReportTranslator could remove the whole suggestion from the array of suggestions.

@DMartens just to verify, is that the expected behavior?

@DMartens
Copy link
Author

@DMartens DMartens commented Oct 1, 2020

I would not remove the suggestion from the suggestions, as the user might think that the suggestion is available but it is not presented to them.
I filed this as an error for the rule tester as I thought it should be fixed there, by filtering suggestions with no fix. Also it would be great that if a rule developer can check if the fix failed by checking that output: null just like with the fix property.
In other words I think suggestion fixer and normal fixer should be handled the exactly same.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 1, 2020

RuleTester runs linter and checks the final LintMessage objects, not objects passed to context.report().

In this case, we have an invalid LintMessage because it has fix: null in a suggestion object,

fix is mandatory in suggestion objects per our Node.js API documentation, and types.js:

* @property {Array<{desc?: string, messageId?: string, fix: {range: [number, number], text: string}}>} [suggestions] Information for suggestions.

VSCode integration also expects fix and crashes here if it is null or undefined.

RuleTester shouldn't get an invalid LintMessage at that point. In particular, linting shouldn't produce a suggestion that doesn't include a fix. A correct test case would be to check that there are no suggestions, not that there is one suggestion with output: null.

So, this seems to be something that should be fixed in Linter (ReportTranslator), rather than RuleTester.

I think we should update ReportTranslator to either throw right away if fix() returns null, or automatically remove the suggestion in that case (which might be more convenient for rule developers).

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 13, 2020

I'd propose that we update report translator to remove the whole suggestion from the array of suggestions if the suggestion's fix() function returned null (or an empty array/sequence), and I think this change can be done in a minor version.

That way, end-users shouldn't notice a difference, aside from not seeing crashes in the editor due to broken suggestions anymore.

Rule developers will have an additional place where it can be decided to not provide the suggestion: the fix() function.

I believe the only downside is that some tests in plugins might have to be updated after this change, if such tests exist (it's possible, since the output field isn't mandatory in suggestion tests, so the original issue may have been unnoticed).

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 13, 2020

@mdjermanovic I’d much rather update RuleTester to throw when a suggestion doesn’t produce a fix, as I think that’s an error condition that may be easy to miss. I can’t think of a case where it’s not possible to know if a suggestion can be applied before creating the suggestion.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 14, 2020

I’d much rather update RuleTester to throw when a suggestion doesn’t produce a fix, as I think that’s an error condition that may be easy to miss.

RuleTester already throws if the actual number of suggestions doesn't match the expected number.

If we remove the whole suggestion when fix() returns null, test case that triggered that code path will fail on comparing the number of produced suggestions with the number of suggestions expected in the test case.

Assuming that test cases for those rules have the suggestions property, they should be able to catch cases where fix() mistakenly didn't return anything.

I can’t think of a case where it’s not possible to know if a suggestion can be applied before creating the suggestion.

We often use return null to abort fixing based on some data and tokens that matter only for the fixing itself:

fix(fixer) {
if (canBeFixed(left) && canBeFixed(expr.left)) {
const equalsToken = getOperatorToken(node);
const operatorToken = getOperatorToken(expr);
const leftText = sourceCode.getText().slice(node.range[0], equalsToken.range[0]);
const rightText = sourceCode.getText().slice(operatorToken.range[1], node.right.range[1]);
// Check for comments that would be removed.
if (sourceCode.commentsExistBetween(equalsToken, operatorToken)) {
return null;
}
return fixer.replaceText(node, `${leftText}${expr.operator}=${rightText}`);
}
return null;
}

It's always possible to extract the first part of the fix() function and then conditionally add the fix property with a function that contains the rest of the code, but that's much more code than just return null from the fix() function.

If we don't allow return null from suggestion fix functions, all the code that checks if there is something that might interfere with fixing would have to be placed before context.report() and then conditionally add the suggestion.

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 16, 2020

Ah ok, I was unaware that ESLint itself was returning null in some cases. While I still think that’s not a great thing to allow or encourage, I wouldn’t want to make a change to break existing behavior.

With that all said, I’m 👍 to your proposal for removing the suggestion if fix() returns null.

mdjermanovic added a commit that referenced this issue Oct 19, 2020
btmills pushed a commit that referenced this issue Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.