wip(eslint-plugin): add new blank-line-declaration-usage rule and tests for it

[kresimir-coko]

This is an exploration of the rule requested in #15

I've implemented a rudimentary rule here to explore a couple angles while doing so and landed on this current one.

I believe it should be easy to add an autofix to this rule, simply inserting an empty line in between the lines that trigger the error.

LMK if I'm on the right track

[bryceosterhaus]

I would go through DXP or other code reviews and try to gather as many examples of "incorrect" examples and test against that. For example, we would want to check in nested functions, for loops, deeply nested objects, etc. This way we can be sure we aren't testing for some specific path and example, but we can try to cover as many cases as possible.

One way to do this too is to run this rule in DXP and see what examples fail that you wouldn't expect to, or cases that pass that you wouldn't expect to.

[kresimir-coko]

I pushed another use case and its lint, but I'm having issues with TypeError: node.parent.body.indexOf is not a function when running yarn lint. I haven't found a solution for it, yet. It works fine inside AST explorer and the index is behaving as it should. I tried adding a bunch of if (node.parent) etc. blocks before it, but that didn't work.

[bryceosterhaus]

@kresimir-coko the reason you are getting that error is because of this line, https://github.com/liferay/liferay-frontend-projects/pull/713/files#diff-086d7b2631bb42efbb6851eb299a8aeed6fcffc2c9606502a9ab895cb099772bR35

It may work in AST explorer, but that is likely because of the small scope that it tests against. When running yarn lint, we are testing against our actual real code and so that indicates there is some code we aren't guarding against. Probably the best solution for testing this is to figure out which piece of code is causing that error, you can do that by checking what file that error comes from (const filePath = context.getFilename();).

[bryceosterhaus]

Just had a thought of one use case we need to be mindful of, where multiple variables are declared in a row.

// Valid
const fooVar = 'foo';
const barVar = 'bar';

const fooLength = foo.length

// Invalid
const fooVar = 'foo';
const barVar = 'bar';
const fooLength = foo.length

[bryceosterhaus]

Hey @kresimir-coko! I was taking a look at your code so far and it was looking like it was getting pretty complicated. From my experience with these eslint rules, the more complicated it gets and more specific I have to write for each edge case, it makes me think there is possibly a simpler way to do it. I recently was trying to utilize some of the SourceCode apis(docs) through eslint, and it made me think it might be useful for this case.

Check out my example below, I took a quick stab at it and I think this might help you out a bit.

module.exports = {
	create(context) {
		const sourceCode = context.getSourceCode();

		return {
			VariableDeclaration(node) {
              	if (node.parent.type === 'ForOfStatement') {
                  return;
                }
              
				const {references} = context.getDeclaredVariables(node)[0];

				const declarationLine = references[0].identifier.loc.start.line;

				references.slice(1).forEach((reference) => {
					const referenceLine = reference.identifier.loc.start.line;

					if (
						sourceCode.lines
							.slice(declarationLine, referenceLine - 1)
							.indexOf('') === -1
					) {
						context.report({
							message: 'line needed between delcaration and use',
							node: reference.identifier,
						});
					}
				});
			},
		};
	},

	meta: {
		docs: {
			category: 'Best Practices',
			description: 'error msg',
			recommended: false,
			url: 'https://github.com/liferay/eslint-config-liferay/issues/139',
		},
		fixable: 'code',
		schema: [],
		type: 'problem',
	},
};

You might need to check some edge cases(like I did for the ForOfStatement, we will need to add other iterative statements, for, while, etc) but I think this generally does a good job at catching our issue at hand. One thing you should also look at is how we can auto-fix existing code in DXP as I would be worried this might raise lots of errors, and we don't want to have to manually fix everything right now.

[kresimir-coko]

I was going with a slightly different approach this time, hence why the code looks so complicated. But still, your proposition looks much cleaner. I wanted to make all the rules work and then do a pass on efficiency and code cleanliness. I'll check out your example and see how it fits, but yea it looks spicy, thanks!

