Skip to content

fix(eslint-plugin): [init-declarations] refine report locations #8893

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 68 additions & 1 deletion packages/eslint-plugin/src/rules/init-declarations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,51 @@ export default createRule<Options, MessageIds>({
},
defaultOptions: ['always'],
create(context, [mode]) {
const rules = baseRule.create(context);
// Make a custom context to adjust the the loc of reports where the base
// rule's behavior is a bit too aggressive with TS-specific syntax (namely,
// type annotations).
function getBaseContextOverride(): typeof context {
const reportOverride: typeof context.report = descriptor => {
if ('node' in descriptor && descriptor.loc == null) {
const { node, ...rest } = descriptor;
// We only want to special case the report loc when reporting on
// variables declarations that are not initialized. Declarations that
// _are_ initialized get reported by the base rule due to a setting to
// prohibit initializing variables entirely, in which case underlining
// the whole node including the type annotation and initializer is
// appropriate.
if (
node.type === AST_NODE_TYPES.VariableDeclarator &&
node.init == null
) {
context.report({
...rest,
loc: getReportLoc(node),
});
return;
}
}

context.report(descriptor);
};

// `return { ...context, report: reportOverride }` isn't safe because the
// `context` object has some getters that need to be preserved.
//
// `return new Proxy(context, ...)` doesn't work because `context` has
// non-configurable properties that throw when constructing a Proxy.
//
// So, we'll just use Proxy on a dummy object and use the `get` trap to
// proxy `context`'s properties.
return new Proxy({} as typeof context, {
get: (target, prop, receiver): unknown =>
prop === 'report'
? reportOverride
: Reflect.get(context, prop, receiver),
});
}
Copy link
Member

Choose a reason for hiding this comment

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

😬 It's a little awkward having to Proxy the base rule. Is there an issue that was ever filed on ESLint about better handling of report locations in this case? I couldn't find one. But even if it's declined as out-of-scope, it'd be good for reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure... I basically copied this pattern from

// we can't directly proxy `context` because its `report` property is non-configurable
// and non-writable. So we proxy `customContext` and redirect all
// property access to the original context except for `report`
const proxiedContext = new Proxy<typeof context>(
customContext as typeof context,
{
get(target, path, receiver): unknown {
if (path !== 'report') {
return Reflect.get(context, path, receiver);
}
return Reflect.get(target, path, receiver);
},
},
);

and

// we can't directly proxy `context` because its `report` property is non-configurable
// and non-writable. So we proxy `customContext` and redirect all
// property access to the original context except for `report`
return new Proxy<Context>(customContext as typeof context, {
get(target, path, receiver): unknown {
if (path !== 'report') {
return Reflect.get(context, path, receiver);
}
return Reflect.get(target, path, receiver);
},
});

And modified it with stylistic preferences.

Following the trail through the git history, it doesn't look like any investigation into alternatives or issues with eslint were documented in those.

(breadcrumbs)

Will plan to check for eslint issues on the topic before this PR gets merged

Copy link
Member

Choose a reason for hiding this comment

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

before this PR gets merged

Oh @kirkwaiblinger I was going to merge today for the Monday release and consider this a followup. But in that case I'll just let you merge whenever you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Followup makes sense to me too! 👍
Happy for us to merge for the release

Copy link
Member Author

Choose a reason for hiding this comment

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


const rules = baseRule.create(getBaseContextOverride());

return {
'VariableDeclaration:exit'(node: TSESTree.VariableDeclaration): void {
Expand Down Expand Up @@ -65,3 +109,26 @@ export default createRule<Options, MessageIds>({
}
},
});

/**
* When reporting an uninitialized variable declarator, get the loc excluding
* the type annotation.
*/
function getReportLoc(
node: TSESTree.VariableDeclarator,
): TSESTree.SourceLocation {
const start: TSESTree.Position = structuredClone(node.loc.start);
const end: TSESTree.Position = {
line: node.loc.start.line,
// `if (id.type === AST_NODE_TYPES.Identifier)` is a condition for
// reporting in the base rule (as opposed to things like destructuring
// assignment), so the type assertion should always be valid.
column:
node.loc.start.column + (node.id as TSESTree.Identifier).name.length,
};

return {
start,
end,
};
}
Loading