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

Allow visibility on constructors #4174

Closed
wants to merge 14 commits into from
Closed

Allow visibility on constructors #4174

wants to merge 14 commits into from

Conversation

AbubakerB
Copy link
Contributor

Fixes #2341.

Changes:
1 - Allows private, protected and public accessibilities on constructors.
2 - Disallows classes with private constructors from being extended.
3 - Now throws an error on constructor overloads with different access modifiers (than the constructors implementation).

@msftclas
Copy link

msftclas commented Aug 6, 2015

Hi @AbubakerB, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@RyanCavanaugh
Copy link
Member

For this to work properly, the visibility needs to be part of the construct signature rather than being a declaration-based check that happens during signature resolution. Otherwise it's going to be too difficult to enforce assignability constraints based on constructor visibility.

A relevant testcase to add would be:

class Foo {
  private constructor() { }
}
class Bar {
  protected constructor() { }
}
let x: new() => any;
x = Foo; // Should be an error
x = Bar; // Should be an error

@AbubakerB
Copy link
Contributor Author

@RyanCavanaugh SignaturesRelatedTo now checks visibility of constructor.

@AbubakerB
Copy link
Contributor Author

Should i close this off? or is this in the pipeline?


function checkConstructorVisibility(signature: Signature, node: NewExpression) {
let constructor = result.declaration;
if (!constructor || !node.expression) return;
Copy link
Member

Choose a reason for hiding this comment

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

Can you wrap these return statements with curlies and place them on their own line?

if (!constructor || !node.expression) {
    return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@DanielRosenwasser
Copy link
Member

The declaration emitter will need to be amended - add // @declaration: true to the top of every affected test file.

@DanielRosenwasser
Copy link
Member

The following test cases would be useful too:

class Base {
    protected constructor() {
    }
}

class Derived extends Base {
    private constructor() {
        super();
    }
}
class Base {
    protected constructor() {
    }
}

class Derived extends Base {
    protected constructor() {
        super();
    }
}

var baseCtor = Base;
baseCtor = Derived;

@DanielRosenwasser
Copy link
Member

@AbubakerB sorry this fell off of our radar, we've been hard at work on TypeScript 1.6.

@ahejlsberg @mhegazy @vladima any thoughts?

@AbubakerB
Copy link
Contributor Author

@DanielRosenwasser I see, that's fine :)

  1. I've added // @declaration: true to the three test files.
  2. Added test cases under classConstructorAccessibility2.ts. Although, I have one question... Can a X ctor signature be assigned to a non-X ctor signature? (where X is public/protected/private).
  3. Renamed the diagnostic message for error code 2656 to be more clear.

tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(10,5): error TS1089: 'protected' modifier cannot appear on a constructor declaration.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(23,9): error TS1089: 'private' modifier cannot appear on a constructor declaration.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(27,9): error TS1089: 'protected' modifier cannot appear on a constructor declaration.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(20,9): error TS2655: Constructor '(x: number): D' is private and only accessible within class 'D'.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be new (x: number): D?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the best way of adding the new before the signature output?
I can't seem to find any method that writes 'new' + signature.

@RyanCavanaugh
Copy link
Member

@AbubakerB can you get this up-to-date and resubmit in a new PR? Very sorry we left this hanging for so long.

@AbubakerB
Copy link
Contributor Author

Sure. I've opened a new PR for this.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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.

None yet

4 participants