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

Support for protected members in classes #688

Merged
merged 6 commits into from Sep 19, 2014

Conversation

Projects
None yet
7 participants
@ahejlsberg
Copy link
Member

commented Sep 17, 2014

This change adds support for protected members in classes as proposed in #1. Also, this change introduces better error messages for assignability and subtype checks involving private and optional properties.

Protected members are modeled after C# and Java. Protected members can be accessed only within the declaring class and subclasses of the declaring class. Furthermore, access to a protected instance member is required to take place through an instance of the enclosing class type or a class type constructed from it (this prevents "sibling" class access).

Protected members can be made public in derived classes. Similar to private members, constructors and signature members cannot be protected.

I will follow up with a more formal specification of the rules but wanted to put up the code for folks to experiment with.

Update: Formal specification provided in #700.

Debug.assert(sourceProp);
if (!targetProp) {
return false;
}

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

I'm glad we are getting rid of this check

if (!targetProp) {
return false;
}

// Two members are considered identical when
// - they are public properties with identical names, optionality, and types,

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

trailing comma

return true;
}
var sourcePropVisibility = getDeclarationFlagsFromSymbol(sourceProp) & (NodeFlags.Private || NodeFlags.Protected);
var targetPropVisibility = getDeclarationFlagsFromSymbol(targetProp) & (NodeFlags.Private || NodeFlags.Protected);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Use bitwise OR

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Also can you name them sourcePropIsNonPublic, targetPropIsNonPublic

