fix: Don't crash on type spread in no-dupe-keys

[lydell]

The no-dupe-keys rule used to crash when encountering type spread
(type A = { ...B }) with "TypeError: Cannot read property 'type' of
undefined". This commit makes it no longer crash.

For reference, I’ve encountered this in some real world code and had to use this workaround for now:

// Use Flow’s comment syntax here to avoid a problem in eslint-plugin-flowtype:
/* ::
type Two {
  a: number,
  ...One
}
*/

[lydell]

I had to update babel-eslint from 6 to 7 for the type spread. Unfortunately, that caused some use-flow-type and define-flow-type tests to break.

These tests:

eslint-plugin-flowtype/tests/rules/assertions/useFlowType.js

Lines 38 to 93 in 6eceb89

	{
	code: 'import type A from "a"; (function<T: A>(): T {})',
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: '(function<T: A>(): T {}); import type A from "a"',
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: 'import type {A} from "a"; (function<T: A>(): T {})',
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: '(function<T: A>(): T {}); import type {A} from "a"',
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: '(function<T: A>(): T {}); import type {a as A} from "a"',
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: 'type A = {}; function x<Y: A>(i: Y) { i }; x()',
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: 'function x<Y: A>(i: Y) { i }; type A = {}; x()',
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: 'type A = {}; function x<Y: A.B.C>(i: Y) { i }; x()',
	// QualifiedTypeIdentifier -------^
	errors: [
	'\'A\' is defined but never used.'
	]
	},
	{
	code: 'function x<Y: A.B.C>(i: Y) { i }; type A = {}; x()',
	// ^- QualifiedTypeIdentifier
	errors: [
	'\'A\' is defined but never used.'
	]
	}

Those cases are expected to be invalid, but I can't understand why. To me they look valid. So I'm unsure what to do here! Some help would be nice.

EDIT: Now I get it. They're testing that no-unused-vars would have reported something in those cases, since no-unused-vars is not Flow-aware. So babel-eslint has changed something that causes no-unused-vars (and no-undef) to no longer recognize some patterns. Interesting. Looks like I need to dig into babel-eslint as well...

[lydell]

Progress: I’ve git bisect:ed babel-eslint, and this is the first “bad” commit: babel/babel-eslint@a2c3b30

[lydell]

Ahh! babel-eslint now intentionally makes sure that no-unused-vars does not report anything for these cases: babel/babel-eslint#444

So I guess I can just (re)move the tests?

[lydell]

For reference, the PR which handles no-undef for interface declarations is here: babel/babel-eslint#449

I’ve now moved the failing tests from VALID_WITH_USE_FLOW_TYPE to ALWAYS_VALID so that the tests pass again.

I also removed the now dead code from the use-flow-type rule and the define-flow-type rule, but I just realized that by doing so we lose babel-eslint@6 compatibility (actually, only babel-eslint@^7.2.1 works). Do you want me to restore that code to maintain compatibility with older babel-eslint versions?

[lydell]

@gajus or anyone:

There are 3 ways this PR can be done:

1

Add these 3 lines:

if (identifierNode.type === 'ObjectTypeSpreadProperty') {
  return;
}

And this test (commented out on purpose):

// TODO: Uncomment when we update the tests to use babel-eslint@^7.2.0.
// {
//   code: 'type One = { c: number }; type Two = { a: number, b: string, ...One }'
// }

2

Same as above, but also update the tests to use babel-eslint@^7.2.0 so the new test does not have to be commented out. This also requires moving some tests from VALID_WITH_USE_FLOW_TYPE to ALWAYS_VALID, but it should be fine.

3

Same as above, but also remove dead code since babel-eslint@^7.1.0 or so.

---

This PR is currently at alternative 3 – are you happy with this, or would you like me to change to alternative 1 or 2 to get this merged?

Thanks! :)

[gajus]

It is fine as is, just waiting for Babel v7 to be released.

[lydell]

Ok! 👍

Curious question: What has Babel v7 to do with this? :)

[gajus]

babel-eslint depends on Babel core libs, which are in alpha at the moment.

https://github.com/babel/babel-eslint/blob/42d0c5b20cf65ae4100d06aa8dcaf372723f378d/package.json#L15-L18

[lydell]

@gajus Since it is not known when Babel v7 goes out of alpha, would you have anything against adding the following three lines without tests:

if (identifierNode.type === 'ObjectTypeSpreadProperty') {
  return;
}

This way we could do a patch release immediately so people can use type spreads without hassle. When Babel v7 goes out of alpha, we can merge this PR.

[gajus]

Babel 7 is not far from getting released. Days, maybe weeks.

https://github.com/babel/babel/milestone/9

I would recommend simply disabling the no-dupe-keys in the meantime.

[lydell]

Looks like I need to merge master again ... and now even more tests need to be updated because of babel-eslint. I'll try to do this soon.

[kusmierz]

@lydell any news here? :)

[lydell]

Yes: I never got around merging master again. And I'm not particularly keen on doing it.

[lydell]

I don’t think this PR is relevant anymore.