fix: address ci failures

[michaelfaith]

This change addresses the two different kind of type check failures:

• 2 rules weren't using explicit types when they were needed
• eslint-scope's ScopeManager type is incompatible with eslint's built-in Scope.Scopemanager

Normally I would have solved this by using patch-package to patch the @types/eslint-scope package directly, but patch-package requires a lock file in the repo 😢. So, I just forked the types entirely, and replace ScopeManager with eslint's Scope.ScopeManager.

Closes #575

[michaelfaith]

Hmm, these are failing in main too. The 4 tests that are failing are from no-property-in-node, but I'm unable to repro locally. Still looking into it...

[aladdin-add]

Hmm, these are failing in main too. The 4 tests that are failing are from no-property-in-node, but I'm unable to repro locally. Still looking into it...

can it be addressed by #569?

[michaelfaith]

Hmm, these are failing in main too. The 4 tests that are failing are from no-property-in-node, but I'm unable to repro locally. Still looking into it...

can it be addressed by #569?

Could be. I just pushed those changes here (they still pass locally)

[michaelfaith]

No, still failing. I'm not sure why I can't reproduce the failures locally.

[aladdin-add]

In most cases, it is caused by a newly released version of a package (due to not committing lockfile).

have you tried a clean install, and retry?

[michaelfaith]

In most cases, it is caused by a newly released version of a package (due to not committing lockfile).

have you tried a clean install, and retry?

I have nuked node_modules and npm clean cache a couple of times. Doesn't seem to change the behavior.

Does it fail for you locally?

[aladdin-add]

No, just retried and it passed on my windows. 😂

[michaelfaith]

No, just retried and it passed on my windows. 😂

I'm on windows too...

[michaelfaith]

I wonder if those rules are actually failing now, but a bug in how it's running on Windows is disguising it for us. I notice the rules has these filename regex patterns. It looks like it's attempting to be cross-platform compatible, but that's a common place for cross-platform compatibility to break.

[michaelfaith]

I just pushed a console.log on the filename to see what's going through that code in CI.

[michaelfaith]

Yeah, so, on CI it's not resolving to the type files.

This is what CI is reporting for file names

This is why my local (and probably yours) is reporting. The ones where it's @types/estree or @typescript-eslint those are successful test evaluations.

[michaelfaith]

This is the first point where these 4 tests started failing in CI: https://github.com/eslint-community/eslint-plugin-eslint-plugin/actions/runs/19694297600/job/56416156941 (November 26th)

[bmish]

If adding a lockfile makes things easier, I'm not opposed to adding one. We just didn't need one in the past. But no strong reason to avoid it.

[StyleShit]

No, just retried and it passed on my windows. 😂

I'm on windows too...

I can reproduce it on my Mac using CI=true npm test
It seems to be related to the @typescript-eslint/parser version. The last "good" version is 8.47.0. I pinned this version, and it stopped failing.

Weirdly, when I run each test independently, it passes...

I tried comparing the versions, but I'm not seeing anything too suspicious:
typescript-eslint/typescript-eslint@v8.47.0...v8.48.0

I debugged this, and seems like checker.getTypeAtLocation() returns a different result in this version

8.47.0:

8.48.0:

Note specifically the missing .types array, which is being used in isAstNodeType.

eslint-plugin-eslint-plugin/lib/rules/no-property-in-node.ts