[kresimir-coko]

@bryceosterhaus Looking over it in detail and understanding how you made it work, it's a very clever approach here, the sourceCode property is so powerful. I've tested it in our liferay-frontend-projects codebase and it's spitting out 1445 errors 😱

Next week I'll work on the autofix for it. I'm excited to get this into DXP and open a PR that changes like 69k files 🤣

[kresimir-coko]

I've pushed a couple commits, one adds your changes @bryceosterhaus and the initial implementation of the fix. The other one has the autofixes applied. Note that this is not complete as there's a missing invalid usage that the rule doesn't pick up:

const {foo, ray, life} = {foo: 'bar', ray: 'life', life: 'ray'};
ray.length;

When ray is the first of the declared variables in this manner, it works fine, but otherwise it doesn't. My preliminary research led me to the conclusion that it is the fault of const {references} = context.getDeclaredVariables(node)[0]; having [0] in there.

While the fixer did fix about 150 files, there's 450 or so more that produce errors.

[kresimir-coko]

I checked this random doc I found online for all types of expressions/statements and assumed that these 4 I added on in 3356e35.

I've fixed the message, added outputs and fixed the duplicate variables.

@bryceosterhaus We seem to have the new line problem again, with yarn test  reporting failed tests and differences being spaces and tabs. Couldn't remember how we handled it back in empty-line-between-elements but I remember that we had to account for different parsers due to JSXElements, so not sure if that applies here.

[kresimir-coko]

Hey @bryceosterhaus I've adjusted the implementation of the rule so that it handles when multiple vars are declared on the same line, like this:

const {pog, kekw, lolw} = obj;
kekw.length;

It used to work and autofix only if the first var was being used in the subsequent line, but now I modified it so that it works if any of them are used.

Let me know what you think and if I should squash, and add the autofixes to the PR.

[bryceosterhaus]

@kresimir-coko yeah this is starting to look good, you can probably clean up the commits at this point.

Also, can you run this over DXP and analyze the errors and what is required to fix everything in dxp?

[kresimir-coko]

@bryceosterhaus I've squashed everything into 2 commits, one that adds the rule and the other that contains the linted code.

I notice that we still have some edge cases to lint against, yarn lint spits out 215 errors after running the autofixer.

I'm not sure why the last one of these 4 examples below throws an error, because it doesn't in ASTExplorer. But the first 3 are valid and I should improve the rule to fix those too. Important thing to note is that the rule does pick up on all these, but doesn't autofix them.

var urlLeft = /(?!left.*\/)(left)(?=.*\))/g;
var urlRight = /(?!right.*\/)(right)(?=.*\))/g;
var leftRe = /\s(left)\s/;
var rightRe = /\s(right)\s/;
var percRe = /(\d+)%/;
var plug = function (r2) {
  var r2bgimage = function (v) {
    if (urlLeft.test(v)) {
      v = v.replace(urlLeft, 'right');
    }
    else if (urlRight.test(v)) {
      v = v.replace(urlRight, 'left');
    }

    return v;
  };
}
---------------------------------------------------------------
const length = permutation.length;
const result = [[...permutation]];
const c = new Array(length).fill(0);
---------------------------------------------------------------
const getEstimatedTimeMock = jest.fn(
  getEstimatedTimeMockFactory(10)
);
const {getByDisplayValue} = renderReviewExperimentModal({
  getEstimatedTimeMock,
});
---------------------------------------------------------------
this.xhr.onCreate = (xhr) => {
  requests.push(xhr);
};

DXP

In DXP, there are 1595 errors, and after autofixing, there are 615 errors left.

[bryceosterhaus]

@kresimir-coko you can ignore this rule in the senna project, you just need to add it to its eslint config.

Additionally, can you add the manual changes to satisfy this PR? I think after ignoring the senna it shouldn't be too bad.

Another thought is to try and come up with some logic to auto fix these multiline declarations.

