Skip to content

Constructor Visibility #6885

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.

Sign up for GitHub

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
mhegazy merged 18 commits into from
Feb 17, 2016
Merged

Constructor Visibility #6885

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f869b41
Added private and protected modifiers to constructors
AbubakerB Feb 3, 2016
c351ffc
Added and updated tests for constructor visibility
AbubakerB Feb 3, 2016
bbf92ce
Added constructor visibility in the declaration emitter
AbubakerB Feb 4, 2016
3d529a8
Update baselines
AbubakerB Feb 4, 2016
4200051
Addressed PR feedback.
AbubakerB Feb 4, 2016
4c93eb2
Accept baselines
AbubakerB Feb 4, 2016
5ce0202
Refactored visibility text on modifier grammar check
AbubakerB Feb 4, 2016
16b54e0
Addressed PR. Diagnostic messages
AbubakerB Feb 4, 2016
037b657
Accept baselines
AbubakerB Feb 4, 2016
a6a5a22
Addressed PR
AbubakerB Feb 5, 2016
d0843e1
Merge branch 'ts-master' into constructorAccessibility
AbubakerB Feb 12, 2016
8ef9599
Allow extending and instantiating a private or protected class within…
AbubakerB Feb 13, 2016
ec7e80e
Added tests and accept baselines
AbubakerB Feb 13, 2016
91ea83c
Addressed PR
AbubakerB Feb 13, 2016
208830e
Updated tests and accepted baseline
AbubakerB Feb 13, 2016
2d7a0f4
Merge branch 'master' into constructorAccessibility
AbubakerB Feb 14, 2016
ba8b168
Included previously ignored baseline .js file and slight refactoring
AbubakerB Feb 14, 2016
de23e2f
Merge branch 'master' into constructorAccessibility
AbubakerB Feb 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Addressed PR feedback.
  • Loading branch information
@AbubakerB
AbubakerB committed Feb 4, 2016
commit 42000519347ecefe60687b9976e654781a69bbec
30 changes: 18 additions & 12 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1711,6 +1711,16 @@ namespace ts {
return result;
}

function visibilityToString(flags: NodeFlags) {
if (flags === NodeFlags.Private) {
return "private";
}
if (flags === NodeFlags.Protected) {
return "protected";
}
return "public";
}

function getTypeAliasForTypeLiteral(type: Type): Symbol {
if (type.symbol && type.symbol.flags & SymbolFlags.TypeLiteral) {
let node = type.symbol.declarations[0].parent;
Expand Down Expand Up @@ -6059,28 +6069,24 @@ namespace ts {

function constructorRelatedTo(sourceSignature: Signature, targetSignature: Signature, reportErrors: boolean) {
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Feb 4, 2016

Choose a reason for hiding this comment

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

It looks like a better name would be constructorVisibilitiesAreCompatible

Copy link
Copy Markdown
Contributor Author

@AbubakerB AbubakerB Feb 5, 2016

Choose a reason for hiding this comment

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

Done

if (sourceSignature && targetSignature && sourceSignature.declaration && targetSignature.declaration) {
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Feb 4, 2016

Choose a reason for hiding this comment

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

When would sourceSignature and targetSignature be undefined?

Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Feb 4, 2016

Choose a reason for hiding this comment

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

Same with declaration - those appear to be non-optional.

Copy link
Copy Markdown
Contributor Author

@AbubakerB AbubakerB Feb 5, 2016

Choose a reason for hiding this comment

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

There are cases where the declarations are undefined. I checked and ran the tests, which caused lots of errors.
But I've removed sourceSignature and targetSignature.

// A public, protected and private signature is assignable to a private signature.
// A public and protected signature is assignable to a protected signature.
// And only a public signature is assignable to public signature.
const sourceAccessibility = sourceSignature.declaration.flags & (NodeFlags.Private | NodeFlags.Protected);
const targetAccessibility = targetSignature.declaration.flags & (NodeFlags.Private | NodeFlags.Protected);

const isRelated = sourceAccessibility === targetAccessibility;
const isRelated = targetAccessibility === NodeFlags.Private
|| (targetAccessibility === NodeFlags.Protected && sourceAccessibility !== NodeFlags.Private)
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Feb 4, 2016

Choose a reason for hiding this comment

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

Indent these

Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Feb 5, 2016

Choose a reason for hiding this comment

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

It'd just be clearer if you formatted this as

// A public, protected and private signature is assignable to a private signature.
if (targetAccessibility === NodeFlags.Private) {
    return true;
}

// A public and protected signature is assignable to a protected signature.
if (targetAccessibility === NodeFlags.Protected && sourceAccessibility !== NodeFlags.Private) {
    return true;
}

// Only a public signature is assignable to public signature.
if (targetAccessibility !== NodeFlags.Protected && !sourceAccessibility) {
    return true;
}

if (reportErrors) {
    reportError(Diagnostics.Cannot_assign_a_0_constructor_type_to_a_1_constructor_type, visibilityToString(sourceAccessibility), visibilityToString(targetAccessibility));
}
return false;

Copy link
Copy Markdown
Contributor Author

@AbubakerB AbubakerB Feb 5, 2016

Choose a reason for hiding this comment

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

👍 Done

|| (targetAccessibility !== NodeFlags.Protected && !(sourceAccessibility & (NodeFlags.Private | NodeFlags.Protected)));
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Feb 4, 2016

Choose a reason for hiding this comment

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

Why sourceAccessibility & (NodeFlags.Private | NodeFlags.Protected)? Isn't that just sourceAccessibility as it is?

Copy link
Copy Markdown
Contributor Author

@AbubakerB AbubakerB Feb 5, 2016

Choose a reason for hiding this comment

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

Yep, my bad!


if (!isRelated && reportErrors) {
reportError(Diagnostics.Cannot_assign_a_0_constructor_type_to_a_1_constructor_type, flagsToString(sourceAccessibility), flagsToString(targetAccessibility));
reportError(Diagnostics.Cannot_assign_a_0_constructor_type_to_a_1_constructor_type, visibilityToString(sourceAccessibility), visibilityToString(targetAccessibility));
}

return isRelated;
}

return true;

function flagsToString(flags: NodeFlags) {
if (flags === NodeFlags.Private) {
return "private";
}
if (flags === NodeFlags.Protected) {
return "protected";
}
return "public";
}
}
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Feb 4, 2016

Choose a reason for hiding this comment

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

If you don't get rid of the if, please add an explicit return statement.

}

Expand Down