Skip to content

Generated d.ts includes implicit any for recursive types #55832

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

Open
CraigMacomber opened this issue Sep 22, 2023 · 9 comments
Open

Generated d.ts includes implicit any for recursive types #55832

CraigMacomber opened this issue Sep 22, 2023 · 9 comments
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@CraigMacomber
Copy link

CraigMacomber commented Sep 22, 2023

πŸ”Ž Search Terms

recursive any noImplicitAny d.ts

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried (3.3 through 5.2), and I reviewed the FAQ for entries about anything related to this.

⏯ Playground Link

playground link

πŸ’» Code

const a = () => a;

πŸ™ Actual behavior

Any is implicitly introduced in the d.ts:

declare const a: () => any;

Note that the local type checking (not using the d.ts) correctly handles this recursive type without introducing "any", and builds fine with noImplicitAny enabled.

πŸ™‚ Expected behavior

Either generate something without implicitly introducing any, or generate an error if noImplicitAny is enabled.
In this case, the below code generation for the d.ts would work:

declare const a: () => typeof a;

Additional information about the issue

No response

@fatcerberus
Copy link

fatcerberus commented Sep 22, 2023

I’m more surprised this doesn’t have a circularity error, since a genuinely refers to itself in its own initializer and the fully expanded type of a is therefore () => () => () => ... ad infinitum (I don’t think TS generally infers typeof types)

@CraigMacomber
Copy link
Author

I’m more surprised this doesn’t have a circularity error, since a genuinely refers to itself in its own initializer and the fully expanded type of a is therefore () => () => () => ... ad infinitum (I don’t think TS generally infers typeof types)

I just found a workaround which actually both avoids the need for inferring a typeof type in the d.ts, which for some reason also generates a typeof type in the d.ts. Well, two redundant workarounds for the price of one is still a workaround :)

/**
 * Interface which carries the runtime and compile type data (from the generic type paramater) in a member.
 * This is also a constructor so that instances of it can be extended as classes.
 * Using classes in this way allows introducing a named type and a named value at the same time, helping keep the runtime and compile time information together and easy to refer to un a uniform way.
 * Addationally, this works around https://github.com/microsoft/TypeScript/issues/55832 which causes similar patterns with less explicit types to infer "any" in the d.ts file.
 */
interface Schema<ChildSchema> extends SchemaData<ChildSchema> {
	// We don't actually ever call this constructor,
	// but having it allows using classes to introduce runtime and named types at the same time, working around https://github.com/microsoft/TypeScript/issues/55832
	new (dummy: never): SchemaData<ChildSchema>;
}

/**
 * Used as both a class and an interface.
 * Helper for declaring Schema.
 */
class SchemaData<T> {
	constructor(public readonly data: T) {}
}

/**
 * Builder for the Schema interface.
 */
function build<T>(childType: T): Schema<T> {
	return class extends SchemaData<T> {
		static readonly data = childType;
		constructor() {
			super(childType);
		}
	}
}

// example use
class MySchema extends build(() => MySchema) {}

// Strong recursive typing works correctly:
const child: MySchema = MySchema.data().data().data();

The d.ts does not produce any, instead it uses typeof and adds a MySchema_base constant that does not exist in the JS.

declare const MySchema_base: Schema<() => typeof MySchema>;
declare class MySchema extends MySchema_base {
}

playground link

@fatcerberus
Copy link

fatcerberus commented Sep 23, 2023

Looking at this again, I think there might be a genuine bug here, just not the one presupposed in the OP; this isn't actually an implicit any and in fact does something I didn't even know was possible:

image

It seems like TypeScript knows the type is infinitely expanding, and that's totally fine. The problem only arises when it has to generate a declaration for it, at which point it apparently falls over and defaults to any. a is not treated as any locally; you can't assign it to a number or string for example, but can write any number of pairs of parentheses after a() and it still typechecks. Very odd.

@CraigMacomber
Copy link
Author

@fatcerberus That is the bug I was attempting to report. Everything is correct locally, but the generated d.ts contains "any" which was not explicitly in the source anywhere. I'll update it to clarify that it works locally in the description.

@fatcerberus
Copy link

@CraigMacomber I thought so, but I wanted to be sure after you said this:

Either generate something without implicitly introducing any, or generate an error if noImplicitAny is enabled.

which isn’t really relevant IMO since there’s no implicit any in the sense that noImplicitAny is meant to guard against (i.e. type inference failure introducing an implicit any); it’s seemingly just the declaration emitter falling over on something whose type was already correctly inferred.

