parser: v4.1+ breaks an eslint-plugin-react no-unused-vars test that passes on v4.0

[ljharb]

Specifically, https://github.com/ljharb/eslint-plugin-react/runs/3415051454 shows the failures, but when I pin it to v4.0, they pass locally and in CI: https://github.com/ljharb/eslint-plugin-react/actions/runs/1164023886

This seems like a breaking change, but if there's a simple config change I can do to make the tests pass (and to tell users if they complain) that would likely suffice.

[bradzacher]

v4.0 introduced our new scope analyser, and v4.1 added support for handling the JSX pragma reference (pr: #2498).

The reason your test is failing is because it's expecting there to be a no-unused-vars error, but there's not because our scope analyser creates a reference to React which no-unused-vars is able to understand.

No users should complain unless for some weird reason they want to see unused variable errors against React.

There are config options you can provide to control this behaviour:

• https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/parser#parseroptionsjsxpragma
• https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/parser#parseroptionsjsxfragmentname

[ljharb]

Hmm, so you're saying that the TS parser as of v4.1 automatically adds a React import even if it's not present in the code??

[bradzacher]

To clarify:

As of 4.1 our scope analyser just understands that

var React;
<></>

transpiles to

var React;
React.jsx(React.Fragment,{});

So the scope analyser creates a reference at the JSX location to the top-level React variable.

Example:

This code fragment:

typescript-eslint/packages/scope-manager/tests/fixtures/jsx/factory/default-jsxPragma-fragment.tsx

Lines 3 to 5 in 043462e

	import React from 'react';
	<></>;

Creates a scope tree with the React variable (note the references array has one reference in it):

typescript-eslint/packages/scope-manager/tests/fixtures/jsx/factory/default-jsxPragma-fragment.tsx.shot

Lines 7 to 27 in 043462e

	Variable$2 {
	defs: Array [
	ImportBindingDefinition$1 {
	name: Identifier<"React">,
	node: ImportDefaultSpecifier$1,
	},
	],
	name: "React",
	references: Array [
	Reference$1 {
	identifier: Identifier<"React">,
	isRead: true,
	isTypeReference: false,
	isValueReference: true,
	isWrite: false,
	resolved: Variable$2,
	},
	],
	isValueVariable: true,
	isTypeVariable: true,
	},

---

Put another way - if they are using at least v4.1 of our parser - users do not need the react/jsx-uses-react rule as it will do nothing.

[ljharb]

ah, gotcha, thanks - that clarifies it.

That's a bit unfortunate in one way - I'm encouraging people to extend react/jsx-runtime when using the new JSX runtime, and that won't configure the TS resolver properly.

[bradzacher]

No reason why you couldn't add parserOptions: { jsxPragma: null } to the config!
All parsers (that I know of) will ignore that option - so it'll just impact people using our parser.

[ljharb]

hmm, that doesn't seem to be making the test pass - is there a way i can modify the test case so the TS eslint parser doesn't mark React as used for me, since that's the entire purpose of react/jsx-uses-react?

[smashercosmo]

Experiencing the same issue. Getting false negatives from no-unused-vars rule. I'm not using react's automatic runtime, but typescript-eslint reports React as unused variable. Related #3303

[ljharb]

@bradzacher ping on the above question :-)

[bradzacher]

It should be jsxPragma: null - that will tell our scope analyser to not automatically reference React

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

Lines 1580 to 1605 in 02c6ff3

	{
	code: `
	import React from 'react';
	export const ComponentFoo = () => {
	return <div>Foo Foo</div>;
	};
	`,
	parserOptions: {
	ecmaFeatures: {
	jsx: true,
	},
	jsxPragma: null,
	},
	errors: [
	{
	messageId: 'unusedVar',
	line: 2,
	data: {
	varName: 'React',
	action: 'defined',
	additional: '',
	},
	},
	],
	},

---

@smashercosmo - please don't piggy back on issues - file a new issue and follow the issue template to get support.

[ljharb]

@bradzacher i'm still not seeing this work. For example, in our jsx-no-undef rule, we have this test case:

        /*eslint no-undef:1*/
        var React;
        class Hello extends React.Component {
          render() {
            return <this.props.tag />
          }
        }

In v4 latest of the TS parser, we get this error output:

{
    ruleId: 'no-undef',
    severity: 1,
    message: "'this' is not defined.",
    line: 6,
    column: 21,
    nodeType: 'JSXIdentifier',
    messageId: 'undef',
    endLine: 6,
    endColumn: 25
  }

However, this is absolutely defined inside an instance method on the class, and eslint itself should know that - unless there's something the TS parser in v4.1+ is doing differently. Adding jsxPragma: null to parserOptions has no effect.

[bradzacher]

@ljharb - that issue is unrelated to this one.

That issue is because our scope analyser does not understand that JSXIdentifier[name="this"] should be treated the same as a ThisExpression.

I didn't actually know that was even valid JSX (do people actually write that!?!) which is why it's not supported.

File #4055 to track that bug.

Funnily whilst playing around - TS type-checks this correctly; however flow does not.

[ljharb]

Thanks; I'm also seeing a few other examples (this has 2 but should have 1, the extra is below):

/*eslint no-undef:1*/ var React; React.render(<appp.foo.Bar />);

{
    ruleId: 'no-undef',
    severity: 1,
    message: "'appp' is not defined.",
    line: 1,
    column: 48,
    nodeType: 'JSXIdentifier',
    messageId: 'undef',
    endLine: 1,
    endColumn: 52
  }

