Skip to content

Allow assignment to readonly parameter property within the constructor#8712

Merged
3 commits merged into
masterfrom
readonly_assignment_in_constructor
May 23, 2016
Merged

Allow assignment to readonly parameter property within the constructor#8712
3 commits merged into
masterfrom
readonly_assignment_in_constructor

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented May 20, 2016

Fixes #8689

@msftclas
Copy link
Copy Markdown

Hi @Andy-MS, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Andy Hanson). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented May 20, 2016

👍

Comment thread src/compiler/checker.ts Outdated
const func = getContainingFunction(expr);
return !(func && func.kind === SyntaxKind.Constructor && func.parent === symbol.valueDeclaration.parent);
return !isInConstructor(getContainingFunction(expr));
function isInConstructor(func: FunctionLikeDeclaration) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this shouldn't be nested. @vladima there is some performance hint when nesting the function like this if I recall correctly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this would read better closer to the old style -- just const func = getContainingFunction(expr); if (!func) ....

The trailing nested function strikes me as a Haskell-ism.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: could you renamed the function -> the current name imply me that you are checking if func is in constructor. But my understanding is that you are checking if symbol is inside the constructor of the class that defines it..isSymbolReferredInsideOriginalConstructor or something like that. Others may have better idea for name 😅

@sandersn
Copy link
Copy Markdown
Member

👍

@yuit
Copy link
Copy Markdown
Contributor

yuit commented May 20, 2016

what happen in this case?

class A {
     constructor(readonly x: number) {
         this.x = 7;
     }
 }

class B extends A {
     constructor(readonly x: number) {
           this.x = 10;
    }
}

Otherwise lgtm 🍰

@yuit
Copy link
Copy Markdown
Contributor

yuit commented May 23, 2016

👍

@ghost ghost merged commit 0a54a3b into master May 23, 2016
@ghost ghost deleted the readonly_assignment_in_constructor branch May 23, 2016 20:21
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
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.

4 participants