Currently, it doesn't support this:

import { Type } from './types';

export function (arg: Type) { /* ... */ }

but only things like this:

import type { Type1 } from './types';
import { type Type2 } from './types';
import { type Type3 as Type4 } from './types';

because IDK if I can know for sure whether the imported name is a type or not
any idea if this could be done?

The type checker can determine whether something is in type space and/or value space.

But for that case of export function (arg: Type) { /* ... */ }, do we need to know whether it's type and/or value? I'd say that should be a complaint in the rule no matter what, no?

Done

OK,
After thinking about it for a little bit...

What should we do in cases where someone imports a type from another module and uses it in their exported function?

// my-package/src/types.ts
export type A = number;

// my-package/src/function.ts
import type { A } from './types';

export function f(): A {
	return 1;
}

// my-package/src/index.ts
export { f } from './function';

Then, in user-land:

// my-code.ts
import { f } from 'my-package';

let value; // What type is it?
value = f();

I mean... on the one hand, we can't know whether the type is exported somehow to the user.
On the other hand, (I think?) we don't want to force everyone to (re)export the types they imported 😓

On a pure scope analysis level, we do know this. The A identifier in function.ts is part of what's exported with f().

I think it'd be very preferable to stick with the pure scope analysis strategy. Dipping into types land is much harder to work with, since type aliases sometimes become new things and sometimes are just references to existing things.

Does that answer what you're looking for?

The rule would need to check: for every identifier in the return type, is that also exported in the current file (module)?

My current logic is:
"Does this type exist in the outside world (whether it's imported/exported in the current file)?"
- If so: Great!
- Otherwise: Report

I think we do, and that that's the goal of this rule!

I don't fully agree
I mean... yeah, that's the goal of the rule, but the type might already be exported through the index file, for example, and we'll force developers to export it multiple times in their code.

Let's take my example above and modify it a little bit:

// my-package/src/types.ts
export type A = number;

// my-package/src/f1.ts
import type { A } from './types';

export function f1(): A {
	return 1;
}

// my-package/src/f2.ts
import type { A } from './types';

export function f2(): A {
	return 2;
}

// my-package/src/index.ts
export { f1 } from './f1';
export { f2 } from './f2';

export * from './types';

Then, in user-land:

// my-code.ts
import { f1 } from 'my-package';
import type { A } from 'my-package';

let value: A; // I know what the type is! YAY!
value = f1();

This code is totally fine. But with the changes you suggest, it'll have 2 more redundant exports (and it'll increase linearly based on the type usage inside other modules).
Yeah, it probably won't hurt anybody, but it will make the DX awful, don't you think?

If we don't enforce that a export function fn(): A; is in a file that also exports A, what is the point of this rule?

you could say the same about not exporting it from the index, no?
I mean, you'll always have holes in the rule unless you read the whole codebase, starting from the bundler entrypoint(s)

It's a small code change to enforce that imported types are also exported, I'm just trying to understand where the line lies in this case

For lint rules, the line generally lies with the single file being linted. Because lint rules operate on a single file at a time, that naturally falls out of the architecture.

It's pretty common that lint rules will only be able to catch issues within the small scope of a single rule at a time. For example, no-unused-vars won't detect things that are exported from a file but never imported. Hence Knip.

do you wanna loop in the triage team so we can agree on something together?

[Refactor] I don't think simpleTraverse is what we should use here. It's tailored to the way it's currently used in packages/typescript-estree:

• There's no way to break, but it looks like this usage would want to halt traversal after finding a ReturnStatement
• It allows configurable selectors with 'enter', but this code just uses ReturnStatement

I think it'd be better to avoid simpleTraverse here, and instead do some or all of:

• Look into the existing forEachReturnStatement, maybe it's exactly what you need?
• Failing that, write a dedicated simpler traverser for just this rule that halts on ReturnStatements, omitting anything around parent pointers or 'enter'
• Reuse some of the design and logic from one of the rules that does similar things: explicit-module-boundary-types, explicit-function-return-type
  • This one I'm low confidence in, but figured I'd mention...

Created a custom traverser, inspired by the original forEachReturnStatement

[Testing] Nothing fails with the Program > removed. Either there's a missing test or this is unnecessary.

Added missing tests

[Testing] I'd think keyof and/or typeof types should be flagged too:

const fruits = { apple: true };

export function getFruit<Key extends keyof typeof fruits>(
  key: Key,
): (typeof fruits)[Key] {
  return fruits[key];
}

Thoughts?

That's interesting. So we'll need to flag both types and values?

If so - I'll probably need to rework the reporting logic to also support values, and we should probably rename the rule (?)

EDIT:
Not sure that the developers will always want to export the value, since it's being exported by reference, which means external consumers will be able to change the internals of the code.
Maybe that's the case most of the times?

In this case, it's really the keyof typeof fruits that needs to be exported. So I'd assume the squiggly would be on there:

export function getFruit<Key extends keyof typeof fruits>(
//                                   ~~~~~~~~~~~~~~~~~~~

This would certainly make it difficult to write an auto-fixer 😅 glad that that's out of scope for this PR.

I think we need to report only the typeof part, because that's what really blocks the developers from using it, no?

Do you think we should have a different message for this case? Maybe something like "typeof needs to be exported. please extract it to a type alias"?

only the typeof part, because that's what really blocks the developers from using it, no?

Well, it's certainly better...

export type Fruits = typeof fruits;

export function getFruit<Key extends keyof Fruits>(
//                                   ~~~~~~~~~~~~

...but it strikes me as a kind of arbitrary exception to allow a keyof ... to be inline. What would make this so special?

different message for this case

+1, yes!

What would make this so special?

IDK, what's the difference between these two?

export declare function(key: keyof Items);
export declare function({ items }: { items: Items });

In both cases you construct a type inline based on a type reference. So why wouldn't we force every inline type to be extracted to an alias?

What makes keyof special compared to a regular TSTypeLiteral in this case?

EDIT:
And sorry for all of the notifications you get from my "Done" comments 😅
it helps me keep track of my progress, because I come back to this PR multiple times after switching context

Taking a step back: the core idea of the rule is that you should be able to directly import anything that's visible as a type from a module. You shouldn't have to keyof something, something['some-key'], ReturnType<typeof something>, or use any other indirection in the type system.

Looking at those two examples, here's how the rule should squiggly them:

export declare function(key: keyof Items);
//                           ~~~~~~~~~~~
export declare function({ items }: { items: Items });
//                                 ~~~~~~~~~~~~~~~~

So why wouldn't we force every inline type to be extracted to an alias?

We should.

umm... great
now I'll need to redo a lot of my work 😂

(I didn't understand this until now, which is stupid, because it's literally in the issue)

anyway, I think it's gonna simplify a lot of my code