-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Error on redeclarations of undefined #5420
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
@@ -1168,6 +1168,10 @@ | |||
"category": "Error", | |||
"code": 2396 | |||
}, | |||
"Declaration duplicates builtin global identifier '{0}'.": { |
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.
just curious, why not use Duplicate identifier '{0}'
error?
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'd prefer a more helpful message. This is the better direction IMO.
Though, :%s/duplicates/name conflicts
:%s/builtin/built-in
. P.S. I don't know if those are valid vim replacement commands.
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 am fine with the direction. Just curious and I do like the new error :)
@@ -336,6 +341,13 @@ namespace ts { | |||
target[id] = source[id]; | |||
} | |||
else { | |||
if (target === globals && source !== builtinGlobals) { |
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.
so why not just let them merge, then later on make sure that the undefined symbol is the one we have created, or that it does not have any declarations?
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.
We can get some wonky undefined behavior while typechecking if we actually let them merge (eg, the creation of a class which is also undefined
), so we have to guard against it in the merge function so we don't let it poison the type checking process. Outright overriding the symbol is okay (since then we still have the real undefined type symbol to use internally) and is what #5387 allowed, but merging with the builtin undefined symbol is unallowable.
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 a corner case, if the user ran into it, we just should let them know, i do not think we need to complicate our response here.
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.
By "wonky undefined behavior" I mean crashing/exceptions.
👍 🍸 |
Error on redeclarations of undefined
From discussion on #5387.
Instead of this variety of implementation, another other option is actually exposing the
undefined
type as a primitive-like type (likesymbol
ornumber
) and addingdeclare var undefined: undefined
to the lib. That would be more consistent - and could begin to pave the way to more easily explicitly "nullable" types.Fixes #3087.