const a = 'foo';
const b = 'foo';
const aLength = a.length;

// fix
const a = 'foo';
const b = 'foo';

const aLength = a.length;

I played around with ASTExplorer a bit and with your changes. See if this fixer works, I only tried it on a few things but It seemed to work.

context.report({
	fix: (fixer) => {
		const declarationEndLine = node.loc.end.line;

		if (referenceLine === declarationEndLine + 1) {
				return fixer.insertTextAfter(node, '\n');
		} else {
	            const tokensBetween = sourceCode.getTokensBetween(node, varReference.identifier);
	            const nodeStartLine = tokensBetween.findIndex(item => item.loc.start.line === referenceLine);
	  
	            return fixer.insertTextAfter(tokensBetween[nodeStartLine - 1], '\n')
	  
	          }
	},
	message,
	node: reference.references[1].identifier,
});

[kresimir-coko]

I quickly gave it a glance over in our repo and it's failing on this:

for (let i = active.size; i < MAX_CONCURRENT_CHECKS; i++) {
	for (const [link, files] of pending.entries()) {
		const promise = new Promise((resolve, reject) => {
			check(link, files)
				.then(resolve)
				.catch(reject)
				.finally(() => active.delete(promise));
		});

		active.add(promise);
		pending.delete(link);

		break;
	}
}

I'll give it a better look tomorrow after I'm done wiping my tears caused by @jbalsas leaving

[kresimir-coko]

@bryceosterhaus I've got no clue how to make this autofixer work with:

var urlLeft = /(?!left.*\/)(left)(?=.*\))/g;
var urlRight = /(?!right.*\/)(right)(?=.*\))/g;
var leftRe = /\s(left)\s/;
var rightRe = /\s(right)\s/;
var percRe = /(\d+)%/;
var plug = function (r2) {
  var r2bgimage = function (v) {
    if (urlLeft.test(v)) {
      v = v.replace(urlLeft, 'right');
    }
    else if (urlRight.test(v)) {
      v = v.replace(urlRight, 'left');
    }

    return v;
  };
}

I went through the following ideas:

• Use sourceCode.isSpaceBetween which sadly doesn't differentiate between empty lines and empty spaces, so it returns true always
• Tried checking all tokens between the variable declaration and reference to find an empty line, but an empty line is not a token
• Tried getting the lines between, but couldn't make it useful, as the line could be anywhere and we would have a false positive

One idea I didn't manage to try is to get the reference node and crawl up its parents until we get the parent that is a sibling of the declaration node, and then putting an empty line above the reference parent. The reason I couldn't try it is because varReference.identifier.parent returns undefined but in the console it exists 🤷

[bryceosterhaus]

@kresimir-coko how many instances are there of this failure? Keep in mind that we don't necessarily need to auto-fix every single error. The auto-fix is primarily helpful for the initial refactor.

Additionally, since there seems to be so many one-off errors or non-fixes for this rule, we may consider just putting this on hold and picking it up at a later date... It isn't necessarily vital, and more of a "nice to have" rule.

[kresimir-coko]

Hey @bryceosterhaus

There are 215 errors in liferay-frontend-projects where the declaration and usage aren't separated by an empty line, but instead by other code.

It's also throwing an error on this line const {flags = 'g', pattern, replacement} = options; 🤔
So yea, I'm not sure about how to handle all of them, there seem to be some false positives too.

Maybe we should throw warnings instead of errors, and autofix the simple stuff, so that we get a lot of value and warn devs that they might be doing something wrong. IDK 🤷 maybe we'd have to get rid of false positives before that.

[bryceosterhaus]

@kresimir-coko ah I see, seems like still a lot to parse through. In that case, let's just put this on hold for now and address at a later date. Might be nice to let liferay-portal settle a bit too before adding more large changes like this. Thanks for all your work on this though, even though we won't merge it yet, it is helpful to have this research done into it.

We can just live this PR open, or copy/paste the rule logic to the open issue for someone to investigate in the future.