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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [no-unused-vars] clear error report range #8640

Merged
merged 8 commits into from
Apr 28, 2024
Merged

fix(eslint-plugin): [no-unused-vars] clear error report range #8640

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
74944e5
fix: clear error code range
developer-bandi Mar 10, 2024
d95260e
test: add test code that cover end line
developer-bandi Mar 10, 2024
56ff4f2
refactor: rename variable name and delete not effect property
developer-bandi Apr 7, 2024
4062305
test: add test case
developer-bandi Apr 19, 2024
8819bd3
fix: remove type property
developer-bandi Apr 28, 2024
00c75a4
fix: remove type property
developer-bandi Apr 28, 2024
164798e
fix: remove type property
developer-bandi Apr 28, 2024
702b828
Merge branch 'main' into feat/no-unused-var-patch
kirkwaiblinger Apr 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions packages/eslint-plugin/src/rules/no-unused-vars.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,10 +419,24 @@ export default createRule<Options, MessageIds>({
ref.from.variableScope === unusedVar.scope.variableScope,
);

const node = writeReferences.length
? writeReferences[writeReferences.length - 1].identifier
: unusedVar.identifiers[0];

const { start } = node.loc;
const nodeVariableLength = node.name.length;
Copy link
Contributor

@yeonjuan yeonjuan Apr 6, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 What do you think about changing the variable names a bit?

Suggested change
const node = writeReferences.length
? writeReferences[writeReferences.length - 1].identifier
: unusedVar.identifiers[0];
const { start } = node.loc;
const nodeVariableLength = node.name.length;
const id = writeReferences.length
? writeReferences[writeReferences.length - 1].identifier
: unusedVar.identifiers[0];
const { start } = node.loc;
const idLength = node.name.length;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it make sense, so i change variable name. thank you!


const loc = {
start,
end: {
line: start.line,
column: start.column + nodeVariableLength,
},
};

context.report({
node: writeReferences.length
? writeReferences[writeReferences.length - 1].identifier
: unusedVar.identifiers[0],
node,

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i delete node. thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, looking closer at this... I may have led you astray, at least partially. Sorry about that! The test failures in CI are due to having removed the node from the report, since some of the tests specifically check for the type of the node
(the following function generates error assertions that demand an Identifier be reported on via the type field)

typescript-eslint/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars-eslint.test.ts

Lines 56 to 77 in 584db29

/**
* Returns an expected error for assigned-but-not-used variables.
* @param varName The name of the variable
* @param [additional] The additional text for the message data
* @param [type] The node type (defaults to "Identifier")
* @returns An expected error object
*/
function assignedError(
varName: string,
additional = '',
type = AST_NODE_TYPES.Identifier,
): TSESLint.TestCaseError<MessageIds> {
return {
messageId: 'unusedVar',
data: {
varName,
action: 'assigned a value',
additional,
},
type,
};
}

Now, it's still not clear to me that the value of the node is actually used in any way other than being tested, though, when the loc is also provided. I ran into the same problem in #8874 and just removed the type from the test cases, but I'm not totally sure if that is sound.

My thought is
if it has no effect => we should remove the type from the test, since it's not testing anything at all, but it looks like it is
if it does have an effect => just put node back.

Maybe @JoshuaKGoldberg or @bradzacher will know for sure? Would either of you know whether there's any reason to provide a report node if loc is being provided?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you only need to provide one or the other.

loc,
messageId: 'unusedVar',
data: unusedVar.references.some(ref => ref.isWrite())
? getAssignedMessageData(unusedVar)
Expand Down
19 changes: 19 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(testing)
Could you also add a test case that uses the declare modifier with the full loc data?

FYI - here and in other comments, there's probably no need to add new test cases; modifying existing ones to have the full loc data would work too 🙂

Original file line number Diff line number Diff line change
Expand Up @@ -1950,5 +1950,24 @@ export namespace Bar {
},
],
},
{
code: `
const foo: number = 1;
`,
errors: [
{
messageId: 'unusedVar',
data: {
varName: 'foo',
action: 'assigned a value',
additional: '',
},
line: 2,
column: 7,
endLine: 2,
endColumn: 10,
},
],
},
],
});