@CraigMacomber
Copy link
Author

@CraigMacomber I thought so, but I wanted to be sure after you said this:

Either generate something without implicitly introducing any, or generate an error if noImplicitAny is enabled.

which isn’t really relevant IMO since there’s no implicit any in the sense that noImplicitAny is meant to guard against (i.e. type inference failure introducing an implicit any); it’s seemingly just the declaration emitter falling over on something whose type was already correctly inferred.

Its implicitly introducing any in the API for my package via the d.ts file.

TypeScript has a known design decision/limitation that the API of .dts files doesn't match the local type checking exactly, so the fact that they are different is not always a bug.
Its also a known design limitation of TypeScript that sometimes recursive types don't work arbitrarily, and you get any instead. This is also not a bug.
Thus I'm highlighting that the produced type is violating the noImplicitAny rule, which I think makes it a bug.

@RyanCavanaugh RyanCavanaugh added Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases Help Wanted You can do this labels Oct 3, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Oct 3, 2023
@RyanCavanaugh
Copy link
Member

It seems very reasonable to emit a noImplicitAny error when this happens specifically during declaration emit.

@andrewbranch
Copy link
Member

#56479 should be included as a test case for this.

@jakebailey
Copy link
Member

It seems very reasonable to emit a noImplicitAny error when this happens specifically during declaration emit.

Further, it'd be awesome to emit some sort of error any time dts emit produces an any out of nowhere, but when we were talking in the isolatedDeclaration meeting, such a thing seemed quite challenging given this is all pretty deep in typeToTypeString. But, maybe some sort of flag or something would be sufficient...

CraigMacomber added a commit to microsoft/FluidFramework that referenced this issue Nov 28, 2023
## Description

To support recursive schema using classes for the schema is useful to
work around microsoft/TypeScript#55832 .

If we are going to use classes to enable that workaround, and also
create a separate simple-tree specific schema system, there are some
other interesting approaches which can be taken.

If we don't support "Any" as a type allowed in fields, then all schema
are reachable from the root.
This means we can skip the schema library concept, and just pass in the
root schema in the configuration.
This has the side-effect of making the actual schema objects available
programmatically be the actual class objects declared in the schema file
(instead of the classes which those extend, which is what a library
would have captured).
This same reachability from the root also opens the door to supporting
contextual view schema where the same type in different locations in the
tree could have different schema subclasses, and its parent would
unambiguously determine which to use for it.

Thus all together this approach makes it possible to make the tree nodes
instances of classes which the user declared.
This has several benefits:

- Users don't have to use "typeof" or invoke any type meta-functions to
get the node types they want to pass around: just use the class/schema
name as the type.
- Intelisense is much cleaner when referring to types defined in schema:
It just uses the class name. or "typeof ClassName". (These
simplifications are what resolve the d.ts issue noted above with
recursive types)
- normal JS/TS type narrowing with `instanceof` can be used with schema
defined types.
- Its possible to add view/session local state to instances as
properties, as well as adding methods by just putting them in the class
like any other class.
- Since the schema/class defines the API for the node, the
implementation of the tree API implicitly gets defined in a way that is
trivially extensible for adding new node kinds.

TODO:

- [X]  support recursive times.
- [x]  implement runtime behavior for object fields.
- [X]  implement runtime behavior for list indexing.
- [x]  structurally typed maps and lists.
- [x] runtime constructing of hydrated nodes (ensure flex tree is type
erased).
-  runtime constructing and later hydrating of unhydrated nodes.
   - [x] objects
   - [ ] map
   - [ ] list
- [x]  Generation of view schema from simple schema.
- [x] Implement full simple-tree API surface
- [ ] Evaluate impact on simple-tree API (for example exposing of
prototypes impacting deep equals).
- [x] Basic end to end testing
- [ ] Full unit testing.

## Breaking Changes

The `schematize` API for simple-trees not using the new class based APIs
has been renamed to `schematizeOld`:
either rename the call site or migrate to the new class based schema.

Some types which would have collided have their old simple-tree or
flex-tree non class based version's renamed:

```typeScript
TreeNodeSchema as FlexTreeNodeSchema,
TreeListNode as TreeListNodeOld,
Tree as TreeOld,
TreeApi as TreeApiOld,
TreeView as TreeViewOld,
```

The previous version of all these names are now used for class based
APIs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

No branches or pull requests

5 participants