which seems like a similar issue around namespaced jsx?

However, with jsxPragma set to null, i'd expect the same behavior in v4.0.0 of the parser - ie, using the old jsx transform.

[bradzacher]

(I might be misunderstanding here)
Reporting no-undef should be correct <app.foo.Bar /> is transpiled to _jsx(appp.foo.Bar, {}, void 0)), which references an undefined variable appp.

[ljharb]

hmm, you're right - in that case i'm confused why with v4.0 or below i don't get that error and why v4.1+ i do. The whole point of the jsx-no-undef rule is that the eslint parser doesn't understand jsx - eslint runs on source code, and shouldn't be caring about what the transpilation does.

[ljharb]

Put another way, someone using typescript and jsx could be using any kind of jsx transpilation they like, via babel - so how can this parser make any assumptions about what the jsx transpiles to?

All that said, appp would still have been an undefined variable, so I remain confused about how v4.1 was the first version of the TS parser where i see this error, and how i don't see the extra error in any babel eslint version, or with espree.

[bradzacher]

Before 4.0 we essentially relied on the eslint-scope package for scope analysis - with a few poorly written and hacked in modifications to it to support typescript types.
eslint-scope is intentionally built to not do anything special with JSX - thus that scope manager does not touch JSXIdentifiers.

In 4.0 I forked and rebuilt the scope analyser completely so that it properly understands TypeScript types.
As part of this work I made the decision that the scope analyser should understand the exact same things that TS understands. Namely because I wanted @typescript-eslint/no-unused-vars to be able to completely replace TS's --noUnusedLocals/--noUnusedParameters compiler options.
In order to do this properly - it has to support analysis of JSX the same way that TS supports it - by understanding the transform that TS implements.

Because of this you will find that some of eslint-plugin-react's rules will be unnecessary with our scope analyser:

1. react/jsx-uses-react - the scope analyser creates a reference to the React variable (or the whatever configured JSX pragma is).
2. react/jsx-no-undef - the scope analyser creates a reference to a variable for the JSXIdentifier, which will be left open if no such variable exists (triggering the lint).
3. react/jsx-uses-vars - same as jsx-no-undef except the reference is resolved if the variable exists.

It's worth noting that it's not just JSX that we do this extended analysis for - we also do it for decorators!

TS has support for an old version of the decorator spec. With the --emitDecoratorMetadata compiler option it will add additional code which references variables in specific ways.

class Foo {}

@decorator
class Bar {
  constructor(arg: Foo){ }
}

repl

In this code looks like Foo is only referenced as a type - but it's also referenced as a value because TS emits this code as part of the transpilation:

// .......
Bar = __decorate([
    decorator,
    __metadata("design:paramtypes", [Foo])
], Bar);

Why is this important? Because if - for example - that isn't a class declaration, but is instead an import - then it's really important what type of reference it is.

Rules like our consistent-type-imports rule look at how an import is referenced to determine if it should be marked as a type-only import. If we don't mark the reference to Foo as a value reference, then the rule will autofix import Bar from 'Bar' to import type Bar from 'Bar' - and then the code can break at runtime!

This extended analysis has a huge benefit in that it means that users no longer have to turn on 2 additional rules to make no-unused-vars work. This has been a pretty common pain point for people setting up their ESLint config - so I was happy to remove that hurdle.

[ljharb]

Doesn't that presume that TS users are using tsc for transpiling, and using jsx with react? (something eslint-plugin-react can assume, but not something a generic typescript eslint parser can presume)

Is there a way I can disable these three behaviors via an option? If not, then I probably need to just disable all the "new TS parser" tests entirely for these three rules.

[bradzacher]

Doesn't that presume that TS users are using tsc for transpiling

Yes, but TS's type checking assumes this as well - it assumes certain semantics about how the transform will work - it has to or else it could not typecheck JSX at all.

Given how huge react is - they simply assume a "react-like" transform which will do something akin to a JSX expression being transformed to this:
<factory>(<component Identifier> | <string>, { <props> }, ...children)

I.e. it assumes that:

• the component name will be a reference if it's upper-case or a dot (.) namespace (member expression)
• the component name will be a string if it's lower case or if it's a colon (:) namespace.
• attribute names are strings
• attribute values are JS expressions
• attribute spreads values are JS expressions
• children are JSX expressions or other JS expressions

Which are all generally applicable assumptions to make for JSX implementations I believe.

using jsx with react

No - by default TS will assume react because it's the largest - but it's user-configurable via the --jsxFactory, --jsxFragmentFactory and --jsxImportSource compiler options.

TS also supports --jsx=preserve which means that the JSX will not be transpiled at all, but that's rarely used, and even still it retains the aforementioned assumptions about how the JSX is used.

Is there a way I can disable these three behaviors via an option?

No, sorry. They're all baked into the scope analyser's logic with no gating.

I probably need to just disable all the "new TS parser" tests entirely for these three rules

I probably would do that yeah, unless you can annotate different expected errors depending on the version in case you wanted to make sure the rules "play nice" with the scope analyser.

Considering how "simple" your rules are (they just mark variables as used) there's no reason they should ever clash.

[ljharb]

re jsx=preserve, "rarely used" is not my experience - now that you mention it, my latest 3 jobs all use it, so that they can use babel for transpiling (since it does a much better job than tsc).

Thanks, I'll go ahead and close this if in fact, these three rules are the only breakage on v4.1+.

[ljharb]

Confirmed; those three rules were the only failures on v4.1+. I have almost 200 failures on v5, but that's probably something different :-)