for (var i = 0, len = sourceProperties.length; i < len; ++i) {
var sourceProp = sourceProperties[i];
var targetProp = getPropertyOfType(target, sourceProp.name);

if (!isPropertyIdenticalToRecursive(sourceProp, targetProp, reportErrors, isRelatedTo)) {
if (!targetProp || !isPropertyIdenticalToRecursive(sourceProp, targetProp, reportErrors, isRelatedTo)) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Yeah this is definitely a better place to check targetProp

@@ -2948,48 +2938,70 @@ module ts {
for (var i = 0; i < properties.length; i++) {
var targetProp = properties[i];
var sourceProp = getPropertyOfApparentType(source, targetProp.name);
if (sourceProp === targetProp) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

I think I like this check better than wrapping everything in if (sourceProp !== targetProp).

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Also, there is a change in release-1.1 that you need to merge with. It compares sourceProp.valueDeclaration to targetProp.valueDeclaration. The change is #660

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

I've now merged release-1.1 into master, so if you merge master, you will get this change.

if (!isRelatedTo(getTypeOfSymbol(sourceProp), getTypeOfSymbol(targetProp), reportErrors)) {
if (reportErrors) {
reportError(Diagnostics.Types_of_property_0_are_incompatible_Colon, symbolToString(targetProp));
if (targetFlags & NodeFlags.Protected) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Should this be else if?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Sep 17, 2014

Author Member

No reason to, the previous if block ends in an unconditional return.

if (flags & NodeFlags.Private) {
return declaringClass === enclosingClass;
}
if (node.left.kind === SyntaxKind.SuperKeyword) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

It seems a little odd that we would have to check this case here. Should we really be calling into this function if it is a super access?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Sep 17, 2014

Author Member

We still need the other checks in a super property access and I don't really want to duplicate them. But yeah, it is a bit messy to have that here.

return true;
}
if (!(flags & NodeFlags.Static)) {
if (!(getTargetType(type).flags & (TypeFlags.Class | TypeFlags.Interface) && hasBaseType(<InterfaceType>type, enclosingClass))) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

This is a rather confusing condition. Can you break it up a bit and put a comment? Also, why only if it is not static?

return false;
}
}
return hasBaseType(enclosingClass, declaringClass);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

I would have an explicit check for protected, and then assert if you don't see it. It makes the intention of the code clearer.

@@ -2368,12 +2368,18 @@ module ts {
if (node.flags & NodeFlags.Private) {

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

these seem repeated, I would move them before the if statement

This comment has been minimized.

Copy link
@mhegazy
@@ -0,0 +1,115 @@
// Class with protected members

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

propablly need to move this to conformance\classes\members\accesibility

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

also breaking it into smaller tests would be appreciated.

@@ -4024,13 +4060,10 @@ module ts {
// where this references the constructor function object of a derived class,
// a super property access is permitted and must specify a public static member function of the base class.
if (node.left.kind === SyntaxKind.SuperKeyword && getDeclarationKindFromSymbol(prop) !== SyntaxKind.Method) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

I would prefer just checking node.left.kind === SyntaxKind.SuperKeyword. Then in a nested if statement, you can check for SyntaxKind.Method. That way, super is left out of the whole isClassPropertyAccessible function.

~~
!!! Class 'B3' incorrectly extends base class 'A3':
!!! Property 'x' is protected in type 'B3' but public in type 'A3'.
protected x;

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

can we add a test for protected in base, private in derived

"Property '{0}' is protected in type '{1}' but public in type '{2}'.": {
"category": "Error",
"code": 2444
},

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Make sure indentation is consistent here

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Sep 17, 2014

Author Member

Not sure what's going on, indentation is fine in the actual file.

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Maybe try opening it in notepad? Visual studio might interpret tabs differently

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Sep 17, 2014

Member

Ensure there are absolutely no tabs here.


Modifier = Export | Ambient | Public | Private | Static
Modifier = Export | Ambient | Public | Private | Protected | Static

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

I think @mhegazy added a flag for accessibility modifiers. Can you check with him, and add Protected to that set too?

class C extends A {
z;
static foo(a: A, b: B, c: C, d: D, e: E) {
a.x = 1; // Error, access must be through C or type derived from C

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Maybe it's worth making this a separate error message

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Also, can you add a case where you try to access the sx property from all 5 classes?

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Sorry one more thing. This test file seems to have a few independent sections. Can you break those up into separate files?

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Sep 17, 2014

Contributor

Also add some fourslash tests for member completion.

d;
}

interface E extends C {

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

Do not see any tests for that, as a matter of fact do not see any tests for interface scenarios:

  • implement an interface with a protected property
  • increase the visibility of a protected property
  • make the member optional
  • inherit multiple protected properties with the same name
  • inherit the same protected property from two interfaces
  • inherit the same protected property from two interfaces but through different classes
  • simultaneously extending two interfaces with one protected property and a conflicting different property
  • simultaneously extending two interfaces with one protected property and a different protected property
  • simultaneously extending two interfaces with one protected property and anther interface with the same property but with higher visibility
  • assign class with protected property to interface that inherits from it or one of its bases
  • access a protected property off an interface extending its defining base class
  • derived class changing the type of the protected member to a sub-type, then checking assignablity scenarios with base
  • derived class changing the type of the protected member to any, then checking assignablity scenarios with base

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

I do not see any .d.ts emit test cases either, we probably need a few tests for:

  • just emitting the protected property
  • protected methods
  • defined in constructors
  • protected property with private type, ensuring the error is still reported.

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

i would take a look at:
tests\cases\conformance\classes\constructorDeclarations\constructorParameters
tests\cases\conformance\classes\members\accessibility
tests\cases\conformance\classes\members\inheritanceAndOverriding

@@ -226,7 +226,7 @@ module ts {
function bindConstructorDeclaration(node: ConstructorDeclaration) {
bindDeclaration(node, SymbolFlags.Constructor, 0);
forEach(node.parameters, p => {
if (p.flags & (NodeFlags.Public | NodeFlags.Private)) {
if (p.flags & (NodeFlags.Public | NodeFlags.Private | NodeFlags.Protected)) {

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

I have recently added NodeFlags.AccessibilityModifier, a merge should bring it to your branch

error(node, Diagnostics.Property_0_is_inaccessible, getFullyQualifiedName(prop));
}
else if (!isClassPropertyAccessible(node, type, prop)) {
error(node, Diagnostics.Property_0_is_inaccessible, getFullyQualifiedName(prop));

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

since we are making errors better in this area, this one can use some attention. specially that you would get it if you are accessing a protected property, in a derived class but on the wrong object, which is not intuitive as a private.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Sep 17, 2014

Author Member

Agreed, we could be more explanatory here.

if (sourceProp === targetProp) {
return true;
}
var sourcePropVisibility = getDeclarationFlagsFromSymbol(sourceProp) & (NodeFlags.Private || NodeFlags.Protected);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Sep 17, 2014

Contributor

using || on numbers/enums is very odd...

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Sep 17, 2014

Author Member

Yup, that is wrong.

class B3 extends A3 {
protected x;
}

This comment has been minimized.

Copy link
@danquirk

danquirk Sep 17, 2014

Member

will need to add significantly more test coverage in the conformance suite

This comment has been minimized.

Copy link
@mhegazy

mhegazy Sep 17, 2014

what about this, is this OK?

class C {
    protected f;
}

interface I extends C {
    f; // make it public
}

var i : I = { f: 0 };
var c: C = i; // OK ?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Sep 18, 2014

Author Member

No, the declaration of I would be in error. An interface can inherit private and protected members from a class but it can't change them in any way. You would have to write it this way:

class C {
    protected f;
}

class D extends C {
    f; // make it public
}

interface I extends D {
}

var i : I = { f: 0 };
var c: C = i; // OK
@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented on src/compiler/checker.ts in 4aa04a9 Sep 18, 2014

great comments thanks. it really makes it easy to read through and understand what's going on.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented on src/compiler/checker.ts in 4aa04a9 Sep 18, 2014

this line doesn't match the comment. You don't check for the case that you're in the declaring calss, only that you're in a class derived form it.

This comment has been minimized.

Copy link
Member Author

replied Sep 18, 2014

Actually, hasBaseType returns true for the type itself so the check is correct. But maybe we need a better name for hasBaseType. Ideas?

@mhegazy

This comment has been minimized.

Copy link

commented on c990c42 Sep 19, 2014

Error messages are much better. thanks!

@jonathandturner

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2014

Result from the design meeting:

Everyone is for adding 'protected'. We'd like to get this in.

ahejlsberg added a commit that referenced this pull request Sep 19, 2014

Merge pull request #688 from Microsoft/protectedMembers
Support for protected members in classes

@ahejlsberg ahejlsberg merged commit 7cc6bbb into master Sep 19, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ahejlsberg ahejlsberg deleted the protectedMembers branch Sep 19, 2014

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.