Skip to content

Fixes #17080 #21328

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

Merged
merged 5 commits into from
Jan 22, 2018
Merged

Fixes #17080 #21328

merged 5 commits into from
Jan 22, 2018

Conversation

Lazarus535
Copy link
Contributor

Changes are in src/compiler.checker.ts only
The second arguments to the function "removeOptionalityFromDeclaredType" has been changed from "getRootDeclaration(declaration)" to "declaration".

Fixes #

Changes are in src/compiler.checker.ts only
The second arguments to the function "removeOptionalityFromDeclaredType" has been changed from "getRootDeclaration(declaration)" to "declaration".
@msftclas
Copy link

msftclas commented Jan 22, 2018

CLA assistant check
All CLA requirements met.

microsoft#17080
Added testcases from the Github bugreport (all working as intended now).
Signed CLA.
@sandersn sandersn self-requested a review January 22, 2018 16:21
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll approve and merge after I understand why getRootDeclaration was added in the first place, and after a couple of minor fixes.

@@ -2,15 +2,16 @@
Thank you for submitting a pull request!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this file was supposed to be part of the PR.

useBar(bar);
}

function useBar(bar: number) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just declare function useBar(bar: number): void;

@@ -13046,7 +13046,7 @@ namespace ts {
node.parent.kind === SyntaxKind.NonNullExpression ||
declaration.kind === SyntaxKind.VariableDeclaration && (<VariableDeclaration>declaration).exclamationToken ||
declaration.flags & NodeFlags.Ambient;
const initialType = assumeInitialized ? (isParameter ? removeOptionalityFromDeclaredType(type, getRootDeclaration(declaration) as VariableLikeDeclaration) : type) :
const initialType = assumeInitialized ? (isParameter ? removeOptionalityFromDeclaredType(type, declaration as VariableLikeDeclaration) : type) :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea why getRootDeclaration was here? Was this copied from some other source that would also have an erroneous call to it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tracked it back to #14498, which switched to the current narrowing approach that this PR fixes. From my memory of it, I think that I inserted getRootDeclaration during development, then forgot to remove it after changing other parts of the code. Then none of our test cases caught the bug.

@sandersn
Copy link
Member

Approved after the two small fixes requested.

Fixed the two requested changes.
1) Deleting the file "pull_request_template.md"
2) Declaring functions in tests, instead of defining
@@ -1,16 +0,0 @@
<!--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this file is deleted, but shouldn't be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sry about that... :-(

Readded untouched pull_request_template.md
@sandersn sandersn merged commit a3387cc into microsoft:master Jan 22, 2018
@sandersn
Copy link
Member

Thanks @Lazarus535 ! Our test coverage was pretty bad here and is now much better.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants