Skip to content
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

References to object properties as default values for typed params are typeable, not circular #48202

Closed
wbt opened this issue Mar 10, 2022 · 7 comments
Labels
Duplicate An existing issue was already created

Comments

@wbt
Copy link

wbt commented Mar 10, 2022

Bug Report

πŸ”Ž Search Terms

circular 7022

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about "Common "Bugs" That Aren't Bugs" and "Common Feature Requests"

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

const NotOKinTS = { //ts(7022) error
	defaultMinimum: 24,
	exceedsMinimum: function(actualValue : number, minimum : number = NotOKinTS.defaultMinimum) : boolean {
		return actualValue > minimum;
	}
}

const OKinTS = {
	defaultMinimum: 24,
	//The next line could be made more concise by deleting `| undefined = undefined` with the same result.
	exceedsMinimum: function(actualValue : number, minimum : number | undefined = undefined) : boolean {
		if(typeof minimum === 'undefined') {
			minimum = OKinTS.defaultMinimum;
		}
		return actualValue > minimum;
	}
}

πŸ™ Actual behavior

ts(7022) error on the first line

πŸ™‚ Expected behavior

The object 'NotOKinTS' should compile just fine, without a ts(7022) error, even if the typing on the second parameter to the function is removed.

Details

There is an error here on the first line: 'myObject' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022). The second object has the exact same meaning in JavaScript but requires an additional three lines and more importantly, represents another case where valid JavaScript code has to be rewritten beyond the addition of types as part of conversion to TypeScript.

Further, there appears to be no reason why the first example should be problematic. The parameter is typed, so the default shouldn’t matter (and the fact that it does really seems like a bug), but I would argue that it shouldn’t have to be as default value refers to a member of the object which is a constant, knowable type.

The solution to this pushed in the TypeScript community is to just write an interface definition and type the object as that interface. For a half-dozen lines like this first object, that may be fine, but for a much more complex piece of code thousands of lines long, that is both a lot of work and reduces maintainability by requiring that any change (e.g. to a function signature) be modified in two places that are not especially close to one another. Having to rewrite valid code into a less maintainable form is really not helping promote adoption of TypeScript.

Related issues:

Though I recognize #43708, which includes complicating async/await keywords, was closed as a duplicate of it and then not considered as part of the issue for being different, #43047 focuses on a variable being modified in a loop. #32950 is related to a nonobvious circularity resulting from a library use. This is a much simpler case which may be easier to address.

@MartinJohns
Copy link
Contributor

The parameter is typed, so the default shouldn’t matter (and the fact that it does really seems like a bug)

It needs to check if the value is compatible, but it can't because the value is not yet defined.

@wbt
Copy link
Author

wbt commented Mar 10, 2022

The value is defined though.
At the least, we could have the check to see if a default value of a typed parameter is compatible with that type come after the check to figure out the shape of the object.
And/or we could recognize that the type of NotOKinTS.defaultMinimum doesn't depend on the type of the function, so the type of that is definitely 'number' and could be used even to assign the type of the parameter if the type wasn't given.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Mar 10, 2022
@RyanCavanaugh
Copy link
Member

Same as #45213

We're usually OK with being slightly under-complete on circularity detection in exchange for generally better performance in all other cases.

@wbt
Copy link
Author

wbt commented Mar 11, 2022

I had a look at #45213 before filing this one, and don’t think this is a duplicate of that. The example code given there is more complex with functions being passed around, dynamic template type inference, and adding a type on the parameter solved the issue. In this case, we have a typed parameter and Typescript still can’t figure out the type of the object as a whole. In that issue, you wrote

Keeping a strict rule that the bind phase only uses declaration constructs and the check phase is the only one that's allowed to use inferred object shapes makes the whole system much easier to reason about, and creates invariants that can be used to improve performance in other cases.

The check phase, of checking to see if the provided default is of the right type, should come AFTER figuring out the type of the object overall (i.e. when types are bound to the object). The check should be done at the same time as in the second example above, which does the same thing and is equivalent from a JavaScript perspective. Having the check phase after the bind phase adds a pass but helps keep things logically straight and simpler.

I also don’t buy this line as applicable to this issue:

in general it doesn't come up that often to justify the additional complexity.

That might apply to the other issue, but the case presented here seems to be a fairly common one in which a module defines some possibly-configurable constants in one place at the start of the module and then uses those as default values for functions later on. Also, checking for type matching after inferring types or doing that at the same time as the check is done in the second case doesn't seem like it should be adding complexity, it seems like it should be simplifying things.

Fixing this particular case might not fix the other more complex one, but adding that default value for a typed parameter should NOT cause type inference on the whole first object in this example to fail.

@RyanCavanaugh
Copy link
Member

If you want to send a simplifying PR that solves this, please go ahead.

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@wbt
Copy link
Author

wbt commented Mar 14, 2022

I don't think this should be automatically closed just because an OP didn't immediately send a PR that fixes the issue. A fix requires a pretty deep dive into the compiler code and isn't something that can be done in just a couple weekday hours!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants