-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Error when variable is circularly referenced in type annotation #2991
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
Conversation
👍 |
error(symbol.valueDeclaration, diagnostic, symbolToString(symbol)); | ||
if ((<VariableLikeDeclaration>symbol.valueDeclaration).type) { | ||
// Variable has type annotation that circularly references the variable itself | ||
links.type = unknownType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the unknown type here as opposed to the 'any' type? What's the mental model i should use for when i should use the 'unknown' type? Thanks! (also a comment in the code explaining this would be useful).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always an error, i.e. it is something that does not make sense. the other one, the compiler sets to any as it could not find a better type (because of the self reference). the second case is not an error, it is the expected behavior, it is just that if you have --noImplictAny, the compiler warns about cases when it silently gives things an any type.
Hmm, turns out my fix only fixes it for the first repro in #2980. This one is still an issue: var varOfAliasedType = func();
function func(): typeAlias { return null; }
type typeAlias = typeof varOfAliasedType; No error is reported for the example above (and The core issue is that we have two rules that are fundamentally at odds: We say variables that circularly reference themselves in their initializers are given type |
Latest commits improve circularity detection such that errors are reported on each declaration in a circular chain. This fixes the order dependency issues and also makes it easier for the user to understand the circular causality. |
// a new entry with that target and an associated result value of true is pushed on the stack, and the value | ||
// true is returned. Otherwise, a circularity has occurred and the result values of the existing entry and | ||
// all entries pushed after it are changed to false, and the value false is returned. The target object has | ||
// no significance other than to provide a unique identity for a particular type resolution result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice comment! Very clear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can you write it in docComment format so that quick info and signature help will display it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to keep it in this format to stay consistent with the rest of the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the comment formats have to be consistent at the expense of usability, but very well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really we should be using doc comments above every function, and every little bit helps.
I just thought of something interesting, an alternate way to fix this issue. I'm not sure if it's simpler, or if it even works, but it's more similar to the old model, where we track cycles by using the |
This idea would require two sentinel objects, but I think it is simple, and does not rely on us having to be so careful about what object we are passing to the pushTypeResolution method. |
You could probably make something like that work but I doubt it would be simpler. Plus it is technically not in the spirit of lazy initialization of an immutable value because the value actually goes through an observable sequence of changes. I think what we have now is the right solution--or certainly right enough. |
Error when variable is circularly referenced in type annotation
Fixes #2980.