Lines 31 to 35 in 61e59ca

	function isAstNodeType(
	type: Type & { types?: Type[] },
	typedNodeSourceFileTesters: RegExp[],
	): boolean {
	return (type.types || [type])

I assumed it's related to the ast-spec.d.ts:

eslint-plugin-eslint-plugin/lib/rules/no-property-in-node.ts

Lines 4 to 7 in 61e59ca

	const defaultTypedNodeSourceFileTesters = [
	/@types[/\\]estree[/\\]index\.d\.ts/,
	/@typescript-eslint[/\\]types[/\\]dist[/\\]generated[/\\]ast-spec\.d\.ts/,
	];

But also nothing too interesting (left is 8.47.0, right is 8.48.0):

https://www.diffchecker.com/RIhn0q4j/

How is it related to the CI env var?
Why tests are passing independently?

TBH, I don't have any idea at this point...

[MichaelDeBoey]

Maybe @JoshuaKGoldberg can point towards a possible solution here?

[michaelfaith]

@StyleShit thanks for verifying. So, it seems that something in the 8.48.0 release broke this rule for linux / macos, which lines up timing-wise with when CI started failing. Since this does appear to be user-facing (unlike the eslint-scope issue), I would propose we:

1. Create a new (more specific) bug issue in this repo tracking the no-property-in-node + typescript-eslint 8.48.0 bug
2. Pin typescript-eslint to 8.47.0 in this PR to unblock main builds
3. Start a new PR to update typescript-eslint again and address 1 (which we may end up needing to file an issue in typescript-eslint to resolve, depending on what insights @JoshuaKGoldberg might have)

[MichaelDeBoey]

Sound like a solid plan @michaelfaith 👍

[michaelfaith]

Looking in the typescript-eslint code around the getTypeAtLocation() function, it looks as though, in createParserService it's setting up the parser service to delegate to typescript for this function, but using a custom ast map to pass in.

The createParserService code hasn't changed in a year. But the ./convert code where the ast map that's used to pass into typescript's getTypeAtLocation has changed, and there were specifically two changes to it that happened in the 8.48.0 release.

https://github.com/typescript-eslint/typescript-eslint/blame/cf79108b6405972fb73f5991e913e1b36de8a67f/packages/typescript-estree/src/createParserServices.ts#L35-L36

The first one, is in the part of the code that deals with SyntaxKind.ImportType. I wonder if that could be at play? The second change only does a type cast (as).

[michaelfaith]

Opened #579 and pinned typescript-eslint

[StyleShit]

I also tend to go for pnpm, we just need to keep in mind that this change means changing also the ci and everything

[MichaelDeBoey]

As @fasttime mentioned in eslint/js#719 (comment), it seems the ESLint team isn't planning on adding a fix for ESLint v9, so we should probably fix @types/eslint-scope ourselves

There are currently no plans to backport the changes in #708 to eslint-scope v8 (used by ESLint v9). Meanwhile, if there's an issue with @types/eslint-scope it can be addressed by submitting a PR at DefinitelyTyped, which is an external repository not maintained by the ESLint team.

We can probably take a few things from DefinitelyTyped/DefinitelyTyped#74066 for that

[michaelfaith]

I marked my comment RE: package managers as off-topic and opened: #580

[michaelfaith]

As @fasttime mentioned in eslint/js#719 (comment), it seems the ESLint team isn't planning on adding a fix for ESLint v9, so we should probably fix @types/eslint-scope ourselves

There are currently no plans to backport the changes in #708 to eslint-scope v8 (used by ESLint v9). Meanwhile, if there's an issue with @types/eslint-scope it can be addressed by submitting a PR at DefinitelyTyped, which is an external repository not maintained by the ESLint team.

We can probably take a few things from DefinitelyTyped/DefinitelyTyped#74066 for that

I'm not sure I understand the entire story for this. It seems the DT PR was closed because they'd resolved to build types into eslint-scope, but they've left the v9 types in a bad state? Sure @types/eslint-scope is a different repo, but if the argument of not fixing it in the DT repo was that they'd be absorbed into the core package, I would expect the DT types to be supported until that happens and not leave users in limbo like that. I feel like I'm missing part of the story.

[MichaelDeBoey]

but they've left the v9 types in a bad state?

I think the idea is "we never were responsible for this ourselves, so we don't/shouldn't care now either"
I personally also don't agree with the fact that they leave ESLint v9 users using TypeScript in a bad state, but @fasttime can probably give more insights into the thought process of this.

Sure @types/eslint-scope is a different repo, but if the argument of not fixing it in the DT repo was that they'd be absorbed into the core package, I would expect the DT types to be supported until that happens and not leave users in limbo like that.

I have the exact same feeling